Re: [PATCH v2] completion: improve doc for complex aliases

From: Linus Arver <linusa@google.com>
Date: Sep 14, 2023, 22:50 UTC
Message-ID: <owlyjzssjro2.fsf@fine.c.googlers.com>
In-Reply-To: <xmqqo7i6khxv.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 27 quoted lines

> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>
>> The completion code can be told to use a particular completion for
>> aliases that shell out by using ': git <cmd> ;' as the first command of
>> the alias. This only works if <cmd> and the semicolon are separated by a
>> space, since if the space is missing __git_aliased_command returns (for
>> example) 'checkout;' instead of just 'checkout', and then
>> __git_complete_command fails to find a completion for 'checkout;'.
>>
>> The examples have that space but it's not clear if it's just for
>> style or if it's mandatory. Explicitly mention it.
>>
>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> ---
>
> Thanks.  I scanned the case statement in the loop in the function
> and thought "hmph, everybody says ': git <cmd> ;' but is 'git'
> really needed?"
>
> I had "git l3" alias that invokes "$HOM#/bin/git-l" command, like so:
>
>     [alias]
>             l3 = "!sh -c ': git log ; git l \"$@\"' -"
>
> but if I did 's/: git log/: log/' it still completes just fine.

Interesting! I searched for the 'git <cmd>' and got some hits in "t9902-completion.sh" when running "git grep -nE 'git <cmd>'":

    t/t9902-completion.sh:2432:test_expect_success "completion uses <cmd> completion for alias: !sh -c 'git <cmd> ...'" '
    t/t9902-completion.sh:2441:test_expect_success 'completion uses <cmd> completion for alias: !f () { VAR=val git <cmd> ... }' '
    t/t9902-completion.sh:2450:test_expect_success 'completion used <cmd> completion for alias: !f() { : git <cmd> ; ... }' '

When I did 's/: git log/: log/' in the test at line 2450, the test still passed. Perhaps we should add this "git"-less version as another test case?

Show 22 quoted lines

> I wonder if this hack is worth adding, instead of (or in addition
> to) requiring the user to insert $IFS to please the "parser", we can
> honor the rather obvious wish of the user in a more direct way.
>
>
>
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git c/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash
> index 19139ac121..e31d71955f 100644
> --- c/contrib/completion/git-completion.bash
> +++ w/contrib/completion/git-completion.bash
> @@ -1183,7 +1183,7 @@ __git_aliased_command ()
>  			:)	: skip null command ;;
>  			\'*)	: skip opening quote after sh -c ;;
>  			*)
> -				cur="$word"
> +				cur="${word%;}"
>  				break
>  			esac
>  		done

I think this is a good defensive technique. This obviously changes the guidance that Phillipe gave in their patch (we no longer have to worry about adding a space or not between "word" and ";", so there's no need to mention this explicitly any more), but to me this seems like a better experience for our users because it's one less thing they have to worry about.