Re: [PATCH v10 1/8] builtin/replay: extract core logic to replay revisions

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 13, 2026, 07:31 UTC
Message-ID: <aWX0055xT2PKzgfa@pks.im>
In-Reply-To: <CABPp-BGOcMRerGpH5HGkUR4-DKPx+VmkWzqRt8qideZoJBrvHg@mail.gmail.com>

On Mon, Jan 12, 2026 at 10:00:06PM -0800, Elijah Newren wrote:

Show 17 quoted lines

> On Mon, Jan 12, 2026 at 6:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> > @@ -253,6 +254,137 @@ static struct commit *pick_regular_commit(struct repository *repo,
> [...]
> > +       if (!result.clean) {
> > +               out->merge_conflict = true;
> > +               ret = -1;
> 
> Even if you keep the special merge_conflict field for other purposes,
> setting ret to -1 here still feels very wrong.  Negative return codes,
> and especially -1, is used throughout the merge machinery to signal
> unexpected errors like failure to read/write to disk.  Further, it's
> inconsistent with how builtin/merge-tree.c works, where both in code
> and in documentation merge result code is 0 == clean, 1 == merge
> conflicts.  I'm worried using -1 here could cause some nasty future
> maintenance headaches trying to understand the field if left this way,
> at least for me.  As mentioned in the last round, the ret value here
> should be 1.

To expand a bit why I prefer my style: it's extremely easy to get the return code wrong. We have a bunch of callsites that set the return code directly, and in theory we now have to vet every single such call site to verify that it will never return an error code of 1. If any function does, then exiting with 1 now becomes ambiguous. This is an error waiting to happen.

With being explicit about the error code and the specific condition we care about we make that issue go away. Furthermore, it also helps to document intent in the code as it is very explicit now.

In any case, I don't want this disagreement to block the patch series, so I'll adapt to make it return a positive value on a conflict.

Show 37 quoted lines

> > @@ -306,21 +438,11 @@ int cmd_replay(int argc,
> [...]
> > -       die_for_incompatible_opt2(!!advance_name_opt, "--advance",
> > -                                 contained, "--contained");
> > +       die_for_incompatible_opt2(!!opts.advance, "--advance",
> > +                                 opts.contained, "--contained");
> 
> This predates your patch, but I'm wondering if there's anything we
> should do to clarify and/or simplify the first check.  The original
> form of the check
>      +       die_for_incompatible_opt2(!!opts.advance, "--advance",
>      +                                 opts.contained, "--contained");
> was created because (a) I had code that allowed --onto to be implicit
> in some cases, and (b) I was thinking only of --onto and --advance
> modes.
> 
> However: (a) we got rid of the implicit mode selection from my private
> branch, and (b) Siddharth added patches which added a --revert mode.
> Those patches caused confusion around the interplay of --contained
> with the new mode
> (https://lore.kernel.org/git/xmqq3460ocv7.fsf@gitster.g/).  I thought
> the synopsis:
>            "([--contained] --onto <newbase> | --advance <branch>) "
> implied clearly enough that --contained is a sub-mode of --onto, but
> apparently that wasn't the case.  Perhaps we can strengthen that
> understanding if we change the check here to instead be something like
>    if (opts.contained && !opts.onto)
>       die("--onto must be specified if --contained is")
> 
> Definitely not critical; but might be a nice cleanup.
> 
> > +       die_for_incompatible_opt2(!!opts.advance, "--advance",
> > +                                 !!opts.onto, "--onto");
> 
> Yeah, and Siddharth can convert this to a die_for_incompatible_opt3()
> call, adding "--revert" to it when he rerolls his series on top of
> yours.

Fair, but this feels somewhat unrelated to my changes. So I'll not touch this part and leave it for a future patch series to clean up, if that's alright with you.

Show 19 quoted lines

> > -       /* Return */
> > -       if (ret < 0)
> > -               exit(128);
> > -       return ret ? 0 : 1;
> > +       if (ret) {
> > +               if (result.merge_conflict)
> > +                       return 1;
> > +               return 128;
> > +       }
> > +
> > +       return 0;
> >  }
> 
> You mentioned that you wanted to keep the merge_conflict field due to
> some future patches beyond the currently submitted series.  I wonder
> if it'd make more sense to introduce that field once you introduce the
> new patches, but i don't feel too strongly about that.  I do feel
> strongly that the place where you set ret to -1 is problematic and
> should be changed to 1.

I'll drop the field for now.

Patrick