From: Christian Couder <christian.couder@gmail.com>
Date: Fri, 31 Oct 2025 07:08:23 GMT
Subject: Re: [PATCH v6 3/3] replay: add replay.refAction config option
Message-ID: <CAP8UFD2xJVtQMEFBQAZJP+kYq5iYCcQYn9WD_x+SO8grauPrZg@mail.gmail.com>
In-Reply-To: <20251030191931.30837-4-siddharthasthana31@gmail.com>

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)

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

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

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