fix(amend): support amending merge commits

git-branchless-lib/benches/benches.rs

@@ -157,9 +157,7 @@ fn bench_get_paths_touched_by_commits(c: &mut Criterion) {
         let oid = repo.get_head_info().unwrap().oid.unwrap();
         let commit = repo.find_commit_or_fail(oid).unwrap();
 
-        b.iter(|| -> Option<HashSet<PathBuf>> {
-            repo.get_paths_touched_by_commit(&commit).unwrap()
-        });
+        b.iter(|| -> HashSet<PathBuf> { repo.get_paths_touched_by_commit(&commit).unwrap() });
     });
 }
 

git-branchless-lib/src/core/check_out.rs

@@ -296,10 +296,7 @@ pub fn restore_snapshot(
             (Stage::Stage2, &snapshot.commit_stage2),
             (Stage::Stage3, &snapshot.commit_stage3),
         ] {
-            let changed_paths = match repo.get_paths_touched_by_commit(commit)? {
-                Some(changed_paths) => changed_paths,
-                None => continue,
-            };
+            let changed_paths = repo.get_paths_touched_by_commit(commit)?;
             for path in changed_paths {
                 let tree = commit.get_tree()?;
                 let tree_entry = tree.get_path(&path)?;

git-branchless-lib/src/core/rewrite/mod.rs

@@ -83,7 +83,7 @@ pub mod testing {
     /// Get the internal touched paths cache for a `RebasePlanBuilder`.
     pub fn get_builder_touched_paths_cache<'a>(
         builder: &'a RebasePlanBuilder,
-    ) -> &'a CHashMap<NonZeroOid, Option<HashSet<PathBuf>>> {
+    ) -> &'a CHashMap<NonZeroOid, HashSet<PathBuf>> {
         &builder.touched_paths_cache
     }
 }

git-branchless-lib/src/core/rewrite/plan.rs

@@ -568,7 +568,7 @@ pub struct RebasePlanBuilder<'a> {
     /// Cache mapping from commit OID to the paths changed in the diff for that
     /// commit. The value is `None` if the commit doesn't have an associated
     /// diff (i.e. is a merge commit).
-    pub(crate) touched_paths_cache: Arc<CHashMap<NonZeroOid, Option<HashSet<PathBuf>>>>,
+    pub(crate) touched_paths_cache: Arc<CHashMap<NonZeroOid, HashSet<PathBuf>>>,
 }
 
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
@@ -1356,7 +1356,6 @@ impl<'a> RebasePlanBuilder<'a> {
         let local_touched_paths: Vec<HashSet<PathBuf>> = touched_commits
             .into_iter()
             .map(|commit| repo.get_paths_touched_by_commit(&commit))
-            .filter_map(|x| x.transpose())
             .try_collect()?;
 
         let filtered_path = {
@@ -1435,19 +1434,14 @@ impl<'a> RebasePlanBuilder<'a> {
     }
 
     fn should_check_patch_id(
-        upstream_touched_paths: &Option<HashSet<PathBuf>>,
+        upstream_touched_paths: &HashSet<PathBuf>,
         local_touched_paths: &[HashSet<PathBuf>],
     ) -> bool {
-        match upstream_touched_paths {
-            Some(upstream_touched_paths) => {
-                // It's possible that the same commit was applied after a parent
-                // commit renamed a certain path. In that case, this check won't
-                // trigger. We'll rely on the empty-commit check after the
-                // commit has been made to deduplicate the commit in that case.
-                // FIXME: this code path could be optimized further.
-                local_touched_paths.iter().contains(upstream_touched_paths)
-            }
-            None => true,
-        }
+        // It's possible that the same commit was applied after a parent
+        // commit renamed a certain path. In that case, this check won't
+        // trigger. We'll rely on the empty-commit check after the
+        // commit has been made to deduplicate the commit in that case.
+        // FIXME: this code path could be optimized further.
+        local_touched_paths.iter().contains(upstream_touched_paths)
     }
 }

git-branchless-lib/src/git/repo.rs

@@ -399,9 +399,6 @@ pub enum CreateCommitFastError {
         conflicting_paths: HashSet<PathBuf>,
     },
 
-    #[error("could not get paths touched by commit {commit}")]
-    GetPatch { commit: NonZeroOid },
-
     #[error("could not get conflicts generated by cherry-pick of {commit} onto {onto}: {source}")]
     GetConflicts {
         source: git2::Error,
@@ -743,24 +740,22 @@ impl Repo {
     /// If the commit has more than one parent, returns `None`.
     #[instrument]
     pub fn get_patch_for_commit(&self, effects: &Effects, commit: &Commit) -> Result<Option<Diff>> {
-        let changed_paths = match self.get_paths_touched_by_commit(commit)? {
-            None => return Ok(None),
-            Some(changed_paths) => changed_paths,
-        };
+        let changed_paths = self.get_paths_touched_by_commit(commit)?;
         let dehydrated_commit = self.dehydrate_commit(
             commit,
             changed_paths
                 .iter()
-                .map(|x| -> &Path { x })
+                .map(|x| x.as_path())
                 .collect_vec()
                 .as_slice(),
             true,
         )?;
 
-        let parent = dehydrated_commit.get_only_parent();
-        let parent_tree = match &parent {
-            Some(parent) => Some(parent.get_tree()?),
-            None => None,
+        let parent = dehydrated_commit.get_parents();
+        let parent_tree = match parent.as_slice() {
+            [] => None,
+            [parent] => Some(parent.get_tree()?),
+            [..] => return Ok(None),
         };
         let current_tree = dehydrated_commit.get_tree()?;
         let diff = self.get_diff_between_trees(effects, parent_tree.as_ref(), &current_tree, 3)?;
@@ -834,21 +829,27 @@ impl Repo {
     /// If the commit has no parents, returns all of the file paths in that
     /// commit's tree.
     ///
-    /// If the commit has more than one parent, returns `None`.
+    /// If the commit has more than one parent, returns all file paths changed
+    /// with respect to any parent.
     #[instrument]
-    pub fn get_paths_touched_by_commit(&self, commit: &Commit) -> Result<Option<HashSet<PathBuf>>> {
+    pub fn get_paths_touched_by_commit(&self, commit: &Commit) -> Result<HashSet<PathBuf>> {
+        let current_tree = commit.get_tree()?;
         let parent_commits = commit.get_parents();
-        let parent_tree = match parent_commits.as_slice() {
-            [] => None,
-            [only_parent] => Some(only_parent.get_tree()?),
-            [..] => return Ok(None),
+        let changed_paths = if parent_commits.is_empty() {
+            get_changed_paths_between_trees(self, None, Some(&current_tree))
+                .map_err(Error::GetChangedPaths)?
+        } else {
+            let mut result: HashSet<PathBuf> = Default::default();
+            for parent_commit in parent_commits {
+                let parent_tree = parent_commit.get_tree()?;
+                let changed_paths =
+                    get_changed_paths_between_trees(self, Some(&parent_tree), Some(&current_tree))
+                        .map_err(Error::GetChangedPaths)?;
+                result.extend(changed_paths);
+            }
+            result
         };
-
-        let current_tree = commit.get_tree()?;
-        let changed_paths =
-            get_changed_paths_between_trees(self, parent_tree.as_ref(), Some(&current_tree))
-                .map_err(Error::GetChangedPaths)?;
-        Ok(Some(changed_paths))
+        Ok(changed_paths)
     }
 
     /// Get the patch ID for this commit.
@@ -1240,9 +1241,6 @@ impl Repo {
 
         let changed_pathbufs = self
             .get_paths_touched_by_commit(patch_commit)?
-            .ok_or_else(|| CreateCommitFastError::GetPatch {
-                commit: patch_commit.get_oid(),
-            })?
             .into_iter()
             .collect_vec();
         let changed_paths = changed_pathbufs.iter().map(PathBuf::borrow).collect_vec();
@@ -1433,15 +1431,8 @@ impl Repo {
         parent_commit: &Commit,
         opts: &AmendFastOptions,
     ) -> std::result::Result<Tree, CreateCommitFastError> {
-        let parent_commit_pathbufs = self
-            .get_paths_touched_by_commit(parent_commit)?
-            .ok_or_else(|| CreateCommitFastError::GetPatch {
-                commit: parent_commit.get_oid(),
-            })?
-            .into_iter()
-            .collect_vec();
         let changed_paths: Vec<PathBuf> = {
-            let mut result: HashSet<PathBuf> = parent_commit_pathbufs.into_iter().collect();
+            let mut result = self.get_paths_touched_by_commit(parent_commit)?;
             match opts {
                 AmendFastOptions::FromIndex { paths } => result.extend(paths.iter().cloned()),
                 AmendFastOptions::FromWorkingCopy { ref status_entries } => {
@@ -1450,9 +1441,7 @@ impl Repo {
                     }
                 }
                 AmendFastOptions::FromCommit { commit } => {
-                    if let Some(paths) = self.get_paths_touched_by_commit(commit)? {
-                        result.extend(paths.iter().cloned());
-                    }
+                    result.extend(self.get_paths_touched_by_commit(commit)?);
                 }
             };
             result.into_iter().collect_vec()
@@ -1516,7 +1505,6 @@ impl Repo {
                     },
                 )?;
                 self.get_paths_touched_by_commit(commit)?
-                    .unwrap_or_default()
                     .iter()
                     .filter_map(|path| match amended_tree.get_path(path) {
                         Ok(Some(entry)) => {

git-branchless-revset/src/builtins.rs

@@ -338,17 +338,9 @@ fn fn_path_changed(ctx: &mut Context, name: &str, args: &[Expr]) -> EvalResult {
         name,
         args,
         Box::new(move |repo: &Repo, commit: &Commit| {
-            let touched_paths = match repo
+            let touched_paths = repo
                 .get_paths_touched_by_commit(commit)
-                .map_err(PatternError::Repo)?
-            {
-                Some(touched_paths) => touched_paths,
-                None => {
-                    // FIXME: it might be more intuitive to check all changed
-                    // paths with respect to any parent.
-                    return Ok(false);
-                }
-            };
+                .map_err(PatternError::Repo)?;
             let result = touched_paths.into_iter().any(|path| {
                 let path = match path.to_str() {
                     Some(path) => path,

git-branchless/tests/test_amend.rs

@@ -1135,3 +1135,66 @@ fn test_amend_move_detached_branch() -> eyre::Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn test_amend_merge_commit() -> eyre::Result<()> {
+    let git = make_git()?;
+
+    if !git.supports_reference_transactions()? {
+        return Ok(());
+    }
+    git.init_repo()?;
+    git.detach_head()?;
+    let test1_oid = git.commit_file("test1", 1)?;
+    git.run(&["checkout", "HEAD^"])?;
+    git.commit_file("test2", 2)?;
+    git.run(&["merge", &test1_oid.to_string()])?;
+
+    git.write_file_txt("test1", "new test1 contents\n")?;
+    {
+        let (stdout, _stderr) = git.branchless("amend", &[])?;
+        insta::assert_snapshot!(stdout, @r###"
+        branchless: running command: <git-executable> reset 3ebbc8fdaff7b5d5d0f1101feb3640d06b0297a2
+        Amended with 1 uncommitted change.
+        "###);
+    }
+
+    {
+        let stdout = git.smartlog()?;
+        insta::assert_snapshot!(stdout, @r###"
+        O f777ecc (master) create initial.txt
+        |\
+        | o 62fc20d create test1.txt
+        | & (merge) 3ebbc8f Merge commit '62fc20d2a290daea0d52bdc2ed2ad4be6491010e' into HEAD
+        |
+        o fe65c1f create test2.txt
+        |
+        | & (merge) 62fc20d create test1.txt
+        |/
+        @ 3ebbc8f Merge commit '62fc20d2a290daea0d52bdc2ed2ad4be6491010e' into HEAD
+        "###);
+    }
+
+    {
+        let (stdout, _stderr) = git.run(&["show"])?;
+        insta::assert_snapshot!(stdout, @r###"
+        commit 3ebbc8fdaff7b5d5d0f1101feb3640d06b0297a2
+        Merge: fe65c1f 62fc20d
+        Author: Testy McTestface <[email protected]>
+        Date:   Thu Oct 29 12:34:56 2020 +0000
+
+            Merge commit '62fc20d2a290daea0d52bdc2ed2ad4be6491010e' into HEAD
+
+        diff --cc test1.txt
+        index 0000000,7432a8f..2121042
+        mode 000000,100644..100644
+        --- a/test1.txt
+        +++ b/test1.txt
+        @@@ -1,0 -1,1 +1,1 @@@
+         -test1 contents
+        ++new test1 contents
+        "###);
+    }
+
+    Ok(())
+}