From: Elijah Newren Date: Fri, 31 Oct 2025 18:49:46 GMT Subject: Re: [PATCH v6 3/3] replay: add replay.refAction config option Message-ID: In-Reply-To: <20251030191931.30837-4-siddharthasthana31@gmail.com> 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. > 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. > 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. > 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. > > # 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. > + > # 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. > ' > > +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.