Re: [PATCH 05/21] builtin/config: move actions into `cmd_config_actions()`

From: Kyle Lippincott <spectral@google.com>
Date: May 10, 2024, 20:42 UTC
Message-ID: <CAO_smVjXjW6pM-bWbQmCWxVi8LLZ2iB2p6RYhJhtc529cgDxHA@mail.gmail.com>
In-Reply-To: <2e308393edb0c34593e78603b43bee8d3a45dd8a.1715339393.git.ps@pks.im>

On Fri, May 10, 2024 at 4:26 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 39 quoted lines

>
> We only use actions in the legacy mode. Convert them to an enum and move
> them into `cmd_config_actions()` to clearly demonstrate that they are
> not used anywhere else.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/config.c | 44 +++++++++++++++++++-------------------------
>  1 file changed, 19 insertions(+), 25 deletions(-)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index e9956574fe..8f3342b593 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -87,30 +87,6 @@ static int show_origin;
>  static int show_scope;
>  static int fixed_value;
>
> -#define ACTION_GET (1<<0)
> -#define ACTION_GET_ALL (1<<1)
> -#define ACTION_GET_REGEXP (1<<2)
> -#define ACTION_REPLACE_ALL (1<<3)
> -#define ACTION_ADD (1<<4)
> -#define ACTION_UNSET (1<<5)
> -#define ACTION_UNSET_ALL (1<<6)
> -#define ACTION_RENAME_SECTION (1<<7)
> -#define ACTION_REMOVE_SECTION (1<<8)
> -#define ACTION_LIST (1<<9)
> -#define ACTION_EDIT (1<<10)
> -#define ACTION_SET (1<<11)
> -#define ACTION_SET_ALL (1<<12)
> -#define ACTION_GET_COLOR (1<<13)
> -#define ACTION_GET_COLORBOOL (1<<14)
> -#define ACTION_GET_URLMATCH (1<<15)
> -
> -/*
> - * The actions "ACTION_LIST | ACTION_GET_*" which may produce more than
> - * one line of output and which should therefore be paged.
> - */

We lost this comment when inlining this. Not sure if that was intentional (comment deemed to be of low value) or accidental. I think as someone unfamiliar with this logic that the comment would be helpful to keep - it describes _why_ those items are checked for (vs. needing to infer it from the action that's taken when there's a match).

Show 43 quoted lines

> -#define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
> -                       ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
> -
>  #define TYPE_BOOL              1
>  #define TYPE_INT               2
>  #define TYPE_BOOL_OR_INT       3
> @@ -1031,6 +1007,24 @@ static int cmd_config_edit(int argc, const char **argv, const char *prefix)
>
>  static int cmd_config_actions(int argc, const char **argv, const char *prefix)
>  {
> +       enum {
> +               ACTION_GET = (1<<0),
> +               ACTION_GET_ALL = (1<<1),
> +               ACTION_GET_REGEXP = (1<<2),
> +               ACTION_REPLACE_ALL = (1<<3),
> +               ACTION_ADD = (1<<4),
> +               ACTION_UNSET = (1<<5),
> +               ACTION_UNSET_ALL = (1<<6),
> +               ACTION_RENAME_SECTION = (1<<7),
> +               ACTION_REMOVE_SECTION = (1<<8),
> +               ACTION_LIST = (1<<9),
> +               ACTION_EDIT = (1<<10),
> +               ACTION_SET = (1<<11),
> +               ACTION_SET_ALL = (1<<12),
> +               ACTION_GET_COLOR = (1<<13),
> +               ACTION_GET_COLORBOOL = (1<<14),
> +               ACTION_GET_URLMATCH = (1<<15),
> +       };
>         const char *comment_arg = NULL;
>         int actions = 0;
>         struct option opts[] = {
> @@ -1147,7 +1141,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix)
>
>         comment = git_config_prepare_comment_string(comment_arg);
>
> -       if (actions & PAGING_ACTIONS)
> +       if (actions & (ACTION_LIST | ACTION_GET_ALL | ACTION_GET_REGEXP | ACTION_GET_URLMATCH))
>                 setup_auto_pager("config", 1);
>
>         if (actions == ACTION_LIST) {
> --
> 2.45.GIT
>