Re: [PATCH v2 1/1] replay: make atomic ref updates the default behavior

> On Thu, Oct 2, 2025 at 4:27 PM Siddharth Asthana
> <siddharthasthana31@gmail.com> wrote:
>>>> For users needing the traditional pipeline workflow, --output-commands
>>>> preserves the original behavior:
>>>>
>>>>       git replay --output-commands --onto main topic1..topic2 | git update-ref --stdin
>>> This is good.  Did you also add a config option so that someone can
>>> just set that option once and use the old behavior?  (as per the
>>> suggestion at https://lore.kernel.org/git/xmqq5xdrvand.fsf@gitster.g/
>>> ?)
>>
>> I didn't, but I should have. I will add a config option for v3.
>>
>> For naming, I am thinking either:
>>     - replay.updateRefs (boolean: true = update, false = output-commands)
>>     - replay.defaultOutput (string: "update" | "commands")
>>
>> The boolean feels simpler, but the string might be more extensible if we
>> add other output modes later. Which pattern feels more consistent with
>> existing Git config conventions? Looking at rebase.* they're mostly
>> boolean toggles, but am I missing a better example to follow?
> replay.updateRefs sounds better to me.  defaultOutput with "update"
> doesn't make sense to me.
>
>> You are right - I don't have a concrete use case. I was trying to
>> anticipate potential needs but ended up adding unjustified complexity.
>>
>> I will remove --allow-partial entirely from v3. This simplifies to exactly
>> two modes with clear purposes:
>>     1. Default: atomic ref updates (all-or-nothing)
>>     2. --output-commands: traditional pipeline for special cases
>>
>> Much cleaner design.
> Note that once you add a config option, you'll also need an additional
> command line flag (or make it possible to invert an existing one), so
> that users can override the config and get the default behavior.
> Maybe --[no-]update-refs would make sense after all, where
> --update-refs is the default and --no-update-refs is your current
> --output-commands?

>
> (I know you all talked elsewhere in this thread about "avoiding a name
> collision" with rebase, but I don't quite see it as a collision.  When
> Stolee suggested the flag for rebase, I pointed out it's roughly what
> I'm doing in replay, so it doesn't feel like a conflict to me.  I'm
> also open to an alternative flag name if it makes sense, but we
> probably want whatever the command line flag is to be similar to the
> config name and "defaultOutput"/--default-output don't make sense as a
> name to me.)
>
>>>> @@ -330,9 +361,12 @@ int cmd_replay(int argc,
>>>>                   usage_with_options(replay_usage, replay_options);
>>>>           }
>>>>
>>>> -       if (advance_name_opt && contained)
>>>> -               die(_("options '%s' and '%s' cannot be used together"),
>>>> -                   "--advance", "--contained");
>>>> +       die_for_incompatible_opt2(!!advance_name_opt, "--advance",
>>>> +                                 contained, "--contained");
>>> Broken indentation.  Also, should this have been done as a preparatory
>>> cleanup patch?
>>
>> Good catches. I will fix the indentation.
>>
>> On making it a preparatory patch: should I split it out as a separate
>> cleanup commit, or is it minor enough to fold into the main change? I am
>> leaning toward folding it in since it's directly related to the option
>> handling changes
> Given that it was directly adjacent to the other
> die_for_incompatible_opt2() call, if that were still the case, I could
> see making it part of the same commit.  However, dropping the
> --allow-partial flag means you don't need to add that other call
> anymore, so it makes this remaining die_for_incompataible_opt2() call
> an entirely orthogonal change to the rest of your patch.  As such, I
> think it belongs in a separate patch; it could either be a preparatory
> patch or a follow-up.

>
>>>> @@ -407,6 +452,8 @@ int cmd_replay(int argc,
>>>>                   khint_t pos;
>>>>                   int hr;
>>>>
>>>> +               commits_processed = 1;
>>>> +
>>>>                   if (!commit->parents)
>>>>                           die(_("replaying down to root commit is not supported yet!"));
>>>>                   if (commit->parents->next)
>>>> @@ -457,9 +535,17 @@ int cmd_replay(int argc,
>>>>                   strset_clear(update_refs);
>>>>                   free(update_refs);
>>>>           }
>>>> -       ret = result.clean;
>>>> +
>>>> +       /* Handle empty ranges: if no commits were processed, treat as success */
>>>> +       if (!commits_processed)
>>>> +               ret = 1; /* Success - no commits to replay is not an error */
>>>> +       else
>>>> +               ret = result.clean;
>>> The change to treat empty ranges as success is an orthogonal change
>>> that I think at a minimum belongs in a separate patch.  Out of
>>> curiosity, how did you discover the exit status with an empty commit
>>> range?  Why does someone specify such a range, and what form or forms
>>> might it come in?  And is merely returning a successful result enough,
>>> or is there more that needs to be done for correctness?
>>
>> I was thinking about automated scripts that compute ranges dynamically -
>> they might generate A..B where it turns out A==B, and treating that as
>> "no work needed, success" seemed reasonable for scripting.
>>
>> But you raise a good point: A..A seems like obvious user error (why would
>> anyone do that intentionally?), and B..A where B contains A is likely a
>> mistake that maybe should error rather than silently succeed.
>>
>> I am inclined to drop it entirely from this series. If there's real demand
>> for specific empty-range handling, we can add it later with proper
>> discussion of the actual use cases. Does that sound reasonable?
> Yep, dropping it makes sense to me.  Alternatively, documenting what
> happens in the case of empty ranges, as Christian suggests, also makes
> sense to me though I might suggest that it be done in an entirely
> separate series rather than just a separate patch of this series.