From: Christian Couder <christian.couder@gmail.com>
Date: Fri, 24 Oct 2025 11:01:32 GMT
Subject: Re: [PATCH v4 3/3] replay: add replay.refAction config option
Message-ID: <CAP8UFD3Bz+Yn4qtCrFoKcE=u-dAtK0cXON1nFMRL8n9wBSS8pg@mail.gmail.com>
In-Reply-To: <20251022185045.29256-4-siddharthasthana31@gmail.com>

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";

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);
}

[...]

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

Thanks.