Skip to content

Commit

Permalink
fix(move): Support on-disk rebases with move --fixup
Browse files Browse the repository at this point in the history
  • Loading branch information
claytonrcarter committed Jul 23, 2023
1 parent cf222a0 commit 001e094
Show file tree
Hide file tree
Showing 5 changed files with 587 additions and 12 deletions.
11 changes: 9 additions & 2 deletions git-branchless-hook/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -576,9 +576,16 @@ pub fn command_main(ctx: CommandContext, args: HookArgs) -> EyreExitOr<()> {
hook_register_extra_post_rewrite_hook()?;
}

HookSubcommand::SkipUpstreamAppliedCommit { commit_oid } => {
HookSubcommand::SkipUpstreamAppliedCommit {
commit_oid,
rewritten_oid,
} => {
let commit_oid: NonZeroOid = commit_oid.parse()?;
hook_skip_upstream_applied_commit(&effects, commit_oid)?;
let rewritten_oid: MaybeZeroOid = match rewritten_oid {
Some(rewritten_oid) => rewritten_oid.parse()?,
None => MaybeZeroOid::Zero,
};
hook_skip_upstream_applied_commit(&effects, commit_oid, rewritten_oid)?;
}
}

Expand Down
75 changes: 70 additions & 5 deletions git-branchless-lib/src/core/rewrite/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::sync::Arc;

use chashmap::CHashMap;
use eyre::Context;
use itertools::Itertools;
use itertools::{intersperse, Itertools};
use rayon::{prelude::*, ThreadPool};
use tracing::{instrument, warn};

Expand Down Expand Up @@ -148,7 +148,65 @@ impl ToString for RebaseCommand {
} => match commits_to_apply_oids.as_slice() {
[] => String::new(),
[commit_oid] => format!("pick {commit_oid}"),
[..] => unimplemented!("On-disk fixups"),
[pick_oid, fixup_oids @ ..] => {
let mut picks = vec![format!("pick {pick_oid}")];
let mut fixups = fixup_oids
.iter()
.map(|oid| format!("fixup {oid}"))
.collect::<Vec<String>>();
let mut cleanups = vec![];

// Since 0ca8681, the intermediate commits created as each
// fixup is applied are left behind in the smartlog. This
// forcibly removes all but the last of them. I don't
// understand why this happens during `git branchless`
// initiated rebases, but not during "normal" fixup rebases,
// but this makes these artifacts go away.
if fixups.len() > 1 {
fixups = intersperse(
fixups,
// FIXME Is $(...) portable?
// I had assumed not, but the tests seem to pass on all platforms, so maybe this is OK?
"exec git branchless hook-skip-upstream-applied-commit $(git rev-parse HEAD)".to_string()
).collect()
}

// If the destination commit (ie `original_commit_oid`) does
// not come first topologically among the commits being
// rebased, then the final squashed commit will end up with
// the wrong metadata. (It will end up with the metadata
// from the commit that *does* come first, ie `pick_oid`.)
// We have to add some additional steps to make sure the
// smartlog and commit metadata are left as the user
// expects.
if pick_oid != original_commit_oid {
// See above comment related to 0ca8681
picks.insert(
1,
"exec git branchless hook-skip-upstream-applied-commit $(git rev-parse HEAD)".to_string()
);

cleanups = vec![
// Hide the final squashed commit
"exec git branchless hook-skip-upstream-applied-commit $(git rev-parse HEAD)".to_string(),

// Create a new final commit by applying the
// metadata from the destination commit to the just
// now hidden commit.
format!("exec git commit --amend --no-edit --reuse-message {original_commit_oid}"),

// Finally, register the new final commit as the
// rewritten version of original_commit_oid
format!("exec git branchless hook-skip-upstream-applied-commit {original_commit_oid} $(git rev-parse HEAD)")
];
}

picks
.iter()
.chain(fixups.iter())
.chain(cleanups.iter())
.join("\n")
}
},
RebaseCommand::Merge {
commit_oid,
Expand Down Expand Up @@ -1172,9 +1230,16 @@ impl<'a> RebasePlanBuilder<'a> {

let first_parent_oid = *parent_oids.first().unwrap();
first_dest_oid.get_or_insert(first_parent_oid);
acc.push(RebaseCommand::Reset {
target: OidOrLabel::Oid(first_parent_oid),
});
// FIXME This feels heavy handed, but seems to be necessary for some fixup cases.
if !state
.constraints
.commits_to_move()
.contains(&first_parent_oid)
{
acc.push(RebaseCommand::Reset {
target: OidOrLabel::Oid(first_parent_oid),
});
}

let upstream_patch_ids = if *detect_duplicate_commits_via_patch_id {
let (effects, _progress) =
Expand Down
9 changes: 6 additions & 3 deletions git-branchless-lib/src/core/rewrite/rewrite_hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -516,11 +516,14 @@ pub fn hook_drop_commit_if_empty(
Ok(())
}

/// For rebases, if a commit is known to have been applied upstream, skip it
/// without attempting to apply it.
/// For rebases, update the status of a commit that is known to have been
/// applied upstream. It can either be skipped entirely (when called with
/// `MaybeZeroOid::Zero`) or be marked as having been rewritten to a
/// different commit entirely.
pub fn hook_skip_upstream_applied_commit(
effects: &Effects,
commit_oid: NonZeroOid,
rewritten_oid: MaybeZeroOid,
) -> eyre::Result<()> {
let repo = Repo::from_current_dir()?;
let commit = repo.find_commit_or_fail(commit_oid)?;
Expand All @@ -546,7 +549,7 @@ pub fn hook_skip_upstream_applied_commit(
add_rewritten_list_entries(
&repo.get_tempfile_dir(),
&repo.get_rebase_state_dir_path().join("rewritten-list"),
&[(commit_oid, MaybeZeroOid::Zero)],
&[(commit_oid, rewritten_oid)],
)?;

Ok(())
Expand Down
4 changes: 4 additions & 0 deletions git-branchless-opts/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,10 @@ pub enum HookSubcommand {
/// The OID of the commit that was skipped.
#[clap(value_parser)]
commit_oid: String,

/// The OID of the commit that was skipped (if Some) or removed (if None).
#[clap(value_parser)]
rewritten_oid: Option<String>,
},
}

Expand Down
Loading

0 comments on commit 001e094

Please sign in to comment.