Skip to content

Commit

Permalink
jj rebase --abandon-empty: revert commits cfcc7c5 and becbc88
Browse files Browse the repository at this point in the history
This mostly reverts jj-vcs#2901 as well as its
fixup jj-vcs#2903. The related bug is reopened,
see jj-vcs#2869 (comment).

The problem is that while the fix did fix jj-vcs#2896 in most cases, it did
reintroduce the more severe bug jj-vcs#2760
in one case, if the working copy is the commit being rebased.

For example, suppose you have the tree

```
root -> A -> B -> @ (empty) -> C
```

### Before this commit

#### Case 1

`jj rebase -s B -d root` would work perfectly before this commit, resulting in

```
root -> A
  \-------B -> C
           \- @ (new, empty)
```

#### Case 2

Unfortunately, if you run `jj rebase -s @ -d A`, you'd have the following result
(before this commit), which shows the reintroduction of jj-vcs#2760:

```
root -> A @ -> C
         \-- B
```

with the working copy at `A`. The reason for this is explained in
jj-vcs#2901 (comment).

### After this commit

After this commit, both case 1 and case 2 will be wrong in the sense of jj-vcs#2896,
it will not exhibit the worse bug jj-vcs#2760.

Case 1 would result in:

```
root -> A
  \-------B -> @ (empty) -> C
```

Case 2 would result in:

```
root -> A -> @ -> C
         \-- B
```

with the working copy remaining a descendant of A
  • Loading branch information
ilyagr committed Feb 3, 2024
1 parent 0182595 commit 9290475
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 58 deletions.
39 changes: 10 additions & 29 deletions lib/src/rewrite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,27 +112,15 @@ 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, bool), TreeMergeError> {
) -> Result<Commit, TreeMergeError> {
let old_parents = old_commit.parents();
let old_parent_trees = old_parents
.iter()
Expand Down Expand Up @@ -176,26 +164,23 @@ 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 {
if should_abandon && !mut_repo.view().is_wc_commit_id(old_commit.id()) {
// 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(), true));
return Ok(parent.clone());
}
}
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()?,
false,
))
Ok(mut_repo
.rewrite_commit(settings, old_commit)
.set_parents(new_parent_ids)
.set_tree_id(new_tree_id)
.write()?)
}

pub fn rebase_to_dest_parent(
Expand Down Expand Up @@ -580,7 +565,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, abandoned_old_commit) = rebase_commit_with_options(
let new_commit = rebase_commit_with_options(
self.settings,
self.mut_repo,
&old_commit,
Expand All @@ -598,11 +583,7 @@ 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()],
abandoned_old_commit,
)?;
self.update_references(old_commit_id, vec![new_commit.id().clone()], false)?;
Ok(())
}

Expand Down
51 changes: 22 additions & 29 deletions lib/tests/test_rewrite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1622,21 +1622,24 @@ fn test_rebase_abandoning_empty() {
// Rebase B onto B2, where B2 and B have the same tree, abandoning all empty
// commits.
//
// We expect B, D, E and G to be skipped because they're empty. F remains
// as it's not empty. Since the working copy commit E is skipped, a new working
// copy commit should be created in its stead, just as would have happened if we
// did a normal rebase and then manually abandoned the working copy with `jj
// abandon`.
// 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)
// |/
// E (WC, empty) D (empty) F' new_working_copy_commit (new, WC, empty)
// | / | /
// F G (empty) F'
// |/ |
// E (WC, empty) D (empty) E' (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 @@ -1697,29 +1700,19 @@ 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());
assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_e, new_commit_c.id());
let new_commit_e =
assert_rebased_onto(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_c.id()]);
assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_g, new_commit_c.id());
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());

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(), new_working_copy_commit_id}
hashset! {new_commit_f.id().clone()}
);

assert_eq!(
tx.mut_repo().view().get_wc_commit_id(&workspace),
Some(new_commit_e.id())
);
}

0 comments on commit 9290475

Please sign in to comment.