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

[Duy Nguyen]

On Fri, May 11, 2018 at 5:05 PM, SZEDER Gábor  wrote:
> On Thu, May 10, 2018 at 10:46 AM, Nguyễn Thái Ngọc Duy
>  wrote:
>> 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-branchnot often used
>> - get-tar-commit-idnot often used
>> - imap-sendnot 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-commitnot often used
>
> 'git name-rev' is plumbing as well.

So? name-rev remains completable like before and is not mentioned in
the above list. Am I missing something?

> 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.

I will. Though the first commit still changes the output slightly
because there are three commands not in command-list.txt. To keep the
output unchanged, I would need to add them back in command-list.txt
first (and find the right group for them) only to remove those lines
later (two of them deprecated, the other does not even have a man
page). It's not worth the effort.

>>  {
>> 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?

It is needed for __git_list_all_commands() because that one still
essentially grabs "git help -a", which includes command--helpers.
-- 
Duy

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

[SZEDER Gábor]

On Thu, May 10, 2018 at 10:46 AM, Nguyễn Thái Ngọc Duy
 wrote:
> 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-branchnot often used
> - get-tar-commit-idnot often used
> - imap-sendnot 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-commitnot 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.

> 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 
> ---
>  command-list.txt   |  37 
>  contrib/completion/git-completion.bash | 117 ++---
>  t/t9902-completion.sh  |   5 +-
>  3 files changed, 48 insertions(+), 111 deletions(-)

> 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.

>  {
> 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?

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

[Nguyễn Thái Ngọc Duy]

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-branchnot often used
- get-tar-commit-idnot often used
- imap-sendnot 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-commitnot often used

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 
---
 command-list.txt   |  37 
 contrib/completion/git-completion.bash | 117 ++---
 t/t9902-completion.sh  |   5 +-
 3 files changed, 48 insertions(+), 111 deletions(-)

diff --git a/command-list.txt b/command-list.txt
index 9c70c69193..3e21ddfcfb 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -47,11 +47,11 @@
 git-add mainporcelain   worktree
 git-am  mainporcelain
 git-annotateancillaryinterrogators
-git-apply   plumbingmanipulators
+git-apply   plumbingmanipulators
complete
 git-archimport  foreignscminterface
 git-archive mainporcelain
 git-bisect  mainporcelain   info
-git-blame   ancillaryinterrogators
+git-blame   ancillaryinterrogators  
complete
 git-branch  mainporcelain   history
 git-bundle  mainporcelain
 git-cat-fileplumbinginterrogators
@@ -61,7 +61,7 @@ git-check-mailmap   purehelpers
 git-checkoutmainporcelain   history
 git-checkout-index  plumbingmanipulators
 git-check-ref-formatpurehelpers
-git-cherry  ancillaryinterrogators
+git-cherry  ancillaryinterrogators  
complete
 git-cherry-pick mainporcelain
 git-citool  mainporcelain
 git-clean   mainporcelain
@@ -69,7 +69,7 @@ git-clone   mainporcelain 
  init
 git-column  purehelpers
 git-commit  mainporcelain   history
 git-commit-tree plumbingmanipulators
-git-config  ancillarymanipulators
+git-config  ancillarymanipulators   
complete
 git-count-objects   ancillaryinterrogators
 git-credential  purehelpers
 git-credential-cachepurehelpers
@@ -83,7 +83,7 @@ git-diffmainporcelain 
  history
 git-diff-files  plumbinginterrogators
 git-diff-index  plumbinginterrogators
 git-diff-tree   plumbinginterrogators
-git-difftoolancillaryinterrogators
+git-difftoolancillaryinterrogators  
complete
 git-fast-export ancillarymanipulators
 git-fast-import ancillarymanipulators
 git-fetch   mainporcelain   remote
@@ -92,20 +92,20 @@ git-filter-branch   
ancillarymanipulators
 git-fmt-merge-msg   purehelpers
 git-for-each-refplumbinginterrogators
 git-format-patch