From: Siddharth Asthana Date: Wed, 05 Nov 2025 19:10:48 GMT Subject: Re: [PATCH v6 3/3] replay: add replay.refAction config option Message-ID: <3d1dcfe2-3d41-4c96-b44f-0611b11ce853@gmail.com> In-Reply-To: On 01/11/25 00:19, Elijah Newren wrote: > On Thu, Oct 30, 2025 at 12:20 PM Siddharth Asthana > 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 >> Helped-by: Elijah Newren >> Helped-by: Christian Couder >> Helped-by: Phillip Wood >> Signed-off-by: Siddharth Asthana >> --- >> 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!