Re: [PATCH v9 2/7] builtin/replay: move core logic into "libgit.a"

From: Elijah Newren <newren@gmail.com>
Date: Jan 13, 2026, 06:00 UTC
Message-ID: <CABPp-BGM4AxoedD3uUnS+12n5c0egd8pw-=cRsO64oDs+G9RkA@mail.gmail.com>
In-Reply-To: <aWTw4ezgeloLB0R2@pks.im>

On Mon, Jan 12, 2026 at 5:02 AM Patrick Steinhardt <ps@pks.im> wrote:

>
> On Fri, Jan 09, 2026 at 05:16:41PM -0800, Elijah Newren wrote:

[...]

Show 9 quoted lines

> > > +int replay_revisions(struct repository *repo, struct rev_info *revs,
> > > +                    struct replay_revisions_options *opts,
> > > +                    struct replay_result *out);
> > > +
> >
> > stray extra line?
>
> We typically have an empty line between the last declaration and the
> `#endif` in our headers.

Yeah, I was just confused; sorry.

Show 16 quoted lines

> > > +#endif
> >
> > It feels duplicative to have replay_result include a merge_conflict
> > field and to have replay_revisions() return an int which signifies
> > whether there's a conflict.  Can we remove one of the two?  (Perhaps
> > the merge_conflict field was only a workaround to the weird ret
> > setting from the previous patch?)
>
> The idea here is that we have a generic error code that tells the caller
> that _something_ happened, whereas `struct replay_result` gives the
> caller a bit more context around what exactly has happened. This allows
> callers to handle merge conflicts differently from any other error and
> makes the different failure modes a lot more explicit.
>
> Some context: at GitLab we actually have the use case to surface more
> information around what commits have conflicted, so there will be a

Interesting, but doesn't answering that question presume first-class conflict handling? How do you determine which commits conflict without that? Or, is the first commit we process that hits a conflict sufficient information and you don't really need the commits that conflict, just one of them? Or, do you presume that all unprocessed commits after the first one that conflicted would have also conflicted (even if it touched files that are conflict-free so far, so that commit would not have contributed to the conflicts)? If this is done without first-class conflict handling, there may also be an assumption here about linear single-branch history, or else some kind of attempt to continue processing whichever commits don't have an ancestor that has conflicted, so that we can enumerate "commits [which] have conflicted".

> follow-up patch series that extends `struct replay_result` to return
> more information about the actual conflict. I'm already planning ahead a
> bit in this patch series.

Wait, above you said you wanted to know the "commits [which] have conflicted", here you seem to be suggesting you want to know about "the actual conflict" which might mean you only care about the first commit that conflicted but you want details about what conflicted within it. Or is it perhaps the set of files that would have had conflicts across replaying the whole sequence of patches (in which case a bunch of the previous questions are still valid)?