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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 24, 2025, 15:30 UTC
Message-ID: <xmqq7bwkqpua.fsf@gitster.g>
In-Reply-To: <CAP8UFD3Bz+Yn4qtCrFoKcE=u-dAtK0cXON1nFMRL8n9wBSS8pg@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 23 quoted lines

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

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.

Show 15 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".

Thanks.