diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index bff298e6f..1ddbb9fab 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -214,8 +214,8 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { fn new_parents(&self, old_ids: &[CommitId]) -> Vec { let mut new_ids = vec![]; for old_id in old_ids { - if let Some(replacements) = self.new_parents.get(old_id) { - new_ids.extend(replacements.clone()); + if let Some(new_parent_ids) = self.new_parents.get(old_id) { + new_ids.extend(new_parent_ids.clone()); } else if let Some(new_parent_id) = self.rebased.get(old_id) { new_ids.push(new_parent_id.clone()); } else { @@ -239,7 +239,18 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { ) } - fn update_branches(&mut self, old_commit_id: CommitId, new_commit_ids: Vec) { + fn update_branches_and_checkout( + &mut self, + old_commit_id: CommitId, + new_commit_ids: Vec, + ) { + if *self.mut_repo.view().checkout() == old_commit_id { + // We arbitrarily pick a new checkout among the candidates. + let new_commit_id = new_commit_ids[0].clone(); + let new_commit = self.mut_repo.store().get_commit(&new_commit_id).unwrap(); + self.mut_repo.check_out(self.settings, &new_commit); + } + if let Some(branch_names) = self.branches.get(&old_commit_id) { let view = self.mut_repo.view(); let mut branch_updates = vec![]; @@ -285,21 +296,23 @@ 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_branches(old_commit_id, new_parent_ids); + self.update_branches_and_checkout(old_commit_id, new_parent_ids); continue; } - if let Some(new_commit_ids) = self.divergent.get(&old_commit_id).cloned() { + if let Some(divergent_ids) = self.divergent.get(&old_commit_id).cloned() { // Leave divergent commits in place. Don't update `new_parents` since we don't // want to rebase descendants either. - self.update_branches(old_commit_id, new_commit_ids); + self.update_branches_and_checkout(old_commit_id, divergent_ids); continue; } let old_commit = self.mut_repo.store().get_commit(&old_commit_id).unwrap(); let old_parent_ids = old_commit.parent_ids(); let new_parent_ids = self.new_parents(&old_parent_ids); if self.to_skip.contains(&old_commit_id) { - // Update the `replacements` map so descendants are rebased correctly. - self.new_parents.insert(old_commit_id, new_parent_ids); + // Update the `new_parents` map so descendants are rebased correctly. + self.new_parents + .insert(old_commit_id.clone(), new_parent_ids.clone()); + self.update_branches_and_checkout(old_commit_id, new_parent_ids); continue; } else if new_parent_ids == old_parent_ids { // The commit is already in place. @@ -320,7 +333,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { .map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id).unwrap()) .collect_vec(); let new_commit = rebase_commit(self.settings, self.mut_repo, &old_commit, &new_parents); - self.update_branches(old_commit_id.clone(), vec![new_commit.id().clone()]); + self.update_branches_and_checkout(old_commit_id.clone(), vec![new_commit.id().clone()]); self.rebased.insert(old_commit_id, new_commit.id().clone()); return Some(RebasedDescendant { old_commit, diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 8a0b06298..e13886644 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -17,7 +17,7 @@ use jujutsu_lib::op_store::RefTarget; use jujutsu_lib::repo_path::RepoPath; use jujutsu_lib::rewrite::DescendantRebaser; use jujutsu_lib::testutils; -use jujutsu_lib::testutils::{assert_rebased, CommitGraphBuilder}; +use jujutsu_lib::testutils::{assert_rebased, create_random_commit, CommitGraphBuilder}; use maplit::{hashmap, hashset}; use test_case::test_case; @@ -836,3 +836,113 @@ fn test_rebase_descendants_rewrite_resolves_branch_conflict() { // // Now we hide B and make A visible instead. When that diff is applied to the // branch, the branch state becomes empty and is thus deleted. + +#[test_case(false ; "local backend")] +#[test_case(true ; "git backend")] +fn test_rebase_descendants_update_checkout_open(use_git: bool) { + let settings = testutils::user_settings(); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); + + // Checked-out, open commit B was replaced by open commit C. C should become + // checked out. + // + // C B + // |/ + // A + let mut tx = repo.start_transaction("test"); + let commit_a = create_random_commit(&settings, &repo).write_to_repo(tx.mut_repo()); + let commit_b = create_random_commit(&settings, &repo) + .set_parents(vec![commit_a.id().clone()]) + .set_open(true) + .write_to_repo(tx.mut_repo()); + tx.mut_repo().set_checkout(commit_b.id().clone()); + let repo = tx.commit(); + + let mut tx = repo.start_transaction("test"); + let commit_c = CommitBuilder::for_rewrite_from(&settings, repo.store(), &commit_b) + .set_description("C".to_string()) + .write_to_repo(tx.mut_repo()); + tx.mut_repo() + .create_descendant_rebaser(&settings) + .rebase_all(); + let repo = tx.commit(); + + assert_eq!(repo.view().checkout(), commit_c.id()); +} + +#[test_case(false ; "local backend")] +#[test_case(true ; "git backend")] +fn test_rebase_descendants_update_checkout_closed(use_git: bool) { + let settings = testutils::user_settings(); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); + + // Checked-out, open commit B was replaced by closed commit C. A child of C + // should become checked out. + // + // C B + // |/ + // A + let mut tx = repo.start_transaction("test"); + let commit_a = create_random_commit(&settings, &repo).write_to_repo(tx.mut_repo()); + let commit_b = create_random_commit(&settings, &repo) + .set_parents(vec![commit_a.id().clone()]) + .set_open(true) + .write_to_repo(tx.mut_repo()); + tx.mut_repo().set_checkout(commit_b.id().clone()); + let repo = tx.commit(); + + let mut tx = repo.start_transaction("test"); + let commit_c = CommitBuilder::for_rewrite_from(&settings, repo.store(), &commit_b) + .set_description("C".to_string()) + .set_open(false) + .write_to_repo(tx.mut_repo()); + tx.mut_repo() + .create_descendant_rebaser(&settings) + .rebase_all(); + let repo = tx.commit(); + + let checkout = repo.store().get_commit(repo.view().checkout()).unwrap(); + assert!(checkout.is_open()); + assert_eq!(checkout.parent_ids(), vec![commit_c.id().clone()]); +} + +#[test_case(false ; "local backend")] +#[test_case(true ; "git backend")] +fn test_rebase_descendants_update_checkout_abandoned_merge(use_git: bool) { + let settings = testutils::user_settings(); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); + + // Checked-out, open merge commit D was abandoned. A parent commit should become + // checked out. + // + // D + // |\ + // B C + // |/ + // A + let mut tx = repo.start_transaction("test"); + let commit_a = create_random_commit(&settings, &repo).write_to_repo(tx.mut_repo()); + let commit_b = create_random_commit(&settings, &repo) + .set_parents(vec![commit_a.id().clone()]) + .write_to_repo(tx.mut_repo()); + let commit_c = create_random_commit(&settings, &repo) + .set_parents(vec![commit_a.id().clone()]) + .write_to_repo(tx.mut_repo()); + let commit_d = create_random_commit(&settings, &repo) + .set_parents(vec![commit_b.id().clone(), commit_c.id().clone()]) + .set_open(true) + .write_to_repo(tx.mut_repo()); + tx.mut_repo().set_checkout(commit_d.id().clone()); + let repo = tx.commit(); + + let mut tx = repo.start_transaction("test"); + tx.mut_repo().record_abandoned_commit(commit_d.id().clone()); + tx.mut_repo() + .create_descendant_rebaser(&settings) + .rebase_all(); + let repo = tx.commit(); + + let checkout = repo.store().get_commit(repo.view().checkout()).unwrap(); + assert!(checkout.is_open()); + assert_eq!(checkout.parent_ids(), vec![commit_b.id().clone()]); +} diff --git a/src/commands.rs b/src/commands.rs index ee217d4d7..010bf9b9c 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -199,12 +199,9 @@ struct RepoCommandHelper { repo: Arc, may_update_working_copy: bool, working_copy_committed: bool, - // Whether to evolve orphans when the transaction + // Whether to rebase descendants when the transaction // finishes. This should generally be true for commands that rewrite commits. rebase_descendants: bool, - // Whether the checkout should be updated to an appropriate successor when the transaction - // finishes. This should generally be true for commands that rewrite commits. - auto_update_checkout: bool, } impl RepoCommandHelper { @@ -222,7 +219,6 @@ impl RepoCommandHelper { may_update_working_copy, working_copy_committed: false, rebase_descendants: true, - auto_update_checkout: true, }) } @@ -231,11 +227,6 @@ impl RepoCommandHelper { self } - fn auto_update_checkout(mut self, value: bool) -> Self { - self.auto_update_checkout = value; - self - } - fn repo(&self) -> &Arc { &self.repo } @@ -431,9 +422,6 @@ impl RepoCommandHelper { writeln!(ui, "Rebased {} descendant commits", num_rebased)?; } } - if self.auto_update_checkout { - update_checkout_after_rewrite(ui, mut_repo)?; - } self.repo = tx.commit(); update_working_copy(ui, &self.repo, &self.repo.working_copy_locked()) } @@ -598,36 +586,6 @@ fn rebase_descendants(settings: &UserSettings, mut_repo: &mut MutableRepo) -> i3 num_rebased } -fn update_checkout_after_rewrite(ui: &mut Ui, mut_repo: &mut MutableRepo) -> io::Result<()> { - // TODO: Perhaps this method should be in MutableRepo. - let new_checkout_candidates = mut_repo - .evolution() - .new_parent(mut_repo.as_repo_ref(), mut_repo.view().checkout()); - if new_checkout_candidates.is_empty() { - return Ok(()); - } - // Filter out heads that already existed. - // TODO: Filter out *commits* that already existed (so we get updated to an - // appropriate new non-head) - let old_heads = mut_repo.base_repo().view().heads().clone(); - let new_checkout_candidates: HashSet<_> = new_checkout_candidates - .difference(&old_heads) - .cloned() - .collect(); - if new_checkout_candidates.is_empty() { - return Ok(()); - } - if new_checkout_candidates.len() > 1 { - ui.write( - "There are several candidates for updating the checkout to -- picking arbitrarily\n", - )?; - } - let new_checkout = new_checkout_candidates.iter().min().unwrap(); - let new_commit = mut_repo.store().get_commit(new_checkout).unwrap(); - mut_repo.check_out(ui.settings(), &new_commit); - Ok(()) -} - fn get_app<'a, 'b>() -> App<'a, 'b> { let init_command = SubCommand::with_name("init") .about("Create a new repo in the given directory") @@ -1384,7 +1342,7 @@ fn cmd_checkout( command: &CommandHelper, sub_matches: &ArgMatches, ) -> Result<(), CommandError> { - let mut repo_command = command.repo_helper(ui)?.auto_update_checkout(false); + let mut repo_command = command.repo_helper(ui)?; let new_commit = repo_command.resolve_revision_arg(ui, sub_matches)?; repo_command.commit_working_copy(ui)?; let mut tx = @@ -2652,10 +2610,7 @@ fn cmd_branch( command: &CommandHelper, sub_matches: &ArgMatches, ) -> Result<(), CommandError> { - let mut repo_command = command - .repo_helper(ui)? - .auto_update_checkout(false) - .rebase_descendants(false); + let mut repo_command = command.repo_helper(ui)?.rebase_descendants(false); let branch_name = sub_matches.value_of("name").unwrap(); if sub_matches.is_present("delete") { let mut tx = repo_command.start_transaction(&format!("delete branch {}", branch_name));