Re: [PATCH] completion: add option --recurse-submodules to "git push"

[Felipe Contreras <felipe.contreras@gmail.com>]

On Fri, Dec 7, 2012 at 11:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Steffen Jaeckel <steffen.jaeckel@stzedn.de> writes:
>
>> Signed-off-by: Steffen Jaeckel <steffen.jaeckel@stzedn.de>
>> ---
>>  contrib/completion/git-completion.bash | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 0b77eb1..5b4d2e1 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -1434,6 +1434,10 @@ _git_pull ()
>>       __git_complete_remote_or_refspec
>>  }
>>
>> +__git_push_recurse_submodules_options="
>> +     check on-demand
>> +"
>
> Most of the existing completion functions do not seem to define
> separate variables like this; instead, they literally embed their
> choices at the point of use.
>
> Is it expected that the same set of choices will appear in the
> completion of many other subcommand options?  [jc: Cc'ed Heiko so
> that we can sanity check the answer to this question].  If so, the
> variable may be justified; otherwise, not.
>
>>  _git_push ()
>>  {
>>       case "$prev" in
>> @@ -1446,10 +1450,15 @@ _git_push ()
>>               __gitcomp_nl "$(__git_remotes)" "" "${cur##--repo=}"
>>               return
>>               ;;
>> +     --recurse-submodules=*)
>> +             __gitcomp "$__git_push_recurse_submodules_options" "" "${cur##--recurse-submodules=}"
>> +             return
>> +             ;;
>
> Owners of the completion script, does this look reasonable?
> [jc: Cc'ed Felipe for this]
>
> This is a tangent, but why is it a double-hash "##" not a
> single-hash "#", other than "because all others use ##"?

Seems OK by me, but I agree, the options should be inline.

-- 
Felipe Contreras