Re: [PATCH v3 29/42] completion: use __gitcomp_builtin in _git_notes

[Duy Nguyen]

On Fri, Feb 23, 2018 at 5:33 PM, Duy Nguyen  wrote:
> On Wed, Feb 14, 2018 at 9:29 PM, SZEDER Gábor  wrote:
>> And we could even go one step further:
>>
>>   *,--*)
>> __gitcomp_builtin notes_$subcommand
>> ;;
>>
>> This would have the benefit that if any of the remaining subcommands
>> learn --options, then they would be completed right away, without the
>> need to add that subcommand to the case arms.
>
> You read my mind ;-) We won't do this in this series though (but I
> will simplify the code as suggested). parse-options.c does not have
> any concept about these subcommands. So a bit more work and can (and
> will) be done separately.

Actually I misread you. This is not about completing subcommands but
subcommand's options, so it should work nicely. As long as we make
sure we handle error cases correctly (e.g. the user try to complete
options of a non-existing subcommand)
-- 
Duy

Re: [PATCH v3 29/42] completion: use __gitcomp_builtin in _git_notes

[Duy Nguyen]

On Wed, Feb 14, 2018 at 10:15 PM, SZEDER Gábor  wrote:
> On Fri, Feb 9, 2018 at 12:02 PM, Nguyễn Thái Ngọc Duy  
> wrote:
>
>> diff --git a/contrib/completion/git-completion.bash 
>> b/contrib/completion/git-completion.bash
>> index c7b8b37f19..60127daebf 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -1835,7 +1835,7 @@ _git_notes ()
>>
>> case "$subcommand,$cur" in
>> ,--*)
>> -   __gitcomp '--ref'
>> +   __gitcomp_builtin notes
>> ;;
>> ,*)
>> case "$prev" in
>
> Hmm, after this patch is applied, this part of _git_notes() looks like
> this:
>
> case "$subcommand,$cur" in
> 
> ,*)
> case "$prev" in
> --ref)
> __git_complete_refs
> ;;
> *)
> __gitcomp "$subcommands --ref"
> ;;
> esac
> ;;
>
> Note that '--ref' option passed to __gitcomp() along with the
> subcommands.
>
> It would be great if that option could come from parse options as well,

Yeah. This is where Eric's option annotation thingy comes in very
handy. If we can tell git-completion.bash that "--ref takes a ref",
then we can do __git_complete_refs automatically.

I'm not so sure how to fit all things together yet. Just like you said
below __gitcomp_builtin is very much completing options, not values.
Worst case scenario, --git-completion-helper could generate the whole
"case" block to be eval'd here, but that also means non-bash
completion is left out.

So I'm not going to do anything about this yet. Not until I study some
more (and your notes here are very helpful)

> but we can't rely on $__gitcomp_builtin_notes being already initialized
> at this point and the current __gitcomp_builtin function tightly couples
> initializing that variable and completing options.
-- 
Duy

Re: [PATCH v3 29/42] completion: use __gitcomp_builtin in _git_notes

[Duy Nguyen]

On Wed, Feb 14, 2018 at 9:29 PM, SZEDER Gábor  wrote:
> On Fri, Feb 9, 2018 at 12:02 PM, Nguyễn Thái Ngọc Duy  
> wrote:
>
>> diff --git a/contrib/completion/git-completion.bash 
>> b/contrib/completion/git-completion.bash
>> index c7b8b37f19..60127daebf 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>
>> @@ -1851,15 +1851,17 @@ _git_notes ()
>> add,--reedit-message=*|append,--reedit-message=*)
>> __git_complete_refs --cur="${cur#*=}"
>> ;;
>> -   add,--*|append,--*)
>> -   __gitcomp '--file= --message= --reedit-message=
>> -   --reuse-message='
>> +   add,--*)
>> +   __gitcomp_builtin notes_add
>> +   ;;
>> +   append,--*)
>> +   __gitcomp_builtin notes_append
>> ;;
>> copy,--*)
>> -   __gitcomp '--stdin'
>> +   __gitcomp_builtin notes_copy
>> ;;
>> prune,--*)
>> -   __gitcomp '--dry-run --verbose'
>> +   __gitcomp_builtin notes_prune
>> ;;
>> prune,*)
>> ;;
>
> This could be simplified to:
>
>   add,--*|append,--*|copy,--*|prune,--*)
> __gitcomp_builtin notes_$subcommand
> ;;
>
> And we could even go one step further:
>
>   *,--*)
> __gitcomp_builtin notes_$subcommand
> ;;
>
> This would have the benefit that if any of the remaining subcommands
> learn --options, then they would be completed right away, without the
> need to add that subcommand to the case arms.

