patch, 7 partscompletion: fix expansion issues with compgen

This series fixes the expansion issues with compgen reported by Jeroen Meijer a while ago in

  http://article.gmane.org/gmane.comp.version-control.git/201596

The problem is that the compgen Bash-builtin performs expansion on all words in the wordlist given to its -W option, breaking Git's completion script when e.g. refs or filenames contain expandable substrings. Since compgen has no option to turn off this expansion and we only use a small subset of compgen's functionality, this series replaces compgen in __gitcomp() and __gitcomp_nl() by some shell logic and a small awk script, respectively.

Patches 1 and 2 are test enhancements and fixes, while 3 and 4 add new tests to make sure I don't break anything and to demonstrate the issues, respectively. The actual bugfixes are in patches 5 and 6. Finally, patch 7 is a cleanup made possible by patch 6 (could be squashed into 6).

In the future we might want/need to fill COMPREPLY directly when completing a path in the <tree>:<path> notation instead using __gitcomp_nl() there, because paths can contain newlines, too.

Compared to Felipe's series from yesterday, this series has more tests, more descriptive commit messages, and it's faster. However, it doesn't include his 1/5 (completion: get rid of empty COMPREPLY assignments).

Footnote: check the patchlist below, and notice how format-patch put
patches 4 and 5 into the same line.

SZEDER Gábor (7):
  completion: make the 'basic' test more tester-friendly
  completion: fix args of run_completion() test helper
  completion: add tests for the __gitcomp_nl() completion helper
    function
  completion: add tests for invalid variable name among completion words  completion: fix expansion issues in __gitcomp_nl()
  completion: fix expansion issue in __gitcomp()
  completion: remove the now unused __gitcomp_1() internal helper
    function

 contrib/completion/git-completion.bash |  57 ++++++++-------
 t/t9902-completion.sh                  | 123 ++++++++++++++++++++++++++++++---
 2 files changed, 147 insertions(+), 33 deletions(-)

The 'basic' test uses 'grep -q' to filter the resulting possible completion words while looking for the presence or absence of certain git commands, and relies on grep's exit status to indicate a failure.

This works fine as long as there are no errors. However, in case of a failure it doesn't give any indication whatsoever about the reason of the failure, i.e. which condition failed.

To make testers' life easier provide some output about the failed condition: store the results of the filtering in a file and compare its contents to the expected results by the good old test_cmp helper. However, to actually get output from test_cmp in case of an error we must make sure that test_cmp is always executed. Since in case of an error grep's exit code aborts the test immediately, before running the subsequent test_cmp, do the filtering using sed instead of grep.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 t/t9902-completion.sh | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

SZEDER Gábor wrote:

[...]

Looks good. I wonder if this points to the need for a test_grep helper more generally.

[...]

Functional change: before, this would fail if "out" contained a blank line, while afterward it does not. I doubt that matters, though.

Thanks and hope that helps, Jonathan

On Sun, Nov 18, 2012 at 12:00 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

It does.

On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

It's not very useful to see a single failure without context. Maybe this:

To simulate that the user hit 'git <TAB>, the 'basic' test sets up the rather strange command line containing the two words

  git ""

i.e. the second word on the command line consists of two double quotes. This is not what happens for real, however, 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(-)

SZEDER Gábor wrote:

Good catch. The change makes sense, the code looks saner after the fix, and since this is test code any breakage should be caught quickly, so

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

I don't like this approach. I prefer

run_completion "git "

So that it's similar to how test_completion is called:

run_completion "git f" test_completion "git f"

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 | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

SZEDER Gábor wrote:

'$' is usually a shell metacharacter, so it would be more comfortable to read a version that escapes it:

	sed -e "s/Z\$//" >expected <<-\EOF

Since '$/' is not a valid parameter expansion, if I understand correctly then POSIX leaves the meaning of the quoted string "s/Z$//" undefined (XCU §2.2.3). Luckily every shell I've tried, including bash, keeps the $ unmolested.

Other parts of the file already use the same style, so I wouldn't suggest changing it in this patch.

Thanks for some nice tests.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

Too much code that can be simplified:

The compgen Bash-builtin performs expansion on all words in the wordlist given to its -W option, breaking Git's completion script in various ways if one of those words can be expanded. The breakage can be simply bogus possible completion words, but it can also be a failure:

  $ git branch '${foo.bar}'
  $ git checkout <TAB>
  bash: ${foo.bar}: bad substitution

