fix(amend): support amending merge commits

Previously, `Repo::get_paths_touched_by_commit` returned `None` if the commit was a merge commit, the idea being that a merge commit doesn't have a single correct "patch". However, no call-site cared about this interpretation, and all call-sites worked just as well or better with the new interpretation, where we return the union of all changed paths with respect to all parents. Usually, the impact is that the dehydrated commit is a slightly larger commit than normal.

`git amend` was failing when `HEAD` was a merge commit for this reason, since the "fast amend" path first dehydrated the current commit. Before this commit, git-branchless would panic; after this commit, it successfully amends the merge commit.

Note that the `--merge` option to `git amend` is unrelated: it deals with restacking and merging descendant commits after the amend has been carried out.

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(())
+}