fix(affected): include not committed changes in --affected (#9133)

### Description

We were including new files that were not checked into git, but not
changes to files already in git but not committed.

### Testing Instructions
Added more testing to `affected.t` for both new files and existing files
with new changes

---------

Co-authored-by: Chris Olszewski <chris.olszewski@vercel.com>

crates/turborepo-lib/src/commands/config.rs

@@ -24,7 +24,7 @@ struct ConfigOutput<'a> {
     daemon: Option<bool>,
     env_mode: EnvMode,
     scm_base: Option<&'a str>,
-    scm_head: &'a str,
+    scm_head: Option<&'a str>,
     cache_dir: &'a Utf8Path,
 }
 

crates/turborepo-lib/src/config/env.rs

@@ -380,7 +380,7 @@ mod test {
         assert_eq!(config.env_mode, None);
         assert!(!config.preflight());
         assert_eq!(config.scm_base(), None);
-        assert_eq!(config.scm_head(), "HEAD");
+        assert_eq!(config.scm_head(), None);
         assert_eq!(config.root_turbo_json_path, None);
         assert!(!config.force());
         assert_eq!(config.log_order(), LogOrder::Auto);

crates/turborepo-lib/src/config/mod.rs

@@ -316,8 +316,8 @@ impl ConfigurationOptions {
         non_empty_str(self.scm_base.as_deref())
     }
 
-    pub fn scm_head(&self) -> &str {
-        non_empty_str(self.scm_head.as_deref()).unwrap_or("HEAD")
+    pub fn scm_head(&self) -> Option<&str> {
+        non_empty_str(self.scm_head.as_deref())
     }
 
     pub fn allow_no_package_manager(&self) -> bool {

crates/turborepo-lib/src/opts.rs

@@ -310,7 +310,7 @@ pub struct ScopeOpts {
     pub pkg_inference_root: Option<AnchoredSystemPathBuf>,
     pub global_deps: Vec<String>,
     pub filter_patterns: Vec<String>,
-    pub affected_range: Option<(Option<String>, String)>,
+    pub affected_range: Option<(Option<String>, Option<String>)>,
 }
 
 impl<'a> TryFrom<OptsInputs<'a>> for ScopeOpts {
@@ -327,7 +327,10 @@ impl<'a> TryFrom<OptsInputs<'a>> for ScopeOpts {
         let affected_range = inputs.execution_args.affected.then(|| {
             let scm_base = inputs.config.scm_base();
             let scm_head = inputs.config.scm_head();
-            (scm_base.map(|b| b.to_owned()), scm_head.to_string())
+            (
+                scm_base.map(|b| b.to_owned()),
+                scm_head.map(|h| h.to_string()),
+            )
         });
 
         Ok(Self {
@@ -518,7 +521,9 @@ mod test {
             pkg_inference_root: None,
             global_deps: vec![],
             filter_patterns: opts_input.filter_patterns,
-            affected_range: opts_input.affected.map(|(base, head)| (Some(base), head)),
+            affected_range: opts_input
+                .affected
+                .map(|(base, head)| (Some(base), Some(head))),
         };
         let opts = Opts {
             run_opts,

crates/turborepo-lib/src/query.rs

@@ -242,8 +242,6 @@ impl Query {
         base: Option<String>,
         head: Option<String>,
     ) -> Result<Vec<Package>, Error> {
-        let base = base.map(|s| s.to_owned());
-        let head = head.unwrap_or_else(|| "HEAD".to_string());
         let mut opts = self.run.opts().clone();
         opts.scope_opts.affected_range = Some((base, head));
 

crates/turborepo-lib/src/run/scope/change_detector.rs

@@ -21,6 +21,7 @@ pub trait GitChangeDetector {
         to_ref: Option<&str>,
         include_uncommitted: bool,
         allow_unknown_objects: bool,
+        merge_base: bool,
     ) -> Result<HashSet<PackageName>, ResolutionError>;
 }
 
@@ -92,13 +93,15 @@ impl<'a> GitChangeDetector for ScopeChangeDetector<'a> {
         to_ref: Option<&str>,
         include_uncommitted: bool,
         allow_unknown_objects: bool,
+        merge_base: bool,
     ) -> Result<HashSet<PackageName>, ResolutionError> {
         let changed_files = match self.scm.changed_files(
             self.turbo_root,
             from_ref,
             to_ref,
             include_uncommitted,
             allow_unknown_objects,
+            merge_base,
         )? {
             ChangedFiles::All => {
                 debug!("all packages changed");

crates/turborepo-lib/src/run/scope/filter.rs

@@ -163,7 +163,7 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> {
     /// It applies the following rules:
     pub(crate) fn resolve(
         &self,
-        affected: &Option<(Option<String>, String)>,
+        affected: &Option<(Option<String>, Option<String>)>,
         patterns: &[String],
     ) -> Result<(HashSet<PackageName>, bool), ResolutionError> {
         // inference is None only if we are in the root
@@ -185,7 +185,7 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> {
 
     fn get_packages_from_patterns(
         &self,
-        affected: &Option<(Option<String>, String)>,
+        affected: &Option<(Option<String>, Option<String>)>,
         patterns: &[String],
     ) -> Result<HashSet<PackageName>, ResolutionError> {
         let mut selectors = patterns
@@ -197,9 +197,10 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> {
             selectors.push(TargetSelector {
                 git_range: Some(GitRange {
                     from_ref: from_ref.clone(),
-                    to_ref: Some(to_ref.to_string()),
+                    to_ref: to_ref.clone(),
                     include_uncommitted: true,
                     allow_unknown_objects: true,
+                    merge_base: true,
                 }),
                 include_dependents: true,
                 ..Default::default()
@@ -549,6 +550,7 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> {
             git_range.to_ref.as_deref(),
             git_range.include_uncommitted,
             git_range.allow_unknown_objects,
+            git_range.merge_base,
         )
     }
 
@@ -1266,6 +1268,7 @@ mod test {
             to: Option<&str>,
             _include_uncommitted: bool,
             _allow_unknown_objects: bool,
+            _merge_base: bool,
         ) -> Result<HashSet<PackageName>, ResolutionError> {
             Ok(self
                 .0

crates/turborepo-lib/src/run/scope/target_selector.rs

@@ -13,6 +13,9 @@ pub struct GitRange {
     // this is useful for shallow clones where objects may not exist.
     // When this happens, we assume that everything has changed.
     pub allow_unknown_objects: bool,
+    // Calculate diff between merge base of the two refs and the second ref
+    // (this is usually what you want for detecting changes)
+    pub merge_base: bool,
 }
 
 #[derive(Debug, Default, PartialEq)]
@@ -159,6 +162,7 @@ impl FromStr for TargetSelector {
                     to_ref: Some(b.to_string()),
                     include_uncommitted: false,
                     allow_unknown_objects: false,
+                    merge_base: true,
                 }
             } else {
                 // If only the start of the range is specified, we assume that
@@ -168,6 +172,7 @@ impl FromStr for TargetSelector {
                     to_ref: None,
                     include_uncommitted: true,
                     allow_unknown_objects: false,
+                    merge_base: false,
                 }
             };
             Some(git_range)
@@ -254,13 +259,13 @@ mod test {
     #[test_case(".", TargetSelector { raw: ".".to_string(), parent_dir: Some(AnchoredSystemPathBuf::try_from(".").unwrap()), ..Default::default() }; "parent dir dot")]
     #[test_case("..", TargetSelector { raw: "..".to_string(), parent_dir: Some(AnchoredSystemPathBuf::try_from("..").unwrap()), ..Default::default() }; "parent dir dot dot")]
     #[test_case("[master]", TargetSelector { raw: "[master]".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), ..Default::default() }; "square brackets master")]
-    #[test_case("[from...to]", TargetSelector { raw: "[from...to]".to_string(), git_range: Some(GitRange { from_ref: Some("from".to_string()), to_ref: Some("to".to_string()), ..Default::default() }), ..Default::default() }; "[from...to]")]
+    #[test_case("[from...to]", TargetSelector { raw: "[from...to]".to_string(), git_range: Some(GitRange { from_ref: Some("from".to_string()), to_ref: Some("to".to_string()), merge_base: true, ..Default::default() }), ..Default::default() }; "[from...to]")]
     #[test_case("{foo}[master]", TargetSelector { raw: "{foo}[master]".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), parent_dir: Some(AnchoredSystemPathBuf::try_from("foo").unwrap()), ..Default::default() }; "{foo}[master]")]
     #[test_case("pattern{foo}[master]", TargetSelector { raw: "pattern{foo}[master]".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), parent_dir: Some(AnchoredSystemPathBuf::try_from("foo").unwrap()), name_pattern: "pattern".to_string(), ..Default::default() }; "pattern{foo}[master]")]
     #[test_case("[master]...", TargetSelector { raw: "[master]...".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), include_dependencies: true, ..Default::default() }; "square brackets master dot dot dot")]
     #[test_case("...[master]", TargetSelector { raw: "...[master]".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), include_dependents: true, ..Default::default() }; "dot dot dot master square brackets")]
     #[test_case("...[master]...", TargetSelector { raw: "...[master]...".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), include_dependencies: true, include_dependents: true, ..Default::default() }; "dot dot dot master square brackets dot dot dot")]
-    #[test_case("...[from...to]...", TargetSelector { raw: "...[from...to]...".to_string(), git_range: Some(GitRange { from_ref: Some("from".to_string()), to_ref: Some("to".to_string()), ..Default::default() }), include_dependencies: true, include_dependents: true, ..Default::default() }; "dot dot dot [from...to] dot dot dot")]
+    #[test_case("...[from...to]...", TargetSelector { raw: "...[from...to]...".to_string(), git_range: Some(GitRange { from_ref: Some("from".to_string()), to_ref: Some("to".to_string()), merge_base: true, ..Default::default() }), include_dependencies: true, include_dependents: true, ..Default::default() }; "dot dot dot [from...to] dot dot dot")]
     #[test_case("foo...[master]", TargetSelector { raw: "foo...[master]".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), name_pattern: "foo".to_string(), match_dependencies: true, ..Default::default() }; "foo...[master]")]
     #[test_case("foo...[master]...", TargetSelector { raw: "foo...[master]...".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), name_pattern: "foo".to_string(), match_dependencies: true, include_dependencies: true, ..Default::default() }; "foo...[master] dot dot dot")]
     #[test_case("{foo}...[master]", TargetSelector { raw: "{foo}...[master]".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), parent_dir: Some(AnchoredSystemPathBuf::try_from("foo").unwrap()), match_dependencies: true, ..Default::default() }; " curly brackets foo...[master]")]

crates/turborepo-scm/src/git.rs

@@ -35,6 +35,7 @@ impl SCM {
         to_commit: Option<&str>,
         include_uncommitted: bool,
         allow_unknown_objects: bool,
+        merge_base: bool,
     ) -> Result<ChangedFiles, Error> {
         fn unable_to_detect_range(error: impl std::error::Error) -> Result<ChangedFiles, Error> {
             warn!(
@@ -45,7 +46,13 @@ impl SCM {
         }
         match self {
             Self::Git(git) => {
-                match git.changed_files(turbo_root, from_commit, to_commit, include_uncommitted) {
+                match git.changed_files(
+                    turbo_root,
+                    from_commit,
+                    to_commit,
+                    include_uncommitted,
+                    merge_base,
+                ) {
                     Ok(files) => Ok(ChangedFiles::Some(files)),
                     Err(ref error @ Error::Git(ref message, _))
                         if allow_unknown_objects && message.contains("no merge base") =>
@@ -110,6 +117,7 @@ impl Git {
         from_commit: Option<&str>,
         to_commit: Option<&str>,
         include_uncommitted: bool,
+        merge_base: bool,
     ) -> Result<HashSet<AnchoredSystemPathBuf>, Error> {
         let turbo_root_relative_to_git_root = self.root.anchor(turbo_root)?;
         let pathspec = turbo_root_relative_to_git_root.as_str();
@@ -118,27 +126,23 @@ impl Git {
 
         let valid_from = self.resolve_base(from_commit)?;
 
-        if let Some(to_commit) = to_commit {
-            let output = self.execute_git_command(
-                &[
-                    "diff",
-                    "--name-only",
-                    &format!("{}...{}", valid_from, to_commit),
-                ],
-                pathspec,
-            )?;
-
-            self.add_files_from_stdout(&mut files, turbo_root, output);
+        let mut args = if let Some(to_commit) = to_commit {
+            vec!["diff", "--name-only", valid_from, to_commit]
         } else {
-            let output =
-                self.execute_git_command(&["diff", "--name-only", valid_from], pathspec)?;
+            vec!["diff", "--name-only", valid_from]
+        };
 
-            self.add_files_from_stdout(&mut files, turbo_root, output);
+        if merge_base {
+            args.push("--merge-base");
         }
 
+        let output = self.execute_git_command(&args, pathspec)?;
+        self.add_files_from_stdout(&mut files, turbo_root, output);
+
         // We only care about non-tracked files if we haven't specified both ends up the
-        // comparison or if we are using `--affected`
+        // comparison
         if include_uncommitted {
+            // Add untracked files, i.e. files that are not in git at all
             let output = self
                 .execute_git_command(&["ls-files", "--others", "--exclude-standard"], pathspec)?;
             self.add_files_from_stdout(&mut files, turbo_root, output);
@@ -281,12 +285,16 @@ mod tests {
         let scm = SCM::new(git_root);
 
         let turbo_root = AbsoluteSystemPathBuf::try_from(turbo_root.as_path())?;
+        // Replicating the `--filter` behavior where we only do a merge base
+        // if both ends of the git range are specified.
+        let merge_base = to_commit.is_some();
         let ChangedFiles::Some(files) = scm.changed_files(
             &turbo_root,
             from_commit,
             to_commit,
             include_uncommitted,
             false,
+            merge_base,
         )?
         else {
             unreachable!("changed_files should always return Some");
@@ -849,7 +857,7 @@ mod tests {
 
         let scm = SCM::new(&root);
         let actual = scm
-            .changed_files(&root, None, Some("HEAD"), true, true)
+            .changed_files(&root, None, Some("HEAD"), true, true, false)
             .unwrap();
 
         assert_matches!(actual, ChangedFiles::All);