feat(move): Add --fixup to squash moved commits into the destination

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

@@ -22,7 +22,7 @@ pub use reference::{
     Branch, BranchType, CategorizedReferenceName, Reference, ReferenceName, ReferenceTarget,
 };
 pub use repo::{
-    message_prettify, AmendFastOptions, CherryPickFastError, CherryPickFastOptions,
+    message_prettify, AmendFastOptions, CherryPickFastOptions, CreateCommitFastError,
     Error as RepoError, GitVersion, PatchId, Repo, ResolvedReferenceInfo, Result as RepoResult,
     Time,
 };

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

@@ -387,10 +387,11 @@ pub struct CherryPickFastOptions {
     pub reuse_parent_tree_if_possible: bool,
 }
 
-/// An error raised when attempting the `Repo::cherry_pick_fast` operation.
+/// An error raised when attempting to create create a commit via
+/// `Repo::cherry_pick_fast`.
 #[allow(missing_docs)]
 #[derive(Debug, Error)]
-pub enum CherryPickFastError {
+pub enum CreateCommitFastError {
     /// A merge conflict occurred, so the cherry-pick could not continue.
     #[error("merge conflict in {} paths", conflicting_paths.len())]
     MergeConflict {
@@ -426,7 +427,7 @@ pub enum CherryPickFastError {
 
 /// Options for `Repo::amend_fast`
 #[derive(Debug)]
-pub enum AmendFastOptions {
+pub enum AmendFastOptions<'repo> {
     /// Amend a set of paths from the current state of the working copy.
     FromWorkingCopy {
         /// The status entries for the files to amend.
@@ -437,14 +438,20 @@ pub enum AmendFastOptions {
         /// The paths to amend.
         paths: Vec<PathBuf>,
     },
+    /// Amend a set of paths from a different commit.
+    FromCommit {
+        /// The commit whose contents will be amended.
+        commit: Commit<'repo>,
+    },
 }
 
-impl AmendFastOptions {
+impl<'repo> AmendFastOptions<'repo> {
     /// Returns whether there are any paths to be amended.
     pub fn is_empty(&self) -> bool {
         match &self {
             AmendFastOptions::FromIndex { paths } => paths.is_empty(),
             AmendFastOptions::FromWorkingCopy { status_entries } => status_entries.is_empty(),
+            AmendFastOptions::FromCommit { commit } => commit.is_empty(),
         }
     }
 }
@@ -1214,7 +1221,7 @@ impl Repo {
         patch_commit: &'repo Commit,
         target_commit: &'repo Commit,
         options: &CherryPickFastOptions,
-    ) -> std::result::Result<Tree<'repo>, CherryPickFastError> {
+    ) -> std::result::Result<Tree<'repo>, CreateCommitFastError> {
         let CherryPickFastOptions {
             reuse_parent_tree_if_possible,
         } = options;
@@ -1233,7 +1240,7 @@ impl Repo {
 
         let changed_pathbufs = self
             .get_paths_touched_by_commit(patch_commit)?
-            .ok_or_else(|| CherryPickFastError::GetPatch {
+            .ok_or_else(|| CreateCommitFastError::GetPatch {
                 commit: patch_commit.get_oid(),
             })?
             .into_iter()
@@ -1252,37 +1259,37 @@ impl Repo {
                 let conflicting_paths = {
                     let mut result = HashSet::new();
                     for conflict in rebased_index.inner.conflicts().map_err(|err| {
-                        CherryPickFastError::GetConflicts {
+                        CreateCommitFastError::GetConflicts {
                             source: err,
                             commit: patch_commit.get_oid(),
                             onto: target_commit.get_oid(),
                         }
                     })? {
                         let conflict =
-                            conflict.map_err(|err| CherryPickFastError::GetConflicts {
+                            conflict.map_err(|err| CreateCommitFastError::GetConflicts {
                                 source: err,
                                 commit: patch_commit.get_oid(),
                                 onto: target_commit.get_oid(),
                             })?;
                         if let Some(ancestor) = conflict.ancestor {
                             result.insert(ancestor.path.into_path_buf().map_err(|err| {
-                                CherryPickFastError::DecodePath {
+                                CreateCommitFastError::DecodePath {
                                     source: err,
                                     item: "ancestor",
                                 }
                             })?);
                         }
                         if let Some(our) = conflict.our {
                             result.insert(our.path.into_path_buf().map_err(|err| {
-                                CherryPickFastError::DecodePath {
+                                CreateCommitFastError::DecodePath {
                                     source: err,
                                     item: "our",
                                 }
                             })?);
                         }
                         if let Some(their) = conflict.their {
                             result.insert(their.path.into_path_buf().map_err(|err| {
-                                CherryPickFastError::DecodePath {
+                                CreateCommitFastError::DecodePath {
                                     source: err,
                                     item: "their",
                                 }
@@ -1296,7 +1303,7 @@ impl Repo {
                     warn!("BUG: A merge conflict was detected, but there were no entries in `conflicting_paths`. Maybe the wrong index entry was used?")
                 }
 
-                return Err(CherryPickFastError::MergeConflict { conflicting_paths });
+                return Err(CreateCommitFastError::MergeConflict { conflicting_paths });
             }
             let rebased_entries: HashMap<PathBuf, Option<(NonZeroOid, FileMode)>> =
                 changed_pathbufs
@@ -1327,7 +1334,7 @@ impl Repo {
                     .collect();
             let rebased_tree_oid =
                 hydrate_tree(self, Some(&target_commit.get_tree()?), rebased_entries)
-                    .map_err(CherryPickFastError::HydrateTree)?;
+                    .map_err(CreateCommitFastError::HydrateTree)?;
             self.find_tree_or_fail(rebased_tree_oid)?
         };
         Ok(rebased_tree)
@@ -1421,10 +1428,14 @@ impl Repo {
     /// See `Repo::cherry_pick_fast` for motivation for performing the operation
     /// in-memory.
     #[instrument]
-    pub fn amend_fast(&self, parent_commit: &Commit, opts: &AmendFastOptions) -> Result<Tree> {
+    pub fn amend_fast(
+        &self,
+        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(|| Error::GetPatch {
+            .ok_or_else(|| CreateCommitFastError::GetPatch {
                 commit: parent_commit.get_oid(),
             })?
             .into_iter()
@@ -1438,6 +1449,11 @@ impl Repo {
                         result.extend(entry.paths().iter().cloned());
                     }
                 }
+                AmendFastOptions::FromCommit { commit } => {
+                    if let Some(paths) = self.get_paths_touched_by_commit(commit)? {
+                        result.extend(paths.iter().cloned());
+                    }
+                }
             };
             result.into_iter().collect_vec()
         };
@@ -1491,6 +1507,25 @@ impl Repo {
                     })
                     .collect::<HashMap<_, _>>()
             }
+            AmendFastOptions::FromCommit { commit } => {
+                let amended_tree = self.cherry_pick_fast(
+                    commit,
+                    parent_commit,
+                    &CherryPickFastOptions {
+                        reuse_parent_tree_if_possible: false,
+                    },
+                )?;
+                self.get_paths_touched_by_commit(commit)?
+                    .unwrap_or_default()
+                    .iter()
+                    .filter_map(|path| match amended_tree.get_path(path) {
+                        Ok(Some(entry)) => {
+                            Some((path.clone(), Some((entry.get_oid(), entry.get_filemode()))))
+                        }
+                        Ok(None) | Err(_) => None,
+                    })
+                    .collect::<HashMap<_, _>>()
+            }
         };
 
         // Merge the new path entries into the existing set of parent tree.

git-branchless-lib/tests/test_rewrite_plan.rs

@@ -655,6 +655,58 @@ fn test_plan_moving_subtree_with_merge_commit() -> eyre::Result<()> {
     Ok(())
 }
 
+#[test]
+fn test_plan_fixup_child_into_parent() -> eyre::Result<()> {
+    let git = make_git()?;
+    git.init_repo()?;
+    git.detach_head()?;
+    let _test1_oid = git.commit_file("test1", 1)?;
+    let test2_oid = git.commit_file("test2", 2)?;
+    let test3_oid = git.commit_file("test3", 3)?;
+
+    create_and_execute_plan(&git, move |builder: &mut RebasePlanBuilder| {
+        builder.fixup_commit(test3_oid, test2_oid)?;
+        Ok(())
+    })?;
+
+    let (stdout, _stderr) = git.run(&["smartlog"])?;
+    insta::assert_snapshot!(stdout, @r###"
+        O f777ecc (master) create initial.txt
+        |
+        o 62fc20d create test1.txt
+        |
+        @ 64da0f2 create test2.txt
+        "###);
+
+    Ok(())
+}
+
+#[test]
+fn test_plan_fixup_parent_into_child() -> eyre::Result<()> {
+    let git = make_git()?;
+    git.init_repo()?;
+    git.detach_head()?;
+    let _test1_oid = git.commit_file("test1", 1)?;
+    let test2_oid = git.commit_file("test2", 2)?;
+    let test3_oid = git.commit_file("test3", 3)?;
+
+    create_and_execute_plan(&git, move |builder: &mut RebasePlanBuilder| {
+        builder.fixup_commit(test2_oid, test3_oid)?;
+        Ok(())
+    })?;
+
+    let (stdout, _stderr) = git.run(&["smartlog"])?;
+    insta::assert_snapshot!(stdout, @r###"
+        O f777ecc (master) create initial.txt
+        |
+        o 62fc20d create test1.txt
+        |
+        @ 1f599a2 create test3.txt
+        "###);
+
+    Ok(())
+}
+
 /// Helper function to handle the boilerplate involved in creating, building
 /// and executing the rebase plan.
 fn create_and_execute_plan(

git-branchless-move/src/lib.rs

@@ -73,6 +73,7 @@ pub fn r#move(
     exacts: Vec<Revset>,
     resolve_revset_options: &ResolveRevsetOptions,
     move_options: &MoveOptions,
+    fixup: bool,
     insert: bool,
 ) -> EyreExitOr<()> {
     let sources_provided = !sources.is_empty();
@@ -278,7 +279,7 @@ pub fn r#move(
             let commits_to_move = commits_to_move.union(&union_all(
                 &exact_components.values().cloned().collect::<Vec<_>>(),
             ));
-            let commits_to_move = if insert {
+            let commits_to_move = if insert || fixup {
                 commits_to_move.union(&dag.query_children(CommitSet::from(dest_oid))?)
             } else {
                 commits_to_move
@@ -297,7 +298,15 @@ pub fn r#move(
 
         let source_roots = dag.query_roots(source_oids.clone())?;
         for source_root in dag.commit_set_to_vec(&source_roots)? {
-            builder.move_subtree(source_root, vec![dest_oid])?;
+            if fixup {
+                let commits = dag.query_descendants(CommitSet::from(source_root))?;
+                let commits = dag.commit_set_to_vec(&commits)?;
+                for commit in commits.iter() {
+                    builder.fixup_commit(*commit, dest_oid)?;
+                }
+            } else {
+                builder.move_subtree(source_root, vec![dest_oid])?;
+            }
         }
 
         let component_roots: CommitSet = exact_components.keys().cloned().collect();
@@ -394,7 +403,14 @@ pub fn r#move(
                 }
             }
 
-            builder.move_subtree(component_root, vec![component_dest_oid])?;
+            if fixup {
+                let commits = dag.commit_set_to_vec(component)?;
+                for commit in commits.iter() {
+                    builder.fixup_commit(*commit, dest_oid)?;
+                }
+            } else {
+                builder.move_subtree(component_root, vec![component_dest_oid])?;
+            }
         }
 
         if insert {

git-branchless-opts/src/lib.rs

@@ -513,6 +513,10 @@ pub enum Command {
         #[clap(flatten)]
         move_options: MoveOptions,
 
+        /// Combine the moved commits and squash them into the destination commit.
+        #[clap(action, short = 'F', long = "fixup", conflicts_with = "insert")]
+        fixup: bool,
+
         /// Insert the subtree between the destination and it's children, if any.
         /// Only supported if the moved subtree has a single head.
         #[clap(action, short = 'I', long = "insert")]

git-branchless-testing/src/lib.rs

@@ -485,19 +485,34 @@ then you can only run tests in the main `git-branchless` and \
         Ok(())
     }
 
-    /// Commit a file with default contents. The `time` argument is used to set
-    /// the commit timestamp, which is factored into the commit hash.
+    /// Get a diff of a commit with the a/b file header removed. Only works for commits
+    /// with a single file.
     #[instrument]
-    pub fn commit_file_with_contents(
+    pub fn get_trimmed_diff(&self, file: &str, commit: &str) -> eyre::Result<String> {
+        let (stdout, _stderr) = self.run(&["show", "--pretty=format:", commit])?;
+        let split_on = format!("+++ b/{file}\n");
+        match stdout.as_str().split_once(split_on.as_str()) {
+            Some((_, diff)) => Ok(diff.to_string()),
+            None => eyre::bail!("Error trimming diff. Could not split on '{split_on}'"),
+        }
+    }
+
+    /// Commit a file with given contents and message. The `time` argument is
+    /// used to set the commit timestamp, which is factored into the commit
+    /// hash. The filename is always appended to the message prefix.
+    #[instrument]
+    pub fn commit_file_with_contents_and_message(
         &self,
         name: &str,
         time: isize,
         contents: &str,
+        message_prefix: &str,
     ) -> eyre::Result<NonZeroOid> {
+        let message = format!("{message_prefix} {name}.txt");
         self.write_file_txt(name, contents)?;
         self.run(&["add", "."])?;
         self.run_with_options(
-            &["commit", "-m", &format!("create {name}.txt")],
+            &["commit", "-m", &message],
             &GitRunOptions {
                 time,
                 ..Default::default()
@@ -512,6 +527,19 @@ then you can only run tests in the main `git-branchless` and \
         Ok(oid)
     }
 
+    /// Commit a file with given contents and a default message. The `time`
+    /// argument is used to set the commit timestamp, which is factored into the
+    /// commit hash.
+    #[instrument]
+    pub fn commit_file_with_contents(
+        &self,
+        name: &str,
+        time: isize,
+        contents: &str,
+    ) -> eyre::Result<NonZeroOid> {
+        self.commit_file_with_contents_and_message(name, time, contents, "create")
+    }
+
     /// Commit a file with default contents. The `time` argument is used to set
     /// the commit timestamp, which is factored into the commit hash.
     pub fn commit_file(&self, name: &str, time: isize) -> eyre::Result<NonZeroOid> {

git-branchless/src/commands/amend.rs

@@ -373,6 +373,9 @@ pub fn amend(
                 "Amended with {uncommitted_changes}.",
             )?;
         }
+        AmendFastOptions::FromCommit { .. } => {
+            unreachable!("BUG: AmendFastOptions::FromCommit should not have been constructed.")
+        }
     }
 
     Ok(Ok(()))

git-branchless/src/commands/mod.rs

@@ -93,6 +93,7 @@ fn command_main(ctx: CommandContext, opts: Opts) -> EyreExitOr<()> {
             exact,
             resolve_revset_options,
             move_options,
+            fixup,
             insert,
         } => git_branchless_move::r#move(
             &effects,
@@ -103,6 +104,7 @@ fn command_main(ctx: CommandContext, opts: Opts) -> EyreExitOr<()> {
             exact,
             &resolve_revset_options,
             &move_options,
+            fixup,
             insert,
         )?,