From 85ff2f0e11fbd44c5807931ab68d57d67594af79 Mon Sep 17 00:00:00 2001 From: Matt Stark Date: Tue, 21 Nov 2023 16:49:31 +1100 Subject: [PATCH] Add a --skip-empty flag to rebase --- CHANGELOG.md | 3 + cli/src/commands/rebase.rs | 12 ++- lib/src/rewrite.rs | 39 +++++++-- lib/tests/test_rewrite.rs | 172 ++++++++++++++++++++++++++++++++++++- 4 files changed, 218 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90379bef6b..c3cba8ab47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 usual behavior where commits that became unreachable in the Git repo are abandoned ([#2504](https://github.com/martinvonz/jj/pull/2504)). +* `jj rebase` now takes the flag `--skip-empty`, which doesn't copy over commits + that would become empty after a rebase. + ### Fixed bugs diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 08b4f938f8..9468c2e899 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -143,6 +143,12 @@ pub(crate) struct RebaseArgs { /// commit) #[arg(long, short, required = true)] destination: Vec, + + /// If true, when rebasing would produce an empty commit, the commit is + /// skipped. + #[arg(long)] + skip_empty: bool, + /// Deprecated. Please prefix the revset with `all:` instead. #[arg(long, short = 'L', hide = true)] allow_large_revsets: bool, @@ -162,8 +168,10 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets } let rebase_options = RebaseOptions { - // TODO: Add a command-line flag for this. - empty: EmptyBehaviour::Keep, + empty: match args.skip_empty { + true => EmptyBehaviour::AbandonAllEmpty, + false => EmptyBehaviour::Keep, + }, }; let mut workspace_command = command.workspace_helper(ui)?; let new_parents = cli_util::resolve_all_revs(&workspace_command, ui, &args.destination)? diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 324d86b02f..3ebd73cc2e 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -118,7 +118,7 @@ pub fn rebase_commit_with_options( mut_repo: &mut MutableRepo, old_commit: &Commit, new_parents: &[Commit], - _options: &RebaseOptions, + options: &RebaseOptions, ) -> Result { let old_parents = old_commit.parents(); let old_parent_trees = old_parents @@ -137,6 +137,26 @@ pub fn rebase_commit_with_options( let new_base_tree = merge_commit_trees(mut_repo, new_parents)?; let old_tree = old_commit.tree()?; let merged_tree = new_base_tree.merge(&old_base_tree, &old_tree)?; + // Ensure we don't abandon commits with multiple parents (merge commits), even + // if they're empty. + if let [parent] = new_parents { + match options.empty { + EmptyBehaviour::AbandonNewlyEmpty | EmptyBehaviour::AbandonAllEmpty => { + if new_base_tree.id() == merged_tree.id() + && (options.empty == EmptyBehaviour::AbandonAllEmpty + || old_base_tree.id() != old_tree.id()) + { + mut_repo.record_abandoned_commit(old_commit.id().clone()); + // Record old_commit as being succeeded by the parent for the purposes of + // the rebase. + // This ensures that when we stack commits, the second commit knows to + // rebase on top of the parent commit, rather than the abandoned commit. + return Ok(parent.clone()); + } + } + EmptyBehaviour::Keep => {} + } + } merged_tree.id() }; let new_parent_ids = new_parents @@ -352,21 +372,30 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { } fn new_parents(&self, old_ids: &[CommitId]) -> Vec { + // This should be a set, but performance of a vec is much better since we expect + // 99% of commits to have <= 2 parents. let mut new_ids = vec![]; + let mut add_parent = |id: &CommitId| { + // This can trigger if we abandon an empty commit, as both the empty commit and + // its parent are succeeded by the same commit. + if !new_ids.contains(id) { + new_ids.push(id.clone()); + } + }; for old_id in old_ids { if let Some(new_parent_ids) = self.new_parents.get(old_id) { for new_parent_id in new_parent_ids { // The new parent may itself have been rebased earlier in the process if let Some(newer_parent_id) = self.rebased.get(new_parent_id) { - new_ids.push(newer_parent_id.clone()); + add_parent(newer_parent_id); } else { - new_ids.push(new_parent_id.clone()); + add_parent(new_parent_id); } } } else if let Some(new_parent_id) = self.rebased.get(old_id) { - new_ids.push(new_parent_id.clone()); + add_parent(new_parent_id); } else { - new_ids.push(old_id.clone()); + add_parent(old_id); }; } new_ids diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 87c1a72a06..a85b2e698c 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -13,11 +13,15 @@ // limitations under the License. use itertools::Itertools as _; +use jj_lib::commit::Commit; use jj_lib::matchers::{EverythingMatcher, FilesMatcher}; +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, DescendantRebaser}; +use jj_lib::rewrite::{ + restore_tree, DescendantRebaser, EmptyBehaviour, RebaseOptions, RebasedDescendant, +}; use maplit::{hashmap, hashset}; use testutils::{ assert_rebased, create_random_commit, create_tree, write_random_commit, CommitGraphBuilder, @@ -66,6 +70,153 @@ fn test_restore_tree() { assert_eq!(restored, expected.id()); } +fn test_empty_commit_option(empty: EmptyBehaviour) { + let settings = testutils::user_settings(); + let test_repo = TestRepo::init(); + let repo = &test_repo.repo; + + // Rebase a previously empty commit, a newly empty commit, and a commit with + // actual changes. + // + // BD (commit B joined with commit D) + // | G + // | | + // | F (clean merge) + // | /|\ + // | C D E (empty) + // | \|/ + // | B + // A__/ + let mut tx = repo.start_transaction(&settings, "test"); + let mut_repo = tx.mut_repo(); + let create_fixed_tree = |paths: &[&str]| { + let content_map = paths + .iter() + .map(|&p| (RepoPath::from_internal_string(p), p)) + .collect_vec(); + let content_map_ref = content_map + .iter() + .map(|(path, content)| (path, *content)) + .collect_vec(); + create_tree(repo, &content_map_ref) + }; + + // The commit_with_parents function generates non-empty merge commits, so it + // isn't suitable for this test case. + let tree_b = create_fixed_tree(&["B"]); + let tree_c = create_fixed_tree(&["B", "C"]); + let tree_d = create_fixed_tree(&["B", "D"]); + let tree_f = create_fixed_tree(&["B", "C", "D"]); + let tree_g = create_fixed_tree(&["B", "C", "D", "G"]); + + let commit_a = create_random_commit(mut_repo, &settings).write().unwrap(); + + let mut create_commit = |parents: &[&Commit], tree: &MergedTree| { + create_random_commit(mut_repo, &settings) + .set_parents( + parents + .iter() + .map(|commit| commit.id().clone()) + .collect_vec(), + ) + .set_tree_id(tree.id()) + .write() + .unwrap() + }; + let commit_b = create_commit(&[&commit_a], &tree_b); + let commit_c = create_commit(&[&commit_b], &tree_c); + let commit_d = create_commit(&[&commit_b], &tree_d); + let commit_e = create_commit(&[&commit_b], &tree_b); + let commit_f = create_commit(&[&commit_c, &commit_d, &commit_e], &tree_f); + let commit_g = create_commit(&[&commit_f], &tree_g); + let commit_bd = create_commit(&[&commit_a], &tree_d); + + let mut rebaser = DescendantRebaser::new( + &settings, + tx.mut_repo(), + hashmap! { + commit_b.id().clone() => hashset!{commit_bd.id().clone()} + }, + hashset! {}, + RebaseOptions { + empty: empty.clone(), + }, + ); + + let new_head = match empty { + EmptyBehaviour::Keep => { + // The commit C isn't empty. + let new_commit_c = + assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]); + let new_commit_d = + assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&commit_bd]); + let new_commit_e = + assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&commit_bd]); + let new_commit_f = assert_rebased( + rebaser.rebase_next().unwrap(), + &commit_f, + &[&new_commit_c, &new_commit_d, &new_commit_e], + ); + assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]) + } + EmptyBehaviour::AbandonAllEmpty => { + // The commit C isn't empty. + let new_commit_c = + assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]); + // D and E are empty, and F is a clean merge with only one child. Thus, F is + // also considered empty. + 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_f, &new_commit_c); + assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_c]) + } + EmptyBehaviour::AbandonNewlyEmpty => { + // The commit C isn't empty. + let new_commit_c = + assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]); + + // The changes in D are included in BD, so D is newly empty. + assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_d, &commit_bd); + // E was already empty, so F is a merge commit with C and E as parents. + // Although it's empty, we still keep it because we don't want to drop merge + // commits. + let new_commit_e = + assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&commit_bd]); + let new_commit_f = assert_rebased( + rebaser.rebase_next().unwrap(), + &commit_f, + &[&new_commit_c, &new_commit_e], + ); + assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]) + } + }; + + assert!(rebaser.rebase_next().unwrap().is_none()); + assert_eq!(rebaser.rebased().len(), 5); + + assert_eq!( + *tx.mut_repo().view().heads(), + hashset! { + new_head.id().clone(), + } + ); +} + +#[test] +fn test_rebase_abandon_all_empty() { + test_empty_commit_option(EmptyBehaviour::AbandonAllEmpty) +} + +#[test] +fn test_rebase_abandon_newly_empty() { + test_empty_commit_option(EmptyBehaviour::AbandonNewlyEmpty) +} + +#[test] +fn test_rebase_keep_empty() { + test_empty_commit_option(EmptyBehaviour::Keep) +} + #[test] fn test_rebase_descendants_sideways() { let settings = testutils::user_settings(); @@ -1497,3 +1648,22 @@ fn test_rebase_descendants_update_checkout_abandoned_merge() { let checkout = repo.store().get_commit(new_checkout_id).unwrap(); assert_eq!(checkout.parent_ids(), vec![commit_b.id().clone()]); } + +pub fn assert_rebase_skipped( + rebased: Option, + expected_old_commit: &Commit, + expected_new_commit: &Commit, +) { + if let Some(RebasedDescendant { + old_commit, + new_commit, + }) = rebased + { + assert_eq!(old_commit, *expected_old_commit,); + assert_eq!(new_commit, *expected_new_commit); + // Since it was abandoned, the change ID should be different. + assert_ne!(old_commit.change_id(), new_commit.change_id()); + } else { + panic!("expected rebased commit: {rebased:?}"); + } +}