patchcompletion: improve doc for complex aliases

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. The examples have that space but it's not clear if it's just for style or if it's mandatory.

Explicitely mention it.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
    completion: improve doc for complex aliases
    
    The completion code can be told to use a particular completion for
    aliases that shell out by using ': git ;' as the first command of the
    alias. This only works if and the semicolon are separated by a space.
    The examples have that space but it's not clear if it's just for style
    or if it's mandatory.
    
    Explicitely mention it.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1585%2Fphil-blain%2Fcompletion-shell-aliases-doc-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1585/phil-blain/completion-shell-aliases-doc-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1585

 contrib/completion/git-completion.bash | 1 +
 1 file changed, 1 insertion(+)

On Sat, Sep 9, 2023 at 12:25 PM Philippe Blain via GitGitGadget <gitgitgadget@gmail.com> wrote:

s/Explicitely/Explicitly/

Hi Philippe,

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

It would be even more helpful if you explain _why_ it is mandatory in the commit message. Is there some Bash-specific behavior or something else going on here?

If you are unable to explain why, then as an alternative you could explain the error or buggy behavior (any error messages encountered, for example) you observe on your system when you do not use the space (which is corrected by applying the suggestion you are adding in this patch).

Thanks!

Hi Linus,

Le 2023-09-11 à 21:04, Linus Arver a écrit :

Yeah, I guess I did not investigate why it did not work without the space, it would be more complete if I did.

There's no error message though, it just does not work without the space (as I wrote in the commit message) in the sense that it won't suggest completion for the git command '<cmd>'.

I'll investigate and update the commit message.

Thanks, Philippe.

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>
---
    completion: improve doc for complex aliases
    
    Changes since v1:
    
     * fixed the typo pointed out by Eric
     * added an explanation of why the space is mandatory, as suggested by
       Linus

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1585%2Fphil-blain%2Fcompletion-shell-aliases-doc-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1585/phil-blain/completion-shell-aliases-doc-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1585

Range-diff vs v1:

 1:  b3621ed25f0 ! 1:  96a0867c5ad completion: improve doc for complex aliases
     @@ Commit message
          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. The examples have that space but it's not clear if it's just for
     -    style or if it's mandatory.
     +    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;'.
      
     -    Explicitely mention it.
     +    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>
      

 contrib/completion/git-completion.bash | 1 +
 1 file changed, 1 insertion(+)

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

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.

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(-)

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

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?

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.

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

Thanks for the investigation. The commit message reads much better now.

This LGTM, but I think Junio's review comments [1] are worth considering. I'll respond there also.

[1] https://lore.kernel.org/git/xmqqo7i6khxv.fsf@gitster.g/#t 

Recently we started to tell users to spell ": git foo ;" with space(s) around 'foo' for an alias to be completed similarly to the 'git foo' command. It however is easy to also allow users to spell it in a more natural way with the semicolon attached to 'foo', i.e. ": git foo;". Also, add a comment to note that 'git' is optional and writing ": foo;" would complete the alias just fine.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * We've discussed this when we reviewed the topic that just hit
   'master'.  Before we forget...

 contrib/completion/git-completion.bash | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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

And here is an obligatory "test" update I will squash into the main patch. The new ones copy a canonical ": git <cmd> ; ..." test and remove 'git' and also the space before the semicolon.

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

LGTM, thanks!

Linus Arver <linusa@google.com> writes:

Thanks for reading.