diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 6c4fbe720..d23c44daf 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -33,6 +33,7 @@ use jj_lib::rewrite::EmptyBehaviour; use jj_lib::rewrite::MoveCommitsStats; use jj_lib::rewrite::MoveCommitsTarget; use jj_lib::rewrite::RebaseOptions; +use jj_lib::rewrite::RewriteRefsOptions; use tracing::instrument; use crate::cli_util::short_commit_hash; @@ -247,6 +248,9 @@ pub(crate) fn cmd_rebase( true => EmptyBehaviour::AbandonNewlyEmpty, false => EmptyBehaviour::Keep, }, + rewrite_refs: RewriteRefsOptions { + delete_abandoned_bookmarks: false, + }, simplify_ancestor_merge: false, }; let mut workspace_command = command.workspace_helper(ui)?; diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 9df464460..1c82e3c3c 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -86,6 +86,7 @@ use crate::rewrite::rebase_commit_with_options; use crate::rewrite::CommitRewriter; use crate::rewrite::RebaseOptions; use crate::rewrite::RebasedCommit; +use crate::rewrite::RewriteRefsOptions; use crate::settings::UserSettings; use crate::signing::SignInitError; use crate::signing::Signer; @@ -932,13 +933,12 @@ impl MutableRepo { /// Record a commit as having been abandoned in this transaction. /// /// This record is used by `rebase_descendants` to know which commits have - /// children that need to be rebased, and where to rebase the children (as - /// well as bookmarks) to. + /// children that need to be rebased, and where to rebase the children to. /// /// The `rebase_descendants` logic will rebase the descendants of the old /// commit to become the descendants of parent(s) of the old commit. Any - /// bookmarks at the old commit would be moved to the parent(s) of the old - /// commit as well. + /// bookmarks at the old commit will be either moved to the parent(s) of the + /// old commit or deleted depending on [`RewriteRefsOptions`]. pub fn record_abandoned_commit(&mut self, old_commit: &Commit) { assert_ne!(old_commit.id(), self.store().root_commit_id()); // Descendants should be rebased onto the commit's parents @@ -1054,20 +1054,27 @@ impl MutableRepo { /// Updates bookmarks, working copies, and anonymous heads after rewriting /// and/or abandoning commits. - pub fn update_rewritten_references(&mut self) -> BackendResult<()> { - self.update_all_references()?; + pub fn update_rewritten_references( + &mut self, + options: &RewriteRefsOptions, + ) -> BackendResult<()> { + self.update_all_references(options)?; self.update_heads(); Ok(()) } - fn update_all_references(&mut self) -> BackendResult<()> { + fn update_all_references(&mut self, options: &RewriteRefsOptions) -> BackendResult<()> { let rewrite_mapping = self.resolve_rewrite_mapping_with(|_| true); - self.update_local_bookmarks(&rewrite_mapping); + self.update_local_bookmarks(&rewrite_mapping, options); self.update_wc_commits(&rewrite_mapping)?; Ok(()) } - fn update_local_bookmarks(&mut self, rewrite_mapping: &HashMap>) { + fn update_local_bookmarks( + &mut self, + rewrite_mapping: &HashMap>, + options: &RewriteRefsOptions, + ) { let changed_branches = self .view() .local_bookmarks() @@ -1079,14 +1086,19 @@ impl MutableRepo { }) .collect_vec(); for (bookmark_name, (old_commit_id, new_commit_ids)) in changed_branches { + let should_delete = options.delete_abandoned_bookmarks + && matches!( + self.parent_mapping.get(old_commit_id), + Some(Rewrite::Abandoned(_)) + ); let old_target = RefTarget::normal(old_commit_id.clone()); - let new_target = RefTarget::from_merge( - MergeBuilder::from_iter( - itertools::intersperse(new_commit_ids, old_commit_id) - .map(|id| Some(id.clone())), - ) - .build(), - ); + let new_target = if should_delete { + RefTarget::absent() + } else { + let ids = itertools::intersperse(new_commit_ids, old_commit_id) + .map(|id| Some(id.clone())); + RefTarget::from_merge(MergeBuilder::from_iter(ids).build()) + }; self.merge_local_bookmark(&bookmark_name, &old_target, &new_target); } @@ -1221,6 +1233,19 @@ impl MutableRepo { pub fn transform_descendants( &mut self, roots: Vec, + callback: impl FnMut(CommitRewriter) -> BackendResult<()>, + ) -> BackendResult<()> { + let options = RewriteRefsOptions::default(); + self.transform_descendants_with_options(roots, &options, callback) + } + + /// Rewrite descendants of the given roots with options. + /// + /// See [`Self::transform_descendants()`] for details. + pub fn transform_descendants_with_options( + &mut self, + roots: Vec, + options: &RewriteRefsOptions, mut callback: impl FnMut(CommitRewriter) -> BackendResult<()>, ) -> BackendResult<()> { let mut to_visit = self.find_descendants_to_rebase(roots)?; @@ -1229,7 +1254,7 @@ impl MutableRepo { let rewriter = CommitRewriter::new(self, old_commit, new_parent_ids); callback(rewriter)?; } - self.update_rewritten_references()?; + self.update_rewritten_references(options)?; // Since we didn't necessarily visit all descendants of rewritten commits (e.g. // if they were rewritten in the callback), there can still be commits left to // rebase, so we don't clear `parent_mapping` here. @@ -1263,7 +1288,7 @@ impl MutableRepo { mut progress: impl FnMut(Commit, RebasedCommit), ) -> BackendResult<()> { let roots = self.parent_mapping.keys().cloned().collect(); - self.transform_descendants(roots, |rewriter| { + self.transform_descendants_with_options(roots, &options.rewrite_refs, |rewriter| { if rewriter.parents_changed() { let old_commit = rewriter.old_commit().clone(); let rebased_commit = rebase_commit_with_options(rewriter, options)?; diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 1676072e0..04e69413a 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -365,14 +365,25 @@ pub enum EmptyBehaviour { // change the RebaseOptions construction in the CLI, and changing the // rebase_commit function to actually use the flag, and ensure we don't need to // plumb it in. -#[derive(Clone, Default, PartialEq, Eq, Debug)] +#[derive(Clone, Debug, Default)] pub struct RebaseOptions { pub empty: EmptyBehaviour, + pub rewrite_refs: RewriteRefsOptions, /// If a merge commit would end up with one parent being an ancestor of the /// other, then filter out the ancestor. pub simplify_ancestor_merge: bool, } +/// Configuration for [`MutableRepo::update_rewritten_references()`]. +#[derive(Clone, Debug, Default)] +pub struct RewriteRefsOptions { + /// Whether or not delete bookmarks pointing to the abandoned commits. + /// + /// If false, bookmarks will be moved to the parents of the abandoned + /// commit. + pub delete_abandoned_bookmarks: bool, +} + #[derive(Default)] pub struct MoveCommitsStats { /// The number of commits in the target set which were rebased. @@ -728,6 +739,7 @@ pub fn move_commits( // Always keep empty commits when rebasing descendants. let rebase_descendant_options = &RebaseOptions { empty: EmptyBehaviour::Keep, + rewrite_refs: options.rewrite_refs.clone(), simplify_ancestor_merge: options.simplify_ancestor_merge, }; @@ -759,7 +771,7 @@ pub fn move_commits( num_skipped_rebases += 1; } } - mut_repo.update_rewritten_references()?; + mut_repo.update_rewritten_references(&options.rewrite_refs)?; Ok(MoveCommitsStats { num_rebased_targets, diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index efd08b088..1c96154d5 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -16,6 +16,7 @@ use itertools::Itertools as _; use jj_lib::commit::Commit; use jj_lib::matchers::EverythingMatcher; use jj_lib::matchers::FilesMatcher; +use jj_lib::merge::Merge; use jj_lib::merged_tree::MergedTree; use jj_lib::op_store::RefTarget; use jj_lib::op_store::RemoteRef; @@ -28,6 +29,7 @@ use jj_lib::rewrite::restore_tree; use jj_lib::rewrite::CommitRewriter; use jj_lib::rewrite::EmptyBehaviour; use jj_lib::rewrite::RebaseOptions; +use jj_lib::rewrite::RewriteRefsOptions; use maplit::hashmap; use maplit::hashset; use test_case::test_case; @@ -1044,8 +1046,9 @@ fn test_rebase_descendants_basic_bookmark_update_with_non_local_bookmark() { ); } -#[test] -fn test_rebase_descendants_update_bookmark_after_abandon() { +#[test_case(false; "slide down abandoned")] +#[test_case(true; "delete abandoned")] +fn test_rebase_descendants_update_bookmark_after_abandon(delete_abandoned_bookmarks: bool) { let test_repo = TestRepo::init(); let repo = &test_repo.repo; @@ -1056,7 +1059,7 @@ fn test_rebase_descendants_update_bookmark_after_abandon() { // | // B main main@origin C2 other // | => | - // A A main + // A A main (if delete_abandoned_bookmarks = false) let mut tx = repo.start_transaction(); let mut graph_builder = CommitGraphBuilder::new(tx.repo_mut()); let commit_a = graph_builder.initial_commit(); @@ -1076,11 +1079,20 @@ fn test_rebase_descendants_update_bookmark_after_abandon() { let mut tx = repo.start_transaction(); tx.repo_mut().record_abandoned_commit(&commit_b); - let options = RebaseOptions::default(); + let options = RebaseOptions { + rewrite_refs: RewriteRefsOptions { + delete_abandoned_bookmarks, + }, + ..Default::default() + }; let rebase_map = rebase_descendants_with_options_return_map(tx.repo_mut(), &options); assert_eq!( tx.repo_mut().get_local_bookmark("main"), - RefTarget::normal(commit_a.id().clone()) + if delete_abandoned_bookmarks { + RefTarget::absent() + } else { + RefTarget::normal(commit_a.id().clone()) + } ); assert_eq!( tx.repo_mut().get_remote_bookmark("main", "origin").target, @@ -1316,16 +1328,22 @@ fn test_rebase_descendants_rewrite_resolves_bookmark_conflict() { ); } -#[test] -fn test_rebase_descendants_bookmark_delete_modify_abandon() { +#[test_case(false; "slide down abandoned")] +#[test_case(true; "delete abandoned")] +fn test_rebase_descendants_bookmark_delete_modify_abandon(delete_abandoned_bookmarks: bool) { let test_repo = TestRepo::init(); let repo = &test_repo.repo; // Bookmark "main" initially points to commit A. One operation rewrites it to // point to B (child of A). A concurrent operation deletes the bookmark. That - // leaves the bookmark pointing to "-A+B". We now abandon B. That should - // result in the bookmark pointing to "-A+A=0", so the bookmark should - // be deleted. + // leaves the bookmark pointing to "0-A+B". We now abandon B. + // + // - If delete_abandoned_bookmarks = false, that should result in the bookmark + // pointing to "0-A+A=0". + // - If delete_abandoned_bookmarks = true, that should result in the bookmark + // pointing to "0-A+0=0". + // + // In both cases, the bookmark should be deleted. let mut tx = repo.start_transaction(); let mut graph_builder = CommitGraphBuilder::new(tx.repo_mut()); let commit_a = graph_builder.initial_commit(); @@ -1338,13 +1356,127 @@ fn test_rebase_descendants_bookmark_delete_modify_abandon() { let mut tx = repo.start_transaction(); tx.repo_mut().record_abandoned_commit(&commit_b); - tx.repo_mut().rebase_descendants().unwrap(); + let options = RebaseOptions { + rewrite_refs: RewriteRefsOptions { + delete_abandoned_bookmarks, + }, + ..Default::default() + }; + let _rebase_map = rebase_descendants_with_options_return_map(tx.repo_mut(), &options); assert_eq!( tx.repo_mut().get_local_bookmark("main"), RefTarget::absent() ); } +#[test_case(false; "slide down abandoned")] +#[test_case(true; "delete abandoned")] +fn test_rebase_descendants_bookmark_move_forward_abandon(delete_abandoned_bookmarks: bool) { + let test_repo = TestRepo::init(); + let repo = &test_repo.repo; + + // Bookmark "main" initially points to commit A. Two concurrent operations + // rewrites it to point to A's children. That leaves the bookmark pointing + // to "B-A+C". We now abandon B. + // + // - If delete_abandoned_bookmarks = false, that should result in the bookmark + // pointing to "A-A+C=C", so the conflict should be resolved. + // - If delete_abandoned_bookmarks = true, that should result in the bookmark + // pointing to "0-A+C". + let mut tx = repo.start_transaction(); + let mut graph_builder = CommitGraphBuilder::new(tx.repo_mut()); + let commit_a = graph_builder.initial_commit(); + let commit_b = graph_builder.commit_with_parents(&[&commit_a]); + let commit_c = graph_builder.commit_with_parents(&[&commit_a]); + tx.repo_mut().set_local_bookmark_target( + "main", + RefTarget::from_merge(Merge::from_vec(vec![ + Some(commit_b.id().clone()), + Some(commit_a.id().clone()), + Some(commit_c.id().clone()), + ])), + ); + let repo = tx.commit("test").unwrap(); + + let mut tx = repo.start_transaction(); + tx.repo_mut().record_abandoned_commit(&commit_b); + let options = RebaseOptions { + rewrite_refs: RewriteRefsOptions { + delete_abandoned_bookmarks, + }, + ..Default::default() + }; + let _rebase_map = rebase_descendants_with_options_return_map(tx.repo_mut(), &options); + assert_eq!( + tx.repo_mut().get_local_bookmark("main"), + if delete_abandoned_bookmarks { + RefTarget::from_merge(Merge::from_vec(vec![ + None, + Some(commit_a.id().clone()), + Some(commit_c.id().clone()), + ])) + } else { + RefTarget::normal(commit_c.id().clone()) + } + ); +} + +#[test_case(false; "slide down abandoned")] +#[test_case(true; "delete abandoned")] +fn test_rebase_descendants_bookmark_move_sideways_abandon(delete_abandoned_bookmarks: bool) { + let test_repo = TestRepo::init(); + let repo = &test_repo.repo; + + // Bookmark "main" initially points to commit A. Two concurrent operations + // rewrites it to point to A's siblings. That leaves the bookmark pointing + // to "B-A+C". We now abandon B. + // + // - If delete_abandoned_bookmarks = false, that should result in the bookmark + // pointing to "A.parent-A+C". + // - If delete_abandoned_bookmarks = true, that should result in the bookmark + // pointing to "0-A+C". + let mut tx = repo.start_transaction(); + let mut graph_builder = CommitGraphBuilder::new(tx.repo_mut()); + let commit_a = graph_builder.initial_commit(); + let commit_b = graph_builder.initial_commit(); + let commit_c = graph_builder.initial_commit(); + tx.repo_mut().set_local_bookmark_target( + "main", + RefTarget::from_merge(Merge::from_vec(vec![ + Some(commit_b.id().clone()), + Some(commit_a.id().clone()), + Some(commit_c.id().clone()), + ])), + ); + let repo = tx.commit("test").unwrap(); + + let mut tx = repo.start_transaction(); + tx.repo_mut().record_abandoned_commit(&commit_b); + let options = RebaseOptions { + rewrite_refs: RewriteRefsOptions { + delete_abandoned_bookmarks, + }, + ..Default::default() + }; + let _rebase_map = rebase_descendants_with_options_return_map(tx.repo_mut(), &options); + assert_eq!( + tx.repo_mut().get_local_bookmark("main"), + if delete_abandoned_bookmarks { + RefTarget::from_merge(Merge::from_vec(vec![ + None, + Some(commit_a.id().clone()), + Some(commit_c.id().clone()), + ])) + } else { + RefTarget::from_merge(Merge::from_vec(vec![ + Some(repo.store().root_commit_id().clone()), + Some(commit_a.id().clone()), + Some(commit_c.id().clone()), + ])) + } + ); +} + #[test] fn test_rebase_descendants_update_checkout() { let test_repo = TestRepo::init(); @@ -1557,6 +1689,9 @@ fn test_empty_commit_option(empty_behavior: EmptyBehaviour) { tx.repo_mut(), &RebaseOptions { empty: empty_behavior, + rewrite_refs: RewriteRefsOptions { + delete_abandoned_bookmarks: false, + }, simplify_ancestor_merge: true, }, ); @@ -1689,6 +1824,9 @@ fn test_rebase_abandoning_empty() { let rebase_options = RebaseOptions { empty: EmptyBehaviour::AbandonAllEmpty, + rewrite_refs: RewriteRefsOptions { + delete_abandoned_bookmarks: false, + }, simplify_ancestor_merge: true, }; let rewriter = CommitRewriter::new(tx.repo_mut(), commit_b, vec![commit_b2.id().clone()]);