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

2018-05-20 Thread Duy Nguyen
On Sun, May 20, 2018 at 4:27 PM, SZEDER Gábor  wrote:
> On Sat, May 19, 2018 at 6:27 AM, Nguyễn Thái Ngọc Duy  
> 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 '.  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.

Maybe we can stop caching "git --list-cmds=..." then. We achieve the
same effect for completion.commands (changes take effect immediately)
and don't add any more overhead (still one git call per completion)?
This can also avoid the per-repository limitation below.

>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>>  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..*::
>>  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 ', 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?

$GIT_COMPLETION_CMD_GROUPS is for really specific customization. I
initially added it because "config" was not default, and because
completion.commands could not exclude commands, only include them. Now
that is possible, perhaps just completion.commands (no
$GIT_COMPLETINO_CMD_GROUPS) would suffice for most cases?

>
>> +
>>  include::diff-config.txt[]
>>
>>  difftool..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
>>  *:*) : 

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

2018-05-20 Thread SZEDER Gábor
On Sat, May 19, 2018 at 6:27 AM, Nguyễn Thái Ngọc Duy  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 '.  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 
> ---
>  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..*::
>  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 ', 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..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 defau