Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rewrite.rs: fix working copy position after jj rebase --abandon-empty #2901

Merged
merged 2 commits into from
Jan 31, 2024
Merged

rewrite.rs: fix working copy position after jj rebase --abandon-empty #2901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2145fcf
rewrite.rs DescendantRebaser: rename variable for clarity
ilyagr Jan 31, 2024
195f4c0
rewrite.rs: fix working copy position after `jj rebase --abandon-empty`
ilyagr Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 36 additions & 17 deletions lib/src/rewrite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,27 @@ pub fn rebase_commit(
new_parents,
&Default::default(),
)
.map(|(commit, abandoned_old_commit)| {
assert!(
!abandoned_old_commit,
"Old commit should never be abandoned since the default options include \
EmptyBehavior::Keep"
);
commit
})
}

/// Returns the new parent commit, and whether the old commit was abandoned
///
/// It should only be possible for the old commit to be abandoned if
/// `options.empty != EmptyBehavior::Keep`
pub fn rebase_commit_with_options(
settings: &UserSettings,
mut_repo: &mut MutableRepo,
old_commit: &Commit,
new_parents: &[Commit],
options: &RebaseOptions,
) -> Result<Commit, TreeMergeError> {
) -> Result<(Commit, bool), TreeMergeError> {
let old_parents = old_commit.parents();
let old_parent_trees = old_parents
.iter()
Expand Down Expand Up @@ -164,23 +176,26 @@ pub fn rebase_commit_with_options(
// 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()) {
if should_abandon {
// 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());
return Ok((parent.clone(), true));
}
}
let new_parent_ids = new_parents
.iter()
.map(|commit| commit.id().clone())
.collect();
Ok(mut_repo
.rewrite_commit(settings, old_commit)
.set_parents(new_parent_ids)
.set_tree_id(new_tree_id)
.write()?)
Ok((
mut_repo
.rewrite_commit(settings, old_commit)
.set_parents(new_parent_ids)
.set_tree_id(new_tree_id)
.write()?,
false,
))
}

pub fn rebase_to_dest_parent(
Expand Down Expand Up @@ -456,10 +471,10 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
&mut self,
old_commit_id: CommitId,
new_commit_ids: Vec<CommitId>,
edit: bool,
abandoned_old_commit: bool,
) -> Result<(), BackendError> {
// We arbitrarily pick a new working-copy commit among the candidates.
self.update_wc_commits(&old_commit_id, &new_commit_ids[0], edit)?;
self.update_wc_commits(&old_commit_id, &new_commit_ids[0], abandoned_old_commit)?;

if let Some(branch_names) = self.branches.get(&old_commit_id).cloned() {
let mut branch_updates = vec![];
Expand Down Expand Up @@ -496,7 +511,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
&mut self,
old_commit_id: &CommitId,
new_commit_id: &CommitId,
edit: bool,
abandoned_old_commit: bool,
) -> Result<(), BackendError> {
let workspaces_to_update = self
.mut_repo
Expand All @@ -507,7 +522,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
}

let new_commit = self.mut_repo.store().get_commit(new_commit_id)?;
let new_wc_commit = if edit {
let new_wc_commit = if !abandoned_old_commit {
new_commit
} else {
self.mut_repo
Expand All @@ -534,13 +549,13 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
// (i.e. it's part of the input for this rebase). We don't need
// to rebase it, but we still want to update branches pointing
// to the old commit.
self.update_references(old_commit_id, new_parent_ids, true)?;
self.update_references(old_commit_id, new_parent_ids, false)?;
continue;
}
if let Some(divergent_ids) = self.divergent.get(&old_commit_id).cloned() {
// Leave divergent commits in place. Don't update `parent_mapping` since we
// don't want to rebase descendants either.
self.update_references(old_commit_id, divergent_ids, true)?;
self.update_references(old_commit_id, divergent_ids, false)?;
continue;
}
let old_parent_ids = old_commit.parent_ids();
Expand All @@ -549,7 +564,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
// Update the `new_parents` map so descendants are rebased correctly.
self.parent_mapping
.insert(old_commit_id.clone(), new_parent_ids.clone());
self.update_references(old_commit_id, new_parent_ids, false)?;
self.update_references(old_commit_id, new_parent_ids, true)?;
continue;
} else if new_parent_ids == old_parent_ids {
// The commit is already in place.
Expand All @@ -568,7 +583,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
.filter(|new_parent| head_set.contains(new_parent))
.map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id))
.try_collect()?;
let new_commit = rebase_commit_with_options(
let (new_commit, abandoned_old_commit) = rebase_commit_with_options(
self.settings,
self.mut_repo,
&old_commit,
Expand All @@ -586,7 +601,11 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
(None, None),
"Trying to rebase the same commit {old_commit_id:?} in two different ways",
);
self.update_references(old_commit_id, vec![new_commit.id().clone()], true)?;
self.update_references(
old_commit_id,
vec![new_commit.id().clone()],
abandoned_old_commit,
)?;
return Ok(Some(RebasedDescendant {
old_commit,
new_commit,
Expand Down
44 changes: 24 additions & 20 deletions lib/tests/test_rewrite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1625,21 +1625,15 @@ fn test_rebase_abandoning_empty() {
// We expect B, D, and G to be skipped because they're empty, but not E because
// it's the working commit. F also remains as it's not empty.
//
// F G (empty) F'
// |/ |
// E (WC, empty) D (empty) E' (WC, empty)
// | / |
// F G (empty)
// |/
// E (WC, empty) D (empty) F' new_working_copy_commit (new, WC, empty)
// | / | /
// C------------- C'
// | => |
// B B2 B2
// |/ |
// A A
//
// TODO(#2869): There is a minor bug here. We'd like the result to be
// equivalent to rebasing and then `jj abandon`-ing all the empty commits.
// In case of the working copy, this would mean that F' would be a direct
// child of C', and the working copy would be a new commit that's also a
// direct child of C'.

let mut tx = repo.start_transaction(&settings);
let commit_a = write_random_commit(tx.mut_repo(), &settings);
Expand Down Expand Up @@ -1700,19 +1694,29 @@ fn test_rebase_abandoning_empty() {
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_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_e, new_commit_c.id());
let new_commit_f =
assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_f, &[&new_commit_e.id()]);
assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_g, new_commit_e.id());
assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_f, &[&new_commit_c.id()]);
assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_g, new_commit_c.id());

let new_working_copy_commit_id = tx
.mut_repo()
.view()
.get_wc_commit_id(&workspace)
.unwrap()
.clone();
let new_working_copy_commit = repo
.store()
.get_commit(&new_working_copy_commit_id)
.unwrap()
.clone();

assert_ne!(new_working_copy_commit.change_id(), commit_c.change_id());
assert_ne!(new_working_copy_commit.change_id(), commit_e.change_id());
assert_ne!(new_working_copy_commit.change_id(), commit_f.change_id());
assert_ne!(new_working_copy_commit.change_id(), commit_g.change_id());
assert_eq!(
*tx.mut_repo().view().heads(),
hashset! {new_commit_f.id().clone()}
);

assert_eq!(
tx.mut_repo().view().get_wc_commit_id(&workspace),
Some(new_commit_e.id())
hashset! {new_commit_f.id().clone(), new_working_copy_commit_id}
);
}
Loading