Skip to content
40 changes: 40 additions & 0 deletions git-branchless-lib/src/core/rewrite/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::ops::Sub;
use std::path::PathBuf;
use std::sync::Arc;

use bstr::ByteSlice;
use chashmap::CHashMap;
use eyre::Context;
use itertools::Itertools;
Expand Down Expand Up @@ -1077,6 +1078,45 @@ impl<'a> RebasePlanBuilder<'a> {
Ok(())
}

/// Generate a sequence of rebase steps that cause the subtree at `source_oid`
/// to be "reparented" by `dest_oids`, namely, keeping all contents
/// of descendant commits exactly the same.
pub fn reparent_subtree(
&mut self,
source_oid: NonZeroOid,
parent_oids: Vec<NonZeroOid>,
repo: &Repo,
) -> eyre::Result<()> {
Comment on lines +1083 to +1091
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[please clarify] Please correct me if I'm wrong, but I don't think that this comment nor the fn name match the actual behavior:

  • typo: I think that "by dest_oids"probably should be "ontoparent_oids`".
  • please confirm: does calling self.replace_commit() also trigger a rebase of the original commit's descendants onto the replacement commit? Or does it just do a replacement w/o rebase, leaving the descendants remaining on the original commit? ? The comment for replace_commit() doesn't mention anything about subtrees, which leads me to believe that this fn is also not actually triggering a rebase, but I don't know.
  • if replace_commit() does trigger a rebase, then we just need to address that first typo; the it doesn't rebase, then we should rewrite the comment and fn name to match

edit: I think I've confirmed that replace_commit does not also trigger a rebase of descendants, but I would love your 2nd opinion!

assert!(!parent_oids.is_empty());

// To keep the contents of all descendant commits the same, forcibly
// replace the children commits, and then rely on normal patch
// application to apply the rest.
let parents: Vec<_> = parent_oids
.into_iter()
.map(|parent_oid| repo.find_commit_or_fail(parent_oid))
.try_collect()?;
let descendant_commit = repo.find_commit_or_fail(source_oid)?;
let descendant_message = descendant_commit.get_message_raw();
let descendant_message = descendant_message.to_str().with_context(|| {
eyre::eyre!(
"Could not decode commit message for descendant commit: {:?}",
descendant_commit
)
})?;
let reparented_descendant_oid = repo.create_commit(
None,
&descendant_commit.get_author(),
&descendant_commit.get_committer(),
descendant_message,
&descendant_commit.get_tree()?,
parents.iter().collect(),
)?;
self.replace_commit(source_oid, reparented_descendant_oid)?;
Comment on lines +1103 to +1119
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick (subjective)] I think that these descendant_* names made sense in the original context, but probably don't make sense now. Changing them to source_* would line up better w/ the input params: eg descendant_commitsource_commit, reparented_descendant_oidreparented_source_oid, etc


Ok(())
}

/// Instruct the rebase planner to replace the commit at `original_oid` with the commit at
/// `replacement_oid`.
pub fn replace_commit(
Expand Down
26 changes: 1 addition & 25 deletions git-branchless/src/commands/amend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ use std::collections::HashMap;
use std::fmt::Write;
use std::time::{SystemTime, UNIX_EPOCH};

use bstr::ByteSlice;

use eyre::Context;
use git_branchless_opts::{MoveOptions, ResolveRevsetOptions};
use itertools::Itertools;
Expand Down Expand Up @@ -250,30 +248,8 @@ pub fn amend(
.collect();
builder.move_subtree(descendant_oid, parent_oids.clone())?;

// To keep the contents of all descendant commits the same, forcibly
// replace the children commits, and then rely on normal patch
// application to apply the rest.
if reparent {
let parents: Vec<_> = parent_oids
.into_iter()
.map(|parent_oid| repo.find_commit_or_fail(parent_oid))
.try_collect()?;
let descendant_message = descendant_commit.get_message_raw();
let descendant_message = descendant_message.to_str().with_context(|| {
eyre::eyre!(
"Could not decode commit message for descendant commit: {:?}",
descendant_commit
)
})?;
let reparented_descendant_oid = repo.create_commit(
None,
&descendant_commit.get_author(),
&descendant_commit.get_committer(),
descendant_message,
&descendant_commit.get_tree()?,
parents.iter().collect(),
)?;
builder.replace_commit(descendant_oid, reparented_descendant_oid)?;
builder.reparent_subtree(descendant_oid, parent_oids, &repo)?;
}
}

Expand Down