Re: [PATCH v6 3/3] replay: add replay.refAction config option

From: Elijah Newren <newren@gmail.com>
Date: Oct 31, 2025, 18:49 UTC
Message-ID: <CABPp-BE_pAQ8f-jjv16Ts-KRTEr3Qc402qRuJKFFW6G3J9shtA@mail.gmail.com>
In-Reply-To: <20251030191931.30837-4-siddharthasthana31@gmail.com>

On Thu, Oct 30, 2025 at 12:20 PM Siddharth Asthana <siddharthasthana31@gmail.com> wrote:

Show 12 quoted lines

>
> Add a configuration option to control the default behavior of git replay
> for updating references. This allows users who prefer the traditional
> pipeline output to set it once in their config instead of passing
> --ref-action=print with every command.
>
> The config option uses string values that mirror the behavior modes:
>   * replay.refAction = update (default): atomic ref updates
>   * replay.refAction = print: output commands for pipeline
>
> The command-line --ref-action option always overrides the config setting,
> allowing users to temporarily change behavior for a single invocation.

The above paragraph merely states that we follow git practices with this config options and its corresponding command line; I think we'd need to call it out if we didn't do that, but calling out that we do follow git conventions seems unnecessary.

Show 30 quoted lines

> Implementation details:
>
> In cmd_replay(), after parsing command-line options, we check if
> --ref-action was provided. If not, we read the configuration using
> repo_config_get_string_tmp(). If the config variable is set, we validate
> the value and use it to set the ref_action_str:
>
>   Config value      Internal mode    Behavior
>   ──────────────────────────────────────────────────────────────
>   "update"          "update"         Atomic ref updates (default)
>   "print"           "print"          Pipeline output
>   (not set)         "update"         Atomic ref updates (default)
>   (invalid)         error            Die with helpful message
>
> If an invalid value is provided, we die() immediately with an error
> message explaining the valid options. This catches configuration errors
> early and provides clear guidance to users.
>
> The command-line --ref-action option, when provided, overrides the
> config value. This precedence allows users to set their preferred default
> while still having per-invocation control:
>
>   git config replay.refAction print         # Set default
>   git replay --ref-action=update --onto main topic  # Override once
>
> The config and command-line option use the same value names ('update'
> and 'print') for consistency and clarity. This makes it immediately
> obvious how the config maps to the command-line option, addressing
> feedback about the relationship between configuration and command-line
> options being clear to users.

An implementation details section may make sense if it answers a "why?" question, or it explains something counter-intuitive, or it provides high enough level details that it makes the patch easier to read/follow, or it otherwise does something more than just repackage the patch in an alternate format. I appreciate the attempt to provide these, but I think they simply make the commit message longer without adding value.

Show 13 quoted lines

> Examples:
>
> $ git config --global replay.refAction print
> $ git replay --onto main topic1..topic2 | git update-ref --stdin
>
> $ git replay --ref-action=update --onto main topic1..topic2
>
> $ git config replay.refAction update
> $ git replay --onto main topic1..topic2  # Updates refs directly
>
> The implementation follows Git's standard configuration precedence:
> command-line options override config values, which matches user
> expectations across all Git commands.

I don't find the Examples section helpful either; it's yet another re-iteration that we're following conventions.

Show 104 quoted lines

> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Elijah Newren <newren@gmail.com>
> Helped-by: Christian Couder <christian.couder@gmail.com>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
> ---
>  Documentation/config/replay.adoc | 11 ++++++++
>  builtin/replay.c                 | 39 ++++++++++++++++++--------
>  t/t3650-replay-basics.sh         | 48 +++++++++++++++++++++++++++++++-
>  3 files changed, 86 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/config/replay.adoc
>
> diff --git a/Documentation/config/replay.adoc b/Documentation/config/replay.adoc
> new file mode 100644
> index 0000000000..7d549d2f0e
> --- /dev/null
> +++ b/Documentation/config/replay.adoc
> @@ -0,0 +1,11 @@
> +replay.refAction::
> +       Specifies the default mode for handling reference updates in
> +       `git replay`. The value can be:
> ++
> +--
> +       * `update`: Update refs directly using an atomic transaction (default behavior).
> +       * `print`: Output update-ref commands for pipeline use.
> +--
> ++
> +This setting can be overridden with the `--ref-action` command-line option.
> +When not configured, `git replay` defaults to `update` mode.
> diff --git a/builtin/replay.c b/builtin/replay.c
> index 0564d4d2e7..810068f8ef 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -8,6 +8,7 @@
>  #include "git-compat-util.h"
>
>  #include "builtin.h"
> +#include "config.h"
>  #include "environment.h"
>  #include "hex.h"
>  #include "lockfile.h"
> @@ -289,6 +290,31 @@ static struct commit *pick_regular_commit(struct repository *repo,
>         return create_commit(repo, result->tree, pickme, replayed_base);
>  }
>
> +static enum ref_action_mode parse_ref_action_mode(const char *ref_action, const char *source)
> +{
> +       if (!ref_action || !strcmp(ref_action, "update"))
> +               return REF_ACTION_UPDATE;
> +       if (!strcmp(ref_action, "print"))
> +               return REF_ACTION_PRINT;
> +       die(_("invalid %s value: '%s'"), source, ref_action);
> +}
> +
> +static enum ref_action_mode get_ref_action_mode(struct repository *repo, const char *ref_action_str)
> +{
> +       const char *config_value = NULL;
> +
> +       /* Command line option takes precedence */
> +       if (ref_action_str)
> +               return parse_ref_action_mode(ref_action_str, "--ref-action");
> +
> +       /* Check config value */
> +       if (!repo_config_get_string_tmp(repo, "replay.refAction", &config_value))
> +               return parse_ref_action_mode(config_value, "replay.refAction");
> +
> +       /* Default to update mode */
> +       return REF_ACTION_UPDATE;
> +}
> +
>  static int handle_ref_update(enum ref_action_mode mode,
>                              struct ref_transaction *transaction,
>                              const char *refname,
> @@ -367,17 +393,8 @@ int cmd_replay(int argc,
>         die_for_incompatible_opt2(!!advance_name_opt, "--advance",
>                                   contained, "--contained");
>
> -       /* 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;
> -       else if (!strcmp(ref_action_str, "print"))
> -               ref_action = REF_ACTION_PRINT;
> -       else
> -               die(_("unknown --ref-action mode '%s'"), ref_action_str);
> +       /* Parse ref action mode from command line or config */
> +       ref_action = get_ref_action_mode(repo, ref_action_str);
>
>         advance_name = xstrdup_or_null(advance_name_opt);
>
> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
> index 123734b49f..2e90227c2f 100755
> --- a/t/t3650-replay-basics.sh
> +++ b/t/t3650-replay-basics.sh
> @@ -219,7 +219,8 @@ test_expect_success 'merge.directoryRenames=false' '
>
>  test_expect_success 'default atomic behavior updates refs directly' '
>         # Store original state for cleanup
> -       test_when_finished "git branch -f topic2 topic1" &&
> +       START=$(git rev-parse topic2) &&
> +       test_when_finished "git branch -f topic2 $START" &&

Yes, these three lines are a good fix, but they belong in the previous patch.

Show 10 quoted lines

>
>         # Test default atomic behavior (no output, refs updated)
>         git replay --onto main topic1..topic2 >output &&
> @@ -232,6 +233,10 @@ test_expect_success 'default atomic behavior updates refs directly' '
>  '
>
>  test_expect_success 'atomic behavior in bare repository' '
> +       # Store original state for cleanup
> +       START=$(git rev-parse topic2) &&
> +       test_when_finished "git branch -f topic2 $START" &&

Yes, these three lines are good but they belong in a separate patch.

Show 6 quoted lines

> +
>         # Test atomic updates work in bare repo
>         git -C bare replay --onto main topic1..topic2 >output &&
>         test_must_be_empty output &&
> @@ -245,4 +250,45 @@ test_expect_success 'atomic behavior in bare repository' '
>         git -C bare update-ref refs/heads/topic2 $(git -C bare rev-parse topic1)

And this line should be removed in the previous patch.

Show 46 quoted lines

>  '
>
> +test_expect_success 'replay.refAction config option' '
> +       # Store original state
> +       START=$(git rev-parse topic2) &&
> +       test_when_finished "git branch -f topic2 $START" &&
> +
> +       # Set config to print
> +       test_config replay.refAction print &&
> +       git replay --onto main topic1..topic2 >output &&
> +       test_line_count = 1 output &&
> +       test_grep "^update refs/heads/topic2 " output &&
> +
> +       # Reset and test update mode
> +       git branch -f topic2 $START &&
> +       test_config replay.refAction update &&
> +       git replay --onto main topic1..topic2 >output &&
> +       test_must_be_empty output &&
> +
> +       # Verify ref was updated
> +       git log --format=%s topic2 >actual &&
> +       test_write_lines E D M L B A >expect &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'command-line --ref-action overrides config' '
> +       # Store original state
> +       START=$(git rev-parse topic2) &&
> +       test_when_finished "git branch -f topic2 $START" &&
> +
> +       # Set config to update but use --ref-action=print
> +       test_config replay.refAction update &&
> +       git replay --ref-action=print --onto main topic1..topic2 >output &&
> +       test_line_count = 1 output &&
> +       test_grep "^update refs/heads/topic2 " output
> +'
> +
> +test_expect_success 'invalid replay.refAction value' '
> +       test_config replay.refAction invalid &&
> +       test_must_fail git replay --onto main topic1..topic2 2>error &&
> +       test_grep "invalid.*replay.refAction.*value" error
> +'
> +
>  test_done
> --
> 2.51.0

Looks good otherwise.