From 45299a9a7e13c2553fa6d55c31943a13e4d2fffd Mon Sep 17 00:00:00 2001 From: Clayton Carter Date: Thu, 4 May 2023 16:18:05 -0400 Subject: [PATCH] fix(move): Support on-disk rebases with `move --fixup` --- git-branchless-hook/src/lib.rs | 11 +- git-branchless-lib/src/core/rewrite/plan.rs | 75 ++- .../src/core/rewrite/rewrite_hooks.rs | 9 +- git-branchless-opts/src/lib.rs | 4 + git-branchless/tests/test_move.rs | 500 +++++++++++++++++- 5 files changed, 587 insertions(+), 12 deletions(-) diff --git a/git-branchless-hook/src/lib.rs b/git-branchless-hook/src/lib.rs index cf3cbfff1..13417fafc 100644 --- a/git-branchless-hook/src/lib.rs +++ b/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)?; } } diff --git a/git-branchless-lib/src/core/rewrite/plan.rs b/git-branchless-lib/src/core/rewrite/plan.rs index e08a24779..1eec27e90 100644 --- a/git-branchless-lib/src/core/rewrite/plan.rs +++ b/git-branchless-lib/src/core/rewrite/plan.rs @@ -6,7 +6,7 @@ use std::sync::Arc; use chashmap::CHashMap; use eyre::Context; -use itertools::Itertools; +use itertools::{intersperse, Itertools}; use rayon::{prelude::*, ThreadPool}; use tracing::{instrument, warn}; @@ -148,7 +148,65 @@ 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::>(); + 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. + if fixups.len() > 1 { + fixups = intersperse( + fixups, + // FIXME Is $(...) portable? + // I had assumed not, but the tests seem to pass on all platforms, so maybe this is OK? + "exec git branchless hook-skip-upstream-applied-commit $(git rev-parse HEAD)".to_string() + ).collect() + } + + // 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 just + // now hidden commit. + format!("exec git commit --amend --no-edit --reuse-message {original_commit_oid}"), + + // Finally, register the new final 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, @@ -1172,9 +1230,16 @@ 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 feels heavy handed, but seems to be necessary for some fixup cases. + 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) = diff --git a/git-branchless-lib/src/core/rewrite/rewrite_hooks.rs b/git-branchless-lib/src/core/rewrite/rewrite_hooks.rs index d9a61c2f5..758aa60d8 100644 --- a/git-branchless-lib/src/core/rewrite/rewrite_hooks.rs +++ b/git-branchless-lib/src/core/rewrite/rewrite_hooks.rs @@ -516,11 +516,14 @@ pub fn hook_drop_commit_if_empty( Ok(()) } -/// For rebases, if a commit is known to have been applied upstream, skip it -/// without attempting to apply it. +/// For rebases, update the status of a commit that is known to have been +/// applied upstream. It can either be skipped entirely (when called with +/// `MaybeZeroOid::Zero`) or be marked as having been rewritten to a +/// different commit entirely. 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 +549,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(()) diff --git a/git-branchless-opts/src/lib.rs b/git-branchless-opts/src/lib.rs index aafa5755b..7f67a5e61 100644 --- a/git-branchless-opts/src/lib.rs +++ b/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, + + /// The OID of the commit that was skipped (if Some) or removed (if None). + #[clap(value_parser)] + rewritten_oid: Option, }, } diff --git a/git-branchless/tests/test_move.rs b/git-branchless/tests/test_move.rs index 8ca62d861..dc3a29540 100644 --- a/git-branchless/tests/test_move.rs +++ b/git-branchless/tests/test_move.rs @@ -4906,6 +4906,37 @@ fn test_move_fixup_head_into_parent() -> eyre::Result<()> { +line 3 "###); + // --on-disk + { + let git = git.duplicate_repo()?; + git.run(&[ + "move", + "--on-disk", + "--fixup", + "-s", + "HEAD", + "-d", + &test2_oid.to_string(), + ])?; + + let (stdout, _stderr) = git.run(&["smartlog"])?; + insta::assert_snapshot!(stdout, @r###" + O f777ecc (master) create initial.txt + | + o 307a04c create test.txt + | + @ 8c3aa56 update 2 test.txt + "###); + + let diff = git.get_trimmed_diff("test.txt", "HEAD")?; + insta::assert_snapshot!(diff, @r###" + @@ -1 +1,3 @@ + +line 1 + line 2 + +line 3 + "###); + } + // --in-memory { let (stdout, _stderr) = git.run(&[ @@ -4973,6 +5004,13 @@ fn test_move_fixup_parent_into_head() -> eyre::Result<()> { @ da38dc8 update 3 test.txt "###); + let diff = git.get_trimmed_diff("test.txt", "HEAD~")?; + insta::assert_snapshot!(diff, @r###" + @@ -1 +1,2 @@ + +line 1 + line 2 + "###); + let diff = git.get_trimmed_diff("test.txt", "HEAD")?; insta::assert_snapshot!(diff, @r###" @@ -1,2 +1,3 @@ @@ -4981,6 +5019,32 @@ fn test_move_fixup_parent_into_head() -> eyre::Result<()> { +line 3 "###); + // --on-disk + let on_disk_commit_snapshot = { + let git = git.duplicate_repo()?; + git.run_with_options( + &["move", "--on-disk", "--fixup", "-x", &test2_oid.to_string()], + // Use the same mocked system time as the destination commit to coax + // the commit hashs to match their in-mem counterparts. + &GitRunOptions { + time: 3, + ..Default::default() + }, + )?; + + let (stdout, _stderr) = git.run(&["smartlog"])?; + insta::assert_snapshot!(stdout, @r###" + O f777ecc (master) create initial.txt + | + o 307a04c create test.txt + | + @ ff6183f update 3 test.txt + "###); + + let (stdout, _stderr) = git.run(&["show", "HEAD", "--pretty=fuller"])?; + stdout + }; + // --in-memory { let (stdout, _stderr) = git.run(&[ @@ -5003,8 +5067,23 @@ fn test_move_fixup_parent_into_head() -> eyre::Result<()> { In-memory rebase succeeded. "###); - let diff = git.get_trimmed_diff("test.txt", "HEAD")?; - insta::assert_snapshot!(diff, @r###" + let (stdout, _stderr) = git.run(&["show", "HEAD", "--pretty=fuller"])?; + + assert_eq!(stdout, on_disk_commit_snapshot); + + insta::assert_snapshot!(stdout, @r###" + commit ff6183fc6b71c3fcf6f26247162bde4e34bc5193 + Author: Testy McTestface + AuthorDate: Thu Oct 29 12:34:56 2020 -0300 + Commit: Testy McTestface + CommitDate: Thu Oct 29 12:34:56 2020 -0300 + + update 3 test.txt + + diff --git a/test.txt b/test.txt + index b7e242c..a92d664 100644 + --- a/test.txt + +++ b/test.txt @@ -1 +1,3 @@ +line 1 line 2 @@ -5054,6 +5133,36 @@ fn test_move_fixup_head_into_ancestor() -> eyre::Result<()> { +line 3 "###); + // --on-disk + { + let git = git.duplicate_repo()?; + git.run(&[ + "move", + "--on-disk", + "--fixup", + "-s", + "HEAD", + "-d", + &test1_oid.to_string(), + ])?; + + let (stdout, _stderr) = git.run(&["smartlog"])?; + insta::assert_snapshot!(stdout, @r###" + O f777ecc (master) create initial.txt + | + @ 963fb93 create test.txt + | + o b9da1e0 update 2 test.txt + "###); + + let diff = git.get_trimmed_diff("test.txt", "HEAD")?; + insta::assert_snapshot!(diff, @r###" + @@ -0,0 +1,2 @@ + +line 2 + +line 3 + "###); + } + // --in-memory { let (stdout, _stderr) = git.run(&[ @@ -5145,6 +5254,40 @@ fn test_move_fixup_ancestor_into_head() -> eyre::Result<()> { +line 4 "###); + // --on-disk + { + let git = git.duplicate_repo()?; + git.run_with_options( + &["move", "--on-disk", "--fixup", "-x", &test2_oid.to_string()], + // Use the same mocked system time as the destination commit to coax + // the commit hashs to match their in-mem counterparts. + &GitRunOptions { + time: 4, + ..Default::default() + }, + )?; + + let (stdout, _stderr) = git.run(&["smartlog"])?; + insta::assert_snapshot!(stdout, @r###" + O f777ecc (master) create initial.txt + | + o 307a04c create test.txt + | + o dbcd17a update 3 test.txt + | + @ 2b97091 update 4 test.txt + "###); + + let diff = git.get_trimmed_diff("test.txt", "HEAD")?; + insta::assert_snapshot!(diff, @r###" + @@ -1,2 +1,4 @@ + line 1 + line 2 + +line 3 + +line 4 + "###); + } + // --in-memory { let (stdout, _stderr) = git.run(&[ @@ -5224,6 +5367,41 @@ fn test_move_fixup_multiple_into_head() -> eyre::Result<()> { +line 3 "###); + // --on-disk + { + let git = git.duplicate_repo()?; + git.run_with_options( + &[ + "move", + "--on-disk", + "--fixup", + "-x", + &format!("{}+{}", test1_oid, test2_oid), + ], + // Use the same mocked system time as the destination commit to coax + // the commit hashs to match their in-mem counterparts. + &GitRunOptions { + time: 3, + ..Default::default() + }, + )?; + + let (stdout, _stderr) = git.run(&["smartlog"])?; + insta::assert_snapshot!(stdout, @r###" + O f777ecc (master) create initial.txt + | + @ c4f6746 update 3 test.txt + "###); + + let diff = git.get_trimmed_diff("test.txt", "HEAD")?; + insta::assert_snapshot!(diff, @r###" + @@ -0,0 +1,3 @@ + +line 1 + +line 2 + +line 3 + "###); + } + // --in-memory { let (stdout, _stderr) = git.run(&[ @@ -5311,6 +5489,53 @@ fn test_move_fixup_multiple_into_ancestor_with_unmoved_head() -> eyre::Result<() +line 4 "###); + // --on-disk + { + let git = git.duplicate_repo()?; + git.run_with_options( + &[ + "move", + "--on-disk", + "--fixup", + "-x", + &format!("{}+{}", test2_oid, test3_oid), + "-d", + &test1_oid.to_string(), + ], + // Use the same mocked system time as the destination commit to coax + // the commit hashs to match their in-mem counterparts. + &GitRunOptions { + time: 1, + ..Default::default() + }, + )?; + + let (stdout, _stderr) = git.run(&["smartlog"])?; + insta::assert_snapshot!(stdout, @r###" + O f777ecc (master) create initial.txt + | + o 23d4bdd create test.txt + | + @ 797ef91 update 4 test.txt + "###); + + let diff = git.get_trimmed_diff("test.txt", "HEAD~")?; + insta::assert_snapshot!(diff, @r###" + @@ -0,0 +1,3 @@ + +line 1 + +line 2 + +line 3 + "###); + let diff = git.get_trimmed_diff("test.txt", "HEAD")?; + insta::assert_snapshot!(diff, @r###" + @@ -1,3 +1,4 @@ + line 1 + line 2 + line 3 + +line 4 + "###); + } + // --in-memory { let (stdout, _stderr) = git.run(&[ @@ -5488,6 +5713,65 @@ fn test_move_fixup_multiple_disconnected_into_ancestor() -> eyre::Result<()> { Line C-1 "###); + // --on-disk + { + let git = git.duplicate_repo()?; + git.run_with_options( + &[ + "move", + "--on-disk", + "--fixup", + "-x", + &format!("{}+{}", test3_oid, test5_oid), + "-d", + &test1_oid.to_string(), + ], + // Use the same mocked system time as the destination commit to coax + // the commit hashs to match their in-mem counterparts. + &GitRunOptions { + time: 1, + ..Default::default() + }, + )?; + + let (stdout, _stderr) = git.run(&["smartlog"])?; + insta::assert_snapshot!(stdout, @r###" + O f777ecc (master) create initial.txt + | + o 38caaaf create test.txt + | + o 6783c86 update 2 test.txt + | + o 7e2a64a update 4 test.txt + | + @ 472b70b update 6 test.txt + "###); + + // diff for "create test.txt" + let diff = git.get_trimmed_diff("test.txt", "38caaaf")?; + insta::assert_snapshot!(diff, @r###" + @@ -0,0 +1,8 @@ + +# Section A + +. + +. + +# Section B + +. + +. + +# Section 3 + +Line C-1 + "###); + let diff = git.get_trimmed_diff("test.txt", "HEAD")?; + insta::assert_snapshot!(diff, @r###" + @@ -5,5 +5,5 @@ Line A-1 + Line B-1 + Line B-2 + . + -# Section 3 + +# Section C + Line C-1 + "###); + } + // --in-memory { let (stdout, _stderr) = git.run(&[ @@ -5680,6 +5964,62 @@ fn test_move_fixup_multiple_disconnected_into_head() -> eyre::Result<()> { Line C-1 "###); + // --on-disk + { + let git = git.duplicate_repo()?; + git.run_with_options( + &[ + "move", + "--on-disk", + "--fixup", + "-x", + &format!("{}+{}", test3_oid, test5_oid), + ], + // Use the same mocked system time as the destination commit to coax + // the commit hashs to match their in-mem counterparts. + &GitRunOptions { + time: 6, + ..Default::default() + }, + )?; + + let (stdout, _stderr) = git.run(&["smartlog"])?; + insta::assert_snapshot!(stdout, @r###" + O f777ecc (master) create initial.txt + | + o 64eb9bf create test.txt + | + o 087914d update 2 test.txt + | + o c21e0d7 update 4 test.txt + | + @ 237b381 update 6 test.txt + "###); + + // diff for "update 4" should not include the changes from "update 3" + let diff = git.get_trimmed_diff("test.txt", "c21e0d7")?; + insta::assert_snapshot!(diff, @r###" + @@ -2,6 +2,7 @@ + Line A-1 + . + # Section B + -. + +Line B-1 + +Line B-2 + . + # Section C + "###); + + let diff = git.get_trimmed_diff("test.txt", "HEAD")?; + insta::assert_snapshot!(diff, @r###" + @@ -6,3 +6,4 @@ Line B-1 + Line B-2 + . + # Section C + +Line C-1 + "###); + } + // --in-memory { let (stdout, _stderr) = git.run(&[ @@ -5793,6 +6133,36 @@ fn test_move_fixup_squash_branch() -> eyre::Result<()> { } "###); + // --on-disk + { + let git = git.duplicate_repo()?; + git.run(&[ + "move", + "--on-disk", + "--fixup", + "-s", + &test2_oid.to_string(), + "-d", + &test1_oid.to_string(), + ])?; + + let (stdout, _stderr) = git.run(&["smartlog"])?; + insta::assert_snapshot!(stdout, @r###" + O f777ecc (master) create initial.txt + | + @ c513440 (> test) create test.txt + "###); + + let diff = git.get_trimmed_diff("test.txt", "HEAD")?; + insta::assert_snapshot!(diff, @r###" + @@ -0,0 +1,4 @@ + +function name(): int + +{ + +return 123; + +} + "###); + } + // --in-memory { let (stdout, _stderr) = git.run(&[ @@ -5902,6 +6272,49 @@ fn test_move_fixup_branch_tip() -> eyre::Result<()> { +} "###); + // --on-disk + { + let git = git.duplicate_repo()?; + git.run(&[ + "move", + "--on-disk", + "--fixup", + "-x", + &test3_oid.to_string(), + "-d", + &test1_oid.to_string(), + ])?; + + let (stdout, _stderr) = git.run(&["smartlog"])?; + insta::assert_snapshot!(stdout, @r###" + O f777ecc (master) create initial.txt + | + @ f4321df (> test) create test.txt + | + o 2746e2a update 2 test.txt + "###); + + let diff = git.get_trimmed_diff("test.txt", "HEAD")?; + insta::assert_snapshot!(diff, @r###" + @@ -0,0 +1,3 @@ + +# Just a function + +function name() + +{} + "###); + + // diff for "update 2" + let diff = git.get_trimmed_diff("test.txt", "2746e2a")?; + insta::assert_snapshot!(diff, @r###" + @@ -1,3 +1,5 @@ + # Just a function + function name() + -{} + +{ + +return 123; + +} + "###); + } + // --in-memory { let (stdout, _stderr) = git.run(&[ @@ -6088,6 +6501,68 @@ fn test_move_fixup_tree() -> eyre::Result<()> { . # Section C "###); + + // --on-disk + { + let git = git.duplicate_repo()?; + git.run_with_options( + &[ + "move", + "--on-disk", + "--fixup", + "-x", + &format!("{}+{}", test2_oid, test6_oid), + "-d", + &test4_oid.to_string(), + ], + // Use the same mocked system time as the destination commit to coax + // the commit hashs to match their in-mem counterparts. + &GitRunOptions { + time: 4, + ..Default::default() + }, + )?; + + let (stdout, _stderr) = git.run(&["smartlog"])?; + insta::assert_snapshot!(stdout, @r###" + O f777ecc (master) create initial.txt + | + o 64eb9bf create test.txt + | + o b479fa3 update 3 test.txt + |\ + | o 59a95b3 update 4 test.txt + | | + | o 1e7fdb8 update 5 test.txt + | + @ 5e6d3e4 update 7 test.txt + "###); + + // diff for update 4 test.txt + let diff = git.get_trimmed_diff("test.txt", "59a95b3")?; + insta::assert_snapshot!(diff, @r###" + @@ -1,7 +1,8 @@ + # Section A + +Line A-1 + . + -. + -# Section B + +## Section A-2 + Line B-1 + +Line B-2 + . + # Section C + "###); + let diff = git.get_trimmed_diff("test.txt", "HEAD")?; + insta::assert_snapshot!(diff, @r###" + @@ -5,3 +5,4 @@ + Line B-1 + . + # Section C + +Line C-1 + "###); + } + // --in-memory { let (stdout, _stderr) = git.run(&[ @@ -6160,6 +6635,27 @@ fn test_move_fixup_conflict() -> eyre::Result<()> { git.commit_file_with_contents("test", 1, "line 1\n")?; git.commit_file_with_contents_and_message("test", 2, "line 1, 2\n", "update 2")?; git.commit_file_with_contents_and_message("test", 3, "line 1, 2, 3\n", "update 3")?; + + // --on-disk + { + let git = git.duplicate_repo()?; + let (stdout, _stderr) = git.run_with_options( + &["move", "--on-disk", "--fixup", "-x", "HEAD", "-d", "HEAD~2"], + &GitRunOptions { + expected_exit_code: 1, + ..Default::default() + }, + )?; + + insta::assert_snapshot!(stdout, @r###" + branchless: running command: diff --quiet + Calling Git for on-disk rebase... + branchless: running command: rebase --continue + Auto-merging test.txt + CONFLICT (content): Merge conflict in test.txt + "###); + } + // --in-memory { let (stdout, _stderr) = git.run_with_options(