{"thread":{"id":"53796","subject":"[PATCH] commit: correctly escape @ of stashes","startedAt":"2020-07-03T09:31:00Z","lastAt":"2020-07-09T09:13:31Z","messageCount":5,"participants":["Martin Bektchiev via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"401009","messageId":"pull.815.git.git.1593768655179.gitgitgadget@gmail.com","threadId":"53796","inReplyTo":null,"subject":"[PATCH] commit: correctly escape @ of stashes","fromName":"Martin Bektchiev via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-03T09:30:55Z","receivedAt":"2020-07-03T09:31:00Z","isPatch":true,"sender":{"key":"name:Martin Bektchiev","avatar":null},"body":"From: Martin Bektchiev <martin.bektchiev@progress.com>\n\nAutocomplete suggestions for stashes are broken due to `stash@`\nbeing suggested without escaping.\n\nReproducible on `GNU bash, version 3.2.57(1)-release` and\n`macOS Catalina 10.15.5`.\n\nSigned-off-by: Martin Bektchiev <martin.bektchiev@progress.com>\n---\n    Correctly escape @ of stashes\n    \n    Autocomplete suggestions for stashes are broken due to stash@being\n    suggested without escaping.\n    \n    Reproducible on GNU bash, version 3.2.57(1)-release\n    (x86_64-apple-darwin19)and macOS Catalina 10.15.5.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-815%2Fmbektchiev%2Fpatch-1-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-815/mbektchiev/patch-1-v1\nPull-Request: https://github.com/git/git/pull/815\n\n contrib/completion/git-completion.bash | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex de5d0fbbd1..986a7352ef 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2999,12 +2999,14 @@ _git_stash ()\n \t\t\t\t__git_complete_refs\n \t\t\telse\n \t\t\t\t__gitcomp_nl \"$(__git stash list \\\n-\t\t\t\t\t\t| sed -n -e 's/:.*//p')\"\n+\t\t\t\t\t\t| sed -n -e 's/:.*//p' \\\n+\t\t\t\t\t\t| sed 's/@/\\\\@/')\"\n \t\t\tfi\n \t\t\t;;\n \t\tshow,*|apply,*|drop,*|pop,*)\n \t\t\t__gitcomp_nl \"$(__git stash list \\\n-\t\t\t\t\t| sed -n -e 's/:.*//p')\"\n+\t\t\t\t\t| sed -n -e 's/:.*//p' \\\n+\t\t\t\t\t| sed 's/@/\\\\@/')\"\n \t\t\t;;\n \t\t*)\n \t\t\t;;\n\nbase-commit: a08a83db2bf27f015bec9a435f6d73e223c21c5e\n-- \ngitgitgadget\n"},{"id":"401037","messageId":"xmqqv9j19mgn.fsf@gitster.c.googlers.com","threadId":"53796","inReplyTo":"pull.815.git.git.1593768655179.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit: correctly escape @ of stashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-06T06:39:04Z","receivedAt":"2020-07-06T06:39:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Martin Bektchiev via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Martin Bektchiev <martin.bektchiev@progress.com>\n>\n> Autocomplete suggestions for stashes are broken due to `stash@`\n> being suggested without escaping.\n\nWhat is unclear to readers of this sentence is why '@' needs to be\nquoted in the first place.  '@' is not like '$' that has a syntactic\nsignificance to the shell.\n\nIn fact, I just tried\n\n    $ git stash show stas<TAB>\n\nin a test repository where there is only one stash entry and got it\ncompleted to\n\n    $ git stash show stash@{0}\n\nand pressing <Enter> from this state did exactly what I expected to\nsee.\n\n> Reproducible on `GNU bash, version 3.2.57(1)-release` and\n> `macOS Catalina 10.15.5`.\n\nWhat did you reproduce?  The completion gave me \"stash@{0}\" (without\nsurrounding double quotes) in my above experiment?  If so, that does\nseem reproducible, but I do not see \"suggestions ... are broken\" here.\n\n> @@ -2999,12 +2999,14 @@ _git_stash ()\n>  \t\t\t\t__git_complete_refs\n>  \t\t\telse\n>  \t\t\t\t__gitcomp_nl \"$(__git stash list \\\n> -\t\t\t\t\t\t| sed -n -e 's/:.*//p')\"\n> +\t\t\t\t\t\t| sed -n -e 's/:.*//p' \\\n> +\t\t\t\t\t\t| sed 's/@/\\\\@/')\"\n\nThis is not a new problem introduced by this patch, but we should\nget rid of these unnecessary backslashes and pipe at the beginning\nof the second line, both of which make the resulting code harder\nthan necessary to read.  Ending the line with '|' pipe would give\nenough clue to the shell that you haven't finished talking to it,\nand you can continue to the next line without any backslashes just\nfine.\n\n>  \t\t\tfi\n>  \t\t\t;;\n>  \t\tshow,*|apply,*|drop,*|pop,*)\n>  \t\t\t__gitcomp_nl \"$(__git stash list \\\n> -\t\t\t\t\t| sed -n -e 's/:.*//p')\"\n> +\t\t\t\t\t| sed -n -e 's/:.*//p' \\\n> +\t\t\t\t\t| sed 's/@/\\\\@/')\"\n\nDitto.\n\n>  \t\t\t;;\n>  \t\t*)\n>  \t\t\t;;\n>\n> base-commit: a08a83db2bf27f015bec9a435f6d73e223c21c5e\n"},{"id":"401184","messageId":"nycvar.QRO.7.76.6.2007070114060.50@tvgsbejvaqbjf.bet","threadId":"53796","inReplyTo":"xmqqv9j19mgn.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] commit: correctly escape @ of stashes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-06T23:14:59Z","receivedAt":"2020-07-08T11:37:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Mirroring https://github.com/git/git/pull/815#issuecomment-654683996\nmanually, as I lack the time to figure out why Martin's mails don't make\nit to the mailing list:\n\nHi Junio,\n\nThank you for reviewing this patch! I've amended and pushed the branch and\nyou can find my comments inline:\n\n\n> \"Martin Bektchiev via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Martin Bektchiev <martin.bektchiev@progress.com>\n> >\n> > Autocomplete suggestions for stashes are broken due to `stash@`\n> > being suggested without escaping.\n>\n> What is unclear to readers of this sentence is why '@' needs to be\n> quoted in the first place.  '@' is not like '$' that has a syntactic\n> significance to the shell.\n>\n> In fact, I just tried\n>\n>     $ git stash show stas<TAB>\n>\n> in a test repository where there is only one stash entry and got it\n> completed to\n>\n>     $ git stash show stash@{0}\n>\n> and pressing <Enter> from this state did exactly what I expected to\n> see.\n>\n> > Reproducible on `GNU bash, version 3.2.57(1)-release` and\n> > `macOS Catalina 10.15.5`.\n>\n> What did you reproduce?  The completion gave me \"stash@{0}\" (without\n> surrounding double quotes) in my above experiment?  If so, that does\n> seem reproducible, but I do not see \"suggestions ... are broken\" here.\n\nThe problem is visible when you have more than one stash. If\nyou press TABm autocompletion stops at the `{` and if you then\npress TAB once more for a list of all stashes instead of that\nthe completion changes to `stashstash{` and becomes broken at\nthis point.\n\n>\n> > @@ -2999,12 +2999,14 @@ _git_stash ()\n> >  \t\t\t\t__git_complete_refs\n> >  \t\t\telse\n> >  \t\t\t\t__gitcomp_nl \"$(__git stash list \\\n> > -\t\t\t\t\t\t| sed -n -e 's/:.*//p')\"\n> > +\t\t\t\t\t\t| sed -n -e 's/:.*//p' \\\n> > +\t\t\t\t\t\t| sed 's/@/\\\\@/')\"\n>\n> This is not a new problem introduced by this patch, but we should\n> get rid of these unnecessary backslashes and pipe at the beginning\n> of the second line, both of which make the resulting code harder\n> than necessary to read.  Ending the line with '|' pipe would give\n> enough clue to the shell that you haven't finished talking to it,\n> and you can continue to the next line without any backslashes just\n> fine.\n\nFixed.\n\n>\n> >  \t\t\tfi\n> >  \t\t\t;;\n> >  \t\tshow,*|apply,*|drop,*|pop,*)\n> >  \t\t\t__gitcomp_nl \"$(__git stash list \\\n> > -\t\t\t\t\t| sed -n -e 's/:.*//p')\"\n> > +\t\t\t\t\t| sed -n -e 's/:.*//p' \\\n> > +\t\t\t\t\t| sed 's/@/\\\\@/')\"\n>\n> Ditto.\n\nFixed.\n>\n> >  \t\t\t;;\n> >  \t\t*)\n> >  \t\t\t;;\n> >\n> > base-commit: a08a83db2bf27f015bec9a435f6d73e223c21c5e\n>\n\n\nP.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 ...\n\n--\nYou are receiving this because you commented.\nReply to this email directly or view it on GitHub:\nhttps://github.com/git/git/pull/815#issuecomment-654683996\n"},{"id":"401196","messageId":"xmqqo8op20ez.fsf@gitster.c.googlers.com","threadId":"53796","inReplyTo":"nycvar.QRO.7.76.6.2007070114060.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] commit: correctly escape @ of stashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-08T20:53:56Z","receivedAt":"2020-07-08T20:54:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> In fact, I just tried\n>>\n>>     $ git stash show stas<TAB>\n>>\n>> in a test repository where there is only one stash entry and got it\n>> completed to\n>>\n>>     $ git stash show stash@{0}\n>>\n>> and pressing <Enter> from this state did exactly what I expected to\n>> see.\n>>\n>> > Reproducible on `GNU bash, version 3.2.57(1)-release` and\n>> > `macOS Catalina 10.15.5`.\n>>\n>> What did you reproduce?  The completion gave me \"stash@{0}\" (without\n>> surrounding double quotes) in my above experiment?  If so, that does\n>> seem reproducible, but I do not see \"suggestions ... are broken\" here.\n>\n> The problem is visible when you have more than one stash. If\n> you press TABm autocompletion stops at the `{` and if you then\n> press TAB once more for a list of all stashes instead of that\n> the completion changes to `stashstash{` and becomes broken at\n> this point.\n\nThat does not reproduce for me.\n\n    $ git stash show stas<TAB>\n\ncompletes to\n\n    $ git stash show stash@{\n\nand any more <TAB> changes the command line, which is what I expect,\nas it would be stupid to offer stash@{0} and stash@{1} (and others)\nas possible completion candidates.\n\nAlso I just tried this (manually)\n\n    $ git stash show stash\\@{<TAB>\n\nand did not get any more extra completion.\n\n    $ set | grep ^BASH_VERSION=\n    BASH_VERSION='5.0.16(1)-release'\n\nAs long as the change does not make things worse for users of newer\nbash, it is OK to make things better for users of ancient versions\nof bash.  It might already be considered a worse end-user experience\nthan what we have now to show partly-spelled stash reference as\n\"stash\\@{\" with backslash, though.\n\nAnother thing I noticed is that you shouldn't need two separate\nprocesses of \"sed\" to do what you want to do.\n\n>> > @@ -2999,12 +2999,14 @@ _git_stash ()\n>> >  \t\t\t\t__git_complete_refs\n>> >  \t\t\telse\n>> >  \t\t\t\t__gitcomp_nl \"$(__git stash list \\\n>> > -\t\t\t\t\t\t| sed -n -e 's/:.*//p')\"\n>> > +\t\t\t\t\t\t| sed -n -e 's/:.*//p' \\\n>> > +\t\t\t\t\t\t| sed 's/@/\\\\@/')\"\n\nThe first and the original one -n -e \"s/:.*//p\" wants to show no\nlines by default, unless it finds a colon on the line and strip it\nand everything that follows.  You then further want to tweak that\nand prefix a backslash before the first '@' you find.  So perhaps\nsomething along the lines of...\n\n\tsed -n -e '/:/{\n\t\ts/:.*//\n\t\ts/@/\\\\@/\n\t\tp\n\t}'\n\n... which is \"show no lines by default, but on a line with a colon,\nstrip the colon and everything that follows, and then prefix the\nfirst '@' on the line, if exists, with backslash, and show the result\".\n\nHaving said that, I am very skeptical to any \"solution\" that has to\nshow \"stash\\@{\" to end-users.  And with the patch, newer versions of\nbash does seem to show the stupid \"all stash entries, numbered\",\ni.e.\n\n    $ git stash show stas<TAB>\n    -->\n    $ git stash show stash\\@{\n    \n    $ git stash show stash\\@{<TAB>\n    stash\\@{0}   stash\\@{1}\n\nI wonder what unpleasant end-user experience I may have to endure if\nI had dozens of stash entries.  The list shows only the numbers, so\nit does not help me decide which number to type at all.\n\nA solution to allow older versions of bash to complete\n\n    $ git stash show stas<TAB>\n    -->\n    $ git stash show stash@{\n\n    $ git stash show stash@{<TAB>\n    -->\n    $ git stash show stash@{\n\nwould be very much welcomed, though.\n\nThanks.\n\n\n"},{"id":"401233","messageId":"nycvar.QRO.7.76.6.2007091112310.50@tvgsbejvaqbjf.bet","threadId":"53796","inReplyTo":"CAEVR=HRrtT-vae8edN=Ltnp=amApMfUrt0j+6guWYMWZyz8Ohw@mail.gmail.com","subject":"Re: [PATCH] commit: correctly escape @ of stashes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-09T09:13:20Z","receivedAt":"2020-07-09T09:13:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Martin,\n\nOn Thu, 9 Jul 2020, martinb wrote:\n\n> Ok, then I think it's best if I just close this PR. I didn't know that it\n> worked alright in newer versions of BASH. And I haven't found a way to make\n> it work without the escaping of @. It seems that the patch doesn't bring\n> any value to the community after all...\n>\n> Cheers and thanks once more for the review and insights!\n>\n> Best wishes,\n> Martin\n\nI think I figured out why it does not arrive at the Git mailing list. The\nmailing list administrators installed a rule where HTML mails are\nrejected, and your mail came with a plain text _and_ an HTML part. So it\nwas rejected.\n\nCiao,\nJohannes\n\n>\n> On Wed, Jul 8, 2020, 23:54 Junio C Hamano <gitster@pobox.com> wrote:\n>\n> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> >\n> > >> In fact, I just tried\n> > >>\n> > >>     $ git stash show stas<TAB>\n> > >>\n> > >> in a test repository where there is only one stash entry and got it\n> > >> completed to\n> > >>\n> > >>     $ git stash show stash@{0}\n> > >>\n> > >> and pressing <Enter> from this state did exactly what I expected to\n> > >> see.\n> > >>\n> > >> > Reproducible on `GNU bash, version 3.2.57(1)-release` and\n> > >> > `macOS Catalina 10.15.5`.\n> > >>\n> > >> What did you reproduce?  The completion gave me \"stash@{0}\" (without\n> > >> surrounding double quotes) in my above experiment?  If so, that does\n> > >> seem reproducible, but I do not see \"suggestions ... are broken\" here.\n> > >\n> > > The problem is visible when you have more than one stash. If\n> > > you press TABm autocompletion stops at the `{` and if you then\n> > > press TAB once more for a list of all stashes instead of that\n> > > the completion changes to `stashstash{` and becomes broken at\n> > > this point.\n> >\n> > That does not reproduce for me.\n> >\n> >     $ git stash show stas<TAB>\n> >\n> > completes to\n> >\n> >     $ git stash show stash@{\n> >\n> > and any more <TAB> changes the command line, which is what I expect,\n> > as it would be stupid to offer stash@{0} and stash@{1} (and others)\n> > as possible completion candidates.\n> >\n> > Also I just tried this (manually)\n> >\n> >     $ git stash show stash\\@{<TAB>\n> >\n> > and did not get any more extra completion.\n> >\n> >     $ set | grep ^BASH_VERSION=\n> >     BASH_VERSION='5.0.16(1)-release'\n> >\n> > As long as the change does not make things worse for users of newer\n> > bash, it is OK to make things better for users of ancient versions\n> > of bash.  It might already be considered a worse end-user experience\n> > than what we have now to show partly-spelled stash reference as\n> > \"stash\\@{\" with backslash, though.\n> >\n> > Another thing I noticed is that you shouldn't need two separate\n> > processes of \"sed\" to do what you want to do.\n> >\n> > >> > @@ -2999,12 +2999,14 @@ _git_stash ()\n> > >> >                            __git_complete_refs\n> > >> >                    else\n> > >> >                            __gitcomp_nl \"$(__git stash list \\\n> > >> > -                                          | sed -n -e 's/:.*//p')\"\n> > >> > +                                          | sed -n -e 's/:.*//p' \\\n> > >> > +                                          | sed 's/@/\\\\@/')\"\n> >\n> > The first and the original one -n -e \"s/:.*//p\" wants to show no\n> > lines by default, unless it finds a colon on the line and strip it\n> > and everything that follows.  You then further want to tweak that\n> > and prefix a backslash before the first '@' you find.  So perhaps\n> > something along the lines of...\n> >\n> >         sed -n -e '/:/{\n> >                 s/:.*//\n> >                 s/@/\\\\@/\n> >                 p\n> >         }'\n> >\n> > ... which is \"show no lines by default, but on a line with a colon,\n> > strip the colon and everything that follows, and then prefix the\n> > first '@' on the line, if exists, with backslash, and show the result\".\n> >\n> > Having said that, I am very skeptical to any \"solution\" that has to\n> > show \"stash\\@{\" to end-users.  And with the patch, newer versions of\n> > bash does seem to show the stupid \"all stash entries, numbered\",\n> > i.e.\n> >\n> >     $ git stash show stas<TAB>\n> >     -->\n> >     $ git stash show stash\\@{\n> >\n> >     $ git stash show stash\\@{<TAB>\n> >     stash\\@{0}   stash\\@{1}\n> >\n> > I wonder what unpleasant end-user experience I may have to endure if\n> > I had dozens of stash entries.  The list shows only the numbers, so\n> > it does not help me decide which number to type at all.\n> >\n> > A solution to allow older versions of bash to complete\n> >\n> >     $ git stash show stas<TAB>\n> >     -->\n> >     $ git stash show stash@{\n> >\n> >     $ git stash show stash@{<TAB>\n> >     -->\n> >     $ git stash show stash@{\n> >\n> > would be very much welcomed, though.\n> >\n> > Thanks.\n> >\n> >\n> >\n>\n"}]}