From: Elijah Newren Date: Sat, 10 Jan 2026 01:14:48 GMT Subject: Re: [PATCH v9 1/7] builtin/replay: extract core logic to replay revisions Message-ID: In-Reply-To: <20260109-b4-pks-history-builtin-v9-1-8766101814c6@pks.im> On Fri, Jan 9, 2026 at 12:35 AM Patrick Steinhardt wrote: > > We're about to move the core logic used to replay revisions onto a new > base into the "libgit.a" library. Prepare for this by pulling out the > logic into a new function `replay_revisions()` that: > > 1. Takes a set of revisions to replay and some options that tell it how > it ought to replay the revisions. > > 2. Replays the commits. > > 3. Records any reference updates that would be caused by replaying the > commits in a structure that is owned by the caller. > > The logic itself will be moved into a separate file in the next commit. > This change is not expected to cause user-visible change in behaviour. > > Signed-off-by: Patrick Steinhardt > --- > builtin/replay.c | 274 +++++++++++++++++++++++++++++++++---------------------- > 1 file changed, 165 insertions(+), 109 deletions(-) > > diff --git a/builtin/replay.c b/builtin/replay.c > index 1960bbbee8..df3b32a52d 100644 > --- a/builtin/replay.c > +++ b/builtin/replay.c > @@ -278,6 +278,137 @@ static enum ref_action_mode get_ref_action_mode(struct repository *repo, const c > return REF_ACTION_UPDATE; > } > > +struct replay_revisions_options { > + const char *advance; > + const char *onto; > + int contained; > +}; > + > +struct replay_result { > + struct replay_ref_update { > + char *refname; > + struct object_id old_oid; > + struct object_id new_oid; > + } *updates; > + size_t updates_nr, updates_alloc; > + > + bool merge_conflict; > +}; > + > +static void replay_result_release(struct replay_result *result) > +{ > + for (size_t i = 0; i < result->updates_nr; i++) > + free(result->updates[i].refname); > + free(result->updates); > +} > + > +static void replay_result_queue_update(struct replay_result *result, > + const char *refname, > + const struct object_id *old_oid, > + const struct object_id *new_oid) > +{ > + ALLOC_GROW(result->updates, result->updates_nr + 1, result->updates_alloc); > + result->updates[result->updates_nr].refname = xstrdup(refname); > + result->updates[result->updates_nr].old_oid = *old_oid; > + result->updates[result->updates_nr].new_oid = *new_oid; > + result->updates_nr++; > +} > + > +static int replay_revisions(struct repository *repo, struct rev_info *revs, > + struct replay_revisions_options *opts, > + struct replay_result *out) Why have both repo & revs? Can't we get repo from revs->repo? > +{ > + kh_oid_map_t *replayed_commits = NULL; > + struct strset *update_refs = NULL; > + struct commit *last_commit = NULL; > + struct commit *commit; > + struct commit *onto = NULL; > + struct merge_options merge_opt; > + struct merge_result result; > + char *advance; > + int ret; > + > + advance = xstrdup_or_null(opts->advance); > + set_up_replay_mode(repo, &revs->cmdline, opts->onto, &advance, > + &onto, &update_refs); > + > + /* FIXME: Should allow replaying commits with the first as a root commit */ > + > + if (prepare_revision_walk(revs) < 0) { > + ret = error(_("error preparing revisions")); > + goto out; > + } > + > + init_basic_merge_options(&merge_opt, repo); > + memset(&result, 0, sizeof(result)); > + merge_opt.show_rename_progress = 0; > + last_commit = onto; > + replayed_commits = kh_init_oid_map(); > + while ((commit = get_revision(revs))) { > + const struct name_decoration *decoration; > + khint_t pos; > + int hr; > + > + if (!commit->parents) > + die(_("replaying down from root commit is not supported yet!")); > + if (commit->parents->next) > + die(_("replaying merge commits is not supported yet!")); > + > + last_commit = pick_regular_commit(repo, commit, replayed_commits, > + onto, &merge_opt, &result); > + if (!last_commit) > + break; > + > + /* Record commit -> last_commit mapping */ > + pos = kh_put_oid_map(replayed_commits, commit->object.oid, &hr); > + if (hr == 0) > + BUG("Duplicate rewritten commit: %s\n", > + oid_to_hex(&commit->object.oid)); > + kh_value(replayed_commits, pos) = last_commit; > + > + /* Update any necessary branches */ > + if (advance) > + continue; > + decoration = get_name_decoration(&commit->object); > + if (!decoration) > + continue; > + while (decoration) { > + if (decoration->type == DECORATION_REF_LOCAL && > + (opts->contained || strset_contains(update_refs, > + decoration->name))) { > + replay_result_queue_update(out, decoration->name, > + &commit->object.oid, > + &last_commit->object.oid); > + } > + decoration = decoration->next; > + } > + } > + > + if (!result.clean) { > + out->merge_conflict = true; > + ret = -1; > + goto out; What? In the replay code before this series: ret = result.clean; [...] /* Return */ if (ret < 0) exit(128); return ret ? 0 : 1; So, !result.clean should result in a return code of 1, not -1. > + } > + > + /* In --advance mode, advance the target ref */ > + if (advance) > + replay_result_queue_update(out, advance, > + &onto->object.oid, > + &last_commit->object.oid); > + > + ret = 0; > + > +out: > + if (update_refs) { > + strset_clear(update_refs); > + free(update_refs); > + } > + kh_destroy_oid_map(replayed_commits); > + merge_finalize(&merge_opt, &result); > + free(advance); > + return ret; > +} > + > static int handle_ref_update(enum ref_action_mode mode, > struct ref_transaction *transaction, > const char *refname, > @@ -306,21 +437,11 @@ int cmd_replay(int argc, > const char *prefix, > struct repository *repo) > { > - const char *advance_name_opt = NULL; > - char *advance_name = NULL; > - struct commit *onto = NULL; > - const char *onto_name = NULL; > - int contained = 0; > + struct replay_revisions_options opts = { 0 }; > + struct replay_result result = { 0 }; > const char *ref_action = NULL; > enum ref_action_mode ref_mode; > - > struct rev_info revs; > - struct commit *last_commit = NULL; > - struct commit *commit; > - struct merge_options merge_opt; > - struct merge_result result; > - struct strset *update_refs = NULL; > - kh_oid_map_t *replayed_commits; > struct ref_transaction *transaction = NULL; > struct strbuf transaction_err = STRBUF_INIT; > struct strbuf reflog_msg = STRBUF_INIT; > @@ -333,13 +454,13 @@ int cmd_replay(int argc, > NULL > }; > struct option replay_options[] = { > - OPT_STRING(0, "advance", &advance_name_opt, > + OPT_STRING(0, "advance", &opts.advance, > N_("branch"), > N_("make replay advance given branch")), > - OPT_STRING(0, "onto", &onto_name, > + OPT_STRING(0, "onto", &opts.onto, > N_("revision"), > N_("replay onto given commit")), > - OPT_BOOL(0, "contained", &contained, > + OPT_BOOL(0, "contained", &opts.contained, > N_("update all branches that point at commits in ")), > OPT_STRING(0, "ref-action", &ref_action, > N_("mode"), > @@ -350,19 +471,17 @@ int cmd_replay(int argc, > argc = parse_options(argc, argv, prefix, replay_options, replay_usage, > PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT); > > - if (!onto_name && !advance_name_opt) { > + if (!opts.onto && !opts.advance) { > error(_("option --onto or --advance is mandatory")); > usage_with_options(replay_usage, replay_options); > } > > - die_for_incompatible_opt2(!!advance_name_opt, "--advance", > - contained, "--contained"); > + die_for_incompatible_opt2(!!opts.advance, "--advance", > + opts.contained, "--contained"); > > /* Parse ref action mode from command line or config */ > ref_mode = get_ref_action_mode(repo, ref_action); > > - advance_name = xstrdup_or_null(advance_name_opt); > - > repo_init_revisions(repo, &revs, prefix); > > /* > @@ -414,18 +533,19 @@ int cmd_replay(int argc, > revs.simplify_history = 0; > } > > - set_up_replay_mode(repo, &revs.cmdline, > - onto_name, &advance_name, > - &onto, &update_refs); > - > - /* FIXME: Should allow replaying commits with the first as a root commit */ > + ret = replay_revisions(repo, &revs, &opts, &result); > + if (ret) > + goto cleanup; > > /* Build reflog message */ > - if (advance_name_opt) > - strbuf_addf(&reflog_msg, "replay --advance %s", advance_name_opt); > - else > - strbuf_addf(&reflog_msg, "replay --onto %s", > - oid_to_hex(&onto->object.oid)); > + if (opts.advance) { > + strbuf_addf(&reflog_msg, "replay --advance %s", opts.advance); > + } else { > + struct object_id oid; > + if (repo_get_oid_committish(repo, opts.onto, &oid)) > + BUG("--onto commit should have been resolved beforehand already"); > + strbuf_addf(&reflog_msg, "replay --onto %s", oid_to_hex(&oid)); > + } > > /* Initialize ref transaction if using update mode */ > if (ref_mode == REF_ACTION_UPDATE) { > @@ -438,78 +558,19 @@ int cmd_replay(int argc, > } > } > > - if (prepare_revision_walk(&revs) < 0) { > - ret = error(_("error preparing revisions")); > - goto cleanup; > - } > - > - init_basic_merge_options(&merge_opt, repo); > - memset(&result, 0, sizeof(result)); > - merge_opt.show_rename_progress = 0; > - last_commit = onto; > - replayed_commits = kh_init_oid_map(); > - while ((commit = get_revision(&revs))) { > - const struct name_decoration *decoration; > - khint_t pos; > - int hr; > - > - if (!commit->parents) > - die(_("replaying down from root commit is not supported yet!")); > - if (commit->parents->next) > - die(_("replaying merge commits is not supported yet!")); > - > - last_commit = pick_regular_commit(repo, commit, replayed_commits, > - onto, &merge_opt, &result); > - if (!last_commit) > - break; > - > - /* Record commit -> last_commit mapping */ > - pos = kh_put_oid_map(replayed_commits, commit->object.oid, &hr); > - if (hr == 0) > - BUG("Duplicate rewritten commit: %s\n", > - oid_to_hex(&commit->object.oid)); > - kh_value(replayed_commits, pos) = last_commit; > - > - /* Update any necessary branches */ > - if (advance_name) > - continue; > - decoration = get_name_decoration(&commit->object); > - if (!decoration) > - continue; > - while (decoration) { > - if (decoration->type == DECORATION_REF_LOCAL && > - (contained || strset_contains(update_refs, > - decoration->name))) { > - if (handle_ref_update(ref_mode, transaction, > - decoration->name, > - &last_commit->object.oid, > - &commit->object.oid, > - reflog_msg.buf, > - &transaction_err) < 0) { > - ret = error(_("failed to update ref '%s': %s"), > - decoration->name, transaction_err.buf); > - goto cleanup; > - } > - } > - decoration = decoration->next; > - } > - } > - > - /* In --advance mode, advance the target ref */ > - if (result.clean == 1 && advance_name) { > - if (handle_ref_update(ref_mode, transaction, advance_name, > - &last_commit->object.oid, > - &onto->object.oid, > - reflog_msg.buf, > - &transaction_err) < 0) { > + for (size_t i = 0; i < result.updates_nr; i++) { > + ret = handle_ref_update(ref_mode, transaction, result.updates[i].refname, > + &result.updates[i].new_oid, &result.updates[i].old_oid, > + reflog_msg.buf, &transaction_err); > + if (ret) { > ret = error(_("failed to update ref '%s': %s"), > - advance_name, transaction_err.buf); > + result.updates[i].refname, transaction_err.buf); > goto cleanup; > } > } > > /* Commit the ref transaction if we have one */ > - if (transaction && result.clean == 1) { > + if (transaction) { > if (ref_transaction_commit(transaction, &transaction_err)) { > ret = error(_("failed to commit ref transaction: %s"), > transaction_err.buf); > @@ -517,24 +578,19 @@ int cmd_replay(int argc, > } > } > > - merge_finalize(&merge_opt, &result); > - kh_destroy_oid_map(replayed_commits); > - if (update_refs) { > - strset_clear(update_refs); > - free(update_refs); > - } > - ret = result.clean; > - > cleanup: > if (transaction) > ref_transaction_free(transaction); > + replay_result_release(&result); > strbuf_release(&transaction_err); > strbuf_release(&reflog_msg); > release_revisions(&revs); > - free(advance_name); > > - /* Return */ > - if (ret < 0) > - exit(128); > - return ret ? 0 : 1; > + if (ret) { > + if (result.merge_conflict) > + return 1; This feels like a rather Rube-Goldberg way to get this. Above where I highlighted where you set ret to -1 incorrectly, can we just set it to 1 there and then lose all this special logic in favor of "return ret;" here? Or am I overlooking something? > + return 128; > + } > + > + return 0; > } > > -- > 2.52.0.542.g9473a8513b.dirty