From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Tue, 28 Oct 2025 19:39:11 GMT
Subject: Re: [PATCH v4 2/3] replay: make atomic ref updates the default behavior
Message-ID: <0c58d734-d7fe-4b2a-8231-c123b74601d6@gmail.com>
In-Reply-To: <CAP8UFD00rE7gF+baidmoi7nYwVKa3UDQgj+TB4wJLtjJF7u9gA@mail.gmail.com>


On 24/10/25 16:07, Christian Couder wrote:
> On Wed, Oct 22, 2025 at 8:51 PM Siddharth Asthana
> <siddharthasthana31@gmail.com> wrote:
>
> [...]
>
>> However, it should be noted that all three of these are somewhat
>> special cases; users, whether on the client or server side, would
>> almost certainly find it more ergonomical to simply have the updating


Hi Christian,
Thanks for the detailed review! All good points:


> Nit: maybe: s/ergonomical/ergonomic/


Will be fixed in v5!


>> of refs be the default.
> [...]
>
>> Change the default behavior to update refs directly, and atomically (at
>> least to the extent supported by the refs backend in use). This
>> eliminates the process coordination overhead for the common case.
>>
>> For users needing the traditional pipeline workflow, add a new
>> --ref-action=<mode> option that preserves the original behavior:
>>
>>    git replay --ref-action=print --onto main topic1..topic2 | git update-ref --stdin
>>
>> The mode can be:
>>    * update (default): Update refs directly using an atomic transaction
>>    * print: Output update-ref commands for pipeline use
> Nit: maybe it should be mentioned that the command is still
> experimental, so it's OK to change the default like this.


Good point, I will add a note in the commit message that since git-replay is
still experimental, changing the default behavior is acceptable


>
>> +--ref-action[=<mode>]::
>> +       Control how references are updated. The mode can be:
>> ++
>> +--
>> +       * `update` (default): Update refs directly using an atomic transaction.
>> +         All refs are updated or none are (all-or-nothing behavior).
>> +       * `print`: Output update-ref commands for pipeline use. This is the
>> +         traditional behavior where output can be piped to `git update-ref --stdin`.
>> +--
>> ++
>> +The default mode can be configured via `replay.refAction` configuration option.
> Nit: s/via `replay.refAction` configuration option/via the
> `replay.refAction` configuration variable/


Good catch, I will standardize on "configuration variable" throughout.


>
> (It seems that "configuration variable" is used around 6 times more
> than "configuration option", so we may want to standardize this
> wording.)
>
>> @@ -54,8 +68,11 @@ include::rev-list-options.adoc[]
>>   OUTPUT
>>   ------
>>
>> -When there are no conflicts, the output of this command is usable as
>> -input to `git update-ref --stdin`.  It is of the form:
>> +By default (with `--ref-action=update`), this command produces no output on
> Nit: s/By default (with `--ref-action=update`)/By default, or with
> `--ref-action=update`,/


Much clearer wording


>
> I think it's better to be very explicit here, especially as we mention
> `--ref-action=print` below.
>
> [...]
>
>> -       const char * const replay_usage[] = {
>> +       const char *const replay_usage[] = {
> Nit: Not sure this change is worth it, but I understand that it might
> help pass some automated/CI tests, so not a big issue.


Actually, Junio mentioned in another thread that the prevalent style in the
codebase is `const char * const` (space on both sides), so I'll revert this
change in v5.


>
> [...]
>
>> +       /* Default to update mode if not specified */
>> +       if (!ref_action_str)
>> +               ref_action_str = "update";
>> +
>> +       /* Parse ref action mode */
>> +       if (!strcmp(ref_action_str, "update"))
>> +               ref_action = REF_ACTION_UPDATE;
> Nit: maybe:
>
>         if (!ref_action_str || !strcmp(ref_action_str, "update"))
>                 ref_action = REF_ACTION_UPDATE;


That's cleaner - I will combine the logic in v5.


>
>> +       else if (!strcmp(ref_action_str, "print"))
>> +               ref_action = REF_ACTION_PRINT;
>> +       else
>> +               die(_("unknown --ref-action mode '%s'"), ref_action_str);
>> +
> [...]
>
>>   test_expect_success 'using replay on bare repo to rebase multiple divergent branches, including contained ones' '
>> -       git -C bare replay --contained --onto main ^main topic2 topic3 topic4 >result &&
>> +       git -C bare replay --ref-action=print --contained --onto main ^main topic2 topic3 topic4 >result &&
>>
>>          test_line_count = 4 result &&
>>          cut -f 3 -d " " result >new-branch-tips &&
> Are there tests with the new default behavior added? It looks like all
> the changes in the test script are about adding "--ref-action=print"
> to an existing test.


Yes, they're in patch 2 - the atomic behavior tests that verify no 
output and direct
ref updates. I should highlight this better in the commit message since 
they test the
absence of output (the new default).


Thanks,
Siddharth