From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Tue, 28 Oct 2025 19:26:16 GMT
Subject: Re: [PATCH v4 3/3] replay: add replay.refAction config option
Message-ID: <3dc2325a-9551-41a4-a747-c9c2c4aeca94@gmail.com>
In-Reply-To: <CAP8UFD3Bz+Yn4qtCrFoKcE=u-dAtK0cXON1nFMRL8n9wBSS8pg@mail.gmail.com>


On 24/10/25 16:31, Christian Couder wrote:
> On Wed, Oct 22, 2025 at 8:51 PM Siddharth Asthana
> <siddharthasthana31@gmail.com> wrote:
>
>> @@ -367,7 +368,20 @@ int cmd_replay(int argc,
>>          die_for_incompatible_opt2(!!advance_name_opt, "--advance",
>>                                    contained, "--contained");
>>
>> -       /* Default to update mode if not specified */
>> +       /* Set default mode from config if not specified on command line */
>> +       if (!ref_action_str) {
>> +               const char *config_value = NULL;
>> +               if (!repo_config_get_string_tmp(repo, "replay.refAction", &config_value)) {
>> +                       if (!strcmp(config_value, "update"))
>> +                               ref_action_str = "update";
>> +                       else if (!strcmp(config_value, "print"))
>> +                               ref_action_str = "print";
>> +                       else
>> +                               die(_("invalid value for replay.refAction: '%s'"), config_value);
>> +               }
>> +       }
>> +
>> +       /* Default to update mode if still not set */
>>          if (!ref_action_str)
>>                  ref_action_str = "update";


Hi Christian,
Thanks for the config parsing improvements!


> It seems to me that a dedicated function could handle this a bit
> better. Maybe something like:


Excellent suggestion! I will extract `parse_ref_action_mode()` and 
`get_ref_action_mode()`
helpers to centralize the string-to-enum conversion and config 
precedence logic, Much
cleaner than the current inline approach.


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


Will switch to `test_grep` throughout for better error reporting.

Thanks,
Siddharth


>
> Thanks.