Skip to content

Commit

Permalink
Ensure you never drop the working commit with --skip-empty
Browse files Browse the repository at this point in the history
See #2766 for discussions
  • Loading branch information
matts1 committed Jan 4, 2024
1 parent b7f36e1 commit 2379145
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 10 deletions.
1 change: 1 addition & 0 deletions cli/src/commands/rebase.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ pub(crate) struct RebaseArgs {
/// If true, when rebasing would produce an empty commit, the commit is
/// skipped.
/// Will never skip merge commits with multiple non-empty parents.
/// Will never skip the working commit.
#[arg(long, conflicts_with = "revision")]
skip_empty: bool,

Expand Down
7 changes: 6 additions & 1 deletion lib/src/rewrite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,12 @@ pub fn rebase_commit_with_options(
}
EmptyBehaviour::AbandonAllEmpty => *parent.tree_id() == new_tree_id,
};
if should_abandon {
// If the user runs `jj checkout foo`, then `jj rebase -s foo -d bar`, and we
// drop the checked out empty commit, then the user will unknowingly
// 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()) {
// 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.
Expand Down
47 changes: 38 additions & 9 deletions lib/tests/test_rewrite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1614,18 +1614,24 @@ fn test_empty_commit_option(empty_behavior: EmptyBehaviour) {
}

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

// Rebase B onto B2
// Rebase B onto B2, where B2 and B have the same tree, abandoning all empty commits.
//
// C
// |
// B B2
// |/
// A
// We expect B and D to be skipped because they're empty, but not E because it's the working
// commit.
//
// D (empty) E (WC, empty) E' (WC, empty)
// | / |
// C--------- C'
// | => |
// B B2 B2
// |/ |
// A 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)
Expand All @@ -1636,12 +1642,27 @@ fn test_rebase_commit_via_rebase_api() {
.set_parents(vec![commit_b.id().clone()])
.write()
.unwrap();
let commit_d = create_random_commit(tx.mut_repo(), &settings)
.set_parents(vec![commit_c.id().clone()])
.set_tree_id(commit_c.tree_id().clone())
.write()
.unwrap();
let commit_e = create_random_commit(tx.mut_repo(), &settings)
.set_parents(vec![commit_c.id().clone()])
.set_tree_id(commit_c.tree_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 workspace = WorkspaceId::new("ws".to_string());
tx.mut_repo()
.set_wc_commit(workspace.clone(), commit_e.id().clone())
.unwrap();

let rebase_options = RebaseOptions {
empty: EmptyBehaviour::AbandonAllEmpty,
};
Expand All @@ -1657,12 +1678,20 @@ fn test_rebase_commit_via_rebase_api() {
.mut_repo()
.rebase_descendants_with_options_return_map(&settings, rebase_options)
.unwrap();
assert_eq!(rebase_map.len(), 1);
assert_eq!(rebase_map.len(), 3);
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_eq!(
*tx.mut_repo().view().heads(),
hashset! {new_commit_c.id().clone()}
hashset! {new_commit_e.id().clone()}
);

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

0 comments on commit 2379145

Please sign in to comment.