commit: correctly escape @ of stashes

From: Martin Bektchiev <[email protected]>

Autocomplete suggestions for stashes are broken due to stash@ being suggested without escaping.

Reproducible on GNU bash, version 3.2.57(1)-release and macOS Catalina 10.15.5.

(quoted diff) +0 -0

 			;;
 		*)
 			;;

base-commit: a08a83db2bf27f015bec9a435f6d73e223c21c5e

Quoted 4 lines · Martin Bektchiev via GitGitGadget

From: Martin Bektchiev <[email protected]>

Autocomplete suggestions for stashes are broken due to stash@ being suggested without escaping.

What is unclear to readers of this sentence is why '@' needs to be quoted in the first place. '@' is not like '$' that has a syntactic significance to the shell.

In fact, I just tried

    $ git stash show stas<TAB>

in a test repository where there is only one stash entry and got it completed to

    $ git stash show stash@{0}

and pressing <Enter> from this state did exactly what I expected to see.

Quoted 2 lines

Reproducible on GNU bash, version 3.2.57(1)-release and macOS Catalina 10.15.5.

What did you reproduce? The completion gave me "stash@{0}" (without surrounding double quotes) in my above experiment? If so, that does seem reproducible, but I do not see "suggestions ... are broken" here.

contrib/completion/git-completion.bash _git_stash () · line 3003

Quoted 8 lines

contrib/completion/git-completion.bash +2 -1

@@ -2999,12 +2999,14 @@ _git_stash ()
 				__git_complete_refs
 			else
 				__gitcomp_nl "$(__git stash list \
-						| sed -n -e 's/:.*//p')"
+						| sed -n -e 's/:.*//p' \
+						| sed 's/@/\\@/')"

This is not a new problem introduced by this patch, but we should get rid of these unnecessary backslashes and pipe at the beginning of the second line, both of which make the resulting code harder than necessary to read. Ending the line with '|' pipe would give enough clue to the shell that you haven't finished talking to it, and you can continue to the next line without any backslashes just fine.

contrib/completion/git-completion.bash _git_stash () · line 3009

Quoted 8 lines

contrib/completion/git-completion.bash +2 -1

 			fi
 			;;
 		show,*|apply,*|drop,*|pop,*)
 			__gitcomp_nl "$(__git stash list \
-					| sed -n -e 's/:.*//p')"
+					| sed -n -e 's/:.*//p' \
+					| sed 's/@/\\@/')"

Ditto.

Quoted 6 lines

(quoted diff) +0 -0

 			;;
 		*)
 			;;

base-commit: a08a83db2bf27f015bec9a435f6d73e223c21c5e

Quoted 12 lines

In fact, I just tried

    $ git stash show stas<TAB>

in a test repository where there is only one stash entry and got it completed to

    $ git stash show stash@{0}

and pressing <Enter> from this state did exactly what I expected to see.

Quoted 6 lines

Quoted 2 lines

Reproducible on GNU bash, version 3.2.57(1)-release and macOS Catalina 10.15.5.

What did you reproduce? The completion gave me "stash@{0}" (without surrounding double quotes) in my above experiment? If so, that does seem reproducible, but I do not see "suggestions ... are broken" here.

The problem is visible when you have more than one stash. If you press TABm autocompletion stops at the { and if you then press TAB once more for a list of all stashes instead of that the completion changes to stashstash{ and becomes broken at this point.

Quoted 7 lines

contrib/completion/git-completion.bash _git_stash () · line 3003

Quoted 7 lines

contrib/completion/git-completion.bash +2 -1

@@ -2999,12 +2999,14 @@ _git_stash ()
 				__git_complete_refs
 			else
 				__gitcomp_nl "$(__git stash list \
-						| sed -n -e 's/:.*//p')"
+						| sed -n -e 's/:.*//p' \
+						| sed 's/@/\\@/')"

Ok, then I think it's best if I just close this PR. I didn't know that it worked alright in newer versions of BASH. And I haven't found a way to make it work without the escaping of @. It seems that the patch doesn't bring any value to the community after all...

Cheers and thanks once more for the review and insights!

Best wishes,
Martin

Quoted 114 lines

Quoted 19 lines · Johannes Schindelin

Quoted 12 lines

In fact, I just tried

    $ git stash show stas<TAB>

in a test repository where there is only one stash entry and got it completed to

    $ git stash show stash@{0}

and pressing <Enter> from this state did exactly what I expected to see.

Quoted 6 lines

Quoted 2 lines

Reproducible on GNU bash, version 3.2.57(1)-release and macOS Catalina 10.15.5.

What did you reproduce? The completion gave me "stash@{0}" (without surrounding double quotes) in my above experiment? If so, that does seem reproducible, but I do not see "suggestions ... are broken" here.

Quoted 5 lines

The problem is visible when you have more than one stash. If you press TABm autocompletion stops at the { and if you then press TAB once more for a list of all stashes instead of that the completion changes to stashstash{ and becomes broken at this point.

That does not reproduce for me.

    $ git stash show stas<TAB>

completes to

    $ git stash show stash@{

and any more <TAB> changes the command line, which is what I expect, as it would be stupid to offer stash@{0} and stash@{1} (and others) as possible completion candidates.

Also I just tried this (manually)

    $ git stash show stash\@{<TAB>

and did not get any more extra completion.

    $ set | grep ^BASH_VERSION=
    BASH_VERSION='5.0.16(1)-release'

As long as the change does not make things worse for users of newer bash, it is OK to make things better for users of ancient versions of bash. It might already be considered a worse end-user experience than what we have now to show partly-spelled stash reference as "stash\@{" with backslash, though.

Another thing I noticed is that you shouldn't need two separate processes of "sed" to do what you want to do.

Quoted 7 lines

Quoted 7 lines

contrib/completion/git-completion.bash _git_stash () · line 3003

Quoted 7 lines

contrib/completion/git-completion.bash +2 -1

@@ -2999,12 +2999,14 @@ _git_stash ()
                           __git_complete_refs
                   else
                           __gitcomp_nl "$(__git stash list \
-                                          | sed -n -e 's/:.*//p')"
+                                          | sed -n -e 's/:.*//p' \
+                                          | sed 's/@/\\@/')"

The first and the original one -n -e "s/:.*//p" wants to show no lines by default, unless it finds a colon on the line and strip it and everything that follows. You then further want to tweak that and prefix a backslash before the first '@' you find. So perhaps something along the lines of...

        sed -n -e '/:/{
                s/:.*//
                s/@/\\@/
                p
        }'

... which is "show no lines by default, but on a line with a colon, strip the colon and everything that follows, and then prefix the first '@' on the line, if exists, with backslash, and show the result".

Having said that, I am very skeptical to any "solution" that has to show "stash\@{" to end-users. And with the patch, newer versions of bash does seem to show the stupid "all stash entries, numbered", i.e.

    $ git stash show stas<TAB>
    -->
    $ git stash show stash\@{

    $ git stash show stash\@{<TAB>
    stash\@{0}   stash\@{1}

I wonder what unpleasant end-user experience I may have to endure if I had dozens of stash entries. The list shows only the numbers, so it does not help me decide which number to type at all.

A solution to allow older versions of bash to complete

    $ git stash show stas<TAB>
    -->
    $ git stash show stash@{

    $ git stash show stash@{<TAB>
    -->
    $ git stash show stash@{

would be very much welcomed, though.

Thanks.