Skip to content

Commit

Permalink
rewrite.rs: fix working copy position after jj rebase --abandon-empty
Browse files Browse the repository at this point in the history 
Fixes #2869
  • Loading branch information
@ilyagr
ilyagr committed Jan 31, 2024
1 parent 1fff6e3 commit becbc88
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 30 deletions.
39 changes: 29 additions & 10 deletions lib/src/rewrite.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,27 @@ pub fn rebase_commit(
new_parents,
&Default::default(),
)
.map(|(commit, abandoned_old_commit)| {
assert!(
!abandoned_old_commit,
"Old commit should never be abandoned since the default options include \
EmptyBehavior::Keep"
);
commit
})
}

/// Returns the new parent commit, and whether the old commit was abandoned
///
/// It should only be possible for the old commit to be abandoned if
/// `options.empty != EmptyBehavior::Keep`
pub fn rebase_commit_with_options(
settings: &UserSettings,
mut_repo: &mut MutableRepo,
old_commit: &Commit,
new_parents: &[Commit],
options: &RebaseOptions,
) -> Result<Commit, TreeMergeError> {
) -> Result<(Commit, bool), TreeMergeError> {
let old_parents = old_commit.parents();
let old_parent_trees = old_parents
.iter()
Expand Down Expand Up @@ -164,23 +176,26 @@ pub fn rebase_commit_with_options(
// have done the equivalent of `jj edit foo` instead of `jj checkout
// foo`. Thus, we never allow dropping the working commit. See #2766 and
// #2760 for discussions.
if should_abandon && !mut_repo.view().is_wc_commit_id(old_commit.id()) {
if should_abandon {
// Record old_commit as being succeeded by the parent.
// This ensures that when we stack commits, the second commit knows to
// rebase on top of the parent commit, rather than the abandoned commit.
mut_repo.record_rewritten_commit(old_commit.id().clone(), parent.id().clone());
return Ok(parent.clone());
return Ok((parent.clone(), true));
}
}
let new_parent_ids = new_parents
.iter()
.map(|commit| commit.id().clone())
.collect();
Ok(mut_repo
.rewrite_commit(settings, old_commit)
.set_parents(new_parent_ids)
.set_tree_id(new_tree_id)
.write()?)
Ok((
mut_repo
.rewrite_commit(settings, old_commit)
.set_parents(new_parent_ids)
.set_tree_id(new_tree_id)
.write()?,
false,
))
}

pub fn rebase_to_dest_parent(
Expand Down Expand Up @@ -568,7 +583,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
.filter(|new_parent| head_set.contains(new_parent))
.map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id))
.try_collect()?;
let new_commit = rebase_commit_with_options(
let (new_commit, abandoned_old_commit) = rebase_commit_with_options(
self.settings,
self.mut_repo,
&old_commit,
Expand All @@ -586,7 +601,11 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
(None, None),
"Trying to rebase the same commit {old_commit_id:?} in two different ways",
);
self.update_references(old_commit_id, vec![new_commit.id().clone()], false)?;
self.update_references(
old_commit_id,
vec![new_commit.id().clone()],
abandoned_old_commit,
)?;
return Ok(Some(RebasedDescendant {
old_commit,
new_commit,
Expand Down
44 changes: 24 additions & 20 deletions lib/tests/test_rewrite.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1625,21 +1625,15 @@ fn test_rebase_abandoning_empty() {
// We expect B, D, and G to be skipped because they're empty, but not E because
// it's the working commit. F also remains as it's not empty.
//
// F G (empty) F'
// |/ |
// E (WC, empty) D (empty) E' (WC, empty)
// | / |
// F G (empty)
// |/
// E (WC, empty) D (empty) F' new_working_copy_commit (new, WC, empty)
// | / | /
// C------------- C'
// | => |
// B B2 B2
// |/ |
// A A
//
// TODO(#2869): There is a minor bug here. We'd like the result to be
// equivalent to rebasing and then `jj abandon`-ing all the empty commits.
// In case of the working copy, this would mean that F' would be a direct
// child of C', and the working copy would be a new commit that's also a
// direct child of C'.

let mut tx = repo.start_transaction(&settings);
let commit_a = write_random_commit(tx.mut_repo(), &settings);
Expand Down Expand Up @@ -1700,19 +1694,29 @@ fn test_rebase_abandoning_empty() {
let new_commit_c =
assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[&commit_b2.id()]);
assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_d, new_commit_c.id());
let new_commit_e =
assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_e, &[&new_commit_c.id()]);
assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_e, new_commit_c.id());
let new_commit_f =
assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_f, &[&new_commit_e.id()]);
assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_g, new_commit_e.id());
assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_f, &[&new_commit_c.id()]);
assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_g, new_commit_c.id());

let new_working_copy_commit_id = tx
.mut_repo()
.view()
.get_wc_commit_id(&workspace)
.unwrap()
.clone();
let new_working_copy_commit = repo
.store()
.get_commit(&new_working_copy_commit_id)
.unwrap()
.clone();

assert_ne!(new_working_copy_commit.change_id(), commit_c.change_id());
assert_ne!(new_working_copy_commit.change_id(), commit_e.change_id());
assert_ne!(new_working_copy_commit.change_id(), commit_f.change_id());
assert_ne!(new_working_copy_commit.change_id(), commit_g.change_id());
assert_eq!(
*tx.mut_repo().view().heads(),
hashset! {new_commit_f.id().clone()}
);

assert_eq!(
tx.mut_repo().view().get_wc_commit_id(&workspace),
Some(new_commit_e.id())
hashset! {new_commit_f.id().clone(), new_working_copy_commit_id}
);
}

0 comments on commit becbc88

Please sign in to comment.