${foo.bar} is an invalid variable name, which triggers the failure when compgen attempts to expand it, completely breaking refs completion. The same applies to e.g. completing the <tree>:<path> notation when a filename contains a similar expandable substring.

Since both __gitcomp() and __gitcomp_nl() rely on compgen, both are affected by this issue. So add a simple test for each of them to check that such a word doesn't cause failures (but don't check the resulting possible completion words for now, because that should be quoted properly, and that's a separate topic).

Reported-by: Jeroen Meijer <jjgmeijer@hotmail.com>
Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 t/t9902-completion.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

SZEDER Gábor wrote:

Or arbitrary code execution:

	$ git branch '$(>kilroy-was-here)'
	$ git checkout <TAB>
	$ ls -l kilroy-was-here
	-rw-rw-r-- 1 jrn jrn 0 nov 17 15:40 kilroy-was-here

The final version of this patch should go to maint. Thanks for catching it.

On Sun, Nov 18, 2012 at 12:40 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Shouldn't this go to the commit message?

On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

Why in a subshell?

The compgen Bash-builtin performs expansion on all words in the wordlist given to its -W option, breaking Git's completion script when refs or filenames passed to __gitcomp_nl() contain expandable substrings. At least one user can't use ref completion at all in a repository, which contains tags with metacharacters like

  Build%20V%20${bamboo.custom.jiraversion.name}%20Build%20721

Such a ref causes a failure in compgen as it tries to expand the variable with invalid name.

Unfortunately, compgen has no option to turn off this expansion. However, in __gitcomp_nl() we use only a small subset of compgen's functionality, namely the filtering of matching possible completion words and adding a prefix and/or suffix to each of those words. So, to avoid compgen's problematic expansion we get rid of compgen, and implement the equivalent functionality in a small awk script. The reason for using awk instead of sed is that awk allows plain (i.e. non-regexp) string comparison, making quoting of regexp characters in the current word unnecessary.

Note, that such expandable words need quoting to be of any use on the command line. This patch doesn't perform that quoting, but it could be implemented later on top by extending the awk script to unquote $cur and to quote matching words. Nonetheless, this patch still a step forward, because at least refs can be completed even in the presence of one with metacharacters.

Also update the function's description a bit.

Reported-by: Jeroen Meijer <jjgmeijer@hotmail.com>
Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

awk doesn't have a prefixcmp(). I'm no awk wizard, so I'm not sure this is the right way to do it.

 contrib/completion/git-completion.bash | 17 +++++++++++++----
 t/t9902-completion.sh                  |  4 ++--
 2 files changed, 15 insertions(+), 6 deletions(-)

On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

Does this really perform better than my alternative?

+       for x in $1; do
+               if [[ "$x" = "$3"* ]]; then
+                       COMPREPLY+=("$2$x$4")
+               fi
+       done

On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:

It does:

  My version:

    $ refs="$(for i in {0..9999} ; do echo branch$i ; done)"
    $ time __gitcomp_nl "$refs"

    real    0m0.109s
    user    0m0.096s
    sys     0m0.012s

  Yours:

    $ time __gitcomp_nl "$refs"

    real    0m0.321s
    user    0m0.312s
    sys     0m0.008s

On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

Yeah, for 10000 refs, which is not the common case:

== 1 == SZEDER: real 0m0.007s user 0m0.003s sys 0m0.000s felipec: real 0m0.000s user 0m0.000s sys 0m0.000s == 10 == SZEDER: real 0m0.004s user 0m0.003s sys 0m0.001s felipec: real 0m0.000s user 0m0.000s sys 0m0.000s == 100 == SZEDER: real 0m0.005s user 0m0.002s sys 0m0.002s felipec: real 0m0.002s user 0m0.002s sys 0m0.000s == 1000 == SZEDER: real 0m0.010s user 0m0.008s sys 0m0.001s felipec: real 0m0.018s user 0m0.017s sys 0m0.001s == 10000 == SZEDER: real 0m0.062s user 0m0.060s sys 0m0.003s felipec: real 0m0.175s user 0m0.174s sys 0m0.000s == 100000 == SZEDER: real 0m0.595s user 0m0.593s sys 0m0.021s felipec: real 0m1.848s user 0m1.843s sys 0m0.003s == 1000000 == SZEDER: real 0m6.258s user 0m6.241s sys 0m0.215s felipec: real 0m18.191s user 0m18.115s sys 0m0.045s

