From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Tue, 28 Oct 2025 20:08:27 GMT
Subject: Re: [PATCH v4 3/3] replay: add replay.refAction config option
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:
> 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.


>
> 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.


>
>> [...]
>>
>>> +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.