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

From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Nov 5, 2025, 19:03 UTC
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:

Show 13 quoted lines

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

Show 35 quoted lines

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

Show 8 quoted lines

>
>> +       /* 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!