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

[Junio C Hamano <gitster@pobox.com>]

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.

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.

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