On Sat, Nov 17, 2012 at 8:08 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

And this beats both in every case:

while read -r x; do
	if [[ "$x" == "$3"* ]]; then
		COMPREPLY+=("$2$x$4")
	fi
done <<< $1

== 1 == one: real 0m0.004s user 0m0.003s sys 0m0.000s two: real 0m0.000s user 0m0.000s sys 0m0.000s == 10 == one: real 0m0.005s user 0m0.002s sys 0m0.002s two: real 0m0.000s user 0m0.000s sys 0m0.000s == 100 == one: real 0m0.005s user 0m0.004s sys 0m0.000s two: real 0m0.001s user 0m0.000s sys 0m0.000s == 1000 == one: real 0m0.010s user 0m0.008s sys 0m0.001s two: real 0m0.004s user 0m0.004s sys 0m0.000s == 10000 == one: real 0m0.061s user 0m0.057s sys 0m0.005s two: real 0m0.045s user 0m0.044s sys 0m0.000s == 100000 == one: real 0m0.582s user 0m0.575s sys 0m0.022s two: real 0m0.487s user 0m0.482s sys 0m0.004s == 1000000 == one: real 0m6.305s user 0m6.284s sys 0m0.216s two: real 0m5.268s user 0m5.194s sys 0m0.065s

On Sat, Nov 17, 2012 at 8:28 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Nevermind that.

Here's another:

	local IFS=$'\n'
	COMPREPLY=($(printf -- "$2%s$4\n" $1 | grep "^$2$3"))

On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

This version is simpler and faster:

	local IFS=$'\n'
	COMPREPLY=($(awk -v cur="${3-$cur}" -v pre="${2-}" -v suf="${4- }"
'$0 ~ cur { print pre$0suf }' <<< "$1" ))

== 10000 == awk 1: real 0m0.067s user 0m0.066s sys 0m0.001s awk 2: real 0m0.057s user 0m0.055s sys 0m0.002s

The compgen Bash-builtin performs expansion on all words in the wordlist given to its -W option, breaking Git's completion script when words passed to __gitcomp() contain expandable substrings.

In __gitcomp() we only use a small subset ot compgen's functionality, namely the filtering of matching possible completion words and adding a prefix to each of those words; suffix is added by the __gitcomp_1() helper function instead. Now, since we already have to iterate over all words in the wordlist to append a trailing space if necessary, we can check in the same loop whether a word matches the word to be completed and store matching words with possible prefix and/or suffix in the COMPREPLY array. This way we achieve the same functionality without the compgen builtin and, more importantly, without it's problematic expansions.

An additional benefit, especially for Windows/MSysGit users, is the loss of two subshells, one to run __gitcomp_1() and the other to run the compgen builtin.

Note, that like the previous patch for __gitcomp_nl(), this patch doesn't quote expandable words either, but that could be implemented later on top by unquoting $cur and then quoting what get stored in COMPREPLY.

Also update the function's description a bit.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 27 ++++++++++++++++++++-------
 t/t9902-completion.sh                  |  2 +-
 2 files changed, 21 insertions(+), 8 deletions(-)

On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

This is not quite the same as before, is it? Before the suffix would be taken into consideration for the comparison with $cur_, but not any more.

Cheers.

On Sat, Nov 17, 2012 at 12:39:24PM +0100, Felipe Contreras wrote:

That's a good catch, thanks.

I remember it puzzled me that the suffix is considered in the comparison (and that a trailing space would be appended even after a given suffix, too, so there seems to be no way to disable the trailing space). However, currently it doesn't make a difference for us, because afaics we never specify a suffix for __gitcomp(). There were two instances in _git_config() where we specified "." as suffix, but those two were converted to __gitcomp_nl(). I changed those callsites back to __gitcomp() and tried to provoke wrong behavior with the above patch, but couldn't.

Anyway, it's better to err on the safe side, so here's the fixup.

-- >8 --
Subject: [PATCH] fixup! completion: fix expansion issue in __gitcomp()

---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Since the __gitcomp_1() helper is basically only an implementation detail of __gitcomp() that exists solely for performance reasons (see ab02dfe5 (bash completion: Improve responsiveness of git-log completion, 2008-07-13)), it's quite unlikely that anyone uses or ever used it besides __gitcomp(). And now __gitcomp() doesn't need it anymore either, so delete it.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 13 -------------
 1 file changed, 13 deletions(-)