* [PATCH] completion: fix shell expansion of items @ 2012-09-20 2:15 Felipe Contreras 2012-09-20 1:46 ` Jeff King 0 siblings, 1 reply; 37+ messages in thread From: Felipe Contreras @ 2012-09-20 2:15 UTC (permalink / raw) To: git; +Cc: Jeff King, Junio C Hamano, Felipe Contreras As reported by Jeroen Meijer[1]; the current code doesn't deal properly with items (tags, branches, etc.) that have ${} in them because they get expaned by bash while using compgen. A simple solution is to quote the items so they get expanded properly (\$\{\}). In order to achieve that I took bash-completion's quote() function, which is rather simple, and renamed it to __git_quote() as per Jeff King's suggestion. Solves the original problem for me. [1] http://article.gmane.org/gmane.comp.version-control.git/201596 Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- contrib/completion/git-completion.bash | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index d743e56..5a5b5a0 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -225,6 +225,13 @@ _get_comp_words_by_ref () fi fi +# Quotes the argument for shell reuse +__git_quote() +{ + local quoted=${1//\'/\'\\\'\'} + printf "'%s'" "$quoted" +} + # Generates completion reply with compgen, appending a space to possible # completion words, if necessary. # It accepts 1 to 4 arguments: @@ -261,7 +268,7 @@ __gitcomp () __gitcomp_nl () { local IFS=$'\n' - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) + COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$(__git_quote "$1")" -- "${3-$cur}")) } __git_heads () -- 1.7.10.3 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH] completion: fix shell expansion of items 2012-09-20 2:15 [PATCH] completion: fix shell expansion of items Felipe Contreras @ 2012-09-20 1:46 ` Jeff King 2012-09-20 16:52 ` Junio C Hamano 2012-09-20 18:11 ` SZEDER Gábor 0 siblings, 2 replies; 37+ messages in thread From: Jeff King @ 2012-09-20 1:46 UTC (permalink / raw) To: Felipe Contreras; +Cc: git, Junio C Hamano On Thu, Sep 20, 2012 at 04:15:15AM +0200, Felipe Contreras wrote: > As reported by Jeroen Meijer[1]; the current code doesn't deal properly > with items (tags, branches, etc.) that have ${} in them because they get > expaned by bash while using compgen. > > A simple solution is to quote the items so they get expanded properly > (\$\{\}). > > In order to achieve that I took bash-completion's quote() function, > which is rather simple, and renamed it to __git_quote() as per Jeff > King's suggestion. > > Solves the original problem for me. Me too. Thanks. -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] completion: fix shell expansion of items 2012-09-20 1:46 ` Jeff King @ 2012-09-20 16:52 ` Junio C Hamano 2012-09-20 18:11 ` SZEDER Gábor 1 sibling, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2012-09-20 16:52 UTC (permalink / raw) To: Jeff King; +Cc: Felipe Contreras, git Jeff King <peff@peff.net> writes: > On Thu, Sep 20, 2012 at 04:15:15AM +0200, Felipe Contreras wrote: > >> As reported by Jeroen Meijer[1]; the current code doesn't deal properly >> with items (tags, branches, etc.) that have ${} in them because they get >> expaned by bash while using compgen. >> >> A simple solution is to quote the items so they get expanded properly >> (\$\{\}). >> >> In order to achieve that I took bash-completion's quote() function, >> which is rather simple, and renamed it to __git_quote() as per Jeff >> King's suggestion. >> >> Solves the original problem for me. > > Me too. Thanks. Thanks, both. Will queue. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] completion: fix shell expansion of items 2012-09-20 1:46 ` Jeff King 2012-09-20 16:52 ` Junio C Hamano @ 2012-09-20 18:11 ` SZEDER Gábor 2012-09-20 18:21 ` Jeff King 2012-09-25 4:31 ` [PATCH] Revert "completion: fix shell expansion of items" Jeff King 1 sibling, 2 replies; 37+ messages in thread From: SZEDER Gábor @ 2012-09-20 18:11 UTC (permalink / raw) To: Jeff King; +Cc: Felipe Contreras, git, Junio C Hamano Hi, On Wed, Sep 19, 2012 at 09:46:08PM -0400, Jeff King wrote: > On Thu, Sep 20, 2012 at 04:15:15AM +0200, Felipe Contreras wrote: > > > As reported by Jeroen Meijer[1]; the current code doesn't deal properly > > with items (tags, branches, etc.) that have ${} in them because they get > > expaned by bash while using compgen. > > > > A simple solution is to quote the items so they get expanded properly > > (\$\{\}). > > > > In order to achieve that I took bash-completion's quote() function, > > which is rather simple, and renamed it to __git_quote() as per Jeff > > King's suggestion. > > > > Solves the original problem for me. > > Me too. Thanks. While it solves the original problem, it seems to break refs completion, as demonstrated by the following POC test: diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 92d7eb47..fab63b95 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -228,4 +228,11 @@ test_expect_success 'general options plus command' ' test_completion "git --no-replace-objects check" "checkout " ' +test_expect_success 'basic refs completion' ' + touch file && + git add file && + git commit -m initial && + test_completion "git branch m" "master " +' + test_done -- 1.7.12.1.438.g7dfa67b which fails with: --- expected 2012-09-20 18:05:23.857752925 +0000 +++ out 2012-09-20 18:05:23.877752925 +0000 @@ -1 +1 @@ -master + ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH] completion: fix shell expansion of items 2012-09-20 18:11 ` SZEDER Gábor @ 2012-09-20 18:21 ` Jeff King 2012-09-20 19:38 ` Felipe Contreras 2012-09-25 4:31 ` [PATCH] Revert "completion: fix shell expansion of items" Jeff King 1 sibling, 1 reply; 37+ messages in thread From: Jeff King @ 2012-09-20 18:21 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Felipe Contreras, git, Junio C Hamano On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote: > > > In order to achieve that I took bash-completion's quote() function, > > > which is rather simple, and renamed it to __git_quote() as per Jeff > > > King's suggestion. > > > > > > Solves the original problem for me. > > > > Me too. Thanks. > > While it solves the original problem, it seems to break refs > completion, as demonstrated by the following POC test: > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index 92d7eb47..fab63b95 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -228,4 +228,11 @@ test_expect_success 'general options plus command' ' > test_completion "git --no-replace-objects check" "checkout " > ' > > +test_expect_success 'basic refs completion' ' > + touch file && > + git add file && > + git commit -m initial && > + test_completion "git branch m" "master " > +' Hmm. I notice that Felipe's patch wraps the _whole_ input to __gitcomp_nl in single quotes. So if there are multiple completions we would end up with: 'one two quo\'ted three' I wonder if that is OK to feed to compgen -W, or if it wants to expand it line-by-line. Just guessing at this point, though. -Peff > + > test_done > -- > 1.7.12.1.438.g7dfa67b > > > which fails with: > > --- expected 2012-09-20 18:05:23.857752925 +0000 > +++ out 2012-09-20 18:05:23.877752925 +0000 > @@ -1 +1 @@ > -master > + > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] completion: fix shell expansion of items 2012-09-20 18:21 ` Jeff King @ 2012-09-20 19:38 ` Felipe Contreras 0 siblings, 0 replies; 37+ messages in thread From: Felipe Contreras @ 2012-09-20 19:38 UTC (permalink / raw) To: Jeff King; +Cc: SZEDER Gábor, git, Junio C Hamano On Thu, Sep 20, 2012 at 8:21 PM, Jeff King <peff@peff.net> wrote: > On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote: > >> > > In order to achieve that I took bash-completion's quote() function, >> > > which is rather simple, and renamed it to __git_quote() as per Jeff >> > > King's suggestion. >> > > >> > > Solves the original problem for me. >> > >> > Me too. Thanks. >> >> While it solves the original problem, it seems to break refs >> completion, as demonstrated by the following POC test: >> >> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >> index 92d7eb47..fab63b95 100755 >> --- a/t/t9902-completion.sh >> +++ b/t/t9902-completion.sh >> @@ -228,4 +228,11 @@ test_expect_success 'general options plus command' ' >> test_completion "git --no-replace-objects check" "checkout " >> ' >> >> +test_expect_success 'basic refs completion' ' >> + touch file && >> + git add file && >> + git commit -m initial && >> + test_completion "git branch m" "master " >> +' > > Hmm. I notice that Felipe's patch wraps the _whole_ input to > __gitcomp_nl in single quotes. Wasn't there a patch series that added tests for __gitcomp_nl to catch these issues? -- Felipe Contreras ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH] Revert "completion: fix shell expansion of items" 2012-09-20 18:11 ` SZEDER Gábor 2012-09-20 18:21 ` Jeff King @ 2012-09-25 4:31 ` Jeff King 2012-09-25 16:03 ` Junio C Hamano 2012-09-25 22:37 ` SZEDER Gábor 1 sibling, 2 replies; 37+ messages in thread From: Jeff King @ 2012-09-25 4:31 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Felipe Contreras, git, Junio C Hamano On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote: > > > Solves the original problem for me. > > > > Me too. Thanks. > > While it solves the original problem, it seems to break refs > completion, as demonstrated by the following POC test: > > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index 92d7eb47..fab63b95 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -228,4 +228,11 @@ test_expect_success 'general options plus command' ' > test_completion "git --no-replace-objects check" "checkout " > ' > > +test_expect_success 'basic refs completion' ' > + touch file && > + git add file && > + git commit -m initial && > + test_completion "git branch m" "master " > +' > + Yeah, doing "git checkout jk/<tab>" is not working at all, and I noticed the buggy commit is on the maint track, but has not yet been released. I'm not sure of the solution, but I think we should do this in the meantime: -- >8 -- Subject: Revert "completion: fix shell expansion of items" This reverts commit 25ae7cfd19c8f21721363c64163cd5d9d1135b20. That patch does fix expansion of weird variables in some simple tests, but it also seems to break other things, like expansion of refs by "git checkout". While we're sorting out the correct solution, we are much better with the original bug (people with metacharacters in their completions occasionally see an error message) than the current bug (ref completion does not work at all). Signed-off-by: Jeff King <peff@peff.net> --- contrib/completion/git-completion.bash | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 5a5b5a0..d743e56 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -225,13 +225,6 @@ fi fi fi -# Quotes the argument for shell reuse -__git_quote() -{ - local quoted=${1//\'/\'\\\'\'} - printf "'%s'" "$quoted" -} - # Generates completion reply with compgen, appending a space to possible # completion words, if necessary. # It accepts 1 to 4 arguments: @@ -268,7 +261,7 @@ __gitcomp_nl () __gitcomp_nl () { local IFS=$'\n' - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$(__git_quote "$1")" -- "${3-$cur}")) + COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) } __git_heads () -- 1.7.12.1.17.g7286916 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH] Revert "completion: fix shell expansion of items" 2012-09-25 4:31 ` [PATCH] Revert "completion: fix shell expansion of items" Jeff King @ 2012-09-25 16:03 ` Junio C Hamano 2012-09-25 22:37 ` SZEDER Gábor 1 sibling, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2012-09-25 16:03 UTC (permalink / raw) To: Jeff King; +Cc: SZEDER Gábor, Felipe Contreras, git Jeff King <peff@peff.net> writes: > On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote: > > Yeah, doing "git checkout jk/<tab>" is not working at all, and I noticed > the buggy commit is on the maint track, but has not yet been released. > I'm not sure of the solution, but I think we should do this in the > meantime: Thanks. Reverting is the right solution; it will leave a note in the history why a seemingly simple patch was a bad idea to remind us. It is sad that existing test did not cover something fundamental like this, though. Anybody who's worked on completion wants to make the test coverage better? > -- >8 -- > Subject: Revert "completion: fix shell expansion of items" > > This reverts commit 25ae7cfd19c8f21721363c64163cd5d9d1135b20. > > That patch does fix expansion of weird variables in some > simple tests, but it also seems to break other things, like > expansion of refs by "git checkout". > > While we're sorting out the correct solution, we are much > better with the original bug (people with metacharacters in > their completions occasionally see an error message) than > the current bug (ref completion does not work at all). > > Signed-off-by: Jeff King <peff@peff.net> > --- > contrib/completion/git-completion.bash | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 5a5b5a0..d743e56 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -225,13 +225,6 @@ fi > fi > fi > > -# Quotes the argument for shell reuse > -__git_quote() > -{ > - local quoted=${1//\'/\'\\\'\'} > - printf "'%s'" "$quoted" > -} > - > # Generates completion reply with compgen, appending a space to possible > # completion words, if necessary. > # It accepts 1 to 4 arguments: > @@ -268,7 +261,7 @@ __gitcomp_nl () > __gitcomp_nl () > { > local IFS=$'\n' > - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$(__git_quote "$1")" -- "${3-$cur}")) > + COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) > } > > __git_heads () ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] Revert "completion: fix shell expansion of items" 2012-09-25 4:31 ` [PATCH] Revert "completion: fix shell expansion of items" Jeff King 2012-09-25 16:03 ` Junio C Hamano @ 2012-09-25 22:37 ` SZEDER Gábor 2012-09-25 23:28 ` Junio C Hamano 2012-09-26 21:46 ` Jeff King 1 sibling, 2 replies; 37+ messages in thread From: SZEDER Gábor @ 2012-09-25 22:37 UTC (permalink / raw) To: Jeff King; +Cc: Felipe Contreras, git, Junio C Hamano On Tue, Sep 25, 2012 at 12:31:19AM -0400, Jeff King wrote: > Yeah, doing "git checkout jk/<tab>" is not working at all, and I noticed > the buggy commit is on the maint track, but has not yet been released. > I'm not sure of the solution, but I think we should do this in the > meantime: > > -- >8 -- > Subject: Revert "completion: fix shell expansion of items" I agree with the revert, too. I looked into this issue, but quickly got lost in quoting-escaping hell. Ideally we could do some quoting in __gitcomp_nl(), so it would work for all kinds of input, but I couldn't come up with anything working. Alternatively, we could modify __gitcomp_nl()'s callers, or rather the helper functions supplying input to __gitcomp_nl() to do the quoting or escaping themselves. Actually, that's quite easy for local refs, at least, because for-each-ref's builtin quoting support does the trick: diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index c48cd19f..3dc1ec8c 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -313,7 +313,7 @@ __git_refs () refs="refs/tags refs/heads refs/remotes" ;; esac - git --git-dir="$dir" for-each-ref --format="%($format)" \ + git --git-dir="$dir" for-each-ref --shell --format="%($format)" \ $refs if [ -n "$track" ]; then # employ the heuristic used by git checkout With this change completion of local refs works well, even in the presence of branches ${foo.bar} and foo'bar. Unfortunately, there are many callsites for __gitcomp_nl(), and it is invoked with refs from remote repos, heads, tags, refspecs, remotes, config variables, symbols from ctags, or output from stash or ls-tree... although some of these are OK as they are now (remotes, config variables). Best, Gábor ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH] Revert "completion: fix shell expansion of items" 2012-09-25 22:37 ` SZEDER Gábor @ 2012-09-25 23:28 ` Junio C Hamano 2012-09-26 21:46 ` Jeff King 1 sibling, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2012-09-25 23:28 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Jeff King, Felipe Contreras, git SZEDER Gábor <szeder@ira.uka.de> writes: > On Tue, Sep 25, 2012 at 12:31:19AM -0400, Jeff King wrote: >> Yeah, doing "git checkout jk/<tab>" is not working at all, and I noticed >> the buggy commit is on the maint track, but has not yet been released. >> I'm not sure of the solution, but I think we should do this in the >> meantime: >> >> -- >8 -- >> Subject: Revert "completion: fix shell expansion of items" > > I agree with the revert, too. > > I looked into this issue, but quickly got lost in quoting-escaping > hell. Ideally we could do some quoting in __gitcomp_nl(), so it would > work for all kinds of input, but I couldn't come up with anything > working. Alternatively, we could modify __gitcomp_nl()'s callers, or > rather the helper functions supplying input to __gitcomp_nl() to do > the quoting or escaping themselves. Actually, that's quite easy for > local refs, at least, because for-each-ref's builtin quoting support > does the trick: > > diff --git a/contrib/completion/git-completion.bash > b/contrib/completion/git-completion.bash > index c48cd19f..3dc1ec8c 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -313,7 +313,7 @@ __git_refs () > refs="refs/tags refs/heads refs/remotes" > ;; > esac > - git --git-dir="$dir" for-each-ref --format="%($format)" \ > + git --git-dir="$dir" for-each-ref --shell --format="%($format)" \ > $refs > if [ -n "$track" ]; then > # employ the heuristic used by git checkout > > With this change completion of local refs works well, even in the > presence of branches ${foo.bar} and foo'bar. > > Unfortunately, there are many callsites for __gitcomp_nl(), and it is > invoked with refs from remote repos, heads, tags, refspecs, remotes, > config variables, symbols from ctags, or output from stash or > ls-tree... although some of these are OK as they are now (remotes, > config variables). Whatever you do, please make the first step of that endeavour an addition to the t/t9902 to prevent the same breakage from happening again. Thanks. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] Revert "completion: fix shell expansion of items" 2012-09-25 22:37 ` SZEDER Gábor 2012-09-25 23:28 ` Junio C Hamano @ 2012-09-26 21:46 ` Jeff King 2012-09-26 21:47 ` [PATCH 1/3] t9902: add a few basic completion tests Jeff King ` (2 more replies) 1 sibling, 3 replies; 37+ messages in thread From: Jeff King @ 2012-09-26 21:46 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Felipe Contreras, git, Junio C Hamano On Wed, Sep 26, 2012 at 12:37:25AM +0200, SZEDER Gábor wrote: > I looked into this issue, but quickly got lost in quoting-escaping > hell. Ideally we could do some quoting in __gitcomp_nl(), so it would > work for all kinds of input, but I couldn't come up with anything > working. Alternatively, we could modify __gitcomp_nl()'s callers, or > rather the helper functions supplying input to __gitcomp_nl() to do > the quoting or escaping themselves. Actually, that's quite easy for > local refs, at least, because for-each-ref's builtin quoting support > does the trick: I feel like insanity lies that way, because every caller is going to have to do its own quoting. On the other hand, I think it would be the only way to handle completion of entries with embedded newlines (as it is now, we pass in a newline-delimited list with no opportunity for quoting). Here's a simple patch series that fixes the problem and adds a few basic sanity checks. [1/3]: t9902: add a few basic completion tests [2/3]: t9902: add completion tests for "odd" filenames [3/3]: completion: improve shell expansion of items -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 1/3] t9902: add a few basic completion tests 2012-09-26 21:46 ` Jeff King @ 2012-09-26 21:47 ` Jeff King 2012-09-26 21:51 ` [PATCH 2/3] t9902: add completion tests for "odd" filenames Jeff King 2012-09-26 21:51 ` [PATCH 3/3] completion: improve shell expansion of items Jeff King 2 siblings, 0 replies; 37+ messages in thread From: Jeff King @ 2012-09-26 21:47 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Felipe Contreras, git, Junio C Hamano We were not testing ref or tree completion at all. Let's give them even basic sanity checks to avoid regressions. Signed-off-by: Jeff King <peff@peff.net> --- This would have caught the recent breakage, and also paves the way for testing the new fix. t/t9902-completion.sh | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 92d7eb4..2fc833a 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -61,6 +61,15 @@ test_completion () test_cmp expected out } +# Like test_completion, but reads expectation from stdin, +# which is convenient when it is multiline. We also process "_" into +# spaces to make test vectors more readable. +test_completion_long () +{ + tr _ " " >expected && + test_completion "$1" +} + newline=$'\n' test_expect_success '__gitcomp - trailing space - options' ' @@ -228,4 +237,36 @@ test_expect_success 'general options plus command' ' test_completion "git --no-replace-objects check" "checkout " ' +test_expect_success 'setup for ref completion' ' + echo content >file1 && + echo more >file2 && + git add . && + git commit -m one && + git branch mybranch && + git tag mytag +' + +test_expect_success 'checkout completes ref names' ' + test_completion_long "git checkout m" <<-\EOF + master_ + mybranch_ + mytag_ + EOF +' + +test_expect_success 'show completes all refs' ' + test_completion_long "git show m" <<-\EOF + master_ + mybranch_ + mytag_ + EOF +' + +test_expect_success '<ref>: completes paths' ' + test_completion_long "git show mytag:f" <<-\EOF + file1_ + file2_ + EOF +' + test_done -- 1.7.12.10.g31da6dd ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 2/3] t9902: add completion tests for "odd" filenames 2012-09-26 21:46 ` Jeff King 2012-09-26 21:47 ` [PATCH 1/3] t9902: add a few basic completion tests Jeff King @ 2012-09-26 21:51 ` Jeff King 2012-09-26 21:51 ` [PATCH 3/3] completion: improve shell expansion of items Jeff King 2 siblings, 0 replies; 37+ messages in thread From: Jeff King @ 2012-09-26 21:51 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Felipe Contreras, git, Junio C Hamano We correctly handle completion items with spaces just fine, since we pass the lists around with newline delimiters. However, we do not handle filenames with shell metacharacters, as "compgen -W" performs expansion on the list we give it. Signed-off-by: Jeff King <peff@peff.net> --- Actually, these vectors are not strictly correct, as I think ultimately we would like to return 'name with spaces' with quotes. But this lets us test that at least the basics work, and we can update the test vectors later. It would be nice to test completion on a file with an embedded newline (which will also not work), but I'm not even sure what the result is supposed to look like, so I couldn't write a sane test case. Since I don't plan on fixing that anytime soon anyway, I think it's sane to just leave it until later if somebody actually cares. t/t9902-completion.sh | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 2fc833a..cbd0fb6 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -269,4 +269,23 @@ test_expect_success '<ref>: completes paths' ' EOF ' +test_expect_success 'complete tree filename with spaces' ' + echo content >"name with spaces" && + git add . && + git commit -m spaces && + test_completion_long "git show HEAD:nam" <<-\EOF + name with spaces_ + EOF +' + +test_expect_failure 'complete tree filename with metacharacters' ' + echo content >"name with \${meta}" && + git add . && + git commit -m meta && + test_completion_long "git show HEAD:nam" <<-\EOF + name with ${meta}_ + name with spaces_ + EOF +' + test_done -- 1.7.12.10.g31da6dd ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 3/3] completion: improve shell expansion of items 2012-09-26 21:46 ` Jeff King 2012-09-26 21:47 ` [PATCH 1/3] t9902: add a few basic completion tests Jeff King 2012-09-26 21:51 ` [PATCH 2/3] t9902: add completion tests for "odd" filenames Jeff King @ 2012-09-26 21:51 ` Jeff King 2012-09-26 21:57 ` [PATCH 4/3] completion: quote completions we find Jeff King 2012-09-27 0:37 ` [PATCH 3/3] completion: improve shell expansion of items SZEDER Gábor 2 siblings, 2 replies; 37+ messages in thread From: Jeff King @ 2012-09-26 21:51 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Felipe Contreras, git, Junio C Hamano The current completion code doesn't deal properly with items (tags, branches, etc.) that have ${} in them because they get expanded by bash while using compgen. This patch is a rewrite of Felipe Contreras's 25ae7cf, which attempted to fix the problem by quoting the values we pass to __gitcomp_nl. However, that patch ended up quoting the whole list as a single item, which broke other completions. Instead, we need to split the newline-delimited list into elements and quote each one individually. This is not a complete fix, as the completion result does will still contain metacharacters, so it would need extra quoting to actually be used on a command line. But it's still a step in the right direction, because: 1. The current code's expansion means that things that are broken expansions (e.g., "${foo:bar}") will actually cause the completion function to barf, breaking all completion (even if you weren't going to complete that item). This patch fixes that so you can at least complete "foo" when "${foo:bar}" exists. 2. We don't know yet what the final fix will look like, but this is probably the first step towards it. It handles quoting on the input side of compgen; the next step will likely be handling the quoting on the output side of compgen to yield a usable string for the command line. Signed-off-by: Jeff King <peff@peff.net> --- contrib/completion/git-completion.bash | 14 +++++++++++++- t/t9902-completion.sh | 2 +- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index be800e0..b0416ea 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -225,6 +225,18 @@ fi fi fi +# Quotes each element of an IFS-delimited list for shell reuse +__git_quote() +{ + local i + local delim + for i in $1; do + local quoted=${i//\'/\'\\\'\'} + printf "${delim:+$IFS}'%s'" "$quoted" + delim=t + done +} + # Generates completion reply with compgen, appending a space to possible # completion words, if necessary. # It accepts 1 to 4 arguments: @@ -261,7 +273,7 @@ __gitcomp_nl () __gitcomp_nl () { local IFS=$'\n' - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) + COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$(__git_quote "$1")" -- "${3-$cur}")) } __git_heads () diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index cbd0fb6..da67705 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -278,7 +278,7 @@ test_expect_success 'complete tree filename with spaces' ' EOF ' -test_expect_failure 'complete tree filename with metacharacters' ' +test_expect_success 'complete tree filename with metacharacters' ' echo content >"name with \${meta}" && git add . && git commit -m meta && -- 1.7.12.10.g31da6dd ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 4/3] completion: quote completions we find 2012-09-26 21:51 ` [PATCH 3/3] completion: improve shell expansion of items Jeff King @ 2012-09-26 21:57 ` Jeff King 2012-09-27 21:40 ` SZEDER Gábor 2012-09-27 0:37 ` [PATCH 3/3] completion: improve shell expansion of items SZEDER Gábor 1 sibling, 1 reply; 37+ messages in thread From: Jeff King @ 2012-09-26 21:57 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Felipe Contreras, git, Junio C Hamano On Wed, Sep 26, 2012 at 05:51:19PM -0400, Jeff King wrote: > This is not a complete fix, as the completion result does > will still contain metacharacters, so it would need extra > quoting to actually be used on a command line. But it's > still a step in the right direction, because: > [...] > 2. We don't know yet what the final fix will look like, > but this is probably the first step towards it. It > handles quoting on the input side of compgen; the next > step will likely be handling the quoting on the output > side of compgen to yield a usable string for the > command line. Here is on attempt at that. It seems to work for me, but I know it is not what bash does internally with: $ ls name with spaces $ cat name<TAB> Because in bash's internal case, it knows that "name with spaces" is the real entry (because if you have many entries and double-tab, it shows it without the quotes), and only later adds the quoting. So while this works, I'm not sure if it is optimal or if it has any weird side effects. -- >8 -- Subject: [PATCH] completion: quote completions we find If you try to complete a filename with spaces, you might end up with this: $ git show HEAD:name<TAB> $ git show HEAD:name with spaces which is technically correct, but does not help you, since the shell will split the words as soon as you hit enter. Instead, let's quote completions coming out of __gitcomp_nl to yield: $ git show HEAD:'name with spaces' Signed-off-by: Jeff King <peff@peff.net> --- contrib/completion/git-completion.bash | 23 +++++++++++++++++++++++ t/t9902-completion.sh | 6 +++--- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index b0416ea..1fc43f7 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -274,6 +274,29 @@ __gitcomp_nl () { local IFS=$'\n' COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$(__git_quote "$1")" -- "${3-$cur}")) + + local i + for (( i=0; i < ${#COMPREPLY[@]}; i++ )); do + # Hack to handle the extra space we add for some + # entries, since we use "-o nospace". + local stripped + case "${COMPREPLY[$i]}" in + *\ ) + stripped=' ' + COMPREPLY[$i]=${COMPREPLY[$i]% } + ;; + esac + + # Now we can check if we need actual quoting. + case "${COMPREPLY[$i]}" in + *[\ \$\'\"\(\)]*) + COMPREPLY[$i]="'${COMPREPLY[$i]//\'/\'\\\'\'}'" + ;; + esac + + # And then restore any stripped space. + COMPREPLY[$i]="${COMPREPLY[$i]}$stripped" + done } __git_heads () diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index da67705..d2c5104 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -274,7 +274,7 @@ test_expect_success 'complete tree filename with spaces' ' git add . && git commit -m spaces && test_completion_long "git show HEAD:nam" <<-\EOF - name with spaces_ + '\''name with spaces'\''_ EOF ' @@ -283,8 +283,8 @@ test_expect_success 'complete tree filename with metacharacters' ' git add . && git commit -m meta && test_completion_long "git show HEAD:nam" <<-\EOF - name with ${meta}_ - name with spaces_ + '\''name with ${meta}'\''_ + '\''name with spaces'\''_ EOF ' -- 1.7.12.10.g31da6dd ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 4/3] completion: quote completions we find 2012-09-26 21:57 ` [PATCH 4/3] completion: quote completions we find Jeff King @ 2012-09-27 21:40 ` SZEDER Gábor 2012-09-27 22:31 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: SZEDER Gábor @ 2012-09-27 21:40 UTC (permalink / raw) To: Jeff King; +Cc: Felipe Contreras, git, Junio C Hamano On Wed, Sep 26, 2012 at 05:57:00PM -0400, Jeff King wrote: > + COMPREPLY[$i]="${COMPREPLY[$i]}$stripped" This reminded me to a mini-series collecting dust in my git repo, which converts a few similar var=$var$something constructs to use the += append operator instead. Now, Bash supports this += append operator since v3.1 (bash-3.1-alpha1, to be exact), which is around since July 2005, if I can trust the mtime at ftp://ftp.cwru.edu/pub/bash/. MSysgit ships v3.1 so it already supports this, too. So, what is the oldest Bash version we care about for completion? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/3] completion: quote completions we find 2012-09-27 21:40 ` SZEDER Gábor @ 2012-09-27 22:31 ` Junio C Hamano 2012-09-27 22:58 ` SZEDER Gábor 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2012-09-27 22:31 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Jeff King, Felipe Contreras, git SZEDER Gábor <szeder@ira.uka.de> writes: > On Wed, Sep 26, 2012 at 05:57:00PM -0400, Jeff King wrote: >> + COMPREPLY[$i]="${COMPREPLY[$i]}$stripped" > > This reminded me to a mini-series collecting dust in my git repo, > which converts a few similar var=$var$something constructs to use the > += append operator instead. Is the benefit of rewriting it to var+=$something large enough to worry about the below? > Now, Bash supports this += append operator since v3.1 > (bash-3.1-alpha1, to be exact), which is around since July 2005, if I > can trust the mtime at ftp://ftp.cwru.edu/pub/bash/. MSysgit ships > v3.1 so it already supports this, too. So, what is the oldest Bash > version we care about for completion? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/3] completion: quote completions we find 2012-09-27 22:31 ` Junio C Hamano @ 2012-09-27 22:58 ` SZEDER Gábor 0 siblings, 0 replies; 37+ messages in thread From: SZEDER Gábor @ 2012-09-27 22:58 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Felipe Contreras, git On Thu, Sep 27, 2012 at 03:31:10PM -0700, Junio C Hamano wrote: > SZEDER Gábor <szeder@ira.uka.de> writes: > > > On Wed, Sep 26, 2012 at 05:57:00PM -0400, Jeff King wrote: > >> + COMPREPLY[$i]="${COMPREPLY[$i]}$stripped" > > > > This reminded me to a mini-series collecting dust in my git repo, > > which converts a few similar var=$var$something constructs to use the > > += append operator instead. > > Is the benefit of rewriting it to var+=$something large enough to > worry about the below? That way we can get rid of a subshell in __gitcomp(), which means one less fork() during every command or option completion for Windows folks. We can also get rid of two subshells during loading the completion script. And I would spare myself from a couple of merge conflicts, too ;) > > Now, Bash supports this += append operator since v3.1 > > (bash-3.1-alpha1, to be exact), which is around since July 2005, if I > > can trust the mtime at ftp://ftp.cwru.edu/pub/bash/. MSysgit ships > > v3.1 so it already supports this, too. So, what is the oldest Bash > > version we care about for completion? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 3/3] completion: improve shell expansion of items 2012-09-26 21:51 ` [PATCH 3/3] completion: improve shell expansion of items Jeff King 2012-09-26 21:57 ` [PATCH 4/3] completion: quote completions we find Jeff King @ 2012-09-27 0:37 ` SZEDER Gábor 2012-09-27 6:28 ` Jeff King 1 sibling, 1 reply; 37+ messages in thread From: SZEDER Gábor @ 2012-09-27 0:37 UTC (permalink / raw) To: Jeff King; +Cc: Felipe Contreras, git, Junio C Hamano Hi, On Wed, Sep 26, 2012 at 05:51:19PM -0400, Jeff King wrote: > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index be800e0..b0416ea 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -225,6 +225,18 @@ fi > fi > fi > > +# Quotes each element of an IFS-delimited list for shell reuse > +__git_quote() > +{ > + local i > + local delim > + for i in $1; do > + local quoted=${i//\'/\'\\\'\'} > + printf "${delim:+$IFS}'%s'" "$quoted" > + delim=t > + done > +} > + > # Generates completion reply with compgen, appending a space to possible > # completion words, if necessary. > # It accepts 1 to 4 arguments: > @@ -261,7 +273,7 @@ __gitcomp_nl () > __gitcomp_nl () > { > local IFS=$'\n' > - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) > + COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$(__git_quote "$1")" -- "${3-$cur}")) Iterating through all possible completion words undermines the main reason for __gitcomp_nl()'s existence: to handle potentially large number of possible completion words faster than the old __gitcomp(). If we really have to iterate in a subshell, then it would perhaps be better to drop __gitcomp_nl(), go back to using __gitcomp(), and modify that instead. After all, anyone could drop a file called git-cmd-${meta} on his $PATH, and then get cmd- offered, because completion of git commands still goes through __gitcomp(). ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 3/3] completion: improve shell expansion of items 2012-09-27 0:37 ` [PATCH 3/3] completion: improve shell expansion of items SZEDER Gábor @ 2012-09-27 6:28 ` Jeff King 2012-09-27 6:43 ` Jeff King 0 siblings, 1 reply; 37+ messages in thread From: Jeff King @ 2012-09-27 6:28 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Felipe Contreras, git, Junio C Hamano On Thu, Sep 27, 2012 at 02:37:51AM +0200, SZEDER Gábor wrote: > > +# Quotes each element of an IFS-delimited list for shell reuse > > +__git_quote() > > +{ > > + local i > > + local delim > > + for i in $1; do > > + local quoted=${i//\'/\'\\\'\'} > > + printf "${delim:+$IFS}'%s'" "$quoted" > > + delim=t > > + done > > +} > [...] > > Iterating through all possible completion words undermines the main > reason for __gitcomp_nl()'s existence: to handle potentially large > number of possible completion words faster than the old __gitcomp(). > If we really have to iterate in a subshell, then it would perhaps be > better to drop __gitcomp_nl(), go back to using __gitcomp(), and > modify that instead. Thanks for reminding me to time. I noticed your a31e626 while digging in the history, but forgot that I wanted to do a timing test. Sadly, the results are very discouraging. Doing a similar test to your 10,000-refs, I get: $ refs=$(seq 1 10000) $ . git-completion.bash.old $ time __gitcomp_nl "$refs" real 0m0.065s user 0m0.064s sys 0m0.004s $ . git-completion.bash.new $ time __gitcomp_nl "$refs" real 0m1.799s user 0m1.828s sys 0m0.036s So, a 2700% slowdown. Yuck. I also tried running it through sed instead of iterating in bash. I know that some people will not like the fork, but curiously enough, it was not that much faster. Which makes me wonder if part of the slowdown is actually bash unquoting the result in compgen. > After all, anyone could drop a file called git-cmd-${meta} on his > $PATH, and then get cmd- offered, because completion of git commands > still goes through __gitcomp(). Yeah. I wasn't sure if __gitcomp() actually got used on any arbitrary data, but that's a good example. I'm not sure where to go next. I guess we could try pre-quoting via git when generating the list of refs (or files, or whatever) and hope that is faster. With this patch as it is, I'm not sure the slowdown is worth it. Yes, it's more correct in the face of metacharacters, but those are the uncommon case by a large margin. I'd hate to have a performance regression in the common case just for that. -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 3/3] completion: improve shell expansion of items 2012-09-27 6:28 ` Jeff King @ 2012-09-27 6:43 ` Jeff King 2012-09-27 19:48 ` Jeff King 2012-09-28 10:05 ` SZEDER Gábor 0 siblings, 2 replies; 37+ messages in thread From: Jeff King @ 2012-09-27 6:43 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Felipe Contreras, git, Junio C Hamano On Thu, Sep 27, 2012 at 02:28:55AM -0400, Jeff King wrote: > Thanks for reminding me to time. I noticed your a31e626 while digging in > the history, but forgot that I wanted to do a timing test. Sadly, the > results are very discouraging. Doing a similar test to your 10,000-refs, > I get: > > $ refs=$(seq 1 10000) > > $ . git-completion.bash.old > $ time __gitcomp_nl "$refs" > real 0m0.065s > user 0m0.064s > sys 0m0.004s > > $ . git-completion.bash.new > $ time __gitcomp_nl "$refs" > real 0m1.799s > user 0m1.828s > sys 0m0.036s > > So, a 2700% slowdown. Yuck. > > I also tried running it through sed instead of iterating in bash. I know > that some people will not like the fork, but curiously enough, it was > not that much faster. Which makes me wonder if part of the slowdown is > actually bash unquoting the result in compgen. Ah. The problem is that most of the load comes from my patch 4/3, which does a separate iteration. Here are the numbers after just patch 3: $ time __gitcomp_nl "$refs" real 0m0.344s user 0m0.392s sys 0m0.040s Slower, but not nearly as painful. Here are the numbers using sed[1] instead: $ time __gitcomp_nl "$refs" real 0m0.100s user 0m0.084s sys 0m0.016s So a little slower, but probably acceptable. We could maybe do the same trick on the output side (although it is a little trickier there, because we need it in a bash array). Of course, this is Linux; the fork for sed is way more expensive on some systems. Still, I'd be curious to see it with the callers (e.g., __git_refs) doing their own quoting. I'd worry that it would become a maintenance headache, but perhaps we don't have that many lists we feed (there are a lot of calls to gitcomp_nl, but they are mostly feeding __git_refs). -Peff [1] For reference, that patch is: diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 1fc43f7..5ff3742 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -225,16 +225,15 @@ __git_quote() fi fi -# Quotes each element of an IFS-delimited list for shell reuse +# Quotes each element of a newline-delimited list for shell reuse __git_quote() { - local i - local delim - for i in $1; do - local quoted=${i//\'/\'\\\'\'} - printf "${delim:+$IFS}'%s'" "$quoted" - delim=t - done + echo "$1" | + sed " + s/'/'\\\\''/g + s/^/'/ + s/$/'/ + " } # Generates completion reply with compgen, appending a space to possible ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 3/3] completion: improve shell expansion of items 2012-09-27 6:43 ` Jeff King @ 2012-09-27 19:48 ` Jeff King 2012-09-28 10:05 ` SZEDER Gábor 1 sibling, 0 replies; 37+ messages in thread From: Jeff King @ 2012-09-27 19:48 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Felipe Contreras, git, Junio C Hamano On Thu, Sep 27, 2012 at 02:43:38AM -0400, Jeff King wrote: > Ah. The problem is that most of the load comes from my patch 4/3, which > does a separate iteration. Here are the numbers after just patch 3: > > $ time __gitcomp_nl "$refs" > real 0m0.344s > user 0m0.392s > sys 0m0.040s > > Slower, but not nearly as painful. Here are the numbers using sed[1] > instead: > > $ time __gitcomp_nl "$refs" > real 0m0.100s > user 0m0.084s > sys 0m0.016s > > So a little slower, but probably acceptable. We could maybe do the same > trick on the output side (although it is a little trickier there, > because we need it in a bash array). Of course, this is Linux; the fork > for sed is way more expensive on some systems. So something like the patch below does the quoting correctly (try committing a file like "name with spaces" and doing "git show HEAD:<TAB>"), and isn't too much slower: real 0m0.114s user 0m0.108s sys 0m0.004s That's almost double the time without handling quoting, but keep in mind this is on 10,000 entries (and the real sluggishness we are trying to avoid is an order of magnitude). So it might be acceptable. This is just a proof-of-concept patch. We'd probably want to replace the perl below with a more complicated sed invocation for portability (the trickiness is that the output is shown to the user, so we very much don't want to quote anything that does not have to be). diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index be800e0..20c09ef 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -225,6 +225,17 @@ fi fi fi +# Quotes each element of a newline-delimited list for shell reuse +__git_quote() +{ + echo "$1" | + sed " + s/'/'\\\\''/g + s/^/'/ + s/$/'/ + " +} + # Generates completion reply with compgen, appending a space to possible # completion words, if necessary. # It accepts 1 to 4 arguments: @@ -261,7 +272,10 @@ __gitcomp_nl () __gitcomp_nl () { local IFS=$'\n' - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) + COMPREPLY=($( + compgen -P "${2-}" -S "${4- }" -W "$(__git_quote "$1")" -- "${3-$cur}" | + perl -lne '/(.*?)( ?)$/; print quotemeta($1), $2' + )) } __git_heads () -Peff ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 3/3] completion: improve shell expansion of items 2012-09-27 6:43 ` Jeff King 2012-09-27 19:48 ` Jeff King @ 2012-09-28 10:05 ` SZEDER Gábor 2012-09-28 10:09 ` [PATCH 1/5] completion: fix non-critical bugs in __gitcomp() tests SZEDER Gábor 1 sibling, 1 reply; 37+ messages in thread From: SZEDER Gábor @ 2012-09-28 10:05 UTC (permalink / raw) To: Jeff King; +Cc: Felipe Contreras, git, Junio C Hamano On Thu, Sep 27, 2012 at 02:43:38AM -0400, Jeff King wrote: > Here are the numbers using sed[1] > instead: > -# Quotes each element of an IFS-delimited list for shell reuse > +# Quotes each element of a newline-delimited list for shell reuse > __git_quote() > { > - local i > - local delim > - for i in $1; do > - local quoted=${i//\'/\'\\\'\'} > - printf "${delim:+$IFS}'%s'" "$quoted" > - delim=t > - done > + echo "$1" | > + sed " > + s/'/'\\\\''/g > + s/^/'/ > + s/$/'/ > + " > } > > # Generates completion reply with compgen, appending a space to possible Your usage of sed got me thinking and come up with this. The first two fix benign bugs in completion tests, and the third adds tests for __gitcomp_nl(). These are good to go. The fourth adds __gitcomp() and __gitcomp_nl() tests for this expansion breakage. The expected results might need some adjustments, because they contain special characters unquoted, but I'm tempted to say that a branch called $foo is so rare in practice, that we shouldn't bother. The final one is a proof of concept for inspiration. It gets rid of compgen and its crazy additional expansion replacing it with a small sed script. It needs further work to handle words with whitespaces and special characters. SZEDER Gábor (5): completion: fix non-critical bugs in __gitcomp() tests completion: fix args of run_completion() test helper completion: add tests for the __gitcomp_nl() completion helper function completion: test __gitcomp() and __gitcomp_nl() with expandable words completion: avoid compgen to fix expansion issues in __gitcomp_nl() contrib/completion/git-completion.bash | 6 ++- t/t9902-completion.sh | 91 +++++++++++++++++++++++++++++++--- 2 files changed, 90 insertions(+), 7 deletions(-) ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 1/5] completion: fix non-critical bugs in __gitcomp() tests 2012-09-28 10:05 ` SZEDER Gábor @ 2012-09-28 10:09 ` SZEDER Gábor 2012-09-28 10:09 ` [PATCH 2/5] completion: fix args of run_completion() test helper SZEDER Gábor ` (3 more replies) 0 siblings, 4 replies; 37+ messages in thread From: SZEDER Gábor @ 2012-09-28 10:09 UTC (permalink / raw) To: Jeff King; +Cc: Felipe Contreras, Junio C Hamano, git, SZEDER Gábor When invoking __gitcomp() the $cur variable should hold the word to be completed, but two tests (checking the handling of prefix and suffix) set it to a bogus value. Luckily the bogus values haven't influenced the tests' correctness, because these two tests pass the word-to-be-matched as argument to __gitcomp(), which overrides $cur anyway. Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> --- t/t9902-completion.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 92d7eb47..e7657537 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -121,7 +121,7 @@ test_expect_success '__gitcomp - prefix' ' EOF ( local -a COMPREPLY && - cur="branch.me" && + cur="branch.maint.me" && __gitcomp "remote merge mergeoptions rebase " "branch.maint." "me" && IFS="$newline" && @@ -137,7 +137,7 @@ test_expect_success '__gitcomp - suffix' ' EOF ( local -a COMPREPLY && - cur="branch.me" && + cur="branch.ma" && __gitcomp "master maint next pu " "branch." "ma" "." && IFS="$newline" && -- 1.7.12.1.438.g7dfa67b ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 2/5] completion: fix args of run_completion() test helper 2012-09-28 10:09 ` [PATCH 1/5] completion: fix non-critical bugs in __gitcomp() tests SZEDER Gábor @ 2012-09-28 10:09 ` SZEDER Gábor 2012-09-28 18:04 ` Junio C Hamano 2012-09-28 10:09 ` [PATCH 3/5] completion: add tests for the __gitcomp_nl() completion helper function SZEDER Gábor ` (2 subsequent siblings) 3 siblings, 1 reply; 37+ messages in thread From: SZEDER Gábor @ 2012-09-28 10:09 UTC (permalink / raw) To: Jeff King; +Cc: Felipe Contreras, Junio C Hamano, git, SZEDER Gábor To simulate the the user hit 'git <TAB>, one of the completion tests sets up the rather strange command line git "" i.e. the second word on the command line consists of two double quotes. However, this is not what happens for real, because after 'git <TAB>' the second word on the command line is just an empty string. Luckily, the test works nevertheless. Fix this by passing the command line to run_completion() as separate words. Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> --- t/t9902-completion.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index e7657537..f5e68834 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -49,7 +49,7 @@ run_completion () { local -a COMPREPLY _words local _cword - _words=( $1 ) + _words=( "$@" ) (( _cword = ${#_words[@]} - 1 )) __git_wrap__git_main && print_comp } @@ -57,7 +57,7 @@ run_completion () test_completion () { test $# -gt 1 && echo "$2" > expected - run_completion "$@" && + run_completion $1 && test_cmp expected out } @@ -147,7 +147,7 @@ test_expect_success '__gitcomp - suffix' ' ' test_expect_success 'basic' ' - run_completion "git \"\"" && + run_completion git "" && # built-in grep -q "^add \$" out && # script @@ -155,7 +155,7 @@ test_expect_success 'basic' ' # plumbing ! grep -q "^ls-files \$" out && - run_completion "git f" && + run_completion git f && ! grep -q -v "^f" out ' -- 1.7.12.1.438.g7dfa67b ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 2/5] completion: fix args of run_completion() test helper 2012-09-28 10:09 ` [PATCH 2/5] completion: fix args of run_completion() test helper SZEDER Gábor @ 2012-09-28 18:04 ` Junio C Hamano 2012-09-28 18:38 ` SZEDER Gábor 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2012-09-28 18:04 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Jeff King, Felipe Contreras, git SZEDER Gábor <szeder@ira.uka.de> writes: > To simulate the the user hit 'git <TAB>, one of the completion tests > sets up the rather strange command line > > git "" > > i.e. the second word on the command line consists of two double > quotes. However, this is not what happens for real, because after > 'git <TAB>' the second word on the command line is just an empty > string. Luckily, the test works nevertheless. > > Fix this by passing the command line to run_completion() as separate > words. > > Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> > --- > t/t9902-completion.sh | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index e7657537..f5e68834 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -49,7 +49,7 @@ run_completion () > { > local -a COMPREPLY _words > local _cword > - _words=( $1 ) > + _words=( "$@" ) > (( _cword = ${#_words[@]} - 1 )) > __git_wrap__git_main && print_comp > } > @@ -57,7 +57,7 @@ run_completion () > test_completion () > { > test $# -gt 1 && echo "$2" > expected > - run_completion "$@" && > + run_completion $1 && > test_cmp expected out > } I can understand the other three hunks, but this one is fishy. Shouldn't "$1" be inside a pair of dq? I.e. + run_completion "$1" && > > @@ -147,7 +147,7 @@ test_expect_success '__gitcomp - suffix' ' > ' > > test_expect_success 'basic' ' > - run_completion "git \"\"" && > + run_completion git "" && > # built-in > grep -q "^add \$" out && > # script > @@ -155,7 +155,7 @@ test_expect_success 'basic' ' > # plumbing > ! grep -q "^ls-files \$" out && > > - run_completion "git f" && > + run_completion git f && > ! grep -q -v "^f" out > ' ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/5] completion: fix args of run_completion() test helper 2012-09-28 18:04 ` Junio C Hamano @ 2012-09-28 18:38 ` SZEDER Gábor 2012-09-28 19:23 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: SZEDER Gábor @ 2012-09-28 18:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Felipe Contreras, git On Fri, Sep 28, 2012 at 11:04:05AM -0700, Junio C Hamano wrote: > SZEDER Gábor <szeder@ira.uka.de> writes: > > > To simulate the the user hit 'git <TAB>, one of the completion tests s/the the/that the/ > > sets up the rather strange command line > > > > git "" > > > > i.e. the second word on the command line consists of two double > > quotes. However, this is not what happens for real, because after > > 'git <TAB>' the second word on the command line is just an empty > > string. Luckily, the test works nevertheless. > > > > Fix this by passing the command line to run_completion() as separate > > words. > > > > Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> > > --- > > t/t9902-completion.sh | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > > index e7657537..f5e68834 100755 > > --- a/t/t9902-completion.sh > > +++ b/t/t9902-completion.sh > > @@ -49,7 +49,7 @@ run_completion () > > { > > local -a COMPREPLY _words > > local _cword > > - _words=( $1 ) > > + _words=( "$@" ) > > (( _cword = ${#_words[@]} - 1 )) > > __git_wrap__git_main && print_comp > > } > > @@ -57,7 +57,7 @@ run_completion () > > test_completion () > > { > > test $# -gt 1 && echo "$2" > expected > > - run_completion "$@" && > > + run_completion $1 && > > test_cmp expected out > > } > > I can understand the other three hunks, but this one is fishy. > Shouldn't "$1" be inside a pair of dq? I.e. > > + run_completion "$1" && No. $1 holds all words on the command line. If it was between a pair of dq, then the whole command line would be passed to the completion script as a single word. > > > > @@ -147,7 +147,7 @@ test_expect_success '__gitcomp - suffix' ' > > ' > > > > test_expect_success 'basic' ' > > - run_completion "git \"\"" && > > + run_completion git "" && > > # built-in > > grep -q "^add \$" out && > > # script > > @@ -155,7 +155,7 @@ test_expect_success 'basic' ' > > # plumbing > > ! grep -q "^ls-files \$" out && > > > > - run_completion "git f" && > > + run_completion git f && > > ! grep -q -v "^f" out > > ' ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/5] completion: fix args of run_completion() test helper 2012-09-28 18:38 ` SZEDER Gábor @ 2012-09-28 19:23 ` Junio C Hamano 2012-09-28 19:30 ` Jeff King 2012-09-28 20:28 ` SZEDER Gábor 0 siblings, 2 replies; 37+ messages in thread From: Junio C Hamano @ 2012-09-28 19:23 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Jeff King, Felipe Contreras, git SZEDER Gábor <szeder@ira.uka.de> writes: > On Fri, Sep 28, 2012 at 11:04:05AM -0700, Junio C Hamano wrote: >> SZEDER Gábor <szeder@ira.uka.de> writes: >> >> > To simulate the the user hit 'git <TAB>, one of the completion tests > > s/the the/that the/ > >> > sets up the rather strange command line >> > >> > git "" >> > >> > i.e. the second word on the command line consists of two double >> > quotes. However, this is not what happens for real, because after >> > 'git <TAB>' the second word on the command line is just an empty >> > string. Luckily, the test works nevertheless. >> > >> > Fix this by passing the command line to run_completion() as separate >> > words. >> > >> > Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> >> > --- >> > t/t9902-completion.sh | 8 ++++---- >> > 1 file changed, 4 insertions(+), 4 deletions(-) >> > >> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >> > index e7657537..f5e68834 100755 >> > --- a/t/t9902-completion.sh >> > +++ b/t/t9902-completion.sh >> > @@ -49,7 +49,7 @@ run_completion () >> > { >> > local -a COMPREPLY _words >> > local _cword >> > - _words=( $1 ) >> > + _words=( "$@" ) >> > (( _cword = ${#_words[@]} - 1 )) >> > __git_wrap__git_main && print_comp >> > } >> > @@ -57,7 +57,7 @@ run_completion () >> > test_completion () >> > { >> > test $# -gt 1 && echo "$2" > expected >> > - run_completion "$@" && >> > + run_completion $1 && >> > test_cmp expected out >> > } >> >> I can understand the other three hunks, but this one is fishy. >> Shouldn't "$1" be inside a pair of dq? I.e. >> >> + run_completion "$1" && > > No. $1 holds all words on the command line. If it was between a pair > of dq, then the whole command line would be passed to the completion > script as a single word. And these "words" can be split at $IFS boundaries without any issues? IOW, nobody would ever want to make words array in the run_completion function to ['git' 'foo bar' 'baz']? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/5] completion: fix args of run_completion() test helper 2012-09-28 19:23 ` Junio C Hamano @ 2012-09-28 19:30 ` Jeff King 2012-09-28 19:49 ` Junio C Hamano 2012-09-28 20:28 ` SZEDER Gábor 1 sibling, 1 reply; 37+ messages in thread From: Jeff King @ 2012-09-28 19:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: SZEDER Gábor, Felipe Contreras, git On Fri, Sep 28, 2012 at 12:23:47PM -0700, Junio C Hamano wrote: > >> > @@ -57,7 +57,7 @@ run_completion () > >> > test_completion () > >> > { > >> > test $# -gt 1 && echo "$2" > expected > >> > - run_completion "$@" && > >> > + run_completion $1 && > >> > test_cmp expected out > >> > } > >> > >> I can understand the other three hunks, but this one is fishy. > >> Shouldn't "$1" be inside a pair of dq? I.e. > >> > >> + run_completion "$1" && > > > > No. $1 holds all words on the command line. If it was between a pair > > of dq, then the whole command line would be passed to the completion > > script as a single word. > > And these "words" can be split at $IFS boundaries without any > issues? IOW, nobody would ever want to make words array in the > run_completion function to ['git' 'foo bar' 'baz']? It might be simpler to just convert test_completion into the test_completion_long I added in my series; the latter takes the expected output on stdin, leaving the actual arguments free to represent the real command-line. E.g., your example would become: test_completion git "foo bar" baz <<-\EOF ... expected output ... EOF -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/5] completion: fix args of run_completion() test helper 2012-09-28 19:30 ` Jeff King @ 2012-09-28 19:49 ` Junio C Hamano 2012-09-28 19:55 ` Jeff King 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2012-09-28 19:49 UTC (permalink / raw) To: Jeff King; +Cc: SZEDER Gábor, Felipe Contreras, git Jeff King <peff@peff.net> writes: > On Fri, Sep 28, 2012 at 12:23:47PM -0700, Junio C Hamano wrote: > >> >> > @@ -57,7 +57,7 @@ run_completion () >> >> > test_completion () >> >> > { >> >> > test $# -gt 1 && echo "$2" > expected >> >> > - run_completion "$@" && >> >> > + run_completion $1 && >> >> > test_cmp expected out >> >> > } >> >> >> >> I can understand the other three hunks, but this one is fishy. >> >> Shouldn't "$1" be inside a pair of dq? I.e. >> >> >> >> + run_completion "$1" && >> > >> > No. $1 holds all words on the command line. If it was between a pair >> > of dq, then the whole command line would be passed to the completion >> > script as a single word. >> >> And these "words" can be split at $IFS boundaries without any >> issues? IOW, nobody would ever want to make words array in the >> run_completion function to ['git' 'foo bar' 'baz']? > > It might be simpler to just convert test_completion into the > test_completion_long I added in my series; the latter takes the expected > output on stdin, leaving the actual arguments free to represent the real > command-line. E.g., your example would become: > > test_completion git "foo bar" baz <<-\EOF > ... expected output ... > EOF I realize that the way my question was stated was misleading. It was not meant as a rhetorical "You would never be able to pass ['git' 'foo bar' 'baz'] with that interface, and the patch sucks." but was meant as a pure question "Do we want to pass such word list?". "test_completion is almost always used to test completion with inputs without any $IFS letters in it, so not being able to test such an input via this interface is fine. If needed, we can give another less often used interface to let you pass such an input" is perfectly fine by me. But I suspect that the real reason test_completion requires the caller to express the list of inputs to run_completion as $IFS separate list is because it needs to also get expected from the command line: >> > test_completion () >> > { >> > test $# -gt 1 && echo "$2" > expected >> > - run_completion "$@" && >> > + run_completion $1 && >> > test_cmp expected out >> > } I wonder if doing something like this would be a far simpler solution: test_completion () { case "$1" in '') ;; *) echo "$1" >expect && shift ;; esac && run_completion "$@" && test_cmp expect output } ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/5] completion: fix args of run_completion() test helper 2012-09-28 19:49 ` Junio C Hamano @ 2012-09-28 19:55 ` Jeff King 0 siblings, 0 replies; 37+ messages in thread From: Jeff King @ 2012-09-28 19:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: SZEDER Gábor, Felipe Contreras, git On Fri, Sep 28, 2012 at 12:49:25PM -0700, Junio C Hamano wrote: > >> And these "words" can be split at $IFS boundaries without any > >> issues? IOW, nobody would ever want to make words array in the > >> run_completion function to ['git' 'foo bar' 'baz']? > > > > It might be simpler to just convert test_completion into the > > test_completion_long I added in my series; the latter takes the expected > > output on stdin, leaving the actual arguments free to represent the real > > command-line. E.g., your example would become: > > > > test_completion git "foo bar" baz <<-\EOF > > ... expected output ... > > EOF > > I realize that the way my question was stated was misleading. It > was not meant as a rhetorical "You would never be able to pass > ['git' 'foo bar' 'baz'] with that interface, and the patch sucks." > but was meant as a pure question "Do we want to pass such word > list?". "test_completion is almost always used to test completion > with inputs without any $IFS letters in it, so not being able to > test such an input via this interface is fine. If needed, we can > give another less often used interface to let you pass such an > input" is perfectly fine by me. I think we may eventually want to pass arguments with IFS into the function, just to make sure it works (the tests I added checked for IFS in the completion list rather than the input, but we should probably check both). I'm OK if it needs to be an alternate interface (right now you could do it by calling run_completion yourself). > But I suspect that the real reason test_completion requires the > caller to express the list of inputs to run_completion as $IFS > separate list is because it needs to also get expected from the > command line: Right, that's why I suggested bumping that to stdin for the function. > >> > test_completion () > >> > { > >> > test $# -gt 1 && echo "$2" > expected > >> > - run_completion "$@" && > >> > + run_completion $1 && > >> > test_cmp expected out > >> > } > > I wonder if doing something like this would be a far simpler > solution: > > test_completion () > { > case "$1" in > '') > ;; > *) > echo "$1" >expect && > shift > ;; > esac && > run_completion "$@" && > test_cmp expect output > } That would also work. I mainly suggested the stdin thing because we need it anyway for output that generates multiple answers (well, you don't _need_ it; you can call run_completion yourself, but it saves a few lines at each call site). -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/5] completion: fix args of run_completion() test helper 2012-09-28 19:23 ` Junio C Hamano 2012-09-28 19:30 ` Jeff King @ 2012-09-28 20:28 ` SZEDER Gábor 1 sibling, 0 replies; 37+ messages in thread From: SZEDER Gábor @ 2012-09-28 20:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Felipe Contreras, git On Fri, Sep 28, 2012 at 12:23:47PM -0700, Junio C Hamano wrote: > SZEDER Gábor <szeder@ira.uka.de> writes: > > > On Fri, Sep 28, 2012 at 11:04:05AM -0700, Junio C Hamano wrote: > >> SZEDER Gábor <szeder@ira.uka.de> writes: > >> > >> > To simulate the the user hit 'git <TAB>, one of the completion tests > > > > s/the the/that the/ > > > >> > sets up the rather strange command line > >> > > >> > git "" > >> > > >> > i.e. the second word on the command line consists of two double > >> > quotes. However, this is not what happens for real, because after > >> > 'git <TAB>' the second word on the command line is just an empty > >> > string. Luckily, the test works nevertheless. > >> > > >> > Fix this by passing the command line to run_completion() as separate > >> > words. > >> > > >> > Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> > >> > --- > >> > t/t9902-completion.sh | 8 ++++---- > >> > 1 file changed, 4 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > >> > index e7657537..f5e68834 100755 > >> > --- a/t/t9902-completion.sh > >> > +++ b/t/t9902-completion.sh > >> > @@ -49,7 +49,7 @@ run_completion () > >> > { > >> > local -a COMPREPLY _words > >> > local _cword > >> > - _words=( $1 ) > >> > + _words=( "$@" ) > >> > (( _cword = ${#_words[@]} - 1 )) > >> > __git_wrap__git_main && print_comp > >> > } > >> > @@ -57,7 +57,7 @@ run_completion () > >> > test_completion () > >> > { > >> > test $# -gt 1 && echo "$2" > expected > >> > - run_completion "$@" && > >> > + run_completion $1 && > >> > test_cmp expected out > >> > } > >> > >> I can understand the other three hunks, but this one is fishy. > >> Shouldn't "$1" be inside a pair of dq? I.e. > >> > >> + run_completion "$1" && > > > > No. $1 holds all words on the command line. If it was between a pair > > of dq, then the whole command line would be passed to the completion > > script as a single word. > > And these "words" can be split at $IFS boundaries without any > issues? IOW, nobody would ever want to make words array in the > run_completion function to ['git' 'foo bar' 'baz']? Maybe we would, but making it work would be a separate fix. You can't do that with the current version either. ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 3/5] completion: add tests for the __gitcomp_nl() completion helper function 2012-09-28 10:09 ` [PATCH 1/5] completion: fix non-critical bugs in __gitcomp() tests SZEDER Gábor 2012-09-28 10:09 ` [PATCH 2/5] completion: fix args of run_completion() test helper SZEDER Gábor @ 2012-09-28 10:09 ` SZEDER Gábor 2012-09-28 10:09 ` [PATCH 4/5] completion: test __gitcomp() and __gitcomp_nl() with expandable words SZEDER Gábor 2012-09-28 10:09 ` [POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl() SZEDER Gábor 3 siblings, 0 replies; 37+ messages in thread From: SZEDER Gábor @ 2012-09-28 10:09 UTC (permalink / raw) To: Jeff King; +Cc: Felipe Contreras, Junio C Hamano, git, SZEDER Gábor Test __gitcomp_nl()'s basic functionality, i.e. that trailing space, prefix, and suffix are added correctly. Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> --- t/t9902-completion.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index f5e68834..01f33220 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -146,6 +146,53 @@ test_expect_success '__gitcomp - suffix' ' test_cmp expected out ' +test_expect_success '__gitcomp_nl - trailing space' ' + sed -e "s/Z$//" >expected <<-\EOF && + maint Z + master Z + EOF + ( + local -a COMPREPLY && + cur="m" && + __gitcomp_nl "maint${newline}master${newline}pu" && + IFS="$newline" && + echo "${COMPREPLY[*]}" > out + ) && + test_cmp expected out +' + +test_expect_success '__gitcomp_nl - prefix' ' + sed -e "s/Z$//" >expected <<-\EOF && + --fixup=maint Z + --fixup=master Z + EOF + ( + local -a COMPREPLY && + cur="--fixup=m" && + __gitcomp_nl "maint${newline}master${newline}next${newline}pu"\ + "--fixup=" "m" && + IFS="$newline" && + echo "${COMPREPLY[*]}" > out + ) && + test_cmp expected out +' + +test_expect_success '__gitcomp_nl - suffix' ' + sed -e "s/Z$//" >expected <<-\EOF && + branch.master.Z + branch.maint.Z + EOF + ( + local -a COMPREPLY && + cur="branch.ma" && + __gitcomp_nl "master${newline}maint${newline}next${newline}pu"\ + "branch." "ma" "." && + IFS="$newline" && + echo "${COMPREPLY[*]}" > out + ) && + test_cmp expected out +' + test_expect_success 'basic' ' run_completion git "" && # built-in -- 1.7.12.1.438.g7dfa67b ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 4/5] completion: test __gitcomp() and __gitcomp_nl() with expandable words 2012-09-28 10:09 ` [PATCH 1/5] completion: fix non-critical bugs in __gitcomp() tests SZEDER Gábor 2012-09-28 10:09 ` [PATCH 2/5] completion: fix args of run_completion() test helper SZEDER Gábor 2012-09-28 10:09 ` [PATCH 3/5] completion: add tests for the __gitcomp_nl() completion helper function SZEDER Gábor @ 2012-09-28 10:09 ` SZEDER Gábor 2012-09-28 10:09 ` [POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl() SZEDER Gábor 3 siblings, 0 replies; 37+ messages in thread From: SZEDER Gábor @ 2012-09-28 10:09 UTC (permalink / raw) To: Jeff King; +Cc: Felipe Contreras, Junio C Hamano, git, SZEDER Gábor compgen performs expansion on all words in the list given to its -W option. This breaks completion in various ways if one of those words can be expanded. The funniest breakage is probably this, as the command_not_found_handle kicks in: $ git branch '$(foo)' $ git branch <TAB>No command 'foo' found, did you mean: Command 'fio' from package 'fio' (universe) Command 'goo' from package 'goo' (universe) Command 'fop' from package 'fop' (main) Command 'fox' from package 'objcryst-fox' (universe) Command 'xoo' from package 'xoo' (universe) Command 'zoo' from package 'zoo' (universe) foo: command not found Document this breakage with tests. Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> --- t/t9902-completion.sh | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 01f33220..4af2a149 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -146,6 +146,22 @@ test_expect_success '__gitcomp - suffix' ' test_cmp expected out ' +test_expect_failure '__gitcomp - expandable words' ' + sed -e "s/Z$//" >expected <<-\EOF && + $foo Z + $(bar) Z + ${baz} Z + EOF + ( + local -a COMPREPLY && + cur="" && + __gitcomp "\$foo \$(bar) \${baz}" && + IFS="$newline" && + echo "${COMPREPLY[*]}" > out + ) && + test_cmp expected out +' + test_expect_success '__gitcomp_nl - trailing space' ' sed -e "s/Z$//" >expected <<-\EOF && maint Z @@ -193,6 +209,22 @@ test_expect_success '__gitcomp_nl - suffix' ' test_cmp expected out ' +test_expect_failure '__gitcomp_nl - expandable words' ' + sed -e "s/Z$//" >expected <<-\EOF && + $foo Z + $(bar) Z + ${baz} Z + EOF + ( + local -a COMPREPLY && + cur="" && + __gitcomp_nl "\$foo$newline\$(bar)$newline\${baz}" && + IFS="$newline" && + echo "${COMPREPLY[*]}" > out + ) && + test_cmp expected out +' + test_expect_success 'basic' ' run_completion git "" && # built-in -- 1.7.12.1.438.g7dfa67b ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl() 2012-09-28 10:09 ` [PATCH 1/5] completion: fix non-critical bugs in __gitcomp() tests SZEDER Gábor ` (2 preceding siblings ...) 2012-09-28 10:09 ` [PATCH 4/5] completion: test __gitcomp() and __gitcomp_nl() with expandable words SZEDER Gábor @ 2012-09-28 10:09 ` SZEDER Gábor 2012-09-28 15:08 ` SZEDER Gábor 3 siblings, 1 reply; 37+ messages in thread From: SZEDER Gábor @ 2012-09-28 10:09 UTC (permalink / raw) To: Jeff King; +Cc: Felipe Contreras, Junio C Hamano, git, SZEDER Gábor compgen performs expansion on all words in the list given to its -W option. This breaks completion in various ways if one of those words can be expanded, as demonstrated by failing tests added in a previous commit. In __gitcomp_nl() we use only a small subset of compgen's functionality: to filter matching possible completion words, and to add a prefix and/or suffix to each of them. Since the list of words is newline-separated, we can do all these just as well with a little sed script. This way we can get rid of compgen and its additional expansion of all words and don't need to perform excessive quoting to circumvent it. Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> --- We can also get rid of compgen in __gitcomp(); I already have working code for that, but it still needs a bit of cleanup and commit messages. contrib/completion/git-completion.bash | 6 +++++- t/t9902-completion.sh | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index be800e09..2df865fd 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -261,7 +261,11 @@ __gitcomp () __gitcomp_nl () { local IFS=$'\n' - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) + COMPREPLY=($(echo "$1" |sed -n "/^${3-$cur}/ { + s/^/${2-}/ + s/$/${4- }/ + p + }")) } __git_heads () diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 4af2a149..0ee91f64 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -209,7 +209,7 @@ test_expect_success '__gitcomp_nl - suffix' ' test_cmp expected out ' -test_expect_failure '__gitcomp_nl - expandable words' ' +test_expect_success '__gitcomp_nl - expandable words' ' sed -e "s/Z$//" >expected <<-\EOF && $foo Z $(bar) Z -- 1.7.12.1.438.g7dfa67b ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl() 2012-09-28 10:09 ` [POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl() SZEDER Gábor @ 2012-09-28 15:08 ` SZEDER Gábor 2012-09-28 15:46 ` Jeff King 0 siblings, 1 reply; 37+ messages in thread From: SZEDER Gábor @ 2012-09-28 15:08 UTC (permalink / raw) To: Jeff King; +Cc: Felipe Contreras, Junio C Hamano, git On Fri, Sep 28, 2012 at 12:09:35PM +0200, SZEDER Gábor wrote: > __gitcomp_nl () > { > local IFS=$'\n' > - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) > + COMPREPLY=($(echo "$1" |sed -n "/^${3-$cur}/ { $cur can be a path, so it can contain /, which then breaks this sed expression. Here's a fixup: diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 2df865fd..d30f376f 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -260,8 +260,8 @@ __gitcomp () # appended. __gitcomp_nl () { - local IFS=$'\n' - COMPREPLY=($(echo "$1" |sed -n "/^${3-$cur}/ { + local IFS=$'\n' cur_=${3-$cur} + COMPREPLY=($(echo "$1" |sed -n "/^${cur_//\//\\/}/ { s/^/${2-}/ s/$/${4- }/ p -- 1.7.12.1.490.g14283db ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl() 2012-09-28 15:08 ` SZEDER Gábor @ 2012-09-28 15:46 ` Jeff King 0 siblings, 0 replies; 37+ messages in thread From: Jeff King @ 2012-09-28 15:46 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Felipe Contreras, Junio C Hamano, git On Fri, Sep 28, 2012 at 05:08:52PM +0200, SZEDER Gábor wrote: > On Fri, Sep 28, 2012 at 12:09:35PM +0200, SZEDER Gábor wrote: > > __gitcomp_nl () > > { > > local IFS=$'\n' > > - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) > > + COMPREPLY=($(echo "$1" |sed -n "/^${3-$cur}/ { > > $cur can be a path, so it can contain /, which then breaks this sed > expression. Here's a fixup: Worse than that, can't $cur contain arbitrary regex characters that will be interpreted by sed? -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2012-09-28 20:29 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-09-20 2:15 [PATCH] completion: fix shell expansion of items Felipe Contreras 2012-09-20 1:46 ` Jeff King 2012-09-20 16:52 ` Junio C Hamano 2012-09-20 18:11 ` SZEDER Gábor 2012-09-20 18:21 ` Jeff King 2012-09-20 19:38 ` Felipe Contreras 2012-09-25 4:31 ` [PATCH] Revert "completion: fix shell expansion of items" Jeff King 2012-09-25 16:03 ` Junio C Hamano 2012-09-25 22:37 ` SZEDER Gábor 2012-09-25 23:28 ` Junio C Hamano 2012-09-26 21:46 ` Jeff King 2012-09-26 21:47 ` [PATCH 1/3] t9902: add a few basic completion tests Jeff King 2012-09-26 21:51 ` [PATCH 2/3] t9902: add completion tests for "odd" filenames Jeff King 2012-09-26 21:51 ` [PATCH 3/3] completion: improve shell expansion of items Jeff King 2012-09-26 21:57 ` [PATCH 4/3] completion: quote completions we find Jeff King 2012-09-27 21:40 ` SZEDER Gábor 2012-09-27 22:31 ` Junio C Hamano 2012-09-27 22:58 ` SZEDER Gábor 2012-09-27 0:37 ` [PATCH 3/3] completion: improve shell expansion of items SZEDER Gábor 2012-09-27 6:28 ` Jeff King 2012-09-27 6:43 ` Jeff King 2012-09-27 19:48 ` Jeff King 2012-09-28 10:05 ` SZEDER Gábor 2012-09-28 10:09 ` [PATCH 1/5] completion: fix non-critical bugs in __gitcomp() tests SZEDER Gábor 2012-09-28 10:09 ` [PATCH 2/5] completion: fix args of run_completion() test helper SZEDER Gábor 2012-09-28 18:04 ` Junio C Hamano 2012-09-28 18:38 ` SZEDER Gábor 2012-09-28 19:23 ` Junio C Hamano 2012-09-28 19:30 ` Jeff King 2012-09-28 19:49 ` Junio C Hamano 2012-09-28 19:55 ` Jeff King 2012-09-28 20:28 ` SZEDER Gábor 2012-09-28 10:09 ` [PATCH 3/5] completion: add tests for the __gitcomp_nl() completion helper function SZEDER Gábor 2012-09-28 10:09 ` [PATCH 4/5] completion: test __gitcomp() and __gitcomp_nl() with expandable words SZEDER Gábor 2012-09-28 10:09 ` [POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl() SZEDER Gábor 2012-09-28 15:08 ` SZEDER Gábor 2012-09-28 15:46 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).