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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 6, 2020, 23:14 UTC
Message-ID: <nycvar.QRO.7.76.6.2007070114060.50@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqv9j19mgn.fsf@gitster.c.googlers.com>

Mirroring https://github.com/git/git/pull/815#issuecomment-654683996 manually, as I lack the time to figure out why Martin's mails don't make it to the mailing list:

Hi Junio,

Thank you for reviewing this patch! I've amended and pushed the branch and you can find my comments inline:

Show 29 quoted lines

> "Martin Bektchiev via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Martin Bektchiev <martin.bektchiev@progress.com>
> >
> > 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.
>
> > 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.

Show 16 quoted lines

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

Fixed.

Show 10 quoted lines

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

Fixed.

Show 7 quoted lines

>
> >  			;;
> >  		*)
> >  			;;
> >
> > base-commit: a08a83db2bf27f015bec9a435f6d73e223c21c5e
>

P.S. I responded with `git send-email` but since I still cannot see my email received it might have not been sent successfully. Hopefully you'll see my comment here ...

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/git/git/pull/815#issuecomment-654683996