Re: [PATCH] commit: correctly escape @ of stashes

Hi Martin,

On Thu, 9 Jul 2020, martinb wrote:

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

I think I figured out why it does not arrive at the Git mailing list. The
mailing list administrators installed a rule where HTML mails are
rejected, and your mail came with a plain text _and_ an HTML part. So it
was rejected.

Ciao,
Johannes

On Wed, Jul 8, 2020, 23:54 Junio C Hamano [off-list ref] wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

quoted

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

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.

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

quoted

quoted

@@ -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.