From: Siddharth Asthana <siddharthasthana31@gmail•com>
To: Elijah Newren <newren@gmail•com>
Cc: git@vger•kernel.org, christian.couder@gmail•com,
phillip.wood123@gmail•com, phillip.wood@dunelm•org.uk,
gitster@pobox•com, ps@pks•im, karthik.188@gmail•com,
code@khaugsbakk•name, rybak.a.v@gmail•com, jltobler@gmail•com,
toon@iotcl•com, johncai86@gmail•com, johannes.schindelin@gmx•de
Subject: Re: [PATCH v6 3/3] replay: add replay.refAction config option
Date: Thu, 6 Nov 2025 00:40:48 +0530 [thread overview]
Message-ID: <3d1dcfe2-3d41-4c96-b44f-0611b11ce853@gmail.com> (raw)
In-Reply-To: <CABPp-BE_pAQ8f-jjv16Ts-KRTEr3Qc402qRuJKFFW6G3J9shtA@mail.gmail.com>
On 01/11/25 00:19, Elijah Newren wrote:
> On Thu, Oct 30, 2025 at 12:20 PM Siddharth Asthana
> <siddharthasthana31@gmail•com> wrote:
>> 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.
Fair point. Will remove the paragraph about command-line precedence.
>
>> 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.
Understood. Will remove the implementation details and configuration
precedence table—they're just restating what's in the code.
>
>> 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.
Will remove the Examples section too.
>
>> 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.
Right—the START/test_when_finished cleanup fixes should go in commit 2.
>
>> # 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.
The bare repo cleanup fix should also go in commit 2.
>> +
>> # 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.
Will remove the manual reset line from commit 2.
Thanks for pointing out which fixes belong where!
>
>> '
>>
>> +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.
Thanks for careful review!
next prev parent reply other threads:[~2025-11-05 19:10 UTC|newest]
Thread overview: 125+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-08 4:36 [PATCH 0/2] replay: add --update-refs option Siddharth Asthana
2025-09-08 4:36 ` [PATCH 1/2] " Siddharth Asthana
2025-09-08 9:54 ` Patrick Steinhardt
2025-09-09 6:58 ` Siddharth Asthana
2025-09-09 9:00 ` Patrick Steinhardt
2025-09-09 7:32 ` Elijah Newren
2025-09-10 17:58 ` Siddharth Asthana
2025-09-08 4:36 ` [PATCH 2/2] replay: document --update-refs and --batch options Siddharth Asthana
2025-09-08 6:00 ` Christian Couder
2025-09-09 6:36 ` Siddharth Asthana
2025-09-09 7:26 ` Christian Couder
2025-09-10 20:26 ` Siddharth Asthana
2025-09-08 14:40 ` Kristoffer Haugsbakk
2025-09-09 7:06 ` Siddharth Asthana
2025-09-09 19:20 ` Andrei Rybak
2025-09-10 20:28 ` Siddharth Asthana
2025-09-08 6:07 ` [PATCH 0/2] replay: add --update-refs option Christian Couder
2025-09-09 6:36 ` Siddharth Asthana
2025-09-08 14:33 ` Kristoffer Haugsbakk
2025-09-09 7:04 ` Siddharth Asthana
2025-09-09 7:13 ` Elijah Newren
2025-09-09 7:47 ` Christian Couder
2025-09-09 9:19 ` Elijah Newren
2025-09-09 16:44 ` Junio C Hamano
2025-09-09 19:52 ` Elijah Newren
2025-09-26 23:08 ` [PATCH v2 0/1] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-09-26 23:08 ` [PATCH v2 1/1] " Siddharth Asthana
2025-09-30 8:23 ` Christian Couder
2025-10-02 22:16 ` Siddharth Asthana
2025-10-03 7:30 ` Christian Couder
2025-10-02 22:55 ` Elijah Newren
2025-10-03 7:05 ` Christian Couder
2025-09-30 10:05 ` Phillip Wood
2025-10-02 10:00 ` Karthik Nayak
2025-10-02 22:20 ` Siddharth Asthana
2025-10-02 22:20 ` Siddharth Asthana
2025-10-08 14:01 ` Phillip Wood
2025-10-08 20:09 ` Siddharth Asthana
2025-10-08 20:59 ` Elijah Newren
2025-10-08 21:16 ` Siddharth Asthana
2025-10-09 9:40 ` Phillip Wood
2025-10-02 16:32 ` Elijah Newren
2025-10-02 18:27 ` Junio C Hamano
2025-10-02 23:42 ` Siddharth Asthana
2025-10-02 23:27 ` Siddharth Asthana
2025-10-03 7:59 ` Christian Couder
2025-10-08 19:59 ` Siddharth Asthana
2025-10-03 19:48 ` Elijah Newren
2025-10-03 20:32 ` Junio C Hamano
2025-10-08 20:06 ` Siddharth Asthana
2025-10-08 20:59 ` Junio C Hamano
2025-10-08 21:10 ` Siddharth Asthana
2025-10-08 21:30 ` Elijah Newren
2025-10-08 20:05 ` Siddharth Asthana
2025-10-02 17:14 ` [PATCH v2 0/1] " Kristoffer Haugsbakk
2025-10-02 23:36 ` Siddharth Asthana
2025-10-03 19:05 ` Kristoffer Haugsbakk
2025-10-08 20:02 ` Siddharth Asthana
2025-10-08 20:56 ` Elijah Newren
2025-10-08 21:16 ` Kristoffer Haugsbakk
2025-10-08 21:18 ` Siddharth Asthana
2025-10-13 18:33 ` [PATCH v3 0/3] replay: make atomic ref updates the default Siddharth Asthana
2025-10-13 18:33 ` [PATCH v3 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-10-13 18:33 ` [PATCH v3 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-10-13 22:05 ` Junio C Hamano
2025-10-15 5:01 ` Siddharth Asthana
2025-10-13 18:33 ` [PATCH v3 3/3] replay: add replay.defaultAction config option Siddharth Asthana
2025-10-13 19:39 ` [PATCH v3 0/3] replay: make atomic ref updates the default Junio C Hamano
2025-10-15 4:57 ` Siddharth Asthana
2025-10-15 10:33 ` Christian Couder
2025-10-15 14:45 ` Junio C Hamano
2025-10-22 18:50 ` [PATCH v4 " Siddharth Asthana
2025-10-22 18:50 ` [PATCH v4 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-10-22 18:50 ` [PATCH v4 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-10-22 21:19 ` Junio C Hamano
2025-10-28 19:03 ` Siddharth Asthana
2025-10-24 10:37 ` Christian Couder
2025-10-24 15:23 ` Junio C Hamano
2025-10-28 20:18 ` Siddharth Asthana
2025-10-28 19:39 ` Siddharth Asthana
2025-10-22 18:50 ` [PATCH v4 3/3] replay: add replay.refAction config option Siddharth Asthana
2025-10-24 11:01 ` Christian Couder
2025-10-24 15:30 ` Junio C Hamano
2025-10-28 20:08 ` Siddharth Asthana
2025-10-28 19:26 ` Siddharth Asthana
2025-10-24 13:28 ` Phillip Wood
2025-10-24 13:36 ` Phillip Wood
2025-10-28 19:47 ` Siddharth Asthana
2025-10-28 19:46 ` Siddharth Asthana
2025-10-23 18:47 ` [PATCH v4 0/3] replay: make atomic ref updates the default Junio C Hamano
2025-10-25 16:57 ` Junio C Hamano
2025-10-28 20:19 ` Siddharth Asthana
2025-10-24 9:39 ` Christian Couder
2025-10-28 21:46 ` [PATCH v5 " Siddharth Asthana
2025-10-28 21:46 ` [PATCH v5 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-10-28 21:46 ` [PATCH v5 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-10-28 21:46 ` [PATCH v5 3/3] replay: add replay.refAction config option Siddharth Asthana
2025-10-29 16:19 ` Christian Couder
2025-10-29 17:00 ` Siddharth Asthana
2025-10-30 19:19 ` [PATCH v6 0/3] replay: make atomic ref updates the default Siddharth Asthana
2025-10-30 19:19 ` [PATCH v6 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-10-31 18:47 ` Elijah Newren
2025-11-05 18:39 ` Siddharth Asthana
2025-10-30 19:19 ` [PATCH v6 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-10-31 18:49 ` Elijah Newren
2025-10-31 19:59 ` Junio C Hamano
2025-11-05 19:07 ` Siddharth Asthana
2025-11-03 16:25 ` Phillip Wood
2025-11-03 19:32 ` Siddharth Asthana
2025-11-04 16:15 ` Phillip Wood
2025-10-30 19:19 ` [PATCH v6 3/3] replay: add replay.refAction config option Siddharth Asthana
2025-10-31 7:08 ` Christian Couder
2025-11-05 19:03 ` Siddharth Asthana
2025-10-31 18:49 ` Elijah Newren
2025-11-05 19:10 ` Siddharth Asthana [this message]
2025-10-31 18:51 ` [PATCH v6 0/3] replay: make atomic ref updates the default Elijah Newren
2025-11-05 19:15 ` [PATCH v7 " Siddharth Asthana
2025-11-05 19:15 ` [PATCH v7 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-11-05 19:16 ` [PATCH v7 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-11-05 19:16 ` [PATCH v7 3/3] replay: add replay.refAction config option Siddharth Asthana
2025-11-06 19:32 ` [PATCH v7 0/3] replay: make atomic ref updates the default Elijah Newren
2025-11-08 13:22 ` Siddharth Asthana
2025-11-08 17:11 ` Elijah Newren
2025-11-07 15:48 ` Phillip Wood
2025-11-08 13:23 ` Siddharth Asthana
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3d1dcfe2-3d41-4c96-b44f-0611b11ce853@gmail.com \
--to=siddharthasthana31@gmail$(echo .)com \
--cc=christian.couder@gmail$(echo .)com \
--cc=code@khaugsbakk$(echo .)name \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=jltobler@gmail$(echo .)com \
--cc=johannes.schindelin@gmx$(echo .)de \
--cc=johncai86@gmail$(echo .)com \
--cc=karthik.188@gmail$(echo .)com \
--cc=newren@gmail$(echo .)com \
--cc=phillip.wood123@gmail$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=ps@pks$(echo .)im \
--cc=rybak.a.v@gmail$(echo .)com \
--cc=toon@iotcl$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox