Re: [PATCH 14/14] completion: allow to customize the completable command list

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Sat, May 19, 2018 at 6:27 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> By default we show porcelain, external commands and a couple others
> that are also popular. If you are not happy with this list, you can
> now customize it. See the big comment block for details.
>
> PS. perhaps I should make aliases a group too, which makes it possible
> to _not_ complete aliases by omitting this special group in
> $GIT_COMPLETION_CMD_GROUPS

Note that the completion script reads the configured aliases each time
the user attempts to complete commands.  So if the user adds or
removes an alias, then it will automatically be taken into account the
next time after 'git <TAB>'.  By turning aliases into a group listed
by 'git help' they would be cached like all other commands, so this
would no longer be the case.

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/config.txt               | 10 ++++++++
>  contrib/completion/git-completion.bash | 28 +++++++++++++++++++++-
>  git.c                                  |  2 ++
>  help.c                                 | 33 ++++++++++++++++++++++++++
>  help.h                                 |  1 +
>  5 files changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2659153cb3..91f7eaed7b 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1343,6 +1343,16 @@ credential.<url>.*::
>  credentialCache.ignoreSIGHUP::
>         Tell git-credential-cache--daemon to ignore SIGHUP, instead of quitting.
>
> +completion.commands::
> +       This is only used by git-completion.bash to add or remove
> +       commands from the complete list. Normally only porcelain

s/complete list/list of completed commands/ perhaps?

> +       commands and a few select others are in the complete list. You

s/in the complete list/completed/

> +       can add more commands, separated by space, in this
> +       variable. Prefixing the command with '-' will remove it from
> +       the existing list.
> ++
> +This variable should not be per-repository.

I think this should also mention that changing the value of this
config variable will not immediately affect the commands listed after
'git <TAB>', but the user will have to re-dot-source the completion
script first.

The way I understand the rest of the patch, this config variable
doesn't have any effect if $GIT_COMPLETION_CMD_GROUPS doesn't contain
"config".  If that is indeed the case, then that should be mentioned
here as well.

Having said that, I wonder whether we should really require "config"
in $GIT_COMPLETION_CMD_GROUPS.  Isn't having 'completion.commands' set
in the config a clear enough indication in itself that the user wants
to customize the listed commands?

> +
>  include::diff-config.txt[]
>
>  difftool.<tool>.path::
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index cd1d8e553f..f237eb0ff4 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -38,6 +38,29 @@
>  #
>  #     When set to "1", do not include "DWIM" suggestions in git-checkout
>  #     completion (e.g., completing "foo" when "origin/foo" exists).
> +#
> +#   GIT_COMPLETION_CMD_GROUPS
> +#
> +#     When set, "git --list-cmds=$GIT_COMPLETION_CMD_GROUPS" will be
> +#     used to get the list of completable commands. The default is
> +#     "mainporcelain,others,list-complete" (in English: all porcelain

Mental note #1: "mainporcelain"

> +#     commands and external ones are included. Certain non-porcelain
> +#     commands are also marked for completion in command-list.txt).
> +#     You could for example complete all commands with
> +#
> +#         GIT_COMPLETION_CMD_GROUPS=main,others

Mental note #2: "main"

> +#
> +#     Or you could go with main porcelain only and extra commands in
> +#     the configuration variable completion.commands with
> +#
> +#         GIT_COMPLETION_CMD_GROUPS=mainporcelain,config
> +#
> +#     Or go completely custom group with
> +#
> +#         GIT_COMPLETION_CMD_GROUPS=config
> +#
> +#     Or you could even play with other command categories found in
> +#     command-list.txt.
>
>  case "$COMP_WORDBREAKS" in
>  *:*) : great ;;
> @@ -842,8 +865,11 @@ __git_commands () {
>                 if test -n "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
>                 then
>                         printf "%s" "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
> +               elif test -n "$GIT_COMPLETION_CMD_GROUPS"
> +               then
> +                       git --list-cmds="$GIT_COMPLETION_CMD_GROUPS"
>                 else
> -                       git --list-cmds=list-mainporcelain,others,list-complete
> +                       git --list-cmds=list-mainporcelain,others,list-complete,config

So first it was "mainporcelain", then simply "main", then
"mainporcelain" again, and now "list-mainporcelain"?!
You've lost me here.

Are the possible values documented anywhere?

Furthermore, the default value mentioned in the comments above didn't
include "config", either (but then again, I don't think we really need
"config" in the first place).

>                 fi
>                 ;;
>         all)
> diff --git a/git.c b/git.c
> index 4d5b8a9931..ea4feedd0b 100644
> --- a/git.c
> +++ b/git.c
> @@ -60,6 +60,8 @@ static int list_cmds(const char *spec)
>                         list_all_main_cmds(&list);
>                 else if (match_token(spec, len, "others"))
>                         list_all_other_cmds(&list);
> +               else if (match_token(spec, len, "config"))
> +                       list_cmds_by_config(&list);
>                 else if (len > 5 && !strncmp(spec, "list-", 5)) {
>                         struct strbuf sb = STRBUF_INIT;
>
> diff --git a/help.c b/help.c
> index 23924dd300..abf87205b2 100644
> --- a/help.c
> +++ b/help.c
> @@ -366,6 +366,39 @@ void list_cmds_by_category(struct string_list *list,
>         }
>  }
>
> +void list_cmds_by_config(struct string_list *list)
> +{
> +       const char *cmd_list;
> +
> +       /*
> +        * There's no actual repository setup at this point (and even
> +        * if there is, we don't really care; only global config
> +        * matters). If we accidentally set up a repository, it's ok
> +        * too since the caller (git --list-cmds=) should exit shortly
> +        * anyway.
> +        */
> +       if (git_config_get_string_const("completion.commands", &cmd_list))
> +               return;
> +
> +       string_list_sort(list);
> +       string_list_remove_duplicates(list, 0);
> +
> +       while (*cmd_list) {
> +               struct strbuf sb = STRBUF_INIT;
> +               const char *p = strchrnul(cmd_list, ' ');
> +
> +               strbuf_add(&sb, cmd_list, p - cmd_list);
> +               if (*cmd_list == '-')
> +                       string_list_remove(list, cmd_list + 1, 0);
> +               else
> +                       string_list_insert(list, sb.buf);
> +               strbuf_release(&sb);
> +               while (*p == ' ')
> +                       p++;
> +               cmd_list = p;
> +       }
> +}
> +
>  void list_common_guides_help(void)
>  {
>         struct category_description catdesc[] = {
> diff --git a/help.h b/help.h
> index b2293e99be..3b38292a1b 100644
> --- a/help.h
> +++ b/help.h
> @@ -26,6 +26,7 @@ extern void list_all_main_cmds(struct string_list *list);
>  extern void list_all_other_cmds(struct string_list *list);
>  extern void list_cmds_by_category(struct string_list *list,
>                                   const char *category);
> +extern void list_cmds_by_config(struct string_list *list);
>  extern const char *help_unknown_cmd(const char *cmd);
>  extern void load_command_list(const char *prefix,
>                               struct cmdnames *main_cmds,
> --
> 2.17.0.705.g3525833791
>