Re: [PATCH v4 2/3] replay: make atomic ref updates the default behavior
- From
Siddharth Asthana <siddharthasthana31@gmail.com>
- Date
- Oct 28, 2025, 19:39 UTC
- 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:
Show 8 quoted lines
> 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!
Show 17 quoted lines
>> 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
Show 14 quoted lines
> >> +--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.
Show 14 quoted lines
> > (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
Show 10 quoted lines
>
> 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.
Show 14 quoted lines
> > [...] > >> + /* 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.
Show 17 quoted lines
>
>> + 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