completion: improve doc for complex aliases #1585
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1585.git.1694274592854.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): On Sat, Sep 9, 2023 at 12:25 PM Philippe Blain via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.
s/Explicitely/Explicitly/
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> |
|
User |
|
This branch is now known as |
|
This patch series was integrated into seen via git@098924c. |
|
On the Git mailing list, Linus Arver wrote (reply to this): Hi Philippe,
"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. The examples have that space but it's not clear if it's just for
> style or if it's mandatory.
>
> Explicitely mention it.
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! |
|
User |
|
On the Git mailing list, Philippe Blain wrote (reply to this): Hi Linus,
Le 2023-09-11 à 21:04, Linus Arver a écrit :
> Hi Philippe,
>
> "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. The examples have that space but it's not clear if it's just for
>> style or if it's mandatory.
>>
>> Explicitely mention it.
>
> 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).
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. |
|
User |
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>
phil-blain
force-pushed
the
completion-shell-aliases-doc
branch
from
b3621ed
to
96a0867
Compare
|
/submit |
|
There was a status update in the "New Topics" section about the branch Clarify how "alias.foo = : git cmd ; aliased-command-string" should be spelled with necessary whitespaces around punctuation marks to work. Will merge to 'next'. source: <pull.1585.git.1694274592854.gitgitgadget@gmail.com> |
|
Submitted as pull.1585.v2.git.1694538135853.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
There was a status update in the "New Topics" section about the branch Clarify how "alias.foo = : git cmd ; aliased-command-string" should be spelled with necessary whitespaces around punctuation marks to work. Will merge to 'next'. source: <pull.1585.git.1694274592854.gitgitgadget@gmail.com> |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "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.
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 |
|
This patch series was integrated into seen via git@206cbc2. |
|
This patch series was integrated into seen via git@5077c64. |
|
This patch series was integrated into next via git@b248a5b. |
|
This patch series was integrated into seen via git@25eb2b5. |
|
On the Git mailing list, Linus Arver wrote (reply to this): "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>
> ---
> 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
>
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 |
|
On the Git mailing list, Linus Arver wrote (reply to this): Junio C Hamano <gitster@pobox.com> writes:
> "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?
> 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. |
|
There was a status update in the "Cooking" section about the branch Clarify how "alias.foo = : git cmd ; aliased-command-string" should be spelled with necessary whitespaces around punctuation marks to work. Will merge to 'master'. source: <pull.1585.v2.git.1694538135853.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@9fbfeb6. |
|
This patch series was integrated into seen via git@ee9f285. |
|
There was a status update in the "Cooking" section about the branch Clarify how "alias.foo = : git cmd ; aliased-command-string" should be spelled with necessary whitespaces around punctuation marks to work. Will merge to 'master'. source: <pull.1585.v2.git.1694538135853.gitgitgadget@gmail.com> |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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(-)
diff --git c/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash
index 47fd664ea5..477ef8157a 100644
--- c/contrib/completion/git-completion.bash
+++ w/contrib/completion/git-completion.bash
@@ -28,7 +28,8 @@
# completion style. For example '!f() { : git commit ; ... }; f' will
# tell the completion to use commit completion. This also works with aliases
# of form "!sh -c '...'". For example, "!sh -c ': git commit ; ... '".
-# Be sure to add a space between the command name and the ';'.
+# Note that "git" is optional --- '!f() { : commit; ...}; f' would complete
+# just like the 'git commit' command.
#
# If you have a command that is not part of git, but you would still
# like completion, you can use __git_complete:
@@ -1183,7 +1184,7 @@ __git_aliased_command ()
:) : skip null command ;;
\'*) : skip opening quote after sh -c ;;
*)
- cur="$word"
+ cur="${word%;}"
break
esac
done |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Junio C Hamano <gitster@pobox.com> writes:
> * 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(-)
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.
diff --git c/t/t9902-completion.sh w/t/t9902-completion.sh
index 47e20fb8b1..a7c3b4eb63 100755
--- c/t/t9902-completion.sh
+++ w/t/t9902-completion.sh
@@ -2464,6 +2464,24 @@ test_expect_success 'completion used <cmd> completion for alias: !f() { : git <c
EOF
'
+test_expect_success 'completion used <cmd> completion for alias: !f() { : <cmd> ; ... }' '
+ test_config alias.co "!f() { : checkout ; if ... } f" &&
+ test_completion "git co m" <<-\EOF
+ main Z
+ mybranch Z
+ mytag Z
+ EOF
+'
+
+test_expect_success 'completion used <cmd> completion for alias: !f() { : <cmd>; ... }' '
+ test_config alias.co "!f() { : checkout; if ... } f" &&
+ test_completion "git co m" <<-\EOF
+ main Z
+ mybranch Z
+ mytag Z
+ EOF
+'
+
test_expect_success 'completion without explicit _git_xxx function' '
test_completion "git version --" <<-\EOF
--build-options Z |
|
This patch series was integrated into seen via git@8c71f08. |
|
This patch series was integrated into master via git@8c71f08. |
|
This patch series was integrated into next via git@8c71f08. |
|
Closed via 8c71f08. |
|
On the Git mailing list, Linus Arver wrote (reply to this): Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> * 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(-)
>
> 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.
>
> diff --git c/t/t9902-completion.sh w/t/t9902-completion.sh
> index 47e20fb8b1..a7c3b4eb63 100755
> --- c/t/t9902-completion.sh
> +++ w/t/t9902-completion.sh
> @@ -2464,6 +2464,24 @@ test_expect_success 'completion used <cmd> completion for alias: !f() { : git <c
> EOF
> '
>
> +test_expect_success 'completion used <cmd> completion for alias: !f() { : <cmd> ; ... }' '
> + test_config alias.co "!f() { : checkout ; if ... } f" &&
> + test_completion "git co m" <<-\EOF
> + main Z
> + mybranch Z
> + mytag Z
> + EOF
> +'
> +
> +test_expect_success 'completion used <cmd> completion for alias: !f() { : <cmd>; ... }' '
> + test_config alias.co "!f() { : checkout; if ... } f" &&
> + test_completion "git co m" <<-\EOF
> + main Z
> + mybranch Z
> + mytag Z
> + EOF
> +'
> +
> test_expect_success 'completion without explicit _git_xxx function' '
> test_completion "git version --" <<-\EOF
> --build-options Z
LGTM, thanks! |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Linus Arver <linusa@google.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> * We've discussed this when we reviewed the topic that just hit
>>> 'master'. Before we forget...
> ...
> LGTM, thanks!
Thanks for reading. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes since v1:
CC: Steffen Prohaska prohaska@zib.de
cc: Eric Sunshine sunshine@sunshineco.com
cc: Linus Arver linusa@google.com