Re: [PATCH 4/9] help: add --config to list all available config

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 11, 2018, 22:39 UTC
Message-ID: <CAPig+cR63S-cpihwHYOadK_SbNRoCVjOOQH87Ry7fdCwxdbOxg@mail.gmail.com>
In-Reply-To: <20180510141927.23590-5-pclouds@gmail.com>

On Thu, May 10, 2018 at 10:20 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

> Sometimes it helps to list all available config vars so the user can
> search for something they want. The config man page can also be used
> but it's harder to search if you want to focus on the variable name,
> for example.

Show 5 quoted lines

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/builtin/help.c b/builtin/help.c
> @@ -44,6 +45,7 @@ static struct option builtin_help_options[] = {
>          OPT_BOOL('g', "guides", &show_guides, N_("print list of useful

guides")),

> +       OPT_BOOL('c', "config", &show_config, N_("print list recognized

config variables")),

s/list/& of/

Though, simpler might be better: "print all configuration variable names"

Show 9 quoted lines

> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> @@ -76,6 +76,24 @@ print_command_list () {
> +print_config_list() {
> +       cat <<EOF
> +static const char *config_name_list[] = {
> +EOF
> +       grep '^[a-zA-Z].*\..*::$' Documentation/config.txt |
> +       grep -v deprecated |
> +       sed 's/::$//; s/,  */\n/g' |

Nit: "grep -v" and "sed" could be combined:

     sed '/deprecated/d; s/::$//; s/,  */\n/g' |

Show 21 quoted lines

> +       sort |
> +       while read line
> +       do
> +               echo "  \"$line\","
> +       done
> +       cat <<EOF
> +       NULL,
> +};
> +EOF
> diff --git a/help.c b/help.c
> @@ -409,6 +409,54 @@ void list_common_guides_help(void)
> +void list_config_help(void)
> +{
> +       [...]
> +       for (p = config_name_list; *p; p++) {
> +               const char *var = *p;
> +
> +               for (e = slot_expansions; e->prefix; e++) {
> +                       struct strbuf sb = STRBUF_INIT;
> +
> +                       strbuf_addf(&sb, "%s.%s", e->prefix,

e->placeholder);

> +                       if (strcasecmp(var, sb.buf))
> +                               continue;

Isn't this "continue" leaking the strbuf? It seems that it would be easier to declare the strbuf once outside the loop, strbuf_reset() it at the top of the loop, and finally strbuf_release() it after the loop exits.

Show 8 quoted lines

> +                       e->fn(e->prefix);
> +                       strbuf_release(&sb);
> +                       e->found++;
> +                       break;
> +               }
> +               if (!e->prefix)
> +                       puts(var);
> +       }