From 96ddf0b6406de1a0e5fcc1569502a3bb40286068 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 20 Jul 2024 12:25:02 +0900 Subject: [PATCH 1/7] commit_builder: use .clone_from() to silence nightly clippy warning It's not wrong that String::clone_from() could potentially be cheaper. --- lib/src/commit_builder.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs index ec6ce6a691..610d4e8d9c 100644 --- a/lib/src/commit_builder.rs +++ b/lib/src/commit_builder.rs @@ -63,8 +63,6 @@ impl CommitBuilder<'_> { } /// Only called from [`MutRepo::rewrite_commit`]. Use that function instead. - #[allow(unknown_lints)] // XXX FIXME (aseipp): nightly bogons; re-test this occasionally - #[allow(clippy::assigning_clones)] pub(crate) fn for_rewrite_from<'repo>( mut_repo: &'repo mut MutableRepo, settings: &UserSettings, @@ -78,12 +76,12 @@ impl CommitBuilder<'_> { if commit.author.name.is_empty() || commit.author.name == UserSettings::USER_NAME_PLACEHOLDER { - commit.author.name = commit.committer.name.clone(); + commit.author.name.clone_from(&commit.committer.name); } if commit.author.email.is_empty() || commit.author.email == UserSettings::USER_EMAIL_PLACEHOLDER { - commit.author.email = commit.committer.email.clone(); + commit.author.email.clone_from(&commit.committer.email); } // Reset author timestamp on discardable commits if the author is the From 58b16e1eaf73492b0daa98a061f1ac7d81ecf14a Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 20 Jul 2024 20:01:41 +0900 Subject: [PATCH 2/7] commit_builder: remove redundant boxing from signing fn closure --- lib/src/commit_builder.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs index 610d4e8d9c..69d1bca27a 100644 --- a/lib/src/commit_builder.rs +++ b/lib/src/commit_builder.rs @@ -16,7 +16,7 @@ use std::sync::Arc; -use crate::backend::{self, BackendResult, ChangeId, CommitId, MergedTreeId, Signature, SigningFn}; +use crate::backend::{self, BackendResult, ChangeId, CommitId, MergedTreeId, Signature}; use crate::commit::Commit; use crate::repo::{MutableRepo, Repo}; use crate::settings::{JJRng, SignSettings, UserSettings}; @@ -192,18 +192,15 @@ impl CommitBuilder<'_> { let sign_settings = &self.sign_settings; let store = self.mut_repo.store(); - let mut signing_fn = (store.signer().can_sign() && sign_settings.should_sign(&self.commit)) - .then(|| -> Box { - let store = store.clone(); - Box::new(move |data: &_| store.signer().sign(data, sign_settings.key.as_deref())) - }); + let should_sign = store.signer().can_sign() && sign_settings.should_sign(&self.commit); + let sign_fn = |data: &[u8]| store.signer().sign(data, sign_settings.key.as_deref()); // Commit backend doesn't use secure_sig for writing and enforces it with an // assert, but sign_settings.should_sign check above will want to know // if we're rewriting a signed commit self.commit.secure_sig = None; - let commit = store.write_commit(self.commit, signing_fn.as_deref_mut())?; + let commit = store.write_commit(self.commit, should_sign.then_some(&mut &sign_fn))?; self.mut_repo.add_head(&commit)?; if let Some(rewrite_source) = self.rewrite_source { if rewrite_source.change_id() == commit.change_id() { From 9af53068c8ff66d0e15489c5e7c9c5492441fec7 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 19 Jul 2024 15:50:09 +0900 Subject: [PATCH 3/7] commit_builder: keep Store internally I'm going to extract an inner builder that is free from &mut MutableRepo lifetime. --- lib/src/commit_builder.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs index 69d1bca27a..8d267f7ba1 100644 --- a/lib/src/commit_builder.rs +++ b/lib/src/commit_builder.rs @@ -21,10 +21,12 @@ use crate::commit::Commit; use crate::repo::{MutableRepo, Repo}; use crate::settings::{JJRng, SignSettings, UserSettings}; use crate::signing::SignBehavior; +use crate::store::Store; #[must_use] pub struct CommitBuilder<'repo> { mut_repo: &'repo mut MutableRepo, + store: Arc, rng: Arc, commit: backend::Commit, rewrite_source: Option, @@ -39,10 +41,11 @@ impl CommitBuilder<'_> { parents: Vec, tree_id: MergedTreeId, ) -> CommitBuilder<'repo> { + let store = mut_repo.store().clone(); let signature = settings.signature(); assert!(!parents.is_empty()); let rng = settings.get_rng(); - let change_id = rng.new_change_id(mut_repo.store().change_id_length()); + let change_id = rng.new_change_id(store.change_id_length()); let commit = backend::Commit { parents, predecessors: vec![], @@ -55,6 +58,7 @@ impl CommitBuilder<'_> { }; CommitBuilder { mut_repo, + store, rng, commit, rewrite_source: None, @@ -68,6 +72,7 @@ impl CommitBuilder<'_> { settings: &UserSettings, predecessor: &Commit, ) -> CommitBuilder<'repo> { + let store = mut_repo.store().clone(); let mut commit = predecessor.store_commit().clone(); commit.predecessors = vec![predecessor.id().clone()]; commit.committer = settings.signature(); @@ -96,6 +101,7 @@ impl CommitBuilder<'_> { CommitBuilder { mut_repo, + store, commit, rng: settings.get_rng(), rewrite_source: Some(predecessor.clone()), @@ -141,9 +147,7 @@ impl CommitBuilder<'_> { } pub fn generate_new_change_id(mut self) -> Self { - self.commit.change_id = self - .rng - .new_change_id(self.mut_repo.store().change_id_length()); + self.commit.change_id = self.rng.new_change_id(self.store.change_id_length()); self } @@ -190,7 +194,7 @@ impl CommitBuilder<'_> { pub fn write(mut self) -> BackendResult { let sign_settings = &self.sign_settings; - let store = self.mut_repo.store(); + let store = &self.store; let should_sign = store.signer().can_sign() && sign_settings.should_sign(&self.commit); let sign_fn = |data: &[u8]| store.signer().sign(data, sign_settings.key.as_deref()); From 0ddd86e90ba5c9b565cb16517e9cc0829b2ad6ce Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 19 Jul 2024 17:50:35 +0900 Subject: [PATCH 4/7] commit_builder: extract free function that sets up signing and write commit I'll add another write() method that doesn't consume self, which will have to clone self.commit. --- lib/src/commit_builder.rs | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs index 8d267f7ba1..41de68b718 100644 --- a/lib/src/commit_builder.rs +++ b/lib/src/commit_builder.rs @@ -192,19 +192,8 @@ impl CommitBuilder<'_> { self } - pub fn write(mut self) -> BackendResult { - let sign_settings = &self.sign_settings; - let store = &self.store; - - let should_sign = store.signer().can_sign() && sign_settings.should_sign(&self.commit); - let sign_fn = |data: &[u8]| store.signer().sign(data, sign_settings.key.as_deref()); - - // Commit backend doesn't use secure_sig for writing and enforces it with an - // assert, but sign_settings.should_sign check above will want to know - // if we're rewriting a signed commit - self.commit.secure_sig = None; - - let commit = store.write_commit(self.commit, should_sign.then_some(&mut &sign_fn))?; + pub fn write(self) -> BackendResult { + let commit = write_to_store(&self.store, self.commit, &self.sign_settings)?; self.mut_repo.add_head(&commit)?; if let Some(rewrite_source) = self.rewrite_source { if rewrite_source.change_id() == commit.change_id() { @@ -215,3 +204,19 @@ impl CommitBuilder<'_> { Ok(commit) } } + +fn write_to_store( + store: &Arc, + mut commit: backend::Commit, + sign_settings: &SignSettings, +) -> BackendResult { + let should_sign = store.signer().can_sign() && sign_settings.should_sign(&commit); + let sign_fn = |data: &[u8]| store.signer().sign(data, sign_settings.key.as_deref()); + + // Commit backend doesn't use secure_sig for writing and enforces it with an + // assert, but sign_settings.should_sign check above will want to know + // if we're rewriting a signed commit + commit.secure_sig = None; + + store.write_commit(commit, should_sign.then_some(&mut &sign_fn)) +} From ef8615da0767eea4e7033b17340b0d8e974c2c40 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 19 Jul 2024 16:31:29 +0900 Subject: [PATCH 5/7] commit_builder: extract inner builder which isn't lifetimed by mut_repo This allows us to construct a builder, format description template with an intermediate commit, then write() a final commit object to the repo. I originally considered removing mut_repo from CommitBuilder at all, but rewriter APIs rely on that CommitBuilder has &mut_repo, and splitting them would make call sites uglier. The inner builder methods are based on &mut Self instead of Self, because it's easier to wrap, and users of the inner builder will bind it to a named variable anyway. --- lib/src/commit_builder.rs | 170 +++++++++++++++++++++++++++++++------- 1 file changed, 138 insertions(+), 32 deletions(-) diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs index 41de68b718..7b70229d3e 100644 --- a/lib/src/commit_builder.rs +++ b/lib/src/commit_builder.rs @@ -26,11 +26,7 @@ use crate::store::Store; #[must_use] pub struct CommitBuilder<'repo> { mut_repo: &'repo mut MutableRepo, - store: Arc, - rng: Arc, - commit: backend::Commit, - rewrite_source: Option, - sign_settings: SignSettings, + inner: DetachedCommitBuilder, } impl CommitBuilder<'_> { @@ -41,7 +37,125 @@ impl CommitBuilder<'_> { parents: Vec, tree_id: MergedTreeId, ) -> CommitBuilder<'repo> { - let store = mut_repo.store().clone(); + let inner = DetachedCommitBuilder::for_new_commit(mut_repo, settings, parents, tree_id); + CommitBuilder { mut_repo, inner } + } + + /// Only called from [`MutRepo::rewrite_commit`]. Use that function instead. + pub(crate) fn for_rewrite_from<'repo>( + mut_repo: &'repo mut MutableRepo, + settings: &UserSettings, + predecessor: &Commit, + ) -> CommitBuilder<'repo> { + let inner = DetachedCommitBuilder::for_rewrite_from(mut_repo, settings, predecessor); + CommitBuilder { mut_repo, inner } + } + + pub fn parents(&self) -> &[CommitId] { + self.inner.parents() + } + + pub fn set_parents(mut self, parents: Vec) -> Self { + self.inner.set_parents(parents); + self + } + + pub fn predecessors(&self) -> &[CommitId] { + self.inner.predecessors() + } + + pub fn set_predecessors(mut self, predecessors: Vec) -> Self { + self.inner.set_predecessors(predecessors); + self + } + + pub fn tree_id(&self) -> &MergedTreeId { + self.inner.tree_id() + } + + pub fn set_tree_id(mut self, tree_id: MergedTreeId) -> Self { + self.inner.set_tree_id(tree_id); + self + } + + pub fn change_id(&self) -> &ChangeId { + self.inner.change_id() + } + + pub fn set_change_id(mut self, change_id: ChangeId) -> Self { + self.inner.set_change_id(change_id); + self + } + + pub fn generate_new_change_id(mut self) -> Self { + self.inner.generate_new_change_id(); + self + } + + pub fn description(&self) -> &str { + self.inner.description() + } + + pub fn set_description(mut self, description: impl Into) -> Self { + self.inner.set_description(description); + self + } + + pub fn author(&self) -> &Signature { + self.inner.author() + } + + pub fn set_author(mut self, author: Signature) -> Self { + self.inner.set_author(author); + self + } + + pub fn committer(&self) -> &Signature { + self.inner.committer() + } + + pub fn set_committer(mut self, committer: Signature) -> Self { + self.inner.set_committer(committer); + self + } + + pub fn sign_settings(&self) -> &SignSettings { + self.inner.sign_settings() + } + + pub fn set_sign_behavior(mut self, sign_behavior: SignBehavior) -> Self { + self.inner.set_sign_behavior(sign_behavior); + self + } + + pub fn set_sign_key(mut self, sign_key: Option) -> Self { + self.inner.set_sign_key(sign_key); + self + } + + pub fn write(self) -> BackendResult { + self.inner.write(self.mut_repo) + } +} + +/// Like `CommitBuilder`, but doesn't mutably borrow `MutableRepo`. +#[derive(Debug)] +pub struct DetachedCommitBuilder { + store: Arc, + rng: Arc, + commit: backend::Commit, + rewrite_source: Option, + sign_settings: SignSettings, +} + +impl DetachedCommitBuilder { + fn for_new_commit( + repo: &dyn Repo, + settings: &UserSettings, + parents: Vec, + tree_id: MergedTreeId, + ) -> Self { + let store = repo.store().clone(); let signature = settings.signature(); assert!(!parents.is_empty()); let rng = settings.get_rng(); @@ -56,8 +170,7 @@ impl CommitBuilder<'_> { committer: signature, secure_sig: None, }; - CommitBuilder { - mut_repo, + DetachedCommitBuilder { store, rng, commit, @@ -66,13 +179,8 @@ impl CommitBuilder<'_> { } } - /// Only called from [`MutRepo::rewrite_commit`]. Use that function instead. - pub(crate) fn for_rewrite_from<'repo>( - mut_repo: &'repo mut MutableRepo, - settings: &UserSettings, - predecessor: &Commit, - ) -> CommitBuilder<'repo> { - let store = mut_repo.store().clone(); + fn for_rewrite_from(repo: &dyn Repo, settings: &UserSettings, predecessor: &Commit) -> Self { + let store = repo.store().clone(); let mut commit = predecessor.store_commit().clone(); commit.predecessors = vec![predecessor.id().clone()]; commit.committer = settings.signature(); @@ -94,13 +202,12 @@ impl CommitBuilder<'_> { // with no description in our repo, we'd like to be extra safe. if commit.author.name == commit.committer.name && commit.author.email == commit.committer.email - && predecessor.is_discardable(mut_repo).unwrap_or_default() + && predecessor.is_discardable(repo).unwrap_or_default() { commit.author.timestamp = commit.committer.timestamp.clone(); } - CommitBuilder { - mut_repo, + DetachedCommitBuilder { store, commit, rng: settings.get_rng(), @@ -113,7 +220,7 @@ impl CommitBuilder<'_> { &self.commit.parents } - pub fn set_parents(mut self, parents: Vec) -> Self { + pub fn set_parents(&mut self, parents: Vec) -> &mut Self { assert!(!parents.is_empty()); self.commit.parents = parents; self @@ -123,7 +230,7 @@ impl CommitBuilder<'_> { &self.commit.predecessors } - pub fn set_predecessors(mut self, predecessors: Vec) -> Self { + pub fn set_predecessors(&mut self, predecessors: Vec) -> &mut Self { self.commit.predecessors = predecessors; self } @@ -132,7 +239,7 @@ impl CommitBuilder<'_> { &self.commit.root_tree } - pub fn set_tree_id(mut self, tree_id: MergedTreeId) -> Self { + pub fn set_tree_id(&mut self, tree_id: MergedTreeId) -> &mut Self { self.commit.root_tree = tree_id; self } @@ -141,12 +248,12 @@ impl CommitBuilder<'_> { &self.commit.change_id } - pub fn set_change_id(mut self, change_id: ChangeId) -> Self { + pub fn set_change_id(&mut self, change_id: ChangeId) -> &mut Self { self.commit.change_id = change_id; self } - pub fn generate_new_change_id(mut self) -> Self { + pub fn generate_new_change_id(&mut self) -> &mut Self { self.commit.change_id = self.rng.new_change_id(self.store.change_id_length()); self } @@ -155,7 +262,7 @@ impl CommitBuilder<'_> { &self.commit.description } - pub fn set_description(mut self, description: impl Into) -> Self { + pub fn set_description(&mut self, description: impl Into) -> &mut Self { self.commit.description = description.into(); self } @@ -164,7 +271,7 @@ impl CommitBuilder<'_> { &self.commit.author } - pub fn set_author(mut self, author: Signature) -> Self { + pub fn set_author(&mut self, author: Signature) -> &mut Self { self.commit.author = author; self } @@ -173,7 +280,7 @@ impl CommitBuilder<'_> { &self.commit.committer } - pub fn set_committer(mut self, committer: Signature) -> Self { + pub fn set_committer(&mut self, committer: Signature) -> &mut Self { self.commit.committer = committer; self } @@ -182,23 +289,22 @@ impl CommitBuilder<'_> { &self.sign_settings } - pub fn set_sign_behavior(mut self, sign_behavior: SignBehavior) -> Self { + pub fn set_sign_behavior(&mut self, sign_behavior: SignBehavior) -> &mut Self { self.sign_settings.behavior = sign_behavior; self } - pub fn set_sign_key(mut self, sign_key: Option) -> Self { + pub fn set_sign_key(&mut self, sign_key: Option) -> &mut Self { self.sign_settings.key = sign_key; self } - pub fn write(self) -> BackendResult { + pub fn write(self, mut_repo: &mut MutableRepo) -> BackendResult { let commit = write_to_store(&self.store, self.commit, &self.sign_settings)?; - self.mut_repo.add_head(&commit)?; + mut_repo.add_head(&commit)?; if let Some(rewrite_source) = self.rewrite_source { if rewrite_source.change_id() == commit.change_id() { - self.mut_repo - .set_rewritten_commit(rewrite_source.id().clone(), commit.id().clone()); + mut_repo.set_rewritten_commit(rewrite_source.id().clone(), commit.id().clone()); } } Ok(commit) From 9b8e5bb296ee41d7ccea347cb9593ddeee738c00 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 19 Jul 2024 18:07:44 +0900 Subject: [PATCH 6/7] commit_builder: add public interface that writes temporary commit to store In order to render description template, we'll need a Commit object that represents the old state (with new tree and parents) before updating the commit description. The added functions will help generate an intermediate Commit object. Alternatively, we can create an in-memory Commit object with some fake CommitId. It should be lightweight, but might cause weird issue because the fake id wouldn't be found in the store. I think it's okay to write a temporary commit and rely on GC as we do for merge trees. However, I should note that temporary commits are more likely to be preserved as they are pinned by no-gc refs until "jj util gc". --- lib/src/commit_builder.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs index 7b70229d3e..2bb6393894 100644 --- a/lib/src/commit_builder.rs +++ b/lib/src/commit_builder.rs @@ -51,6 +51,12 @@ impl CommitBuilder<'_> { CommitBuilder { mut_repo, inner } } + /// Detaches from `&'repo mut` lifetime. The returned builder can be used in + /// order to obtain a temporary commit object. + pub fn detach(self) -> DetachedCommitBuilder { + self.inner + } + pub fn parents(&self) -> &[CommitId] { self.inner.parents() } @@ -216,6 +222,15 @@ impl DetachedCommitBuilder { } } + /// Attaches the underlying `mut_repo`. + pub fn attach(self, mut_repo: &mut MutableRepo) -> CommitBuilder<'_> { + assert!(Arc::ptr_eq(&self.store, mut_repo.store())); + CommitBuilder { + mut_repo, + inner: self, + } + } + pub fn parents(&self) -> &[CommitId] { &self.commit.parents } @@ -299,6 +314,7 @@ impl DetachedCommitBuilder { self } + /// Writes new commit and makes it visible in the `mut_repo`. pub fn write(self, mut_repo: &mut MutableRepo) -> BackendResult { let commit = write_to_store(&self.store, self.commit, &self.sign_settings)?; mut_repo.add_head(&commit)?; @@ -309,6 +325,14 @@ impl DetachedCommitBuilder { } Ok(commit) } + + /// Writes new commit without making it visible in the repo. + /// + /// This does not consume the builder, so you can reuse the current + /// configuration to create another commit later. + pub fn write_hidden(&mut self) -> BackendResult { + write_to_store(&self.store, self.commit.clone(), &self.sign_settings) + } } fn write_to_store( From b54ff86e471fe00e081019a728d484f66c8635a6 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 20 Jul 2024 12:40:14 +0900 Subject: [PATCH 7/7] repo: use DetachedCommitBuilder constructors I think this makes it clear that the builder doesn't add any rewrite records to the mut_repo. --- lib/src/commit_builder.rs | 31 ++++++++----------------------- lib/src/repo.rs | 6 +++--- 2 files changed, 11 insertions(+), 26 deletions(-) diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs index 2bb6393894..b1f2f6f736 100644 --- a/lib/src/commit_builder.rs +++ b/lib/src/commit_builder.rs @@ -30,27 +30,6 @@ pub struct CommitBuilder<'repo> { } impl CommitBuilder<'_> { - /// Only called from [`MutRepo::new_commit`]. Use that function instead. - pub(crate) fn for_new_commit<'repo>( - mut_repo: &'repo mut MutableRepo, - settings: &UserSettings, - parents: Vec, - tree_id: MergedTreeId, - ) -> CommitBuilder<'repo> { - let inner = DetachedCommitBuilder::for_new_commit(mut_repo, settings, parents, tree_id); - CommitBuilder { mut_repo, inner } - } - - /// Only called from [`MutRepo::rewrite_commit`]. Use that function instead. - pub(crate) fn for_rewrite_from<'repo>( - mut_repo: &'repo mut MutableRepo, - settings: &UserSettings, - predecessor: &Commit, - ) -> CommitBuilder<'repo> { - let inner = DetachedCommitBuilder::for_rewrite_from(mut_repo, settings, predecessor); - CommitBuilder { mut_repo, inner } - } - /// Detaches from `&'repo mut` lifetime. The returned builder can be used in /// order to obtain a temporary commit object. pub fn detach(self) -> DetachedCommitBuilder { @@ -155,7 +134,8 @@ pub struct DetachedCommitBuilder { } impl DetachedCommitBuilder { - fn for_new_commit( + /// Only called from [`MutRepo::new_commit`]. Use that function instead. + pub(crate) fn for_new_commit( repo: &dyn Repo, settings: &UserSettings, parents: Vec, @@ -185,7 +165,12 @@ impl DetachedCommitBuilder { } } - fn for_rewrite_from(repo: &dyn Repo, settings: &UserSettings, predecessor: &Commit) -> Self { + /// Only called from [`MutRepo::rewrite_commit`]. Use that function instead. + pub(crate) fn for_rewrite_from( + repo: &dyn Repo, + settings: &UserSettings, + predecessor: &Commit, + ) -> Self { let store = repo.store().clone(); let mut commit = predecessor.store_commit().clone(); commit.predecessors = vec![predecessor.id().clone()]; diff --git a/lib/src/repo.rs b/lib/src/repo.rs index fdb2a7cddb..e947921942 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -33,7 +33,7 @@ use crate::backend::{ MergedTreeId, }; use crate::commit::{Commit, CommitByCommitterTimestamp}; -use crate::commit_builder::CommitBuilder; +use crate::commit_builder::{CommitBuilder, DetachedCommitBuilder}; use crate::default_index::{DefaultIndexStore, DefaultMutableIndex}; use crate::default_submodule_store::DefaultSubmoduleStore; use crate::file_util::{IoResultExt as _, PathError}; @@ -843,7 +843,7 @@ impl MutableRepo { parents: Vec, tree_id: MergedTreeId, ) -> CommitBuilder { - CommitBuilder::for_new_commit(self, settings, parents, tree_id) + DetachedCommitBuilder::for_new_commit(self, settings, parents, tree_id).attach(self) } /// Returns a [`CommitBuilder`] to rewrite an existing commit in the repo. @@ -852,7 +852,7 @@ impl MutableRepo { settings: &UserSettings, predecessor: &Commit, ) -> CommitBuilder { - CommitBuilder::for_rewrite_from(self, settings, predecessor) + DetachedCommitBuilder::for_rewrite_from(self, settings, predecessor).attach(self) // CommitBuilder::write will record the rewrite in // `self.rewritten_commits` }