fix(move): Support on-disk rebases with move --fixup

Author: claytonrcarter

git-branchless-hook/src/lib.rs

@@ -576,9 +576,16 @@ pub fn command_main(ctx: CommandContext, args: HookArgs) -> EyreExitOr<()> {
             hook_register_extra_post_rewrite_hook()?;
         }
 
-        HookSubcommand::SkipUpstreamAppliedCommit { commit_oid } => {
+        HookSubcommand::SkipUpstreamAppliedCommit {
+            commit_oid,
+            rewritten_oid,
+        } => {
             let commit_oid: NonZeroOid = commit_oid.parse()?;
-            hook_skip_upstream_applied_commit(&effects, commit_oid)?;
+            let rewritten_oid: MaybeZeroOid = match rewritten_oid {
+                Some(rewritten_oid) => rewritten_oid.parse()?,
+                None => MaybeZeroOid::Zero,
+            };
+            hook_skip_upstream_applied_commit(&effects, commit_oid, rewritten_oid)?;
         }
     }
 

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

@@ -148,7 +148,69 @@ impl ToString for RebaseCommand {
             } => match commits_to_apply_oids.as_slice() {
                 [] => String::new(),
                 [commit_oid] => format!("pick {commit_oid}"),
-                [..] => unimplemented!("On-disk fixups"),
+                [pick_oid, fixup_oids @ ..] => {
+                    let mut picks = vec![format!("pick {pick_oid}")];
+                    let mut fixups = fixup_oids
+                        .iter()
+                        .map(|oid| format!("fixup {oid}"))
+                        .collect::<Vec<String>>();
+                    let mut cleanups = vec![];
+
+                    // Since 0ca8681, the intermediate commits created as each
+                    // fixup is applied are left behind in the smartlog. This
+                    // forcibly removes all but the last of them. I don't
+                    // understand why this happens during `git branchless`
+                    // initiated rebases, but not during "normal" fixup rebases,
+                    // but this makes these artifacts go away.
+                    //
+                    // Re implementation: `intersperse()` would be ideal here,
+                    // but it's only in the nightly API.
+                    if fixups.len() > 1 {
+                        for i in 0..(fixups.len() - 1) {
+                            fixups.insert(
+                                i*2 + 1,
+                                // FIXME I'm assuming that $(...) is not portable and will need to be changed
+                                "exec git branchless hook-skip-upstream-applied-commit $(git rev-parse HEAD)".to_string()
+                            )
+                        }
+                    }
+
+                    // If the destination commit (ie `original_commit_oid`) does
+                    // not come first topologically among the commits being
+                    // rebased, then the final squashed commit will end up with
+                    // the wrong metadata. (It will end up with the metadata
+                    // from the commit that *does* come first, ie `pick_oid`.)
+                    // We have to add some additional steps to make sure the
+                    // smartlog and commit metadata are left as the user
+                    // expects.
+                    if pick_oid != original_commit_oid {
+                        // See above comment related to 0ca8681
+                        picks.insert(
+                            1,
+                            "exec git branchless hook-skip-upstream-applied-commit $(git rev-parse HEAD)".to_string()
+                        );
+
+                        cleanups = vec![
+                            // Hide the final squashed commit
+                            "exec git branchless hook-skip-upstream-applied-commit $(git rev-parse HEAD)".to_string(),
+
+                            // Create a new final commit by applying the
+                            // metadata from the destination commit to the (now
+                            // hidden) new commit.
+                            format!("exec git commit --amend --no-edit --reuse-message {original_commit_oid}"),
+
+                            // register the new commit as the rewritten version
+                            // of original_commit_oid
+                            format!("exec git branchless hook-skip-upstream-applied-commit {original_commit_oid} $(git rev-parse HEAD)")
+                        ];
+                    }
+
+                    picks
+                        .iter()
+                        .chain(fixups.iter())
+                        .chain(cleanups.iter())
+                        .join("\n")
+                }
             },
             RebaseCommand::Merge {
                 commit_oid,
@@ -1169,9 +1231,18 @@ impl<'a> RebasePlanBuilder<'a> {
             };
             let first_parent_oid = *parent_oids.first().unwrap();
             first_dest_oid.get_or_insert(first_parent_oid);
-            acc.push(RebaseCommand::Reset {
-                target: OidOrLabel::Oid(first_parent_oid),
-            });
+            // FIXME this may be necessary in some fixup cases, but feels drastic
+            // I *think* it makes sense, though: if we're building from roots down (roots out?)
+            // then parents will be (should be) dealt with first anyway ... no maybe it's OK?
+            if !state
+                .constraints
+                .commits_to_move()
+                .contains(&first_parent_oid)
+            {
+                acc.push(RebaseCommand::Reset {
+                    target: OidOrLabel::Oid(first_parent_oid),
+                });
+            }
 
             let upstream_patch_ids = if *detect_duplicate_commits_via_patch_id {
                 let (effects, _progress) =

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

@@ -516,11 +516,12 @@ pub fn hook_drop_commit_if_empty(
     Ok(())
 }
 
-/// For rebases, if a commit is known to have been applied upstream, skip it
+/// TODO For rebases, if a commit is known to have been applied upstream, skip it
 /// without attempting to apply it.
 pub fn hook_skip_upstream_applied_commit(
     effects: &Effects,
     commit_oid: NonZeroOid,
+    rewritten_oid: MaybeZeroOid,
 ) -> eyre::Result<()> {
     let repo = Repo::from_current_dir()?;
     let commit = repo.find_commit_or_fail(commit_oid)?;
@@ -546,7 +547,7 @@ pub fn hook_skip_upstream_applied_commit(
     add_rewritten_list_entries(
         &repo.get_tempfile_dir(),
         &repo.get_rebase_state_dir_path().join("rewritten-list"),
-        &[(commit_oid, MaybeZeroOid::Zero)],
+        &[(commit_oid, rewritten_oid)],
     )?;
 
     Ok(())

git-branchless-opts/src/lib.rs

@@ -232,6 +232,10 @@ pub enum HookSubcommand {
         /// The OID of the commit that was skipped.
         #[clap(value_parser)]
         commit_oid: String,
+
+        /// FIXME The (optional) OID of the commit that was skipped.
+        #[clap(value_parser)]
+        rewritten_oid: Option<String>,
     },
 }