From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Wed, 05 Nov 2025 19:03:32 GMT
Subject: Re: [PATCH v6 3/3] replay: add replay.refAction config option
Message-ID: <33578d71-2145-4256-8c90-0039ecf8fdb9@gmail.com>
In-Reply-To: <CAP8UFD2xJVtQMEFBQAZJP+kYq5iYCcQYn9WD_x+SO8grauPrZg@mail.gmail.com>


On 31/10/25 12:38, Christian Couder wrote:
> On Thu, Oct 30, 2025 at 8:20 PM Siddharth Asthana
> <siddharthasthana31@gmail.com> wrote:
>
>> +static enum ref_action_mode parse_ref_action_mode(const char *ref_action, const char *source)
>> +{
>> +       if (!ref_action || !strcmp(ref_action, "update"))
>> +               return REF_ACTION_UPDATE;
>> +       if (!strcmp(ref_action, "print"))
>> +               return REF_ACTION_PRINT;
>> +       die(_("invalid %s value: '%s'"), source, ref_action);
>> +}
>> +
>> +static enum ref_action_mode get_ref_action_mode(struct repository *repo, const char *ref_action_str)


Hi Christian,


> I think it could be "ref_action" (instead of "ref_action_str" ) in
> this function too.


Good catch. Will make this consistent in v7.


>> +{
>> +       const char *config_value = NULL;
>> +
>> +       /* Command line option takes precedence */
>> +       if (ref_action_str)
>> +               return parse_ref_action_mode(ref_action_str, "--ref-action");
>> +
>> +       /* Check config value */
>> +       if (!repo_config_get_string_tmp(repo, "replay.refAction", &config_value))
>> +               return parse_ref_action_mode(config_value, "replay.refAction");
>> +
>> +       /* Default to update mode */
>> +       return REF_ACTION_UPDATE;
>> +}
>> +
>>   static int handle_ref_update(enum ref_action_mode mode,
>>                               struct ref_transaction *transaction,
>>                               const char *refname,
>> @@ -367,17 +393,8 @@ int cmd_replay(int argc,
>>          die_for_incompatible_opt2(!!advance_name_opt, "--advance",
>>                                    contained, "--contained");
>>
>> -       /* Default to update mode if not specified */
>> -       if (!ref_action_str)
>> -               ref_action_str = "update";
>> -
>> -       /* Parse ref action mode */
>> -       if (!strcmp(ref_action_str, "update"))
>> -               ref_action = REF_ACTION_UPDATE;
>> -       else if (!strcmp(ref_action_str, "print"))
>> -               ref_action = REF_ACTION_PRINT;
>> -       else
>> -               die(_("unknown --ref-action mode '%s'"), ref_action_str);
> Maybe parse_ref_action_mode() could have been introduced in the
> previous commit already?


You're right—since parse_ref_action_mode() is actually used for 
validation in commit 2, it makes more sense to introduce it there rather 
than wait until commit 3. Will move it to the earlier commit.


>
>> +       /* Parse ref action mode from command line or config */
>> +       ref_action = get_ref_action_mode(repo, ref_action_str);
> Here it could be:
>
>        ref_mode = get_ref_action_mode(repo, ref_action);
>
> Thanks!


Agreed. The variable naming was inconsistent—I had both `ref_action`
(for the string) and `ref_action` (for the enum) which was confusing.
Will use `ref_action` for the string parameter and `ref_mode` for the
enum variable throughout for clarity.

Thanks for the careful review!