Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(move): Support on-disk rebases with move --fixup
Browse files Browse the repository at this point in the history
@claytonrcarter
claytonrcarter committed Jul 15, 2023
1 parent dfeb795 commit 8f5ebec
Showing 5 changed files with 587 additions and 12 deletions.
11 changes: 9 additions & 2 deletions git-branchless-hook/src/lib.rs
View file
Original file line number Diff line number Diff line change
@@ -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)?;
}
}

75 changes: 70 additions & 5 deletions git-branchless-lib/src/core/rewrite/plan.rs
View file
Original file line number Diff line number Diff line change
@@ -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::<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.
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) =
9 changes: 6 additions & 3 deletions git-branchless-lib/src/core/rewrite/rewrite_hooks.rs
View file
Original file line number Diff line number Diff line change
@@ -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(())
4 changes: 4 additions & 0 deletions git-branchless-opts/src/lib.rs
View file
Original file line number Diff line number Diff line change
@@ -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<String>,
},
}

500 changes: 498 additions & 2 deletions git-branchless/tests/test_move.rs
View file
Original file line number Diff line number Diff line change
@@ -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 <test@example.com>
AuthorDate: Thu Oct 29 12:34:56 2020 -0300
Commit: Testy McTestface <test@example.com>
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: <git-executable> diff --quiet
Calling Git for on-disk rebase...
branchless: running command: <git-executable> rebase --continue
Auto-merging test.txt
CONFLICT (content): Merge conflict in test.txt
"###);
}

// --in-memory
{
let (stdout, _stderr) = git.run_with_options(

0 comments on commit 8f5ebec

Please sign in to comment.