Re: [PATCH v3 4/6] git.c: implement --list-cmds=porcelain

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Apr 23, 2018, 13:32 UTC
Message-ID: <CAM0VKjkd7OZspeTPumi4NVOM=7yXEq=nSjCNntkJ9d-8mPfR2w@mail.gmail.com>
In-Reply-To: <20180421165414.30051-5-pclouds@gmail.com>

On Sat, Apr 21, 2018 at 6:54 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

> This is useful for git-completion.bash because it needs this set of
> commands. Right now we have to maintain a separate command category in
> there.

I don't really understand this paragraph, in particular its second sentence. I would have described this change like this:

  To get the list of commands offered after 'git <TAB>', the
  completion script filters out plumbing commands from the list of all
  git commands.  To do so, it contains a long hard-coded list of the
  names of all known plumbing commands, which is redundant with the
  categorization in 'command-list.txt', is a maintenance burden, and
  tends to get out-of-sync when new plumbing commands are added.

  Implement 'git --list-cmds=porcelain' to list only commands
  categorized as porcelains, so we can get rid of that long hard-coded
  command list from the completion script.

But then I noticed that it's not an accurate description of the current situation, because there is a wide grey area between porcelains and plumbing, and the completion script doesn't "filter out plumbing commands", but rather filters out commands that can be considered too low-level to be useful for "general" usage. Consequently, after 'git <TAB>' we also list:

  - some 'ancillaryinterrogators': blame, annotate, difftool, fsck,
    help
  - some 'ancillarymanipulators': config, mergetool, remote
  - some 'foreignscminterface': p4, request-pull, svn, send-email
  - even some plumbing: apply, name-rev (though 'name-rev' could be
    omitted; we have 'git describe')
  - and also all "unknown" 'git-foo' commands that can be found in
    $PATH, which can be the user's own git scripts or other
    git-related tools ('git-annex', Git LFS, etc.).

With this change we wouldn't list any of the above commands, but only those that are explicitly categorized as 'mainporcelain'. I'd much prefer the current behaviour.

Show 18 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.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 106 +++----------------------
>  git.c                                  |   2 +
>  help.c                                 |  12 +++
>  help.h                                 |   1 +
>  t/t9902-completion.sh                  |   6 +-
>  5 files changed, 31 insertions(+), 96 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index a5f13ade20..7d17ca23f6 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash

Show 16 quoted lines

> @@ -859,101 +864,14 @@ __git_all_commands=
>  __git_compute_all_commands ()
>  {
>         test -n "$__git_all_commands" ||
> -       __git_all_commands=$(__git_list_all_commands)
> -}
> -
> -__git_list_porcelain_commands ()
> -{
> -       local i IFS=" "$'\n'
> -       __git_compute_all_commands
> -       for i in $__git_all_commands
> -       do
> -               case $i in
> -               *--*)             : helper pattern;;
> -               applymbox)        : ask gittus;;

<...>

> -               verify-tag)       : plumbing;;

I will be glad to see this enormous list go.

Show 5 quoted lines

> -               *) echo $i;;
> -               esac
> -       done
> +       __git_all_commands=$(__git_list_commands all)
>  }