Re: [PATCH v9 1/7] builtin/replay: extract core logic to replay revisions
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jan 12, 2026, 13:02 UTC
- Message-ID
- <aWTwyiNuiybFIFAW@pks.im>
- In-Reply-To
- <CABPp-BFmQHjyeT0pXYV5eE5cnG5H6XXHeT_RsjWxq83ck3781w@mail.gmail.com>
On Fri, Jan 09, 2026 at 05:14:48PM -0800, Elijah Newren wrote:
Show 50 quoted lines
> On Fri, Jan 9, 2026 at 12:35 AM Patrick Steinhardt <ps@pks.im> wrote:
> > 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?True indeed, will change.
Show 33 quoted lines
> > @@ -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?You're not really overlooking anything, no. The reason I did it this way here is to make the logic more explicit -- a simple return value is very fragile, and I broke this in v8 of this patch series already.
Furthermore, this is preparing for us to move this code into a separate library. Not all callers may want to handle conflicts the exact same, so I think it's sensible to explicitly tell the caller about such error cases. One way to do this is to use e.g. an enum to surface the info, but that is somewhat limiting. We may for example tell the caller later on what the conflicting commit was, and that's becoming way easier by having the info in `sturct replay_result`.
Patrick