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

From: Elijah Newren <newren@gmail.com>
Date: Jan 13, 2026, 06:00 UTC
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: [...]

Show 5 quoted lines

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

Show 12 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.