{"thread":{"id":"31614","subject":"[PATCH] completion: fix shell expansion of items","startedAt":"2012-09-20T01:46:08Z","lastAt":"2012-09-28T20:28:16Z","messageCount":37,"participants":["Felipe Contreras","Jeff King","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"199561","messageId":"20120920014608.GA27782@sigill.intra.peff.net","threadId":"31614","inReplyTo":"1348107315-25095-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] completion: fix shell expansion of items","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-20T01:46:08Z","receivedAt":"2012-09-20T01:46:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 20, 2012 at 04:15:15AM +0200, Felipe Contreras wrote:\n\n> As reported by Jeroen Meijer[1]; the current code doesn't deal properly\n> with items (tags, branches, etc.) that have ${} in them because they get\n> expaned by bash while using compgen.\n> \n> A simple solution is to quote the items so they get expanded properly\n> (\\$\\{\\}).\n> \n> In order to achieve that I took bash-completion's quote() function,\n> which is rather simple, and renamed it to __git_quote() as per Jeff\n> King's suggestion.\n> \n> Solves the original problem for me.\n\nMe too. Thanks.\n\n-Peff\n"},{"id":"199555","messageId":"1348107315-25095-1-git-send-email-felipe.contreras@gmail.com","threadId":"31614","inReplyTo":null,"subject":"[PATCH] completion: fix shell expansion of items","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-09-20T02:15:15Z","receivedAt":"2012-09-20T02:15:15Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"As reported by Jeroen Meijer[1]; the current code doesn't deal properly\nwith items (tags, branches, etc.) that have ${} in them because they get\nexpaned by bash while using compgen.\n\nA simple solution is to quote the items so they get expanded properly\n(\\$\\{\\}).\n\nIn order to achieve that I took bash-completion's quote() function,\nwhich is rather simple, and renamed it to __git_quote() as per Jeff\nKing's suggestion.\n\nSolves the original problem for me.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/201596\n\nSigned-off-by: Felipe Contreras \n---\n contrib/completion/git-completion.bash | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex d743e56..5a5b5a0 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -225,6 +225,13 @@ _get_comp_words_by_ref ()\n fi\n fi\n \n+# Quotes the argument for shell reuse\n+__git_quote()\n+{\n+\tlocal quoted=${1//\\'/\\'\\\\\\'\\'}\n+\tprintf \"'%s'\" \"$quoted\"\n+}\n+\n # Generates completion reply with compgen, appending a space to possible\n # completion words, if necessary.\n # It accepts 1 to 4 arguments:\n@@ -261,7 +268,7 @@ __gitcomp ()\n __gitcomp_nl ()\n {\n \tlocal IFS=$'\\n'\n-\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n+\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$(__git_quote \"$1\")\" -- \"${3-$cur}\"))\n }\n \n __git_heads ()\n-- \n1.7.10.3\n"},{"id":"199590","messageId":"7vmx0kbpr2.fsf@alter.siamese.dyndns.org","threadId":"31614","inReplyTo":"20120920014608.GA27782@sigill.intra.peff.net","subject":"Re: [PATCH] completion: fix shell expansion of items","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-20T16:52:49Z","receivedAt":"2012-09-20T16:52:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Sep 20, 2012 at 04:15:15AM +0200, Felipe Contreras wrote:\n>\n>> As reported by Jeroen Meijer[1]; the current code doesn't deal properly\n>> with items (tags, branches, etc.) that have ${} in them because they get\n>> expaned by bash while using compgen.\n>> \n>> A simple solution is to quote the items so they get expanded properly\n>> (\\$\\{\\}).\n>> \n>> In order to achieve that I took bash-completion's quote() function,\n>> which is rather simple, and renamed it to __git_quote() as per Jeff\n>> King's suggestion.\n>> \n>> Solves the original problem for me.\n>\n> Me too. Thanks.\n\nThanks, both. Will queue.\n"},{"id":"199602","messageId":"20120920181152.GA4689@goldbirke","threadId":"31614","inReplyTo":"20120920014608.GA27782@sigill.intra.peff.net","subject":"Re: [PATCH] completion: fix shell expansion of items","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-20T18:11:52Z","receivedAt":"2012-09-20T18:11:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nOn Wed, Sep 19, 2012 at 09:46:08PM -0400, Jeff King wrote:\n> On Thu, Sep 20, 2012 at 04:15:15AM +0200, Felipe Contreras wrote:\n> \n> > As reported by Jeroen Meijer[1]; the current code doesn't deal properly\n> > with items (tags, branches, etc.) that have ${} in them because they get\n> > expaned by bash while using compgen.\n> > \n> > A simple solution is to quote the items so they get expanded properly\n> > (\\$\\{\\}).\n> > \n> > In order to achieve that I took bash-completion's quote() function,\n> > which is rather simple, and renamed it to __git_quote() as per Jeff\n> > King's suggestion.\n> > \n> > Solves the original problem for me.\n> \n> Me too. Thanks.\n\nWhile it solves the original problem, it seems to break refs\ncompletion, as demonstrated by the following POC test:\n\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 92d7eb47..fab63b95 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -228,4 +228,11 @@ test_expect_success 'general options plus command' '\n \ttest_completion \"git --no-replace-objects check\" \"checkout \"\n '\n \n+test_expect_success 'basic refs completion' '\n+\ttouch file &&\n+\tgit add file &&\n+\tgit commit -m initial &&\n+\ttest_completion \"git branch m\" \"master \"\n+'\n+\n test_done\n-- \n1.7.12.1.438.g7dfa67b\n\n\nwhich fails with:\n\n--- expected 2012-09-20 18:05:23.857752925 +0000\n+++ out 2012-09-20 18:05:23.877752925 +0000\n@@ -1 +1 @@\n-master \n+\n"},{"id":"199603","messageId":"20120920182104.GB19204@sigill.intra.peff.net","threadId":"31614","inReplyTo":"20120920181152.GA4689@goldbirke","subject":"Re: [PATCH] completion: fix shell expansion of items","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-20T18:21:04Z","receivedAt":"2012-09-20T18:21:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote:\n\n> > > In order to achieve that I took bash-completion's quote() function,\n> > > which is rather simple, and renamed it to __git_quote() as per Jeff\n> > > King's suggestion.\n> > > \n> > > Solves the original problem for me.\n> > \n> > Me too. Thanks.\n> \n> While it solves the original problem, it seems to break refs\n> completion, as demonstrated by the following POC test:\n> \n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index 92d7eb47..fab63b95 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -228,4 +228,11 @@ test_expect_success 'general options plus command' '\n> \ttest_completion \"git --no-replace-objects check\" \"checkout \"\n> '\n> \n> +test_expect_success 'basic refs completion' '\n> +\ttouch file &&\n> +\tgit add file &&\n> +\tgit commit -m initial &&\n> +\ttest_completion \"git branch m\" \"master \"\n> +'\n\nHmm. I notice that Felipe's patch wraps the _whole_ input to\n__gitcomp_nl in single quotes. So if there are multiple completions we\nwould end up with:\n\n 'one\n two\n quo\\'ted\n three'\n\nI wonder if that is OK to feed to compgen -W, or if it wants to expand\nit line-by-line. Just guessing at this point, though.\n\n-Peff\n\n> +\n> test_done\n> -- \n> 1.7.12.1.438.g7dfa67b\n> \n> \n> which fails with:\n> \n> --- expected 2012-09-20 18:05:23.857752925 +0000\n> +++ out 2012-09-20 18:05:23.877752925 +0000\n> @@ -1 +1 @@\n> -master \n> +\n> \n"},{"id":"199606","messageId":"CAMP44s1daQ_Cs8gGwpTJx0+9eKdRGF28PJkiAKJ8wf-2=WUMng@mail.gmail.com","threadId":"31614","inReplyTo":"20120920182104.GB19204@sigill.intra.peff.net","subject":"Re: [PATCH] completion: fix shell expansion of items","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-09-20T19:38:52Z","receivedAt":"2012-09-20T19:38:52Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Sep 20, 2012 at 8:21 PM, Jeff King wrote:\n> On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote:\n>\n>> > > In order to achieve that I took bash-completion's quote() function,\n>> > > which is rather simple, and renamed it to __git_quote() as per Jeff\n>> > > King's suggestion.\n>> > >\n>> > > Solves the original problem for me.\n>> >\n>> > Me too. Thanks.\n>>\n>> While it solves the original problem, it seems to break refs\n>> completion, as demonstrated by the following POC test:\n>>\n>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n>> index 92d7eb47..fab63b95 100755\n>> --- a/t/t9902-completion.sh\n>> +++ b/t/t9902-completion.sh\n>> @@ -228,4 +228,11 @@ test_expect_success 'general options plus command' '\n>> test_completion \"git --no-replace-objects check\" \"checkout \"\n>> '\n>>\n>> +test_expect_success 'basic refs completion' '\n>> + touch file &&\n>> + git add file &&\n>> + git commit -m initial &&\n>> + test_completion \"git branch m\" \"master \"\n>> +'\n>\n> Hmm. I notice that Felipe's patch wraps the _whole_ input to\n> __gitcomp_nl in single quotes.\n\nWasn't there a patch series that added tests for __gitcomp_nl to catch\nthese issues?\n\n-- \nFelipe Contreras\n"},{"id":"199877","messageId":"20120925043119.GA6208@sigill.intra.peff.net","threadId":"31614","inReplyTo":"20120920181152.GA4689@goldbirke","subject":"[PATCH] Revert \"completion: fix shell expansion of items\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-25T04:31:19Z","receivedAt":"2012-09-25T04:31:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote:\n\n> > > Solves the original problem for me.\n> > \n> > Me too. Thanks.\n> \n> While it solves the original problem, it seems to break refs\n> completion, as demonstrated by the following POC test:\n> \n> \n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index 92d7eb47..fab63b95 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -228,4 +228,11 @@ test_expect_success 'general options plus command' '\n> \ttest_completion \"git --no-replace-objects check\" \"checkout \"\n> '\n> \n> +test_expect_success 'basic refs completion' '\n> +\ttouch file &&\n> +\tgit add file &&\n> +\tgit commit -m initial &&\n> +\ttest_completion \"git branch m\" \"master \"\n> +'\n> +\n\nYeah, doing \"git checkout jk/\" is not working at all, and I noticed\nthe buggy commit is on the maint track, but has not yet been released.\nI'm not sure of the solution, but I think we should do this in the\nmeantime:\n\n-- >8 --\nSubject: Revert \"completion: fix shell expansion of items\"\n\nThis reverts commit 25ae7cfd19c8f21721363c64163cd5d9d1135b20.\n\nThat patch does fix expansion of weird variables in some\nsimple tests, but it also seems to break other things, like\nexpansion of refs by \"git checkout\".\n\nWhile we're sorting out the correct solution, we are much\nbetter with the original bug (people with metacharacters in\ntheir completions occasionally see an error message) than\nthe current bug (ref completion does not work at all).\n\nSigned-off-by: Jeff King \n---\n contrib/completion/git-completion.bash | 9 +--------\n 1 file changed, 1 insertion(+), 8 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 5a5b5a0..d743e56 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -225,13 +225,6 @@ fi\n fi\n fi\n \n-# Quotes the argument for shell reuse\n-__git_quote()\n-{\n-\tlocal quoted=${1//\\'/\\'\\\\\\'\\'}\n-\tprintf \"'%s'\" \"$quoted\"\n-}\n-\n # Generates completion reply with compgen, appending a space to possible\n # completion words, if necessary.\n # It accepts 1 to 4 arguments:\n@@ -268,7 +261,7 @@ __gitcomp_nl ()\n __gitcomp_nl ()\n {\n \tlocal IFS=$'\\n'\n-\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$(__git_quote \"$1\")\" -- \"${3-$cur}\"))\n+\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n }\n \n __git_heads ()\n-- \n1.7.12.1.17.g7286916\n"},{"id":"199895","messageId":"7vpq5aw0m5.fsf@alter.siamese.dyndns.org","threadId":"31614","inReplyTo":"20120925043119.GA6208@sigill.intra.peff.net","subject":"Re: [PATCH] Revert \"completion: fix shell expansion of items\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-25T16:03:46Z","receivedAt":"2012-09-25T16:03:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote:\n>\n> Yeah, doing \"git checkout jk/\" is not working at all, and I noticed\n> the buggy commit is on the maint track, but has not yet been released.\n> I'm not sure of the solution, but I think we should do this in the\n> meantime:\n\nThanks. Reverting is the right solution; it will leave a note in\nthe history why a seemingly simple patch was a bad idea to remind\nus.\n\nIt is sad that existing test did not cover something fundamental\nlike this, though. Anybody who's worked on completion wants to make\nthe test coverage better?\n\n> -- >8 --\n> Subject: Revert \"completion: fix shell expansion of items\"\n>\n> This reverts commit 25ae7cfd19c8f21721363c64163cd5d9d1135b20.\n>\n> That patch does fix expansion of weird variables in some\n> simple tests, but it also seems to break other things, like\n> expansion of refs by \"git checkout\".\n>\n> While we're sorting out the correct solution, we are much\n> better with the original bug (people with metacharacters in\n> their completions occasionally see an error message) than\n> the current bug (ref completion does not work at all).\n>\n> Signed-off-by: Jeff King \n> ---\n> contrib/completion/git-completion.bash | 9 +--------\n> 1 file changed, 1 insertion(+), 8 deletions(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 5a5b5a0..d743e56 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -225,13 +225,6 @@ fi\n> fi\n> fi\n> \n> -# Quotes the argument for shell reuse\n> -__git_quote()\n> -{\n> -\tlocal quoted=${1//\\'/\\'\\\\\\'\\'}\n> -\tprintf \"'%s'\" \"$quoted\"\n> -}\n> -\n> # Generates completion reply with compgen, appending a space to possible\n> # completion words, if necessary.\n> # It accepts 1 to 4 arguments:\n> @@ -268,7 +261,7 @@ __gitcomp_nl ()\n> __gitcomp_nl ()\n> {\n> \tlocal IFS=$'\\n'\n> -\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$(__git_quote \"$1\")\" -- \"${3-$cur}\"))\n> +\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n> }\n> \n> __git_heads ()\n"},{"id":"199911","messageId":"20120925223725.GA12658@goldbirke","threadId":"31614","inReplyTo":"20120925043119.GA6208@sigill.intra.peff.net","subject":"Re: [PATCH] Revert \"completion: fix shell expansion of items\"","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-25T22:37:25Z","receivedAt":"2012-09-25T22:37:25Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Sep 25, 2012 at 12:31:19AM -0400, Jeff King wrote:\n> Yeah, doing \"git checkout jk/\" is not working at all, and I noticed\n> the buggy commit is on the maint track, but has not yet been released.\n> I'm not sure of the solution, but I think we should do this in the\n> meantime:\n> \n> -- >8 --\n> Subject: Revert \"completion: fix shell expansion of items\"\n\nI agree with the revert, too.\n\nI looked into this issue, but quickly got lost in quoting-escaping\nhell. Ideally we could do some quoting in __gitcomp_nl(), so it would\nwork for all kinds of input, but I couldn't come up with anything\nworking. Alternatively, we could modify __gitcomp_nl()'s callers, or\nrather the helper functions supplying input to __gitcomp_nl() to do\nthe quoting or escaping themselves. Actually, that's quite easy for\nlocal refs, at least, because for-each-ref's builtin quoting support\ndoes the trick:\n\ndiff --git a/contrib/completion/git-completion.bash\nb/contrib/completion/git-completion.bash\nindex c48cd19f..3dc1ec8c 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -313,7 +313,7 @@ __git_refs ()\n \t\t\trefs=\"refs/tags refs/heads refs/remotes\"\n \t\t\t;;\n \t\tesac\n-\t\tgit --git-dir=\"$dir\" for-each-ref --format=\"%($format)\" \\\n+\t\tgit --git-dir=\"$dir\" for-each-ref --shell --format=\"%($format)\" \\\n \t\t\t$refs\n \t\tif [ -n \"$track\" ]; then\n \t\t\t# employ the heuristic used by git checkout\n\nWith this change completion of local refs works well, even in the\npresence of branches ${foo.bar} and foo'bar.\n\nUnfortunately, there are many callsites for __gitcomp_nl(), and it is\ninvoked with refs from remote repos, heads, tags, refspecs, remotes,\nconfig variables, symbols from ctags, or output from stash or\nls-tree... although some of these are OK as they are now (remotes,\nconfig variables).\n\n\nBest,\nGábor\n"},{"id":"199916","messageId":"7vfw65vg17.fsf@alter.siamese.dyndns.org","threadId":"31614","inReplyTo":"20120925223725.GA12658@goldbirke","subject":"Re: [PATCH] Revert \"completion: fix shell expansion of items\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-25T23:28:20Z","receivedAt":"2012-09-25T23:28:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Tue, Sep 25, 2012 at 12:31:19AM -0400, Jeff King wrote:\n>> Yeah, doing \"git checkout jk/\" is not working at all, and I noticed\n>> the buggy commit is on the maint track, but has not yet been released.\n>> I'm not sure of the solution, but I think we should do this in the\n>> meantime:\n>> \n>> -- >8 --\n>> Subject: Revert \"completion: fix shell expansion of items\"\n>\n> I agree with the revert, too.\n>\n> I looked into this issue, but quickly got lost in quoting-escaping\n> hell. Ideally we could do some quoting in __gitcomp_nl(), so it would\n> work for all kinds of input, but I couldn't come up with anything\n> working. Alternatively, we could modify __gitcomp_nl()'s callers, or\n> rather the helper functions supplying input to __gitcomp_nl() to do\n> the quoting or escaping themselves. Actually, that's quite easy for\n> local refs, at least, because for-each-ref's builtin quoting support\n> does the trick:\n>\n> diff --git a/contrib/completion/git-completion.bash\n> b/contrib/completion/git-completion.bash\n> index c48cd19f..3dc1ec8c 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -313,7 +313,7 @@ __git_refs ()\n> \t\t\trefs=\"refs/tags refs/heads refs/remotes\"\n> \t\t\t;;\n> \t\tesac\n> -\t\tgit --git-dir=\"$dir\" for-each-ref --format=\"%($format)\" \\\n> +\t\tgit --git-dir=\"$dir\" for-each-ref --shell --format=\"%($format)\" \\\n> \t\t\t$refs\n> \t\tif [ -n \"$track\" ]; then\n> \t\t\t# employ the heuristic used by git checkout\n>\n> With this change completion of local refs works well, even in the\n> presence of branches ${foo.bar} and foo'bar.\n>\n> Unfortunately, there are many callsites for __gitcomp_nl(), and it is\n> invoked with refs from remote repos, heads, tags, refspecs, remotes,\n> config variables, symbols from ctags, or output from stash or\n> ls-tree... although some of these are OK as they are now (remotes,\n> config variables).\n\nWhatever you do, please make the first step of that endeavour an\naddition to the t/t9902 to prevent the same breakage from happening\nagain.\n\nThanks.\n"},{"id":"199979","messageId":"20120926214653.GA18628@sigill.intra.peff.net","threadId":"31614","inReplyTo":"20120925223725.GA12658@goldbirke","subject":"Re: [PATCH] Revert \"completion: fix shell expansion of items\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-26T21:46:53Z","receivedAt":"2012-09-26T21:46:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 26, 2012 at 12:37:25AM +0200, SZEDER Gábor wrote:\n\n> I looked into this issue, but quickly got lost in quoting-escaping\n> hell. Ideally we could do some quoting in __gitcomp_nl(), so it would\n> work for all kinds of input, but I couldn't come up with anything\n> working. Alternatively, we could modify __gitcomp_nl()'s callers, or\n> rather the helper functions supplying input to __gitcomp_nl() to do\n> the quoting or escaping themselves. Actually, that's quite easy for\n> local refs, at least, because for-each-ref's builtin quoting support\n> does the trick:\n\nI feel like insanity lies that way, because every caller is going to\nhave to do its own quoting. On the other hand, I think it would be the\nonly way to handle completion of entries with embedded newlines (as it\nis now, we pass in a newline-delimited list with no opportunity for\nquoting).\n\nHere's a simple patch series that fixes the problem and adds a few basic\nsanity checks.\n\n [1/3]: t9902: add a few basic completion tests\n [2/3]: t9902: add completion tests for \"odd\" filenames\n [3/3]: completion: improve shell expansion of items\n\n-Peff\n"},{"id":"199980","messageId":"20120926214750.GA18653@sigill.intra.peff.net","threadId":"31614","inReplyTo":"20120926214653.GA18628@sigill.intra.peff.net","subject":"[PATCH 1/3] t9902: add a few basic completion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-26T21:47:51Z","receivedAt":"2012-09-26T21:47:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We were not testing ref or tree completion at all. Let's\ngive them even basic sanity checks to avoid regressions.\n\nSigned-off-by: Jeff King \n---\nThis would have caught the recent breakage, and also paves the way for\ntesting the new fix.\n\n t/t9902-completion.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 41 insertions(+)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 92d7eb4..2fc833a 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -61,6 +61,15 @@ test_completion ()\n \ttest_cmp expected out\n }\n \n+# Like test_completion, but reads expectation from stdin,\n+# which is convenient when it is multiline. We also process \"_\" into\n+# spaces to make test vectors more readable.\n+test_completion_long ()\n+{\n+\ttr _ \" \" >expected &&\n+\ttest_completion \"$1\"\n+}\n+\n newline=$'\\n'\n \n test_expect_success '__gitcomp - trailing space - options' '\n@@ -228,4 +237,36 @@ test_expect_success 'general options plus command' '\n \ttest_completion \"git --no-replace-objects check\" \"checkout \"\n '\n \n+test_expect_success 'setup for ref completion' '\n+\techo content >file1 &&\n+\techo more >file2 &&\n+\tgit add . &&\n+\tgit commit -m one &&\n+\tgit branch mybranch &&\n+\tgit tag mytag\n+'\n+\n+test_expect_success 'checkout completes ref names' '\n+\ttest_completion_long \"git checkout m\" <<-\\EOF\n+\tmaster_\n+\tmybranch_\n+\tmytag_\n+\tEOF\n+'\n+\n+test_expect_success 'show completes all refs' '\n+\ttest_completion_long \"git show m\" <<-\\EOF\n+\tmaster_\n+\tmybranch_\n+\tmytag_\n+\tEOF\n+'\n+\n+test_expect_success ': completes paths' '\n+\ttest_completion_long \"git show mytag:f\" <<-\\EOF\n+\tfile1_\n+\tfile2_\n+\tEOF\n+'\n+\n test_done\n-- \n1.7.12.10.g31da6dd\n"},{"id":"199981","messageId":"20120926215106.GB18653@sigill.intra.peff.net","threadId":"31614","inReplyTo":"20120926214653.GA18628@sigill.intra.peff.net","subject":"[PATCH 2/3] t9902: add completion tests for \"odd\" filenames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-26T21:51:06Z","receivedAt":"2012-09-26T21:51:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We correctly handle completion items with spaces just fine,\nsince we pass the lists around with newline delimiters.\nHowever, we do not handle filenames with shell\nmetacharacters, as \"compgen -W\" performs expansion on the\nlist we give it.\n\nSigned-off-by: Jeff King \n---\nActually, these vectors are not strictly correct, as I think ultimately\nwe would like to return\n\n 'name with spaces'\n\nwith quotes. But this lets us test that at least the basics work, and\nwe can update the test vectors later.\n\nIt would be nice to test completion on a file with an embedded newline\n(which will also not work), but I'm not even sure what the result is\nsupposed to look like, so I couldn't write a sane test case. Since I\ndon't plan on fixing that anytime soon anyway, I think it's sane to just\nleave it until later if somebody actually cares.\n\n t/t9902-completion.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 2fc833a..cbd0fb6 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -269,4 +269,23 @@ test_expect_success ': completes paths' '\n \tEOF\n '\n \n+test_expect_success 'complete tree filename with spaces' '\n+\techo content >\"name with spaces\" &&\n+\tgit add . &&\n+\tgit commit -m spaces &&\n+\ttest_completion_long \"git show HEAD:nam\" <<-\\EOF\n+\tname with spaces_\n+\tEOF\n+'\n+\n+test_expect_failure 'complete tree filename with metacharacters' '\n+\techo content >\"name with \\${meta}\" &&\n+\tgit add . &&\n+\tgit commit -m meta &&\n+\ttest_completion_long \"git show HEAD:nam\" <<-\\EOF\n+\tname with ${meta}_\n+\tname with spaces_\n+\tEOF\n+'\n+\n test_done\n-- \n1.7.12.10.g31da6dd\n"},{"id":"199982","messageId":"20120926215119.GC18653@sigill.intra.peff.net","threadId":"31614","inReplyTo":"20120926214653.GA18628@sigill.intra.peff.net","subject":"[PATCH 3/3] completion: improve shell expansion of items","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-26T21:51:19Z","receivedAt":"2012-09-26T21:51:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The current completion code doesn't deal properly with items\n(tags, branches, etc.) that have ${} in them because they\nget expanded by bash while using compgen.\n\nThis patch is a rewrite of Felipe Contreras's 25ae7cf, which\nattempted to fix the problem by quoting the values we pass\nto __gitcomp_nl. However, that patch ended up quoting the\nwhole list as a single item, which broke other completions.\nInstead, we need to split the newline-delimited list into\nelements and quote each one individually.\n\nThis is not a complete fix, as the completion result does\nwill still contain metacharacters, so it would need extra\nquoting to actually be used on a command line. But it's\nstill a step in the right direction, because:\n\n 1. The current code's expansion means that things that are\n broken expansions (e.g., \"${foo:bar}\") will actually\n cause the completion function to barf, breaking all\n completion (even if you weren't going to complete that\n item). This patch fixes that so you can at least\n complete \"foo\" when \"${foo:bar}\" exists.\n\n 2. We don't know yet what the final fix will look like,\n but this is probably the first step towards it. It\n handles quoting on the input side of compgen; the next\n step will likely be handling the quoting on the output\n side of compgen to yield a usable string for the\n command line.\n\nSigned-off-by: Jeff King \n---\n contrib/completion/git-completion.bash | 14 +++++++++++++-\n t/t9902-completion.sh | 2 +-\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex be800e0..b0416ea 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -225,6 +225,18 @@ fi\n fi\n fi\n \n+# Quotes each element of an IFS-delimited list for shell reuse\n+__git_quote()\n+{\n+\tlocal i\n+\tlocal delim\n+\tfor i in $1; do\n+\t\tlocal quoted=${i//\\'/\\'\\\\\\'\\'}\n+\t\tprintf \"${delim:+$IFS}'%s'\" \"$quoted\"\n+\t\tdelim=t\n+\tdone\n+}\n+\n # Generates completion reply with compgen, appending a space to possible\n # completion words, if necessary.\n # It accepts 1 to 4 arguments:\n@@ -261,7 +273,7 @@ __gitcomp_nl ()\n __gitcomp_nl ()\n {\n \tlocal IFS=$'\\n'\n-\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n+\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$(__git_quote \"$1\")\" -- \"${3-$cur}\"))\n }\n \n __git_heads ()\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex cbd0fb6..da67705 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -278,7 +278,7 @@ test_expect_success 'complete tree filename with spaces' '\n \tEOF\n '\n \n-test_expect_failure 'complete tree filename with metacharacters' '\n+test_expect_success 'complete tree filename with metacharacters' '\n \techo content >\"name with \\${meta}\" &&\n \tgit add . &&\n \tgit commit -m meta &&\n-- \n1.7.12.10.g31da6dd\n"},{"id":"199984","messageId":"20120926215700.GB18628@sigill.intra.peff.net","threadId":"31614","inReplyTo":"20120926215119.GC18653@sigill.intra.peff.net","subject":"[PATCH 4/3] completion: quote completions we find","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-26T21:57:00Z","receivedAt":"2012-09-26T21:57:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 26, 2012 at 05:51:19PM -0400, Jeff King wrote:\n\n> This is not a complete fix, as the completion result does\n> will still contain metacharacters, so it would need extra\n> quoting to actually be used on a command line. But it's\n> still a step in the right direction, because:\n> [...]\n> 2. We don't know yet what the final fix will look like,\n> but this is probably the first step towards it. It\n> handles quoting on the input side of compgen; the next\n> step will likely be handling the quoting on the output\n> side of compgen to yield a usable string for the\n> command line.\n\nHere is on attempt at that. It seems to work for me, but I know it is\nnot what bash does internally with:\n\n $ ls\n name with spaces\n $ cat name\n\nBecause in bash's internal case, it knows that \"name with spaces\" is the\nreal entry (because if you have many entries and double-tab, it shows it\nwithout the quotes), and only later adds the quoting.\n\nSo while this works, I'm not sure if it is optimal or if it has any\nweird side effects.\n\n-- >8 --\nSubject: [PATCH] completion: quote completions we find\n\nIf you try to complete a filename with spaces, you might end\nup with this:\n\n $ git show HEAD:name\n $ git show HEAD:name with spaces\n\nwhich is technically correct, but does not help you, since\nthe shell will split the words as soon as you hit enter.\nInstead, let's quote completions coming out of __gitcomp_nl\nto yield:\n\n $ git show HEAD:'name with spaces'\n\nSigned-off-by: Jeff King \n---\n contrib/completion/git-completion.bash | 23 +++++++++++++++++++++++\n t/t9902-completion.sh | 6 +++---\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex b0416ea..1fc43f7 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -274,6 +274,29 @@ __gitcomp_nl ()\n {\n \tlocal IFS=$'\\n'\n \tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$(__git_quote \"$1\")\" -- \"${3-$cur}\"))\n+\n+\tlocal i\n+\tfor (( i=0; i < ${#COMPREPLY[@]}; i++ )); do\n+\t\t# Hack to handle the extra space we add for some\n+\t\t# entries, since we use \"-o nospace\".\n+\t\tlocal stripped\n+\t\tcase \"${COMPREPLY[$i]}\" in\n+\t\t*\\ )\n+\t\t\tstripped=' '\n+\t\t\tCOMPREPLY[$i]=${COMPREPLY[$i]% }\n+\t\t\t;;\n+\t\tesac\n+\n+\t\t# Now we can check if we need actual quoting.\n+\t\tcase \"${COMPREPLY[$i]}\" in\n+\t\t*[\\ \\$\\'\\\"\\(\\)]*)\n+\t\t\tCOMPREPLY[$i]=\"'${COMPREPLY[$i]//\\'/\\'\\\\\\'\\'}'\"\n+\t\t\t;;\n+\t\tesac\n+\n+\t\t# And then restore any stripped space.\n+\t\tCOMPREPLY[$i]=\"${COMPREPLY[$i]}$stripped\"\n+\tdone\n }\n \n __git_heads ()\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex da67705..d2c5104 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -274,7 +274,7 @@ test_expect_success 'complete tree filename with spaces' '\n \tgit add . &&\n \tgit commit -m spaces &&\n \ttest_completion_long \"git show HEAD:nam\" <<-\\EOF\n-\tname with spaces_\n+\t'\\''name with spaces'\\''_\n \tEOF\n '\n \n@@ -283,8 +283,8 @@ test_expect_success 'complete tree filename with metacharacters' '\n \tgit add . &&\n \tgit commit -m meta &&\n \ttest_completion_long \"git show HEAD:nam\" <<-\\EOF\n-\tname with ${meta}_\n-\tname with spaces_\n+\t'\\''name with ${meta}'\\''_\n+\t'\\''name with spaces'\\''_\n \tEOF\n '\n \n-- \n1.7.12.10.g31da6dd\n"},{"id":"199987","messageId":"20120927003751.GI10144@goldbirke","threadId":"31614","inReplyTo":"20120926215119.GC18653@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] completion: improve shell expansion of items","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-27T00:37:51Z","receivedAt":"2012-09-27T00:37:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nOn Wed, Sep 26, 2012 at 05:51:19PM -0400, Jeff King wrote:\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index be800e0..b0416ea 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -225,6 +225,18 @@ fi\n> fi\n> fi\n> \n> +# Quotes each element of an IFS-delimited list for shell reuse\n> +__git_quote()\n> +{\n> +\tlocal i\n> +\tlocal delim\n> +\tfor i in $1; do\n> +\t\tlocal quoted=${i//\\'/\\'\\\\\\'\\'}\n> +\t\tprintf \"${delim:+$IFS}'%s'\" \"$quoted\"\n> +\t\tdelim=t\n> +\tdone\n> +}\n> +\n> # Generates completion reply with compgen, appending a space to possible\n> # completion words, if necessary.\n> # It accepts 1 to 4 arguments:\n> @@ -261,7 +273,7 @@ __gitcomp_nl ()\n> __gitcomp_nl ()\n> {\n> \tlocal IFS=$'\\n'\n> -\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n> +\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$(__git_quote \"$1\")\" -- \"${3-$cur}\"))\n\nIterating through all possible completion words undermines the main\nreason for __gitcomp_nl()'s existence: to handle potentially large\nnumber of possible completion words faster than the old __gitcomp().\nIf we really have to iterate in a subshell, then it would perhaps be\nbetter to drop __gitcomp_nl(), go back to using __gitcomp(), and\nmodify that instead.\n\nAfter all, anyone could drop a file called git-cmd-${meta} on his\n$PATH, and then get cmd- offered, because completion of git commands\nstill goes through __gitcomp().\n"},{"id":"199993","messageId":"20120927062855.GA22890@sigill.intra.peff.net","threadId":"31614","inReplyTo":"20120927003751.GI10144@goldbirke","subject":"Re: [PATCH 3/3] completion: improve shell expansion of items","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-27T06:28:55Z","receivedAt":"2012-09-27T06:28:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 27, 2012 at 02:37:51AM +0200, SZEDER Gábor wrote:\n\n> > +# Quotes each element of an IFS-delimited list for shell reuse\n> > +__git_quote()\n> > +{\n> > +\tlocal i\n> > +\tlocal delim\n> > +\tfor i in $1; do\n> > +\t\tlocal quoted=${i//\\'/\\'\\\\\\'\\'}\n> > +\t\tprintf \"${delim:+$IFS}'%s'\" \"$quoted\"\n> > +\t\tdelim=t\n> > +\tdone\n> > +}\n> [...]\n>\n> Iterating through all possible completion words undermines the main\n> reason for __gitcomp_nl()'s existence: to handle potentially large\n> number of possible completion words faster than the old __gitcomp().\n> If we really have to iterate in a subshell, then it would perhaps be\n> better to drop __gitcomp_nl(), go back to using __gitcomp(), and\n> modify that instead.\n\nThanks for reminding me to time. I noticed your a31e626 while digging in\nthe history, but forgot that I wanted to do a timing test. Sadly, the\nresults are very discouraging. Doing a similar test to your 10,000-refs,\nI get:\n\n $ refs=$(seq 1 10000)\n\n $ . git-completion.bash.old\n $ time __gitcomp_nl \"$refs\"\n real 0m0.065s\n user 0m0.064s\n sys 0m0.004s\n\n $ . git-completion.bash.new\n $ time __gitcomp_nl \"$refs\"\n real 0m1.799s\n user 0m1.828s\n sys 0m0.036s\n\nSo, a 2700% slowdown. Yuck.\n\nI also tried running it through sed instead of iterating in bash. I know\nthat some people will not like the fork, but curiously enough, it was\nnot that much faster. Which makes me wonder if part of the slowdown is\nactually bash unquoting the result in compgen.\n\n> After all, anyone could drop a file called git-cmd-${meta} on his\n> $PATH, and then get cmd- offered, because completion of git commands\n> still goes through __gitcomp().\n\nYeah. I wasn't sure if __gitcomp() actually got used on any arbitrary\ndata, but that's a good example.\n\nI'm not sure where to go next. I guess we could try pre-quoting via git\nwhen generating the list of refs (or files, or whatever) and hope that\nis faster.\n\nWith this patch as it is, I'm not sure the slowdown is worth it. Yes,\nit's more correct in the face of metacharacters, but those are the\nuncommon case by a large margin. I'd hate to have a performance\nregression in the common case just for that.\n\n-Peff\n"},{"id":"199997","messageId":"20120927064338.GA4789@sigill.intra.peff.net","threadId":"31614","inReplyTo":"20120927062855.GA22890@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] completion: improve shell expansion of items","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-27T06:43:38Z","receivedAt":"2012-09-27T06:43:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 27, 2012 at 02:28:55AM -0400, Jeff King wrote:\n\n> Thanks for reminding me to time. I noticed your a31e626 while digging in\n> the history, but forgot that I wanted to do a timing test. Sadly, the\n> results are very discouraging. Doing a similar test to your 10,000-refs,\n> I get:\n> \n> $ refs=$(seq 1 10000)\n> \n> $ . git-completion.bash.old\n> $ time __gitcomp_nl \"$refs\"\n> real 0m0.065s\n> user 0m0.064s\n> sys 0m0.004s\n> \n> $ . git-completion.bash.new\n> $ time __gitcomp_nl \"$refs\"\n> real 0m1.799s\n> user 0m1.828s\n> sys 0m0.036s\n> \n> So, a 2700% slowdown. Yuck.\n> \n> I also tried running it through sed instead of iterating in bash. I know\n> that some people will not like the fork, but curiously enough, it was\n> not that much faster. Which makes me wonder if part of the slowdown is\n> actually bash unquoting the result in compgen.\n\nAh. The problem is that most of the load comes from my patch 4/3, which\ndoes a separate iteration. Here are the numbers after just patch 3:\n\n $ time __gitcomp_nl \"$refs\"\n real 0m0.344s\n user 0m0.392s\n sys 0m0.040s\n\nSlower, but not nearly as painful. Here are the numbers using sed[1]\ninstead:\n\n $ time __gitcomp_nl \"$refs\"\n real 0m0.100s\n user 0m0.084s\n sys 0m0.016s\n\nSo a little slower, but probably acceptable. We could maybe do the same\ntrick on the output side (although it is a little trickier there,\nbecause we need it in a bash array). Of course, this is Linux; the fork\nfor sed is way more expensive on some systems.\n\nStill, I'd be curious to see it with the callers (e.g., __git_refs)\ndoing their own quoting. I'd worry that it would become a maintenance\nheadache, but perhaps we don't have that many lists we feed (there are a\nlot of calls to gitcomp_nl, but they are mostly feeding __git_refs).\n\n-Peff\n\n[1] For reference, that patch is:\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 1fc43f7..5ff3742 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -225,16 +225,15 @@ __git_quote()\n fi\n fi\n \n-# Quotes each element of an IFS-delimited list for shell reuse\n+# Quotes each element of a newline-delimited list for shell reuse\n __git_quote()\n {\n-\tlocal i\n-\tlocal delim\n-\tfor i in $1; do\n-\t\tlocal quoted=${i//\\'/\\'\\\\\\'\\'}\n-\t\tprintf \"${delim:+$IFS}'%s'\" \"$quoted\"\n-\t\tdelim=t\n-\tdone\n+\techo \"$1\" |\n+\tsed \"\n+\t s/'/'\\\\\\\\''/g\n+\t s/^/'/\n+\t s/$/'/\n+\t\"\n }\n \n # Generates completion reply with compgen, appending a space to possible\n"},{"id":"200043","messageId":"20120927194856.GA10673@sigill.intra.peff.net","threadId":"31614","inReplyTo":"20120927064338.GA4789@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] completion: improve shell expansion of items","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-27T19:48:56Z","receivedAt":"2012-09-27T19:48:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 27, 2012 at 02:43:38AM -0400, Jeff King wrote:\n\n> Ah. The problem is that most of the load comes from my patch 4/3, which\n> does a separate iteration. Here are the numbers after just patch 3:\n> \n> $ time __gitcomp_nl \"$refs\"\n> real 0m0.344s\n> user 0m0.392s\n> sys 0m0.040s\n> \n> Slower, but not nearly as painful. Here are the numbers using sed[1]\n> instead:\n> \n> $ time __gitcomp_nl \"$refs\"\n> real 0m0.100s\n> user 0m0.084s\n> sys 0m0.016s\n> \n> So a little slower, but probably acceptable. We could maybe do the same\n> trick on the output side (although it is a little trickier there,\n> because we need it in a bash array). Of course, this is Linux; the fork\n> for sed is way more expensive on some systems.\n\nSo something like the patch below does the quoting correctly (try\ncommitting a file like \"name with spaces\" and doing \"git show\nHEAD:\"), and isn't too much slower:\n\n real 0m0.114s\n user 0m0.108s\n sys 0m0.004s\n\nThat's almost double the time without handling quoting, but keep in mind\nthis is on 10,000 entries (and the real sluggishness we are trying to\navoid is an order of magnitude). So it might be acceptable.\n\nThis is just a proof-of-concept patch. We'd probably want to replace the\nperl below with a more complicated sed invocation for portability (the\ntrickiness is that the output is shown to the user, so we very much\ndon't want to quote anything that does not have to be).\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex be800e0..20c09ef 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -225,6 +225,17 @@ fi\n fi\n fi\n \n+# Quotes each element of a newline-delimited list for shell reuse\n+__git_quote()\n+{\n+\techo \"$1\" |\n+\tsed \"\n+\t s/'/'\\\\\\\\''/g\n+\t s/^/'/\n+\t s/$/'/\n+\t\"\n+}\n+\n # Generates completion reply with compgen, appending a space to possible\n # completion words, if necessary.\n # It accepts 1 to 4 arguments:\n@@ -261,7 +272,10 @@ __gitcomp_nl ()\n __gitcomp_nl ()\n {\n \tlocal IFS=$'\\n'\n-\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n+\tCOMPREPLY=($(\n+\t compgen -P \"${2-}\" -S \"${4- }\" -W \"$(__git_quote \"$1\")\" -- \"${3-$cur}\" |\n+\t perl -lne '/(.*?)( ?)$/; print quotemeta($1), $2'\n+\t))\n }\n \n __git_heads ()\n\n-Peff\n"},{"id":"200052","messageId":"20120927214046.GJ10144@goldbirke","threadId":"31614","inReplyTo":"20120926215700.GB18628@sigill.intra.peff.net","subject":"Re: [PATCH 4/3] completion: quote completions we find","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-27T21:40:46Z","receivedAt":"2012-09-27T21:40:46Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Sep 26, 2012 at 05:57:00PM -0400, Jeff King wrote:\n> +\t\tCOMPREPLY[$i]=\"${COMPREPLY[$i]}$stripped\"\n\nThis reminded me to a mini-series collecting dust in my git repo,\nwhich converts a few similar var=$var$something constructs to use the\n+= append operator instead.\n\nNow, Bash supports this += append operator since v3.1\n(bash-3.1-alpha1, to be exact), which is around since July 2005, if I\ncan trust the mtime at ftp://ftp.cwru.edu/pub/bash/. MSysgit ships\nv3.1 so it already supports this, too. So, what is the oldest Bash\nversion we care about for completion?\n"},{"id":"200054","messageId":"7vy5jvnln5.fsf@alter.siamese.dyndns.org","threadId":"31614","inReplyTo":"20120927214046.GJ10144@goldbirke","subject":"Re: [PATCH 4/3] completion: quote completions we find","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-27T22:31:10Z","receivedAt":"2012-09-27T22:31:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Wed, Sep 26, 2012 at 05:57:00PM -0400, Jeff King wrote:\n>> +\t\tCOMPREPLY[$i]=\"${COMPREPLY[$i]}$stripped\"\n>\n> This reminded me to a mini-series collecting dust in my git repo,\n> which converts a few similar var=$var$something constructs to use the\n> += append operator instead.\n\nIs the benefit of rewriting it to var+=$something large enough to\nworry about the below?\n\n> Now, Bash supports this += append operator since v3.1\n> (bash-3.1-alpha1, to be exact), which is around since July 2005, if I\n> can trust the mtime at ftp://ftp.cwru.edu/pub/bash/. MSysgit ships\n> v3.1 so it already supports this, too. So, what is the oldest Bash\n> version we care about for completion?\n"},{"id":"200058","messageId":"20120927225809.GK10144@goldbirke","threadId":"31614","inReplyTo":"7vy5jvnln5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/3] completion: quote completions we find","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-27T22:58:09Z","receivedAt":"2012-09-27T22:58:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Sep 27, 2012 at 03:31:10PM -0700, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n> > On Wed, Sep 26, 2012 at 05:57:00PM -0400, Jeff King wrote:\n> >> +\t\tCOMPREPLY[$i]=\"${COMPREPLY[$i]}$stripped\"\n> >\n> > This reminded me to a mini-series collecting dust in my git repo,\n> > which converts a few similar var=$var$something constructs to use the\n> > += append operator instead.\n> \n> Is the benefit of rewriting it to var+=$something large enough to\n> worry about the below?\n\nThat way we can get rid of a subshell in __gitcomp(), which means one\nless fork() during every command or option completion for Windows\nfolks. We can also get rid of two subshells during loading the\ncompletion script.\n\nAnd I would spare myself from a couple of merge conflicts, too ;)\n\n\n> > Now, Bash supports this += append operator since v3.1\n> > (bash-3.1-alpha1, to be exact), which is around since July 2005, if I\n> > can trust the mtime at ftp://ftp.cwru.edu/pub/bash/. MSysgit ships\n> > v3.1 so it already supports this, too. So, what is the oldest Bash\n> > version we care about for completion?\n"},{"id":"200077","messageId":"20120928100530.GL10144@goldbirke","threadId":"31614","inReplyTo":"20120927064338.GA4789@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] completion: improve shell expansion of items","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-28T10:05:30Z","receivedAt":"2012-09-28T10:05:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Sep 27, 2012 at 02:43:38AM -0400, Jeff King wrote:\n> Here are the numbers using sed[1]\n> instead:\n \n> -# Quotes each element of an IFS-delimited list for shell reuse\n> +# Quotes each element of a newline-delimited list for shell reuse\n> __git_quote()\n> {\n> -\tlocal i\n> -\tlocal delim\n> -\tfor i in $1; do\n> -\t\tlocal quoted=${i//\\'/\\'\\\\\\'\\'}\n> -\t\tprintf \"${delim:+$IFS}'%s'\" \"$quoted\"\n> -\t\tdelim=t\n> -\tdone\n> +\techo \"$1\" |\n> +\tsed \"\n> +\t s/'/'\\\\\\\\''/g\n> +\t s/^/'/\n> +\t s/$/'/\n> +\t\"\n> }\n> \n> # Generates completion reply with compgen, appending a space to possible\n\nYour usage of sed got me thinking and come up with this.\n\nThe first two fix benign bugs in completion tests, and the third adds\ntests for __gitcomp_nl(). These are good to go.\n\nThe fourth adds __gitcomp() and __gitcomp_nl() tests for this\nexpansion breakage. The expected results might need some adjustments,\nbecause they contain special characters unquoted, but I'm tempted to\nsay that a branch called $foo is so rare in practice, that we\nshouldn't bother.\n\nThe final one is a proof of concept for inspiration. It gets rid of\ncompgen and its crazy additional expansion replacing it with a small\nsed script. It needs further work to handle words with whitespaces\nand special characters.\n\n\nSZEDER Gábor (5):\n completion: fix non-critical bugs in __gitcomp() tests\n completion: fix args of run_completion() test helper\n completion: add tests for the __gitcomp_nl() completion helper\n function\n completion: test __gitcomp() and __gitcomp_nl() with expandable words\n completion: avoid compgen to fix expansion issues in __gitcomp_nl()\n\n contrib/completion/git-completion.bash | 6 ++-\n t/t9902-completion.sh | 91 +++++++++++++++++++++++++++++++---\n 2 files changed, 90 insertions(+), 7 deletions(-)\n"},{"id":"200078","messageId":"1348826975-2225-1-git-send-email-szeder@ira.uka.de","threadId":"31614","inReplyTo":"20120928100530.GL10144@goldbirke","subject":"[PATCH 1/5] completion: fix non-critical bugs in __gitcomp() tests","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-28T10:09:31Z","receivedAt":"2012-09-28T10:09:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When invoking __gitcomp() the $cur variable should hold the word to be\ncompleted, but two tests (checking the handling of prefix and suffix)\nset it to a bogus value.\n\nLuckily the bogus values haven't influenced the tests' correctness,\nbecause these two tests pass the word-to-be-matched as argument to\n__gitcomp(), which overrides $cur anyway.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t9902-completion.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 92d7eb47..e7657537 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -121,7 +121,7 @@ test_expect_success '__gitcomp - prefix' '\n \tEOF\n \t(\n \t\tlocal -a COMPREPLY &&\n-\t\tcur=\"branch.me\" &&\n+\t\tcur=\"branch.maint.me\" &&\n \t\t__gitcomp \"remote merge mergeoptions rebase\n \t\t\t\" \"branch.maint.\" \"me\" &&\n \t\tIFS=\"$newline\" &&\n@@ -137,7 +137,7 @@ test_expect_success '__gitcomp - suffix' '\n \tEOF\n \t(\n \t\tlocal -a COMPREPLY &&\n-\t\tcur=\"branch.me\" &&\n+\t\tcur=\"branch.ma\" &&\n \t\t__gitcomp \"master maint next pu\n \t\t\t\" \"branch.\" \"ma\" \".\" &&\n \t\tIFS=\"$newline\" &&\n-- \n1.7.12.1.438.g7dfa67b\n"},{"id":"200079","messageId":"1348826975-2225-2-git-send-email-szeder@ira.uka.de","threadId":"31614","inReplyTo":"1348826975-2225-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH 2/5] completion: fix args of run_completion() test helper","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-28T10:09:32Z","receivedAt":"2012-09-28T10:09:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"To simulate the the user hit 'git , one of the completion tests\nsets up the rather strange command line\n\n git \"\"\n\ni.e. the second word on the command line consists of two double\nquotes. However, this is not what happens for real, because after\n'git ' the second word on the command line is just an empty\nstring. Luckily, the test works nevertheless.\n\nFix this by passing the command line to run_completion() as separate\nwords.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t9902-completion.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex e7657537..f5e68834 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -49,7 +49,7 @@ run_completion ()\n {\n \tlocal -a COMPREPLY _words\n \tlocal _cword\n-\t_words=( $1 )\n+\t_words=( \"$@\" )\n \t(( _cword = ${#_words[@]} - 1 ))\n \t__git_wrap__git_main && print_comp\n }\n@@ -57,7 +57,7 @@ run_completion ()\n test_completion ()\n {\n \ttest $# -gt 1 && echo \"$2\" > expected\n-\trun_completion \"$@\" &&\n+\trun_completion $1 &&\n \ttest_cmp expected out\n }\n \n@@ -147,7 +147,7 @@ test_expect_success '__gitcomp - suffix' '\n '\n \n test_expect_success 'basic' '\n-\trun_completion \"git \\\"\\\"\" &&\n+\trun_completion git \"\" &&\n \t# built-in\n \tgrep -q \"^add \\$\" out &&\n \t# script\n@@ -155,7 +155,7 @@ test_expect_success 'basic' '\n \t# plumbing\n \t! grep -q \"^ls-files \\$\" out &&\n \n-\trun_completion \"git f\" &&\n+\trun_completion git f &&\n \t! grep -q -v \"^f\" out\n '\n \n-- \n1.7.12.1.438.g7dfa67b\n"},{"id":"200082","messageId":"1348826975-2225-3-git-send-email-szeder@ira.uka.de","threadId":"31614","inReplyTo":"1348826975-2225-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH 3/5] completion: add tests for the __gitcomp_nl() completion helper function","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-28T10:09:33Z","receivedAt":"2012-09-28T10:09:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Test __gitcomp_nl()'s basic functionality, i.e. that trailing space,\nprefix, and suffix are added correctly.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t9902-completion.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 47 insertions(+)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex f5e68834..01f33220 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -146,6 +146,53 @@ test_expect_success '__gitcomp - suffix' '\n \ttest_cmp expected out\n '\n \n+test_expect_success '__gitcomp_nl - trailing space' '\n+\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\tmaint Z\n+\tmaster Z\n+\tEOF\n+\t(\n+\t\tlocal -a COMPREPLY &&\n+\t\tcur=\"m\" &&\n+\t\t__gitcomp_nl \"maint${newline}master${newline}pu\" &&\n+\t\tIFS=\"$newline\" &&\n+\t\techo \"${COMPREPLY[*]}\" > out\n+\t) &&\n+\ttest_cmp expected out\n+'\n+\n+test_expect_success '__gitcomp_nl - prefix' '\n+\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\t--fixup=maint Z\n+\t--fixup=master Z\n+\tEOF\n+\t(\n+\t\tlocal -a COMPREPLY &&\n+\t\tcur=\"--fixup=m\" &&\n+\t\t__gitcomp_nl \"maint${newline}master${newline}next${newline}pu\"\\\n+\t\t\t\t\"--fixup=\" \"m\" &&\n+\t\tIFS=\"$newline\" &&\n+\t\techo \"${COMPREPLY[*]}\" > out\n+\t) &&\n+\ttest_cmp expected out\n+'\n+\n+test_expect_success '__gitcomp_nl - suffix' '\n+\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\tbranch.master.Z\n+\tbranch.maint.Z\n+\tEOF\n+\t(\n+\t\tlocal -a COMPREPLY &&\n+\t\tcur=\"branch.ma\" &&\n+\t\t__gitcomp_nl \"master${newline}maint${newline}next${newline}pu\"\\\n+\t\t\t\t\"branch.\" \"ma\" \".\" &&\n+\t\tIFS=\"$newline\" &&\n+\t\techo \"${COMPREPLY[*]}\" > out\n+\t) &&\n+\ttest_cmp expected out\n+'\n+\n test_expect_success 'basic' '\n \trun_completion git \"\" &&\n \t# built-in\n-- \n1.7.12.1.438.g7dfa67b\n"},{"id":"200080","messageId":"1348826975-2225-4-git-send-email-szeder@ira.uka.de","threadId":"31614","inReplyTo":"1348826975-2225-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH 4/5] completion: test __gitcomp() and __gitcomp_nl() with expandable words","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-28T10:09:34Z","receivedAt":"2012-09-28T10:09:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"compgen performs expansion on all words in the list given to its -W\noption. This breaks completion in various ways if one of those words\ncan be expanded. The funniest breakage is probably this, as the\ncommand_not_found_handle kicks in:\n\n$ git branch '$(foo)'\n$ git branch No command 'foo' found, did you mean:\nCommand 'fio' from package 'fio' (universe)\nCommand 'goo' from package 'goo' (universe)\nCommand 'fop' from package 'fop' (main)\nCommand 'fox' from package 'objcryst-fox' (universe)\nCommand 'xoo' from package 'xoo' (universe)\nCommand 'zoo' from package 'zoo' (universe)\nfoo: command not found\n\nDocument this breakage with tests.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t9902-completion.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 01f33220..4af2a149 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -146,6 +146,22 @@ test_expect_success '__gitcomp - suffix' '\n \ttest_cmp expected out\n '\n \n+test_expect_failure '__gitcomp - expandable words' '\n+\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\t$foo Z\n+\t$(bar) Z\n+\t${baz} Z\n+\tEOF\n+\t(\n+\t\tlocal -a COMPREPLY &&\n+\t\tcur=\"\" &&\n+\t\t__gitcomp \"\\$foo \\$(bar) \\${baz}\" &&\n+\t\tIFS=\"$newline\" &&\n+\t\techo \"${COMPREPLY[*]}\" > out\n+\t) &&\n+\ttest_cmp expected out\n+'\n+\n test_expect_success '__gitcomp_nl - trailing space' '\n \tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n \tmaint Z\n@@ -193,6 +209,22 @@ test_expect_success '__gitcomp_nl - suffix' '\n \ttest_cmp expected out\n '\n \n+test_expect_failure '__gitcomp_nl - expandable words' '\n+\tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n+\t$foo Z\n+\t$(bar) Z\n+\t${baz} Z\n+\tEOF\n+\t(\n+\t\tlocal -a COMPREPLY &&\n+\t\tcur=\"\" &&\n+\t\t__gitcomp_nl \"\\$foo$newline\\$(bar)$newline\\${baz}\" &&\n+\t\tIFS=\"$newline\" &&\n+\t\techo \"${COMPREPLY[*]}\" > out\n+\t) &&\n+\ttest_cmp expected out\n+'\n+\n test_expect_success 'basic' '\n \trun_completion git \"\" &&\n \t# built-in\n-- \n1.7.12.1.438.g7dfa67b\n"},{"id":"200081","messageId":"1348826975-2225-5-git-send-email-szeder@ira.uka.de","threadId":"31614","inReplyTo":"1348826975-2225-1-git-send-email-szeder@ira.uka.de","subject":"[POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-28T10:09:35Z","receivedAt":"2012-09-28T10:09:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"compgen performs expansion on all words in the list given to its -W\noption. This breaks completion in various ways if one of those words\ncan be expanded, as demonstrated by failing tests added in a previous\ncommit.\n\nIn __gitcomp_nl() we use only a small subset of compgen's\nfunctionality: to filter matching possible completion words, and to\nadd a prefix and/or suffix to each of them. Since the list of words\nis newline-separated, we can do all these just as well with a little\nsed script. This way we can get rid of compgen and its additional\nexpansion of all words and don't need to perform excessive quoting to\ncircumvent it.\n\nSigned-off-by: SZEDER Gábor \n---\n\nWe can also get rid of compgen in __gitcomp(); I already have working code\nfor that, but it still needs a bit of cleanup and commit messages.\n\n contrib/completion/git-completion.bash | 6 +++++-\n t/t9902-completion.sh | 2 +-\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex be800e09..2df865fd 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -261,7 +261,11 @@ __gitcomp ()\n __gitcomp_nl ()\n {\n \tlocal IFS=$'\\n'\n-\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n+\tCOMPREPLY=($(echo \"$1\" |sed -n \"/^${3-$cur}/ {\n+\t\ts/^/${2-}/\n+\t\ts/$/${4- }/\n+\t\tp\n+\t}\"))\n }\n \n __git_heads ()\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 4af2a149..0ee91f64 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -209,7 +209,7 @@ test_expect_success '__gitcomp_nl - suffix' '\n \ttest_cmp expected out\n '\n \n-test_expect_failure '__gitcomp_nl - expandable words' '\n+test_expect_success '__gitcomp_nl - expandable words' '\n \tsed -e \"s/Z$//\" >expected <<-\\EOF &&\n \t$foo Z\n \t$(bar) Z\n-- \n1.7.12.1.438.g7dfa67b\n"},{"id":"200111","messageId":"20120928150852.GA13081@goldbirke","threadId":"31614","inReplyTo":"1348826975-2225-5-git-send-email-szeder@ira.uka.de","subject":"Re: [POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-28T15:08:52Z","receivedAt":"2012-09-28T15:08:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Sep 28, 2012 at 12:09:35PM +0200, SZEDER Gábor wrote:\n> __gitcomp_nl ()\n> {\n> \tlocal IFS=$'\\n'\n> -\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n> +\tCOMPREPLY=($(echo \"$1\" |sed -n \"/^${3-$cur}/ {\n\n$cur can be a path, so it can contain /, which then breaks this sed\nexpression. Here's a fixup:\n\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 2df865fd..d30f376f 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -260,8 +260,8 @@ __gitcomp ()\n # appended.\n __gitcomp_nl ()\n {\n-\tlocal IFS=$'\\n'\n-\tCOMPREPLY=($(echo \"$1\" |sed -n \"/^${3-$cur}/ {\n+\tlocal IFS=$'\\n' cur_=${3-$cur}\n+\tCOMPREPLY=($(echo \"$1\" |sed -n \"/^${cur_//\\//\\\\/}/ {\n \t\ts/^/${2-}/\n \t\ts/$/${4- }/\n \t\tp\n-- \n1.7.12.1.490.g14283db\n"},{"id":"200112","messageId":"20120928154615.GA24933@sigill.intra.peff.net","threadId":"31614","inReplyTo":"20120928150852.GA13081@goldbirke","subject":"Re: [POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-28T15:46:16Z","receivedAt":"2012-09-28T15:46:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 28, 2012 at 05:08:52PM +0200, SZEDER Gábor wrote:\n\n> On Fri, Sep 28, 2012 at 12:09:35PM +0200, SZEDER Gábor wrote:\n> > __gitcomp_nl ()\n> > {\n> > \tlocal IFS=$'\\n'\n> > -\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n> > +\tCOMPREPLY=($(echo \"$1\" |sed -n \"/^${3-$cur}/ {\n> \n> $cur can be a path, so it can contain /, which then breaks this sed\n> expression. Here's a fixup:\n\nWorse than that, can't $cur contain arbitrary regex characters that will\nbe interpreted by sed?\n\n-Peff\n"},{"id":"200119","messageId":"7vehlmm3ca.fsf@alter.siamese.dyndns.org","threadId":"31614","inReplyTo":"1348826975-2225-2-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 2/5] completion: fix args of run_completion() test helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-28T18:04:05Z","receivedAt":"2012-09-28T18:04:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> To simulate the the user hit 'git , one of the completion tests\n> sets up the rather strange command line\n>\n> git \"\"\n>\n> i.e. the second word on the command line consists of two double\n> quotes. However, this is not what happens for real, because after\n> 'git ' the second word on the command line is just an empty\n> string. Luckily, the test works nevertheless.\n>\n> Fix this by passing the command line to run_completion() as separate\n> words.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> t/t9902-completion.sh | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index e7657537..f5e68834 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -49,7 +49,7 @@ run_completion ()\n> {\n> \tlocal -a COMPREPLY _words\n> \tlocal _cword\n> -\t_words=( $1 )\n> +\t_words=( \"$@\" )\n> \t(( _cword = ${#_words[@]} - 1 ))\n> \t__git_wrap__git_main && print_comp\n> }\n> @@ -57,7 +57,7 @@ run_completion ()\n> test_completion ()\n> {\n> \ttest $# -gt 1 && echo \"$2\" > expected\n> -\trun_completion \"$@\" &&\n> +\trun_completion $1 &&\n> \ttest_cmp expected out\n> }\n\nI can understand the other three hunks, but this one is fishy.\nShouldn't \"$1\" be inside a pair of dq? I.e.\n\n\t+\trun_completion \"$1\" &&\n\n> \n> @@ -147,7 +147,7 @@ test_expect_success '__gitcomp - suffix' '\n> '\n> \n> test_expect_success 'basic' '\n> -\trun_completion \"git \\\"\\\"\" &&\n> +\trun_completion git \"\" &&\n> \t# built-in\n> \tgrep -q \"^add \\$\" out &&\n> \t# script\n> @@ -155,7 +155,7 @@ test_expect_success 'basic' '\n> \t# plumbing\n> \t! grep -q \"^ls-files \\$\" out &&\n> \n> -\trun_completion \"git f\" &&\n> +\trun_completion git f &&\n> \t! grep -q -v \"^f\" out\n> '\n"},{"id":"200121","messageId":"20120928183840.GB10719@goldbirke","threadId":"31614","inReplyTo":"7vehlmm3ca.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/5] completion: fix args of run_completion() test helper","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-28T18:38:40Z","receivedAt":"2012-09-28T18:38:40Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Sep 28, 2012 at 11:04:05AM -0700, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n> > To simulate the the user hit 'git , one of the completion tests\n\ns/the the/that the/\n\n> > sets up the rather strange command line\n> >\n> > git \"\"\n> >\n> > i.e. the second word on the command line consists of two double\n> > quotes. However, this is not what happens for real, because after\n> > 'git ' the second word on the command line is just an empty\n> > string. Luckily, the test works nevertheless.\n> >\n> > Fix this by passing the command line to run_completion() as separate\n> > words.\n> >\n> > Signed-off-by: SZEDER Gábor \n> > ---\n> > t/t9902-completion.sh | 8 ++++----\n> > 1 file changed, 4 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> > index e7657537..f5e68834 100755\n> > --- a/t/t9902-completion.sh\n> > +++ b/t/t9902-completion.sh\n> > @@ -49,7 +49,7 @@ run_completion ()\n> > {\n> > \tlocal -a COMPREPLY _words\n> > \tlocal _cword\n> > -\t_words=( $1 )\n> > +\t_words=( \"$@\" )\n> > \t(( _cword = ${#_words[@]} - 1 ))\n> > \t__git_wrap__git_main && print_comp\n> > }\n> > @@ -57,7 +57,7 @@ run_completion ()\n> > test_completion ()\n> > {\n> > \ttest $# -gt 1 && echo \"$2\" > expected\n> > -\trun_completion \"$@\" &&\n> > +\trun_completion $1 &&\n> > \ttest_cmp expected out\n> > }\n> \n> I can understand the other three hunks, but this one is fishy.\n> Shouldn't \"$1\" be inside a pair of dq? I.e.\n> \n> \t+\trun_completion \"$1\" &&\n\nNo. $1 holds all words on the command line. If it was between a pair\nof dq, then the whole command line would be passed to the completion\nscript as a single word.\n\n\n> > \n> > @@ -147,7 +147,7 @@ test_expect_success '__gitcomp - suffix' '\n> > '\n> > \n> > test_expect_success 'basic' '\n> > -\trun_completion \"git \\\"\\\"\" &&\n> > +\trun_completion git \"\" &&\n> > \t# built-in\n> > \tgrep -q \"^add \\$\" out &&\n> > \t# script\n> > @@ -155,7 +155,7 @@ test_expect_success 'basic' '\n> > \t# plumbing\n> > \t! grep -q \"^ls-files \\$\" out &&\n> > \n> > -\trun_completion \"git f\" &&\n> > +\trun_completion git f &&\n> > \t! grep -q -v \"^f\" out\n> > '\n"},{"id":"200127","messageId":"7vzk4aj6ik.fsf@alter.siamese.dyndns.org","threadId":"31614","inReplyTo":"20120928183840.GB10719@goldbirke","subject":"Re: [PATCH 2/5] completion: fix args of run_completion() test helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-28T19:23:47Z","receivedAt":"2012-09-28T19:23:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Fri, Sep 28, 2012 at 11:04:05AM -0700, Junio C Hamano wrote:\n>> SZEDER Gábor writes:\n>> \n>> > To simulate the the user hit 'git , one of the completion tests\n>\n> s/the the/that the/\n>\n>> > sets up the rather strange command line\n>> >\n>> > git \"\"\n>> >\n>> > i.e. the second word on the command line consists of two double\n>> > quotes. However, this is not what happens for real, because after\n>> > 'git ' the second word on the command line is just an empty\n>> > string. Luckily, the test works nevertheless.\n>> >\n>> > Fix this by passing the command line to run_completion() as separate\n>> > words.\n>> >\n>> > Signed-off-by: SZEDER Gábor \n>> > ---\n>> > t/t9902-completion.sh | 8 ++++----\n>> > 1 file changed, 4 insertions(+), 4 deletions(-)\n>> >\n>> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n>> > index e7657537..f5e68834 100755\n>> > --- a/t/t9902-completion.sh\n>> > +++ b/t/t9902-completion.sh\n>> > @@ -49,7 +49,7 @@ run_completion ()\n>> > {\n>> > \tlocal -a COMPREPLY _words\n>> > \tlocal _cword\n>> > -\t_words=( $1 )\n>> > +\t_words=( \"$@\" )\n>> > \t(( _cword = ${#_words[@]} - 1 ))\n>> > \t__git_wrap__git_main && print_comp\n>> > }\n>> > @@ -57,7 +57,7 @@ run_completion ()\n>> > test_completion ()\n>> > {\n>> > \ttest $# -gt 1 && echo \"$2\" > expected\n>> > -\trun_completion \"$@\" &&\n>> > +\trun_completion $1 &&\n>> > \ttest_cmp expected out\n>> > }\n>> \n>> I can understand the other three hunks, but this one is fishy.\n>> Shouldn't \"$1\" be inside a pair of dq? I.e.\n>> \n>> \t+\trun_completion \"$1\" &&\n>\n> No. $1 holds all words on the command line. If it was between a pair\n> of dq, then the whole command line would be passed to the completion\n> script as a single word.\n\nAnd these \"words\" can be split at $IFS boundaries without any\nissues? IOW, nobody would ever want to make words array in the\nrun_completion function to ['git' 'foo bar' 'baz']?\n"},{"id":"200128","messageId":"20120928193008.GA3912@sigill.intra.peff.net","threadId":"31614","inReplyTo":"7vzk4aj6ik.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/5] completion: fix args of run_completion() test helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-28T19:30:08Z","receivedAt":"2012-09-28T19:30:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 28, 2012 at 12:23:47PM -0700, Junio C Hamano wrote:\n\n> >> > @@ -57,7 +57,7 @@ run_completion ()\n> >> > test_completion ()\n> >> > {\n> >> > \ttest $# -gt 1 && echo \"$2\" > expected\n> >> > -\trun_completion \"$@\" &&\n> >> > +\trun_completion $1 &&\n> >> > \ttest_cmp expected out\n> >> > }\n> >> \n> >> I can understand the other three hunks, but this one is fishy.\n> >> Shouldn't \"$1\" be inside a pair of dq? I.e.\n> >> \n> >> \t+\trun_completion \"$1\" &&\n> >\n> > No. $1 holds all words on the command line. If it was between a pair\n> > of dq, then the whole command line would be passed to the completion\n> > script as a single word.\n> \n> And these \"words\" can be split at $IFS boundaries without any\n> issues? IOW, nobody would ever want to make words array in the\n> run_completion function to ['git' 'foo bar' 'baz']?\n\nIt might be simpler to just convert test_completion into the\ntest_completion_long I added in my series; the latter takes the expected\noutput on stdin, leaving the actual arguments free to represent the real\ncommand-line. E.g., your example would become:\n\n test_completion git \"foo bar\" baz <<-\\EOF\n ... expected output ...\n EOF\n\n-Peff\n"},{"id":"200131","messageId":"7vvceyj5bu.fsf@alter.siamese.dyndns.org","threadId":"31614","inReplyTo":"20120928193008.GA3912@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] completion: fix args of run_completion() test helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-28T19:49:25Z","receivedAt":"2012-09-28T19:49:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Sep 28, 2012 at 12:23:47PM -0700, Junio C Hamano wrote:\n>\n>> >> > @@ -57,7 +57,7 @@ run_completion ()\n>> >> > test_completion ()\n>> >> > {\n>> >> > \ttest $# -gt 1 && echo \"$2\" > expected\n>> >> > -\trun_completion \"$@\" &&\n>> >> > +\trun_completion $1 &&\n>> >> > \ttest_cmp expected out\n>> >> > }\n>> >> \n>> >> I can understand the other three hunks, but this one is fishy.\n>> >> Shouldn't \"$1\" be inside a pair of dq? I.e.\n>> >> \n>> >> \t+\trun_completion \"$1\" &&\n>> >\n>> > No. $1 holds all words on the command line. If it was between a pair\n>> > of dq, then the whole command line would be passed to the completion\n>> > script as a single word.\n>> \n>> And these \"words\" can be split at $IFS boundaries without any\n>> issues? IOW, nobody would ever want to make words array in the\n>> run_completion function to ['git' 'foo bar' 'baz']?\n>\n> It might be simpler to just convert test_completion into the\n> test_completion_long I added in my series; the latter takes the expected\n> output on stdin, leaving the actual arguments free to represent the real\n> command-line. E.g., your example would become:\n>\n> test_completion git \"foo bar\" baz <<-\\EOF\n> ... expected output ...\n> EOF\n\nI realize that the way my question was stated was misleading. It\nwas not meant as a rhetorical \"You would never be able to pass\n['git' 'foo bar' 'baz'] with that interface, and the patch sucks.\"\nbut was meant as a pure question \"Do we want to pass such word\nlist?\". \"test_completion is almost always used to test completion\nwith inputs without any $IFS letters in it, so not being able to\ntest such an input via this interface is fine. If needed, we can\ngive another less often used interface to let you pass such an\ninput\" is perfectly fine by me.\n\nBut I suspect that the real reason test_completion requires the\ncaller to express the list of inputs to run_completion as $IFS\nseparate list is because it needs to also get expected from the\ncommand line:\n\n>> > test_completion ()\n>> > {\n>> > \ttest $# -gt 1 && echo \"$2\" > expected\n>> > -\trun_completion \"$@\" &&\n>> > +\trun_completion $1 &&\n>> > \ttest_cmp expected out\n>> > }\n\nI wonder if doing something like this would be a far simpler\nsolution:\n\n\ttest_completion ()\n {\n\t\tcase \"$1\" in\n '')\n\t\t\t;;\n\t\t*)\n\t\t\techo \"$1\" >expect &&\n\t shift\n ;;\n\t\tesac &&\n run_completion \"$@\" &&\n test_cmp expect output\n\t}\n"},{"id":"200132","messageId":"20120928195531.GB3912@sigill.intra.peff.net","threadId":"31614","inReplyTo":"7vvceyj5bu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/5] completion: fix args of run_completion() test helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-28T19:55:31Z","receivedAt":"2012-09-28T19:55:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 28, 2012 at 12:49:25PM -0700, Junio C Hamano wrote:\n\n> >> And these \"words\" can be split at $IFS boundaries without any\n> >> issues? IOW, nobody would ever want to make words array in the\n> >> run_completion function to ['git' 'foo bar' 'baz']?\n> >\n> > It might be simpler to just convert test_completion into the\n> > test_completion_long I added in my series; the latter takes the expected\n> > output on stdin, leaving the actual arguments free to represent the real\n> > command-line. E.g., your example would become:\n> >\n> > test_completion git \"foo bar\" baz <<-\\EOF\n> > ... expected output ...\n> > EOF\n> \n> I realize that the way my question was stated was misleading. It\n> was not meant as a rhetorical \"You would never be able to pass\n> ['git' 'foo bar' 'baz'] with that interface, and the patch sucks.\"\n> but was meant as a pure question \"Do we want to pass such word\n> list?\". \"test_completion is almost always used to test completion\n> with inputs without any $IFS letters in it, so not being able to\n> test such an input via this interface is fine. If needed, we can\n> give another less often used interface to let you pass such an\n> input\" is perfectly fine by me.\n\nI think we may eventually want to pass arguments with IFS into the\nfunction, just to make sure it works (the tests I added checked for IFS\nin the completion list rather than the input, but we should probably\ncheck both).\n\nI'm OK if it needs to be an alternate interface (right now you could do\nit by calling run_completion yourself).\n\n> But I suspect that the real reason test_completion requires the\n> caller to express the list of inputs to run_completion as $IFS\n> separate list is because it needs to also get expected from the\n> command line:\n\nRight, that's why I suggested bumping that to stdin for the function.\n\n> >> > test_completion ()\n> >> > {\n> >> > \ttest $# -gt 1 && echo \"$2\" > expected\n> >> > -\trun_completion \"$@\" &&\n> >> > +\trun_completion $1 &&\n> >> > \ttest_cmp expected out\n> >> > }\n> \n> I wonder if doing something like this would be a far simpler\n> solution:\n> \n> \ttest_completion ()\n> {\n> \t\tcase \"$1\" in\n> '')\n> \t\t\t;;\n> \t\t*)\n> \t\t\techo \"$1\" >expect &&\n> \t shift\n> ;;\n> \t\tesac &&\n> run_completion \"$@\" &&\n> test_cmp expect output\n> \t}\n\nThat would also work. I mainly suggested the stdin thing because we need\nit anyway for output that generates multiple answers (well, you don't\n_need_ it; you can call run_completion yourself, but it saves a few\nlines at each call site).\n\n-Peff\n"},{"id":"200134","messageId":"20120928202816.GC10719@goldbirke","threadId":"31614","inReplyTo":"7vzk4aj6ik.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/5] completion: fix args of run_completion() test helper","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-09-28T20:28:16Z","receivedAt":"2012-09-28T20:28:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Sep 28, 2012 at 12:23:47PM -0700, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n> > On Fri, Sep 28, 2012 at 11:04:05AM -0700, Junio C Hamano wrote:\n> >> SZEDER Gábor writes:\n> >> \n> >> > To simulate the the user hit 'git , one of the completion tests\n> >\n> > s/the the/that the/\n> >\n> >> > sets up the rather strange command line\n> >> >\n> >> > git \"\"\n> >> >\n> >> > i.e. the second word on the command line consists of two double\n> >> > quotes. However, this is not what happens for real, because after\n> >> > 'git ' the second word on the command line is just an empty\n> >> > string. Luckily, the test works nevertheless.\n> >> >\n> >> > Fix this by passing the command line to run_completion() as separate\n> >> > words.\n> >> >\n> >> > Signed-off-by: SZEDER Gábor \n> >> > ---\n> >> > t/t9902-completion.sh | 8 ++++----\n> >> > 1 file changed, 4 insertions(+), 4 deletions(-)\n> >> >\n> >> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> >> > index e7657537..f5e68834 100755\n> >> > --- a/t/t9902-completion.sh\n> >> > +++ b/t/t9902-completion.sh\n> >> > @@ -49,7 +49,7 @@ run_completion ()\n> >> > {\n> >> > \tlocal -a COMPREPLY _words\n> >> > \tlocal _cword\n> >> > -\t_words=( $1 )\n> >> > +\t_words=( \"$@\" )\n> >> > \t(( _cword = ${#_words[@]} - 1 ))\n> >> > \t__git_wrap__git_main && print_comp\n> >> > }\n> >> > @@ -57,7 +57,7 @@ run_completion ()\n> >> > test_completion ()\n> >> > {\n> >> > \ttest $# -gt 1 && echo \"$2\" > expected\n> >> > -\trun_completion \"$@\" &&\n> >> > +\trun_completion $1 &&\n> >> > \ttest_cmp expected out\n> >> > }\n> >> \n> >> I can understand the other three hunks, but this one is fishy.\n> >> Shouldn't \"$1\" be inside a pair of dq? I.e.\n> >> \n> >> \t+\trun_completion \"$1\" &&\n> >\n> > No. $1 holds all words on the command line. If it was between a pair\n> > of dq, then the whole command line would be passed to the completion\n> > script as a single word.\n> \n> And these \"words\" can be split at $IFS boundaries without any\n> issues? IOW, nobody would ever want to make words array in the\n> run_completion function to ['git' 'foo bar' 'baz']?\n\nMaybe we would, but making it work would be a separate fix. You can't\ndo that with the current version either.\n"}]}