From 3a351bee7df5c2f675ee109e5ad92a5ca4fee607 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 5 Jul 2022 23:01:21 -0700 Subject: [PATCH] cli: don't keep Repo references from before transaction start We had a recent bug where we used a repo reference from before we started a transaction and modified the repo. While it's often safe and correct to use such references, it isn't always. This patch removes all such cases. I think it generally makes the code clearer, and better prepared for #50, if we ever get around to that. I found these by temporarily making `WorkspaceCommandHelper::start_transaction()` take a mutable reference. --- lib/src/transaction.rs | 4 ++++ src/commands.rs | 42 +++++++++++++++++++----------------------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/lib/src/transaction.rs b/lib/src/transaction.rs index 5b334b6cf..04b35d971 100644 --- a/lib/src/transaction.rs +++ b/lib/src/transaction.rs @@ -52,6 +52,10 @@ impl Transaction { self.tags.insert(key, value); } + pub fn repo(&self) -> &MutableRepo { + self.repo.as_ref().unwrap() + } + pub fn mut_repo(&mut self) -> &mut MutableRepo { self.repo.as_mut().unwrap() } diff --git a/src/commands.rs b/src/commands.rs index 4f53dcd70..6d8790217 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -3587,7 +3587,6 @@ fn cmd_squash(ui: &mut Ui, command: &CommandHelper, args: &SquashArgs) -> Result let mut workspace_command = command.workspace_helper(ui)?; let commit = workspace_command.resolve_single_rev(ui, &args.revision)?; workspace_command.check_rewriteable(&commit)?; - let repo = workspace_command.repo(); let parents = commit.parents(); if parents.len() != 1 { return Err(CommandError::UserError(String::from( @@ -3598,7 +3597,6 @@ fn cmd_squash(ui: &mut Ui, command: &CommandHelper, args: &SquashArgs) -> Result workspace_command.check_rewriteable(parent)?; let mut tx = workspace_command.start_transaction(&format!("squash commit {}", commit.id().hex())); - let mut_repo = tx.mut_repo(); let instructions = format!( "\ You are moving changes from: {} @@ -3630,7 +3628,8 @@ from the source will be moved into the parent. // Abandon the child if the parent now has all the content from the child // (always the case in the non-interactive case). let abandon_child = - &new_parent_tree_id == commit.tree_id() && !repo.view().is_checkout(commit.id()); + &new_parent_tree_id == commit.tree_id() && !tx.base_repo().view().is_checkout(commit.id()); + let mut_repo = tx.mut_repo(); let new_parent = CommitBuilder::for_rewrite_from(ui.settings(), parent) .set_tree(new_parent_tree_id) .set_predecessors(vec![parent.id().clone(), commit.id().clone()]) @@ -3655,7 +3654,6 @@ fn cmd_unsquash( let mut workspace_command = command.workspace_helper(ui)?; let commit = workspace_command.resolve_single_rev(ui, &args.revision)?; workspace_command.check_rewriteable(&commit)?; - let repo = workspace_command.repo(); let parents = commit.parents(); if parents.len() != 1 { return Err(CommandError::UserError(String::from( @@ -3666,7 +3664,6 @@ fn cmd_unsquash( workspace_command.check_rewriteable(parent)?; let mut tx = workspace_command.start_transaction(&format!("unsquash commit {}", commit.id().hex())); - let mut_repo = tx.mut_repo(); let parent_base_tree = merge_commit_trees(workspace_command.repo().as_repo_ref(), &parent.parents()); let new_parent_tree_id; @@ -3696,21 +3693,23 @@ aborted. } // Abandon the parent if it is now empty (always the case in the non-interactive // case). - if &new_parent_tree_id == parent_base_tree.id() && !repo.view().is_checkout(parent.id()) { - mut_repo.record_abandoned_commit(parent.id().clone()); + if &new_parent_tree_id == parent_base_tree.id() + && !tx.base_repo().view().is_checkout(parent.id()) + { + tx.mut_repo().record_abandoned_commit(parent.id().clone()); // Commit the new child on top of the parent's parents. CommitBuilder::for_rewrite_from(ui.settings(), &commit) .set_parents(parent.parent_ids()) - .write_to_repo(mut_repo); + .write_to_repo(tx.mut_repo()); } else { let new_parent = CommitBuilder::for_rewrite_from(ui.settings(), parent) .set_tree(new_parent_tree_id) .set_predecessors(vec![parent.id().clone(), commit.id().clone()]) - .write_to_repo(mut_repo); + .write_to_repo(tx.mut_repo()); // Commit the new child on top of the new parent. CommitBuilder::for_rewrite_from(ui.settings(), &commit) .set_parents(vec![new_parent.id().clone()]) - .write_to_repo(mut_repo); + .write_to_repo(tx.mut_repo()); } workspace_command.finish_transaction(ui, tx)?; Ok(()) @@ -3840,8 +3839,7 @@ fn cmd_split(ui: &mut Ui, command: &CommandHelper, args: &SplitArgs) -> Result<( let mut workspace_command = command.workspace_helper(ui)?; let commit = workspace_command.resolve_single_rev(ui, &args.revision)?; workspace_command.check_rewriteable(&commit)?; - let repo = workspace_command.repo(); - let base_tree = merge_commit_trees(repo.as_repo_ref(), &commit.parents()); + let base_tree = merge_commit_trees(workspace_command.repo().as_repo_ref(), &commit.parents()); let instructions = format!( "\ You are splitting a commit in two: {} @@ -3868,20 +3866,19 @@ any changes, then the operation will be aborted. } else { let mut tx = workspace_command.start_transaction(&format!("split commit {}", commit.id().hex())); - let mut_repo = tx.mut_repo(); let first_description = edit_description( ui, - repo, + tx.base_repo(), &("JJ: Enter commit description for the first part.\n".to_string() + commit.description()), )?; let first_commit = CommitBuilder::for_rewrite_from(ui.settings(), &commit) .set_tree(tree_id) .set_description(first_description) - .write_to_repo(mut_repo); + .write_to_repo(tx.mut_repo()); let second_description = edit_description( ui, - repo, + tx.base_repo(), &("JJ: Enter commit description for the second part.\n".to_string() + commit.description()), )?; @@ -3890,10 +3887,10 @@ any changes, then the operation will be aborted. .set_tree(commit.tree_id().clone()) .generate_new_change_id() .set_description(second_description) - .write_to_repo(mut_repo); + .write_to_repo(tx.mut_repo()); let mut rebaser = DescendantRebaser::new( ui.settings(), - mut_repo, + tx.mut_repo(), hashmap! { commit.id().clone() => hashset!{second_commit.id().clone()} }, hashset! {}, ); @@ -3904,13 +3901,13 @@ any changes, then the operation will be aborted. } ui.write("First part: ")?; ui.write_commit_summary( - mut_repo.as_repo_ref(), + tx.repo().as_repo_ref(), &workspace_command.workspace_id(), &first_commit, )?; ui.write("\nSecond part: ")?; ui.write_commit_summary( - mut_repo.as_repo_ref(), + tx.repo().as_repo_ref(), &workspace_command.workspace_id(), &second_commit, )?; @@ -3938,17 +3935,16 @@ fn cmd_merge(ui: &mut Ui, command: &CommandHelper, args: &MergeArgs) -> Result<( parent_ids.push(commit.id().clone()); commits.push(commit); } - let repo = workspace_command.repo(); let description = if let Some(message) = &args.message { message.to_string() } else { edit_description( ui, - repo, + workspace_command.repo(), "\n\nJJ: Enter commit description for the merge commit.\n", )? }; - let merged_tree = merge_commit_trees(repo.as_repo_ref(), &commits); + let merged_tree = merge_commit_trees(workspace_command.repo().as_repo_ref(), &commits); let mut tx = workspace_command.start_transaction("merge commits"); CommitBuilder::for_new_commit(ui.settings(), merged_tree.id().clone()) .set_parents(parent_ids)