Re: [PATCH v7 12/13] completion: let git provide the completable command list

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: May 11, 2018, 15:05 UTC
Message-ID: <CAM0VKjmL=OfKcveHRW+eAzvwS2o8TjcOS-uRNzD1JuKY2DYfFw@mail.gmail.com>
In-Reply-To: <20180510084652.25880-13-pclouds@gmail.com>

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

Show 28 quoted lines

> Instead of maintaining a separate list of command classification,
> which often could go out of date, let's centralize the information
> back in git.
>
> While the function in git-completion.bash implies "list porcelain
> commands", that's not exactly what it does. It gets all commands (aka
> --list-cmds=main,others) then exclude certain non-porcelain ones. We
> could almost recreate this list two lists list-mainporcelain and
> others. The non-porcelain-but-included-anyway is added by the third
> category list-complete.
>
> list-complete does not recreate exactly the command list before this
> patch though. The following commands are not part of neither
> list-mainporcelain nor list-complete and as a result no longer
> completes:
>
> - annotate             obsolete, discouraged to use
> - difftool-helper      not an end user command
> - filter-branch        not often used
> - get-tar-commit-id    not often used
> - imap-send            not often used
> - interpreter-trailers not for interactive use
> - lost-found           obsolete
> - p4                   too short and probably not often used (*)
> - peek-remote          deprecated
> - svn                  same category as p4 (*)
> - tar-tree             obsolete
> - verify-commit        not often used

'git name-rev' is plumbing as well.

I think this commit should be split into two:

  - first do the unequivocally beneficial thing and get rid of the
    long, hard-coded command list in __git_list_porcelain_commands(),
    while keeping its output unchanged,

  - then do the arguable thing and change the list of commands.

Show 16 quoted lines

> Note that the current completion script incorrectly classifies
> filter-branch as porcelain and t9902 tests this behavior. We keep it
> this way in t9902 because this test does not really care which
> particular command is porcelain or plubmbing, they're just names.
>
> (*) to be fair, send-email command which is in the same
> foreignscminterface group as svn and p4 does get completion, just
> because it's used by git and kernel development. So maybe should
> include them.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  command-list.txt                       |  37 ++++----
>  contrib/completion/git-completion.bash | 117 ++++++-------------------
>  t/t9902-completion.sh                  |   5 +-
>  3 files changed, 48 insertions(+), 111 deletions(-)

Show 36 quoted lines

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 4e724a5b76..908692ea52 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -835,18 +835,30 @@ __git_complete_strategy ()
>  }
>
>  __git_commands () {
> -       if test -n "${GIT_TESTING_COMMAND_COMPLETION:-}"
> -       then
> -               printf "%s" "${GIT_TESTING_COMMAND_COMPLETION}"
> -       else
> -               git --list-cmds=main,others
> -       fi
> +       case "$1" in
> +       porcelain)
> +               if test -n "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
> +               then
> +                       printf "%s" "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
> +               else
> +                       git --list-cmds=list-mainporcelain,others,list-complete
> +               fi
> +               ;;
> +       all)
> +               if test -n "$GIT_TESTING_ALL_COMMAND_LIST"
> +               then
> +                       printf "%s" "$GIT_TESTING_ALL_COMMAND_LIST"
> +               else
> +                       git --list-cmds=main,others
> +               fi
> +               ;;
> +       esac
>  }
>
> -__git_list_all_commands ()
> +__git_list_commands ()

Please add comments before these functions to document their mandatory argument.

Show 7 quoted lines

>  {
>         local i IFS=" "$'\n'
> -       for i in $(__git_commands)
> +       for i in $(__git_commands $1)
>         do
>                 case $i in
>                 *--*)             : helper pattern;;

Is this loop to exclude helper commands with doubledash in their name still necessary?