ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

rewrite: fix check for newly-empty commit in optimized path

Browse source 
'old_base_tree_id == None' means the rebased tree is unchanged, so the commit
shouldn't be considered newly-empty.
This commit is contained in:
Yuya Nishihara 2023-11-24 17:00:15 +09:00
parent 2f93de9299
commit b7543f8a08
2 changed files with 17 additions and 7 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
lib/src/rewrite.rs
View file

@ -154,7 +154,7 @@ pub fn rebase_commit_with_options(
EmptyBehaviour::Keep => false, EmptyBehaviour::Keep => false,
EmptyBehaviour::AbandonNewlyEmpty => { EmptyBehaviour::AbandonNewlyEmpty => {
*parent.tree_id() == new_tree_id *parent.tree_id() == new_tree_id
&& old_base_tree_id != Some(old_commit.tree_id().clone()) && old_base_tree_id.map_or(false, |id| id != *old_commit.tree_id())
} }
EmptyBehaviour::AbandonAllEmpty => *parent.tree_id() == new_tree_id, EmptyBehaviour::AbandonAllEmpty => *parent.tree_id() == new_tree_id,
}; };

22
lib/tests/test_rewrite.rs
View file

@ -1490,7 +1490,7 @@ fn assert_rebase_skipped(
rebased: Option<RebasedDescendant>, rebased: Option<RebasedDescendant>,
expected_old_commit: &Commit, expected_old_commit: &Commit,
expected_new_commit: &Commit, expected_new_commit: &Commit,
) { ) -> Commit {
if let Some(RebasedDescendant { if let Some(RebasedDescendant {
old_commit, old_commit,
new_commit, new_commit,
@ -1500,6 +1500,7 @@ fn assert_rebase_skipped(
assert_eq!(new_commit, *expected_new_commit); assert_eq!(new_commit, *expected_new_commit);
// Since it was abandoned, the change ID should be different. // Since it was abandoned, the change ID should be different.
assert_ne!(old_commit.change_id(), new_commit.change_id()); assert_ne!(old_commit.change_id(), new_commit.change_id());
new_commit
} else { } else {
panic!("expected rebased commit: {rebased:?}"); panic!("expected rebased commit: {rebased:?}");
} }
@ -1517,11 +1518,13 @@ fn test_empty_commit_option(empty: EmptyBehaviour) {
// actual changes. // actual changes.
// //
// BD (commit B joined with commit D) // BD (commit B joined with commit D)
// | H (empty, no parent tree changes)
// | |
// | G // | G
// | | // | |
// | F (clean merge) // | F (clean merge)
// | /|\ // | /|\
// | C D E (empty) // | C D E (empty, but parent tree changes)
// | \|/ // | \|/
// | B // | B
// A__/ // A__/
@ -1567,6 +1570,7 @@ fn test_empty_commit_option(empty: EmptyBehaviour) {
let commit_e = create_commit(&[&commit_b], &tree_b); let commit_e = create_commit(&[&commit_b], &tree_b);
let commit_f = create_commit(&[&commit_c, &commit_d, &commit_e], &tree_f); let commit_f = create_commit(&[&commit_c, &commit_d, &commit_e], &tree_f);
let commit_g = create_commit(&[&commit_f], &tree_g); let commit_g = create_commit(&[&commit_f], &tree_g);
let commit_h = create_commit(&[&commit_g], &tree_g);
let commit_bd = create_commit(&[&commit_a], &tree_d); let commit_bd = create_commit(&[&commit_a], &tree_d);
let mut rebaser = DescendantRebaser::new( let mut rebaser = DescendantRebaser::new(
@ -1595,7 +1599,9 @@ fn test_empty_commit_option(empty: EmptyBehaviour) {
&commit_f, &commit_f,
&[&new_commit_c, &new_commit_d, &new_commit_e], &[&new_commit_c, &new_commit_d, &new_commit_e],
); );
assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]) let new_commit_g =
assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]);
assert_rebased(rebaser.rebase_next().unwrap(), &commit_h, &[&new_commit_g])
} }
EmptyBehaviour::AbandonAllEmpty => { EmptyBehaviour::AbandonAllEmpty => {
// The commit C isn't empty. // The commit C isn't empty.
@ -1606,7 +1612,9 @@ fn test_empty_commit_option(empty: EmptyBehaviour) {
assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_d, &commit_bd); assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_d, &commit_bd);
assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_e, &commit_bd); assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_e, &commit_bd);
assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_f, &new_commit_c); assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_f, &new_commit_c);
assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_c]) let new_commit_g =
assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_c]);
assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_h, &new_commit_g)
} }
EmptyBehaviour::AbandonNewlyEmpty => { EmptyBehaviour::AbandonNewlyEmpty => {
// The commit C isn't empty. // The commit C isn't empty.
@ -1625,12 +1633,14 @@ fn test_empty_commit_option(empty: EmptyBehaviour) {
&commit_f, &commit_f,
&[&new_commit_c, &new_commit_e], &[&new_commit_c, &new_commit_e],
); );
assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]) let new_commit_g =
assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]);
assert_rebased(rebaser.rebase_next().unwrap(), &commit_h, &[&new_commit_g])
} }
}; };
assert!(rebaser.rebase_next().unwrap().is_none()); assert!(rebaser.rebase_next().unwrap().is_none());
assert_eq!(rebaser.rebased().len(), 5); assert_eq!(rebaser.rebased().len(), 6);
assert_eq!( assert_eq!(
*tx.mut_repo().view().heads(), *tx.mut_repo().view().heads(),