mirrors/jj
1
0
Fork 1
mirror of https://github.com/martinvonz/jj.git synced 2025-02-06 11:34:54 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 10

rewrite: add option to delete abandoned bookmarks

Browse source 
I'll make "jj abandon" delete bookmarks by default. This could be handled by
cmd_abandon(), but we'll need a repo API if we also want to change the behavior
of "jj rebase --skip-emptied".
This commit is contained in:
Yuya Nishihara 2025-01-16 15:20:46 +09:00
parent ace041ec96
commit c50a70906b
4 changed files with 210 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
cli/src/commands/rebase.rs
View file

@ -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)?;

61
lib/src/repo.rs
View file

@ -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<CommitId, Vec<CommitId>>) {
fn update_local_bookmarks(
&mut self,
rewrite_mapping: &HashMap<CommitId, Vec<CommitId>>,
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 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 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 = 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<CommitId>,
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<CommitId>,
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)?;

16
lib/src/rewrite.rs
View file

@ -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,

158
lib/tests/test_rewrite.rs
View file

@ -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"),
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()]);