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

> On Fri, Oct 3, 2025 at 1:27 AM 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.
> You don't need to add that configuration option in the main patch. I
> would suggest adding it in a separate patch after the main one (which
> changes the default behavior of the command).
>
> Note that in the commit message of the main patch, it's nice to say
> that a following commit will add a configuration option for users who
> prefer the previous default behavior.
>
>> For naming, I am thinking either:
>>     - replay.updateRefs (boolean: true = update, false = output-commands)
>>     - replay.defaultOutput (string: "update" | "commands")
> If the command line option is called `--output-commands` then I would
> suggest naming it "replay.outputCommands" and making it a boolean.

>
>>>> @@ -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
> If there is only this additional small cleanup change in the main
> commit, and this small cleanup change is clearly mentioned in the
> commit message as a "while at it small cleanup change", I think it's
> OK.

>
> If you find out that other additional small cleanup changes would be
> nice too, then they should definitely all go into a preparatory patch
> before the main patch.
>
>
>>>> +
>>>> +       /* 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?
> Yeah, I think dropping it from this series is fine.

>
> What happens in those cases should be documented if it isn't already
> though. Those documentation changes should probably be in a separate
> patch.
>
> Thanks.