From: Christian Couder <christian.couder@gmail.com>
Date: Fri, 24 Oct 2025 10:37:02 GMT
Subject: Re: [PATCH v4 2/3] replay: make atomic ref updates the default behavior
Message-ID: <CAP8UFD00rE7gF+baidmoi7nYwVKa3UDQgj+TB4wJLtjJF7u9gA@mail.gmail.com>
In-Reply-To: <20251022185045.29256-3-siddharthasthana31@gmail.com>

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

Nit: maybe: s/ergonomical/ergonomic/

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

> +--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/

(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`,/

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.

[...]

> +       /* 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;

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