fix(move --fixup): Try to get on disk fixups working

Author: claytonrcarter

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

@@ -132,28 +132,35 @@ impl ToString for RebaseCommand {
                 commits_to_apply_oids,
             } => match commits_to_apply_oids.as_slice() {
                 [] => String::new(),
-                [commit_oid] => format!("pick {}", commit_oid),
-                commit_oids => {
-                    // FIXME ondisk rebases are more likely to cause merge
-                    // conflicts if an ancestor fixup commit has a hunk that
-                    // intersects with the dest/target commit. (See
-                    // `test_move_fixup_ancestor_into_head`) This code extracts
-                    // the dest commit as the `pick` and remaining commits are
-                    // `fixup`s. One possible solution would be to just use the
-                    // first commit as the `pick` to preserve the application
-                    // order of the commits/hunks, then to issue a final rebase
-                    // command to clean it all up, eg
-                    // `git commit --amend --no-edit --reuse-message`
-                    // I attempted this, but it resulted in unexpected commit
-                    // graphs and empty commits.
-                    let fixups: String = commit_oids
-                        .iter()
-                        .filter(|oid| oid != &original_commit_oid)
-                        .map(|oid| format!("fixup {}\n", oid))
-                        .collect();
-                    format!("pick {}\n{}", original_commit_oid, fixups)
-                        .trim_end()
-                        .to_string()
+                [commit_oid] => format!("pick {commit_oid}"),
+                [pick_oid, fixup_oids @ ..] => {
+                    // The base pick+fixup plan. This will be sufficient if we
+                    // are squashing all of the commits into original_commit_oid
+                    let mut plan = vec![
+                        format!("pick {pick_oid}"),
+                        fixup_oids
+                            .iter()
+                            .map(|oid| format!("fixup {}\n", oid))
+                            .collect(),
+                    ];
+
+                    if pick_oid != original_commit_oid {
+                        // We aren't squashing into original_commit_oid, so the
+                        // squashed commit will have the wrong metadata. We need
+                        // to apply the correct metadata, hide the commit with
+                        // incorrect metadata, and register the new commit as
+                        // the rewritten version of original_commit_oid.
+                        let cleanup_plan = vec![
+                            format!("exec git commit --amend --no-edit --reuse-message {original_commit_oid}"),
+                            // FIXME This is causing "Skipping commit (was already applied upstream)" to be displayed for each fixup commit
+                            // 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 @{1})".to_string(),
+                            format!("exec git branchless hook-skip-upstream-applied-commit {original_commit_oid} $(git rev-parse HEAD)")
+                        ];
+                        plan.extend(cleanup_plan.into_iter());
+                    }
+
+                    plan.join("\n")
                 }
             },
             RebaseCommand::Merge {
@@ -1170,9 +1177,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

@@ -486,11 +486,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)?;
@@ -516,7 +517,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/src/commands/mod.rs

@@ -26,6 +26,7 @@ use std::convert::TryInto;
 use std::ffi::OsString;
 use std::fmt::Write;
 use std::path::PathBuf;
+use std::str::FromStr;
 use std::time::SystemTime;
 
 use clap::Parser;
@@ -34,6 +35,7 @@ use cursive::utils::markup::StyledString;
 use eyre::Context;
 use itertools::Itertools;
 use lib::core::rewrite::MergeConflictRemediation;
+use lib::git::MaybeZeroOid;
 use lib::git::Repo;
 use lib::git::RepoError;
 use lib::util::ExitCode;
@@ -196,9 +198,17 @@ fn do_main_and_drop_locals() -> eyre::Result<i32> {
             ExitCode(0)
         }
 
-        Command::HookSkipUpstreamAppliedCommit { commit_oid } => {
+        Command::HookSkipUpstreamAppliedCommit {
+            commit_oid,
+            rewritten_oid,
+        } => {
             let commit_oid: NonZeroOid = commit_oid.parse()?;
-            hooks::hook_skip_upstream_applied_commit(&effects, commit_oid)?;
+            let rewritten_oid = MaybeZeroOid::from_str(
+                rewritten_oid
+                    .unwrap_or_else(|| String::from("00000000"))
+                    .as_str(),
+            )?;
+            hooks::hook_skip_upstream_applied_commit(&effects, commit_oid, rewritten_oid)?;
             ExitCode(0)
         }
 

git-branchless/src/opts.rs

@@ -253,6 +253,10 @@ pub enum Command {
         /// 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>,
     },
 
     /// Initialize the branchless workflow for this repository.