Skip to content

Commit

Permalink
Rewrite instead of abandoning empty commits.
Browse files Browse the repository at this point in the history
Fixes #2760


Given the tree:
```
A-B-C
 \
  B2
```
And the command `jj rebase -s B -d B2`

We were previously marking B as abandoned, despite the comment stating that we were marking it as being succeeded by B2. This resulted in a call to `rewrite(rewrites={}, abandoned={B})` instead of `rewrite(rewrites={B=>B2}, abandoned={})`, which then made the new parent of `C` into `A` instead of `B2`
  • Loading branch information
matts1 committed Jan 4, 2024
1 parent 5450e6c commit a4aed23
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 4 deletions.
5 changes: 2 additions & 3 deletions lib/src/rewrite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,10 @@ pub fn rebase_commit_with_options(
EmptyBehaviour::AbandonAllEmpty => *parent.tree_id() == new_tree_id,
};
if should_abandon {
mut_repo.record_abandoned_commit(old_commit.id().clone());
// Record old_commit as being succeeded by the parent for the purposes of
// the rebase.
// 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());
}
}
Expand Down
56 changes: 55 additions & 1 deletion lib/tests/test_rewrite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use jj_lib::merged_tree::MergedTree;
use jj_lib::op_store::{RefTarget, RemoteRef, RemoteRefState, WorkspaceId};
use jj_lib::repo::Repo;
use jj_lib::repo_path::RepoPath;
use jj_lib::rewrite::{restore_tree, EmptyBehaviour, RebaseOptions};
use jj_lib::rewrite::{rebase_commit_with_options, restore_tree, EmptyBehaviour, RebaseOptions};
use maplit::{hashmap, hashset};
use test_case::test_case;
use testutils::{
Expand Down Expand Up @@ -1612,3 +1612,57 @@ fn test_empty_commit_option(empty_behavior: EmptyBehaviour) {
}
);
}

#[test]
fn test_rebase_commit_via_rebase_api() {
let settings = testutils::user_settings();
let test_repo = TestRepo::init();
let repo = &test_repo.repo;

// Rebase B onto B2
//
// C
// |
// B B2
// |/
// A
let mut tx = repo.start_transaction(&settings);
let commit_a = write_random_commit(tx.mut_repo(), &settings);
let commit_b = create_random_commit(tx.mut_repo(), &settings)
.set_parents(vec![commit_a.id().clone()])
.write()
.unwrap();
let commit_c = create_random_commit(tx.mut_repo(), &settings)
.set_parents(vec![commit_b.id().clone()])
.write()
.unwrap();
let commit_b2 = create_random_commit(tx.mut_repo(), &settings)
.set_parents(vec![commit_a.id().clone()])
.set_tree_id(commit_b.tree_id().clone())
.write()
.unwrap();

let rebase_options = RebaseOptions {
empty: EmptyBehaviour::AbandonAllEmpty,
};
rebase_commit_with_options(
&settings,
tx.mut_repo(),
&commit_b,
&[commit_b2.clone()],
&rebase_options,
)
.unwrap();
let rebase_map = tx
.mut_repo()
.rebase_descendants_with_options_return_map(&settings, rebase_options)
.unwrap();
assert_eq!(rebase_map.len(), 1);
let new_commit_c =
assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[&commit_b2.id()]);

assert_eq!(
*tx.mut_repo().view().heads(),
hashset! {new_commit_c.id().clone()}
);
}

0 comments on commit a4aed23

Please sign in to comment.