You read my mind ;-) We won't do this in this series though (but I
will simplify the code as suggested). parse-options.c does not have
any concept about these subcommands. So a bit more work and can (and
will) be done separately.
-- 
Duy

Re: [PATCH v3 29/42] completion: use __gitcomp_builtin in _git_notes

[SZEDER Gábor]

On Fri, Feb 9, 2018 at 12:02 PM, Nguyễn Thái Ngọc Duy  wrote:

> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index c7b8b37f19..60127daebf 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1835,7 +1835,7 @@ _git_notes ()
>
> case "$subcommand,$cur" in
> ,--*)
> -   __gitcomp '--ref'
> +   __gitcomp_builtin notes
> ;;
> ,*)
> case "$prev" in

Hmm, after this patch is applied, this part of _git_notes() looks like
this:

case "$subcommand,$cur" in

,*)
case "$prev" in
--ref)
__git_complete_refs
;;
*)
__gitcomp "$subcommands --ref"
;;
esac
;;

Note that '--ref' option passed to __gitcomp() along with the
subcommands.

It would be great if that option could come from parse options as well,
but we can't rely on $__gitcomp_builtin_notes being already initialized
at this point and the current __gitcomp_builtin function tightly couples
initializing that variable and completing options.

It would be even greater, if all the subcommands could come from parse
options as well, but not sure how that would fit into parse options,
because subcommands are, well, not --options.

There are only a few commands with subcommands whose main command
accepts an --option, too; I could only find 'notes', 'remote', 'stash'
and 'submodule'.  The latter two are scripts, and for 'remote' we don't
offer its '--verbose' option along with its subcommands, but perhaps we
should.  Anyway, considering that there are only two such cases at the
moment, I think we could live with that two hard-coded options.

Re: [PATCH v3 29/42] completion: use __gitcomp_builtin in _git_notes

[SZEDER Gábor]

On Fri, Feb 9, 2018 at 12:02 PM, Nguyễn Thái Ngọc Duy  wrote:

> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index c7b8b37f19..60127daebf 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash

> @@ -1851,15 +1851,17 @@ _git_notes ()
> add,--reedit-message=*|append,--reedit-message=*)
> __git_complete_refs --cur="${cur#*=}"
> ;;
> -   add,--*|append,--*)
> -   __gitcomp '--file= --message= --reedit-message=
> -   --reuse-message='
> +   add,--*)
> +   __gitcomp_builtin notes_add
> +   ;;
> +   append,--*)
> +   __gitcomp_builtin notes_append
> ;;
> copy,--*)
> -   __gitcomp '--stdin'
> +   __gitcomp_builtin notes_copy
> ;;
> prune,--*)
> -   __gitcomp '--dry-run --verbose'
> +   __gitcomp_builtin notes_prune
> ;;
> prune,*)
> ;;

This could be simplified to:

  add,--*|append,--*|copy,--*|prune,--*)
__gitcomp_builtin notes_$subcommand
;;

And we could even go one step further:

  *,--*)
__gitcomp_builtin notes_$subcommand
;;

This would have the benefit that if any of the remaining subcommands
learn --options, then they would be completed right away, without the
need to add that subcommand to the case arms.  Case in point is 'git
notes remove', which already accepts two options, but they are not
completed because of the missing 'remove,--*)' case arm.  The same would
also apply to 'git notes merge's options, except that the completion
script completely misses the 'merge' subcommand in the first place
(blame on me, 2a5da75579 (bash: support more 'git notes' subcommands and
their options, 2010-10-10)).

This also applies to the completion functions of other commands with
subcommands ('remote', 'worktree').

The downside is that we would run 'git cmd subcmd
--git-completion-helper' even if a subcommand doesn't use parse options.
I think that's acceptable.