From: Elijah Newren <newren@gmail.com>
Date: Tue, 13 Jan 2026 06:00:06 GMT
Subject: Re: [PATCH v10 1/8] builtin/replay: extract core logic to replay revisions
Message-ID: <CABPp-BGOcMRerGpH5HGkUR4-DKPx+VmkWzqRt8qideZoJBrvHg@mail.gmail.com>
In-Reply-To: <20260112-b4-pks-history-builtin-v10-1-e3c6aa5b4cec@pks.im>

On Mon, Jan 12, 2026 at 6:17 AM Patrick Steinhardt <ps@pks.im> wrote:
[...]
> -       die_for_incompatible_opt2(!!onto_name, "--onto",
> -                                 !!*advance_name, "--advance");
> +       if (!(!!onto_name ^ !!*advance_name))
> +               BUG("expected either onto_name or *advance_name in this function");
> +

I liked Junio's comments on this section and your response.

> @@ -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.

> @@ -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.

> -       /* 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.