feat(reword): support rewording merge commits

We don't support rebasing merge commits in-memory at present because we don't have a method for resolving conflicts that might arise as part of the attempt to create a new merge commit. Consequently, `git reword` also doesn't work on merge commits, since it uses the rebase machinery. Since `git reword` is guaranteed not to change the contents (or topology) of any of the reworded commits, we can make an exception for this case, and permit rebasing merge commits in memory when a replacement commit is specified, because that guarantees that we won't have to resolve merge conflicts.

Author: arxanas

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

@@ -354,7 +354,8 @@ mod in_memory {
     use crate::core::rewrite::move_branches;
     use crate::core::rewrite::plan::{OidOrLabel, RebaseCommand, RebasePlan};
     use crate::git::{
-        CherryPickFastError, CherryPickFastOptions, GitRunInfo, MaybeZeroOid, NonZeroOid, Repo,
+        CherryPickFastError, CherryPickFastOptions, GitRunInfo, MaybeZeroOid, NonZeroOid,
+        ReferenceName, Repo,
     };
     use crate::util::ExitCode;
 
@@ -390,10 +391,18 @@ mod in_memory {
                 .iter()
                 .find_map(|command| match command {
                     RebaseCommand::Merge {
+                        replacement_commit_oid: None,
                         commit_oid,
                         commits_to_merge: _,
                     } => Some(commit_oid),
-                    RebaseCommand::CreateLabel { .. }
+                    RebaseCommand::Merge {
+                        // If we know exactly what tree to use, we don't
+                        // actually have to carry out a merge in memory.
+                        replacement_commit_oid: Some(_),
+                        commit_oid: _,
+                        commits_to_merge: _,
+                    }
+                    | RebaseCommand::CreateLabel { .. }
                     | RebaseCommand::Reset { .. }
                     | RebaseCommand::Pick { .. }
                     | RebaseCommand::RegisterExtraPostRewriteHook
@@ -585,18 +594,109 @@ mod in_memory {
                 }
 
                 RebaseCommand::Merge {
+                    replacement_commit_oid: None,
                     commit_oid,
                     commits_to_merge: _,
                 } => {
                     warn!(
                         ?commit_oid,
-                        "BUG: Merge commit should have been detected when starting in-memory rebase"
+                        "BUG: Merge commit without replacement should have been detected when starting in-memory rebase"
                     );
                     return Ok(RebaseInMemoryResult::CannotRebaseMergeCommit {
                         commit_oid: *commit_oid,
                     });
                 }
 
+                RebaseCommand::Merge {
+                    replacement_commit_oid: Some(replacement_commit_oid),
+                    commit_oid,
+                    commits_to_merge,
+                } => {
+                    let current_commit = repo
+                        .find_commit_or_fail(current_oid)
+                        .wrap_err("Finding current commit")?;
+                    let commit_to_apply = repo
+                        .find_commit_or_fail(*replacement_commit_oid)
+                        .wrap_err("Finding commit to apply")?;
+                    i += 1;
+
+                    let commit_description = printable_styled_string(
+                        effects.get_glyphs(),
+                        commit_to_apply.friendly_describe(effects.get_glyphs())?,
+                    )?;
+                    let commit_num = format!("[{}/{}]", i, num_picks);
+                    progress.notify_progress(i, num_picks);
+
+                    progress
+                        .notify_status(format!("Applying merge commit: {}", commit_description));
+                    let commit = repo.find_commit_or_fail(*replacement_commit_oid)?;
+                    let commit_tree = commit.get_tree()?;
+                    let commit_message = commit_to_apply.get_message_raw()?;
+                    let commit_message = commit_message.to_str().with_context(|| {
+                        eyre::eyre!(
+                            "Could not decode commit message for commit: {:?}",
+                            replacement_commit_oid,
+                        )
+                    })?;
+
+                    progress
+                        .notify_status(format!("Committing to repository: {}", commit_description));
+                    let committer_signature = if *preserve_timestamps {
+                        commit_to_apply.get_committer()
+                    } else {
+                        commit_to_apply.get_committer().update_timestamp(*now)?
+                    };
+                    let parents = {
+                        let mut result = vec![current_commit];
+                        for parent in commits_to_merge {
+                            let parent = match parent {
+                                OidOrLabel::Oid(oid) => repo.find_commit_or_fail(*oid)?,
+                                OidOrLabel::Label(label) => {
+                                    let reference = repo
+                                        .find_reference(&ReferenceName::from(label.clone()))?
+                                        .ok_or_else(|| {
+                                            eyre::eyre!("Could not find label {label}")
+                                        })?;
+                                    reference.peel_to_commit()?.ok_or_else(|| {
+                                        eyre::eyre!(
+                                            "Reference {label} could not be resolved to a commit"
+                                        )
+                                    })?
+                                }
+                            };
+                            result.push(parent);
+                        }
+                        result
+                    };
+                    let rebased_commit_oid = repo
+                        .create_commit(
+                            None,
+                            &commit_to_apply.get_author(),
+                            &committer_signature,
+                            commit_message,
+                            &commit_tree,
+                            parents.iter().collect(),
+                        )
+                        .wrap_err("Applying rebased commit")?;
+
+                    let commit_description = printable_styled_string(
+                        effects.get_glyphs(),
+                        repo.friendly_describe_commit_from_oid(
+                            effects.get_glyphs(),
+                            rebased_commit_oid,
+                        )?,
+                    )?;
+                    rewritten_oids.push((*commit_oid, MaybeZeroOid::NonZero(rebased_commit_oid)));
+                    current_oid = rebased_commit_oid;
+
+                    writeln!(
+                        effects.get_output_stream(),
+                        "{} Committed as: {}",
+                        commit_num,
+                        commit_description
+                    )?;
+                }
+
                 RebaseCommand::SkipUpstreamAppliedCommit { commit_oid } => {
                     i += 1;
                     let commit_num = format!("[{}/{}]", i, num_picks);

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

@@ -1,5 +1,5 @@
 use std::collections::{HashMap, HashSet};
-use std::fmt::Write;
+use std::fmt::{Display, Write};
 use std::ops::Sub;
 use std::path::PathBuf;
 use std::sync::Arc;
@@ -24,11 +24,11 @@ pub enum OidOrLabel {
     Label(String),
 }
 
-impl ToString for OidOrLabel {
-    fn to_string(&self) -> String {
+impl Display for OidOrLabel {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
-            Self::Oid(oid) => oid.to_string(),
-            Self::Label(label) => label.clone(),
+            OidOrLabel::Oid(oid) => write!(f, "{oid}"),
+            OidOrLabel::Label(label) => write!(f, "{label}"),
         }
     }
 }
@@ -51,7 +51,11 @@ pub enum RebaseCommand {
     },
 
     Merge {
-        /// The original merge commit to copy the commit message from.
+        /// If specified, the new merge commit will use this commit's message
+        /// and tree, rather than attempting a new merge.
+        replacement_commit_oid: Option<NonZeroOid>,
+
+        /// The original merge commit to copy the commit contents from.
         commit_oid: NonZeroOid,
 
         /// The other commits to merge into this one. This may be a list of
@@ -86,12 +90,13 @@ impl ToString for RebaseCommand {
     fn to_string(&self) -> String {
         match self {
             RebaseCommand::CreateLabel { label_name } => format!("label {}", label_name),
-            RebaseCommand::Reset { target } => format!("reset {}", target.to_string()),
+            RebaseCommand::Reset { target } => format!("reset {}", target),
             RebaseCommand::Pick {
                 original_commit_oid: _,
                 commit_to_apply_oid: commit_oid,
             } => format!("pick {}", commit_oid),
             RebaseCommand::Merge {
+                replacement_commit_oid: None,
                 commit_oid,
                 commits_to_merge,
             } => format!(
@@ -102,6 +107,21 @@ impl ToString for RebaseCommand {
                     .map(|commit_id| commit_id.to_string())
                     .join(" ")
             ),
+            RebaseCommand::Merge {
+                replacement_commit_oid: Some(replacement_commit_oid),
+                commit_oid,
+                commits_to_merge,
+            } => {
+                let other_parents = commits_to_merge
+                    .iter()
+                    .map(|parent| format!("-p {parent}"))
+                    .join(" ");
+                // FIXME: Untested, this probably doesn't work. Currently,
+                // on-disk rebases for merges with replacement commits are not
+                // generated by any command, so this code path is never
+                // triggered.
+                format!("exec git show -s --format=%B {replacement_commit_oid} | git commit-tree -p {commit_oid} {other_parents} {replacement_commit_oid}^{{tree}}")
+            }
             RebaseCommand::RegisterExtraPostRewriteHook => {
                 "exec git branchless hook-register-extra-post-rewrite-hook".to_string()
             }
@@ -728,11 +748,36 @@ impl<'a> RebasePlanBuilder<'a> {
 
                 match commits_to_merge {
                     Some(commits_to_merge) => {
-                        // All parents have been committed.
-                        acc.push(RebaseCommand::Merge {
-                            commit_oid: current_commit.get_oid(),
-                            commits_to_merge,
-                        });
+                        if acc.iter().any(|command| {
+                            matches!(command,
+                            RebaseCommand::Merge {
+                                replacement_commit_oid: _,
+                                commit_oid,
+                                commits_to_merge: _,
+                            } if *commit_oid == current_commit.get_oid())
+                        }) {
+                            // Above, we've checked to make sure that all parent
+                            // commits which are being moved have had their
+                            // labels applied. However, if the commit has
+                            // multiple parents which were *not* moved, then we
+                            // would visit this commit and attempt to reapply it
+                            // multiple times. The simplest fix is to not
+                            // reapply merge commits which have already been
+                            // applied.
+                            //
+                            // FIXME: O(n^2) algorithm.
+                            return Ok(acc);
+                        } else {
+                            // All parents have been committed.
+                            acc.push(RebaseCommand::Merge {
+                                replacement_commit_oid: self
+                                    .replacement_commits
+                                    .get(&current_commit.get_oid())
+                                    .copied(),
+                                commit_oid: current_commit.get_oid(),
+                                commits_to_merge,
+                            });
+                        }
                     }
 
                     None => {
@@ -1025,6 +1070,7 @@ impl<'a> RebasePlanBuilder<'a> {
                     commit_to_apply_oid: commit_oid,
                 }
                 | RebaseCommand::Merge {
+                    replacement_commit_oid: _,
                     commit_oid,
                     commits_to_merge: _,
                 }

git-branchless/src/commands/reword.rs

@@ -186,37 +186,17 @@ pub fn reword(
         }
     };
 
-    let subtree_roots = find_subtree_roots(&repo, &dag, &commits)?;
-
     let rebase_plan = {
         let pool = ThreadPoolBuilder::new().build()?;
         let repo_pool = RepoResource::new_pool(&repo)?;
         let mut builder = RebasePlanBuilder::new(&dag, permissions);
 
-        for root_commit in subtree_roots {
-            let only_parent_id = root_commit.get_only_parent().map(|parent| parent.get_oid());
-            let only_parent_id = match only_parent_id {
-                Some(only_parent_id) => only_parent_id,
-                None => {
-                    writeln!(
-                        effects.get_error_stream(),
-                        "Refusing to reword commit {}, which has {} parents.\n\
-                        Rewording is only supported for commits with 1 parent.\n\
-                        Aborting.",
-                        root_commit.get_oid(),
-                        root_commit.get_parents().len(),
-                    )?;
-                    return Ok(ExitCode(1));
-                }
-            };
-            builder.move_subtree(root_commit.get_oid(), vec![only_parent_id])?;
-        }
-
         for commit in commits.iter() {
             let message = messages.get(&commit.get_oid()).unwrap();
             // This looks funny, but just means "leave everything but the message as is"
             let replacement_oid =
                 commit.amend_commit(None, None, None, Some(message.as_str()), None)?;
+            builder.move_subtree(commit.get_oid(), commit.get_parent_oids())?;
             builder.replace_commit(commit.get_oid(), replacement_oid)?;
         }
 
@@ -266,13 +246,7 @@ pub fn reword(
         }
         ExecuteRebasePlanResult::Succeeded {
             rewritten_oids: None,
-        } => {
-            writeln!(
-                effects.get_error_stream(),
-                "BUG: Succeeded rewording commits via on-disk rebase? But reword should be rebasing in-memory!"
-            )?;
-            ExitCode(1)
-        }
+        } => ExitCode(0),
         ExecuteRebasePlanResult::DeclinedToMerge { merge_conflict: _ } => {
             writeln!(
                 effects.get_error_stream(),

git-branchless/tests/command/test_move.rs

@@ -3625,6 +3625,7 @@ fn test_move_merge_commit() -> eyre::Result<()> {
                             label_name: "merge-parent-3",
                         },
                         Merge {
+                            replacement_commit_oid: None,
                             commit_oid: NonZeroOid(28790c73f13f38ce0d3beb6cfeb2d818b32bcd09),
                             commits_to_merge: [
                                 Oid(

git-branchless/tests/command/test_reword.rs

@@ -334,3 +334,73 @@ fn test_reword_exit_early_public_commit() -> eyre::Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn test_reword_merge_commit() -> eyre::Result<()> {
+    let git = make_git()?;
+
+    if !git.supports_reference_transactions()? {
+        return Ok(());
+    }
+    git.init_repo()?;
+
+    git.commit_file("test1", 1)?;
+    git.detach_head()?;
+    let test2_oid = git.commit_file("test2", 2)?;
+    git.run(&["checkout", "HEAD^"])?;
+    git.commit_file("test3", 3)?;
+    git.run(&["merge", &test2_oid.to_string()])?;
+
+    {
+        let (stdout, _stderr) = git.run(&["smartlog"])?;
+        insta::assert_snapshot!(stdout, @r###"
+        :
+        O 62fc20d (master) create test1.txt
+        |\
+        | o 96d1c37 create test2.txt
+        | |
+        | @ a4dd9b0 Merge commit '96d1c37a3d4363611c49f7e52186e189a04c531f' into HEAD
+        |
+        o 4838e49 create test3.txt
+        |
+        @ a4dd9b0 Merge commit '96d1c37a3d4363611c49f7e52186e189a04c531f' into HEAD
+        "###);
+    }
+
+    {
+        let (stdout, stderr) = git.run(&["reword", "-m", "new message"])?;
+        insta::assert_snapshot!(stderr, @r###"
+        branchless: creating working copy snapshot
+        Previous HEAD position was a4dd9b0 Merge commit '96d1c37a3d4363611c49f7e52186e189a04c531f' into HEAD
+        branchless: processing 1 update: ref HEAD
+        HEAD is now at 2fc54bd new message
+        branchless: processing checkout
+        "###);
+        insta::assert_snapshot!(stdout, @r###"
+        Attempting rebase in-memory...
+        [1/1] Committed as: 2fc54bd new message
+        branchless: processing 1 rewritten commit
+        branchless: running command: <git-executable> checkout 2fc54bd59c79078e6d9012df241bcc90f0199596
+        In-memory rebase succeeded.
+        Reworded commit a4dd9b0 as 2fc54bd new message
+        "###);
+    }
+
+    {
+        let (stdout, _stderr) = git.run(&["smartlog"])?;
+        insta::assert_snapshot!(stdout, @r###"
+        :
+        O 62fc20d (master) create test1.txt
+        |\
+        | o 96d1c37 create test2.txt
+        | |
+        | @ 2fc54bd new message
+        |
+        o 4838e49 create test3.txt
+        |
+        @ 2fc54bd new message
+        "###);
+    }
+
+    Ok(())
+}