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

From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Oct 28, 2025, 20:08 UTC
Message-ID: <5ba34d92-1032-43d0-806a-91e190b24524@gmail.com>
In-Reply-To: <xmqq7bwkqpua.fsf@gitster.g>

On 24/10/25 21:00, Junio C Hamano wrote:

Show 28 quoted lines

> Christian Couder <christian.couder@gmail.com> writes:
>
>> It seems to me that a dedicated function could handle this a bit
>> better. Maybe something like:
>>
>> static enum ref_action_mode get_ref_action_mode(const char *ref_action_str)
>> {
>>       const char *config_value = NULL;
>>
>>       if (!strcmp(ref_action_str, "update"))
>>               return REF_ACTION_UPDATE;
>>        if (!strcmp(ref_action_str, "print"))
>>              return REF_ACTION_PRINT;
>>        if (ref_action_str)
>>              die(_("unknown --ref-action mode '%s'"), ref_action_str);
>>
>>        if (repo_config_get_string_tmp(repo, "replay.refAction", &config_value))
>>               return REF_ACTION_UPDATE; /* default */
>>
>>        if (!strcmp(config_value, "update"))
>>               return REF_ACTION_UPDATE;
>>        if (!strcmp(config_value, "print"))
>>              return REF_ACTION_PRINT;
>>        die(_("invalid value for replay.refAction: '%s'"), config_value);
>> }
> You'd want to do "string to enum" helper function just once and call
> that helper from the above function, once for the command line option
> and again for the configuration variable.

That makes perfect sense - a single `parse_ref_action_mode()` helper that both can use will eliminate the duplication, as Christian suggested.

Show 12 quoted lines

>
> Or do so where you would add a call to the above function directly
> without your helper.  I am not convinced that "here is the command
> line option (or perhaps we got nothing); what is the desired
> setting, taking configuration also into consideration?" is
> particularly a good abstraction.  It is more common to have
> git_config() to grab replay.refAction string, and if there is a
> string value, pass the last one to "string to enum" helper and
> remember the result, then call parse_options() to further overwrite
> the result from the command line option string (which again will use
> the "string to enum" helper).  The structure that requires your helper
> function smells rather unusual.

Thanks for the guidance on the standard Git pattern. I had initially planned to follow Christian's combined approach, but you are right that the traditional Git pattern is more conventional. Looking at builtin/am.c and builtin/column.c, I can see they follow:

1. `repo_config()` with callback before `parse_options()`
2. Command-line options naturally override config values
3. Clean separation between config reading and option parsing

I will implement it this way in v5 - using Christian's suggestion for the helper functions but following the established Git config-then-parse-options pattern for the overall structure.

Show 16 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 && git config --unset replay.refAction" &&
>>> +
>>> +       # Set config to print
>>> +       git config replay.refAction print &&
>>> +       git replay --onto main topic1..topic2 >output &&
>>> +       test_line_count = 1 output &&
>>> +       grep "^update refs/heads/topic2 " output &&
>> Nit: here and below, it's a bit better to use test_grep instead of
>> grep for better error reporting.
> Yes, "a bit" -> "much".

Will switch to `test_grep` throughout.

Thanks for the architectural guidance!

Thanks, Siddharth

>
> Thanks.