[PATCH 0/7] completion: fix expansion issues with compgen

[PATCH 0/7] completion: fix expansion issues with compgen

[SZEDER Gábor]

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(-)

-- 
1.8.0.220.g4d14ece

[PATCH 1/7] completion: make the 'basic' test more tester-friendly

[SZEDER Gábor]

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(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 8fa025f9..b56759f7 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -158,14 +158,22 @@ test_expect_success '__gitcomp - suffix' '
 test_expect_success 'basic' '
 	run_completion "git \"\"" &&
 	# built-in
-	grep -q "^add \$" out &&
+	echo "add " >expected &&
+	sed -n "/^add \$/p" out >out2 &&
+	test_cmp expected out2 &&
 	# script
-	grep -q "^filter-branch \$" out &&
+	echo "filter-branch " >expected &&
+	sed -n "/^filter-branch \$/p" out >out2 &&
+	test_cmp expected out2 &&
 	# plumbing
-	! grep -q "^ls-files \$" out &&
+	>expected &&
+	sed -n "/^ls-files \$/p" out >out2 &&
+	test_cmp expected out2 &&
 
 	run_completion "git f" &&
-	! grep -q -v "^f" out
+	>expected &&
+	sed -n "/^[^f]/p" out >out2 &&
+	test_cmp expected out2
 '
 
 test_expect_success 'double dash "git" itself' '
-- 
1.8.0.220.g4d14ece

[PATCH 2/7] completion: fix args of run_completion() test helper

[SZEDER Gábor]

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(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index b56759f7..3af75872 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
 }
 
@@ -156,7 +156,7 @@ test_expect_success '__gitcomp - suffix' '
 '
 
 test_expect_success 'basic' '
-	run_completion "git \"\"" &&
+	run_completion git "" &&
 	# built-in
 	echo "add " >expected &&
 	sed -n "/^add \$/p" out >out2 &&
@@ -170,7 +170,7 @@ test_expect_success 'basic' '
 	sed -n "/^ls-files \$/p" out >out2 &&
 	test_cmp expected out2 &&
 
-	run_completion "git f" &&
+	run_completion git f &&
 	>expected &&
 	sed -n "/^[^f]/p" out >out2 &&
 	test_cmp expected out2
-- 
1.8.0.220.g4d14ece

[PATCH 3/7] completion: add tests for the __gitcomp_nl() completion helper function

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

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3af75872..32b3e8c4 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -155,6 +155,90 @@ 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
+	(
+		declare -a COMPREPLY &&
+		refs="$(cat <<-\EOF
+			maint
+			master
+			next
+			pu
+			EOF
+		)" &&
+		cur="m" &&
+		__gitcomp_nl "$refs" &&
+		print_comp
+	) &&
+	test_cmp expected out
+'
+
+test_expect_success '__gitcomp_nl - prefix' '
+	sed -e "s/Z$//" >expected <<-\EOF &&
+	--fixup=maint Z
+	--fixup=master Z
+	EOF
+	(
+		declare -a COMPREPLY &&
+		refs="$(cat <<-\EOF
+			maint
+			master
+			next
+			pu
+			EOF
+		)" &&
+		cur="--fixup=m" &&
+		__gitcomp_nl "$refs" "--fixup=" "m" &&
+		print_comp
+	) &&
+	test_cmp expected out
+'
+
+test_expect_success '__gitcomp_nl - suffix' '
+	sed -e "s/Z$//" >expected <<-\EOF &&
+	branch.maint.Z
+	branch.master.Z
+	EOF
+	(
+		declare -a COMPREPLY &&
+		refs="$(cat <<-\EOF
+			maint
+			master
+			next
+			pu
+			EOF
+		)" &&
+		cur="branch.ma" &&
+		__gitcomp_nl "$refs" "branch." "ma" "." &&
+		print_comp
+	) &&
+	test_cmp expected out
+'
+
+test_expect_success '__gitcomp_nl - no suffix' '
+	sed -e "s/Z$//" >expected <<-\EOF &&
+	maintZ
+	masterZ
+	EOF
+	(
+		declare -a COMPREPLY &&
+		refs="$(cat <<-\EOF
+			maint
+			master
+			next
+			pu
+			EOF
+		)" &&
+		cur="ma" &&
+		__gitcomp_nl "$refs" "" "ma" "" &&
+		print_comp
+	) &&
+	test_cmp expected out
+'
+
 test_expect_success 'basic' '
 	run_completion git "" &&
 	# built-in
-- 
1.8.0.220.g4d14ece

[PATCH 4/7] completion: add tests for invalid variable name among completion words

[SZEDER Gábor]

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(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 32b3e8c4..a108ec1c 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -71,6 +71,7 @@ test_completion_long ()
 }
 
 newline=$'\n'
+invalid_variable_name="${foo.bar}"
 
 test_expect_success '__gitcomp - trailing space - options' '
 	sed -e "s/Z$//" >expected <<-\EOF &&
@@ -155,6 +156,12 @@ test_expect_success '__gitcomp - suffix' '
 	test_cmp expected out
 '
 
+test_expect_failure '__gitcomp - doesnt fail because of invalid variable name' '
+	(
+		__gitcomp "$invalid_variable_name"
+	)
+'
+
 test_expect_success '__gitcomp_nl - trailing space' '
 	sed -e "s/Z$//" >expected <<-\EOF &&
 	maint Z
@@ -239,6 +246,12 @@ test_expect_success '__gitcomp_nl - no suffix' '
 	test_cmp expected out
 '
 
+test_expect_failure '__gitcomp_nl - doesnt fail because of invalid variable name' '
+	(
+		__gitcomp_nl "$invalid_variable_name"
+	)
+'
+
 test_expect_success 'basic' '
 	run_completion git "" &&
 	# built-in
-- 
1.8.0.220.g4d14ece

[PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

[SZEDER Gábor]

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(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index bc0657a2..65196ddd 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -249,19 +249,28 @@ __gitcomp ()
 	esac
 }
 
-# Generates completion reply with compgen from newline-separated possible
-# completion words by appending a space to all of them.
+# Generates completion reply for the word in $cur from newline-separated
+# possible completion words, appending a space to all of them.
 # It accepts 1 to 4 arguments:
 # 1: List of possible completion words, separated by a single newline.
 # 2: A prefix to be added to each possible completion word (optional).
-# 3: Generate possible completion matches for this word (optional).
+# 3: Generate possible completion matches for this word instead of $cur
+#    (optional).
 # 4: A suffix to be appended to each possible completion word instead of
 #    the default space (optional).  If specified but empty, nothing is
 #    appended.
 __gitcomp_nl ()
 {
 	local IFS=$'\n'
-	COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
+	COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
+		BEGIN {
+			FS="\n";
+			len=length(cur);
+		}
+		{
+			if (cur == substr($1, 1, len))
+				print pfx$1sfx;
+		}' <<< "$1" ))
 }
 
 __git_heads ()
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a108ec1c..fa289324 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -246,7 +246,7 @@ test_expect_success '__gitcomp_nl - no suffix' '
 	test_cmp expected out
 '
 
-test_expect_failure '__gitcomp_nl - doesnt fail because of invalid variable name' '
+test_expect_success '__gitcomp_nl - doesnt fail because of invalid variable name' '
 	(
 		__gitcomp_nl "$invalid_variable_name"
 	)
@@ -383,7 +383,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.8.0.220.g4d14ece

[PATCH 6/7] completion: fix expansion issue in __gitcomp()

[SZEDER Gábor]

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(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 65196ddd..283ef99b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -225,12 +225,13 @@ _get_comp_words_by_ref ()
 fi
 fi
 
-# Generates completion reply with compgen, appending a space to possible
-# completion words, if necessary.
+# Generates completion reply for the word in $cur, appending a space to
+# possible completion words, if necessary.
 # It accepts 1 to 4 arguments:
 # 1: List of possible completion words.
 # 2: A prefix to be added to each possible completion word (optional).
-# 3: Generate possible completion matches for this word (optional).
+# 3: Generate possible completion matches for this word instead of $cur
+#    (optional).
 # 4: A suffix to be appended to each possible completion word (optional).
 __gitcomp ()
 {
@@ -241,10 +242,22 @@ __gitcomp ()
 		COMPREPLY=()
 		;;
 	*)
-		local IFS=$'\n'
-		COMPREPLY=($(compgen -P "${2-}" \
-			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
-			-- "$cur_"))
+		local i=0 c IFS=$' \t\n'
+		for c in $1; do
+			case $c in
+			"$cur_"*)
+				c="$c${4-}"
+				case $c in
+				--*=*|*.) ;;
+				*) c="$c " ;;
+				esac
+				COMPREPLY[$i]="${2-}$c"
+				i=$((++i))
+				;;
+			*)
+				;;
+			esac
+		done
 		;;
 	esac
 }
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index fa289324..d08f4259 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -156,7 +156,7 @@ test_expect_success '__gitcomp - suffix' '
 	test_cmp expected out
 '
 
-test_expect_failure '__gitcomp - doesnt fail because of invalid variable name' '
+test_expect_success '__gitcomp - doesnt fail because of invalid variable name' '
 	(
 		__gitcomp "$invalid_variable_name"
 	)
-- 
1.8.0.220.g4d14ece

[PATCH 7/7] completion: remove the now unused __gitcomp_1() internal helper function

[SZEDER Gábor]

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(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 283ef99b..a281b28d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -56,19 +56,6 @@ __gitdir ()
 	fi
 }
 
-__gitcomp_1 ()
-{
-	local c IFS=$' \t\n'
-	for c in $1; do
-		c="$c$2"
-		case $c in
-		--*=*|*.) ;;
-		*) c="$c " ;;
-		esac
-		printf '%s\n' "$c"
-	done
-}
-
 # The following function is based on code from:
 #
 #   bash_completion - programmable completion functions for bash 3.2+
-- 
1.8.0.220.g4d14ece

Re: [PATCH 6/7] completion: fix expansion issue in __gitcomp()

[Felipe Contreras]

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

> -# Generates completion reply with compgen, appending a space to possible
> -# completion words, if necessary.
> +# Generates completion reply for the word in $cur, appending a space to
> +# possible completion words, if necessary.
>  # It accepts 1 to 4 arguments:
>  # 1: List of possible completion words.
>  # 2: A prefix to be added to each possible completion word (optional).
> -# 3: Generate possible completion matches for this word (optional).
> +# 3: Generate possible completion matches for this word instead of $cur
> +#    (optional).
>  # 4: A suffix to be appended to each possible completion word (optional).
>  __gitcomp ()
>  {
> @@ -241,10 +242,22 @@ __gitcomp ()
>                 COMPREPLY=()
>                 ;;
>         *)
> -               local IFS=$'\n'
> -               COMPREPLY=($(compgen -P "${2-}" \
> -                       -W "$(__gitcomp_1 "${1-}" "${4-}")" \
> -                       -- "$cur_"))
> +               local i=0 c IFS=$' \t\n'
> +               for c in $1; do
> +                       case $c in
> +                       "$cur_"*)
> +                               c="$c${4-}"
> +                               case $c in
> +                               --*=*|*.) ;;
> +                               *) c="$c " ;;
> +                               esac
> +                               COMPREPLY[$i]="${2-}$c"
> +                               i=$((++i))
> +                               ;;
> +                       *)
> +                               ;;
> +                       esac
> +               done

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.

-- 
Felipe Contreras

Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

[Felipe Contreras]

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

>  __gitcomp_nl ()
>  {
>         local IFS=$'\n'
> -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> +       COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
> +               BEGIN {
> +                       FS="\n";
> +                       len=length(cur);
> +               }
> +               {
> +                       if (cur == substr($1, 1, len))
> +                               print pfx$1sfx;
> +               }' <<< "$1" ))
>  }

Does this really perform better than my alternative?

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

-- 
Felipe Contreras

Re: [PATCH 6/7] completion: fix expansion issue in __gitcomp()

[SZEDER Gábor]

On Sat, Nov 17, 2012 at 12:39:24PM +0100, Felipe Contreras wrote:
> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> 
> > -# Generates completion reply with compgen, appending a space to possible
> > -# completion words, if necessary.
> > +# Generates completion reply for the word in $cur, appending a space to
> > +# possible completion words, if necessary.
> >  # It accepts 1 to 4 arguments:
> >  # 1: List of possible completion words.
> >  # 2: A prefix to be added to each possible completion word (optional).
> > -# 3: Generate possible completion matches for this word (optional).
> > +# 3: Generate possible completion matches for this word instead of $cur
> > +#    (optional).
> >  # 4: A suffix to be appended to each possible completion word (optional).
> >  __gitcomp ()
> >  {
> > @@ -241,10 +242,22 @@ __gitcomp ()
> >                 COMPREPLY=()
> >                 ;;
> >         *)
> > -               local IFS=$'\n'
> > -               COMPREPLY=($(compgen -P "${2-}" \
> > -                       -W "$(__gitcomp_1 "${1-}" "${4-}")" \
> > -                       -- "$cur_"))
> > +               local i=0 c IFS=$' \t\n'
> > +               for c in $1; do
> > +                       case $c in
> > +                       "$cur_"*)
> > +                               c="$c${4-}"
> > +                               case $c in
> > +                               --*=*|*.) ;;
> > +                               *) c="$c " ;;
> > +                               esac
> > +                               COMPREPLY[$i]="${2-}$c"
> > +                               i=$((++i))
> > +                               ;;
> > +                       *)
> > +                               ;;
> > +                       esac
> > +               done
> 
> 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.

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(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a1bf732f..29818fb5 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -231,9 +231,9 @@ __gitcomp ()
 	*)
 		local i=0 c IFS=$' \t\n'
 		for c in $1; do
+			c="$c${4-}"
 			case $c in
 			"$cur_"*)
-				c="$c${4-}"
 				case $c in
 				--*=*|*.) ;;
 				*) c="$c " ;;
-- 
1.8.0.220.g4d14ece

Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

[SZEDER Gábor]

On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:
> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> 
> >  __gitcomp_nl ()
> >  {
> >         local IFS=$'\n'
> > -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> > +       COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
> > +               BEGIN {
> > +                       FS="\n";
> > +                       len=length(cur);
> > +               }
> > +               {
> > +                       if (cur == substr($1, 1, len))
> > +                               print pfx$1sfx;
> > +               }' <<< "$1" ))
> >  }
> 
> Does this really perform better than my alternative?
> 
> +       for x in $1; do
> +               if [[ "$x" = "$3"* ]]; then
> +                       COMPREPLY+=("$2$x$4")
> +               fi
> +       done

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

Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

[Felipe Contreras]

On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:
>> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>
>> >  __gitcomp_nl ()
>> >  {
>> >         local IFS=$'\n'
>> > -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
>> > +       COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
>> > +               BEGIN {
>> > +                       FS="\n";
>> > +                       len=length(cur);
>> > +               }
>> > +               {
>> > +                       if (cur == substr($1, 1, len))
>> > +                               print pfx$1sfx;
>> > +               }' <<< "$1" ))
>> >  }
>>
>> Does this really perform better than my alternative?
>>
>> +       for x in $1; do
>> +               if [[ "$x" = "$3"* ]]; then
>> +                       COMPREPLY+=("$2$x$4")
>> +               fi
>> +       done
>
> 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

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

-- 
Felipe Contreras

Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

[Felipe Contreras]

On Sat, Nov 17, 2012 at 8:08 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:
>>> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>>
>>> >  __gitcomp_nl ()
>>> >  {
>>> >         local IFS=$'\n'
>>> > -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
>>> > +       COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
>>> > +               BEGIN {
>>> > +                       FS="\n";
>>> > +                       len=length(cur);
>>> > +               }
>>> > +               {
>>> > +                       if (cur == substr($1, 1, len))
>>> > +                               print pfx$1sfx;
>>> > +               }' <<< "$1" ))
>>> >  }
>>>
>>> Does this really perform better than my alternative?
>>>
>>> +       for x in $1; do
>>> +               if [[ "$x" = "$3"* ]]; then
>>> +                       COMPREPLY+=("$2$x$4")
>>> +               fi
>>> +       done
>>
>> 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
>
> Yeah, for 10000 refs, which is not the common case:

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

-- 
Felipe Contreras

Re: [PATCH 1/7] completion: make the 'basic' test more tester-friendly

[Jonathan Nieder]

SZEDER Gábor wrote:

> 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.
[...]
> 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.

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

[...]
>  	run_completion "git f" &&
> -	! grep -q -v "^f" out
> +	>expected &&
> +	sed -n "/^[^f]/p" out >out2 &&
> +	test_cmp expected out2

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

Re: [PATCH 2/7] completion: fix args of run_completion() test helper

[Jonathan Nieder]

SZEDER Gábor wrote:

> Fix this by passing the command line to run_completion() as separate
> words.

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.

Re: [PATCH 3/7] completion: add tests for the __gitcomp_nl() completion helper function

[Jonathan Nieder]

SZEDER Gábor wrote:

> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -155,6 +155,90 @@ test_expect_success '__gitcomp - suffix' '
>  	test_cmp expected out
>  '
>  
> +test_expect_success '__gitcomp_nl - trailing space' '
> +	sed -e "s/Z$//" >expected <<-\EOF &&

'$' 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>

Re: [PATCH 4/7] completion: add tests for invalid variable name among completion words

[Jonathan Nieder]

SZEDER Gábor wrote:

>                                                      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

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.

Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

[Felipe Contreras]

On Sat, Nov 17, 2012 at 8:28 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Nov 17, 2012 at 8:08 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:
>>>> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>>>
>>>> >  __gitcomp_nl ()
>>>> >  {
>>>> >         local IFS=$'\n'
>>>> > -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
>>>> > +       COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
>>>> > +               BEGIN {
>>>> > +                       FS="\n";
>>>> > +                       len=length(cur);
>>>> > +               }
>>>> > +               {
>>>> > +                       if (cur == substr($1, 1, len))
>>>> > +                               print pfx$1sfx;
>>>> > +               }' <<< "$1" ))
>>>> >  }
>>>>
>>>> Does this really perform better than my alternative?
>>>>
>>>> +       for x in $1; do
>>>> +               if [[ "$x" = "$3"* ]]; then
>>>> +                       COMPREPLY+=("$2$x$4")
>>>> +               fi
>>>> +       done
>>>
>>> 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
>>
>> Yeah, for 10000 refs, which is not the common case:
>
> And this beats both in every case:
>
> while read -r x; do
>         if [[ "$x" == "$3"* ]]; then
>                 COMPREPLY+=("$2$x$4")
>         fi
> done <<< $1

Nevermind that.

Here's another:

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

-- 
Felipe Contreras

Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()

[Felipe Contreras]

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

>  __gitcomp_nl ()
>  {
>         local IFS=$'\n'
> -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> +       COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
> +               BEGIN {
> +                       FS="\n";
> +                       len=length(cur);
> +               }
> +               {
> +                       if (cur == substr($1, 1, len))
> +                               print pfx$1sfx;
> +               }' <<< "$1" ))
>  }

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

-- 
Felipe Contreras

Re: [PATCH 4/7] completion: add tests for invalid variable name among completion words

[Felipe Contreras]

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

> @@ -155,6 +156,12 @@ test_expect_success '__gitcomp - suffix' '
>         test_cmp expected out
>  '
>
> +test_expect_failure '__gitcomp - doesnt fail because of invalid variable name' '
> +       (
> +               __gitcomp "$invalid_variable_name"
> +       )
> +'

Why in a subshell?

-- 
Felipe Contreras

Re: [PATCH 4/7] completion: add tests for invalid variable name among completion words

[Felipe Contreras]

On Sun, Nov 18, 2012 at 12:40 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> SZEDER Gábor wrote:
>
>>                                                      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
>
> 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.

Shouldn't this go to the commit message?

-- 
Felipe Contreras

Re: [PATCH 2/7] completion: fix args of run_completion() test helper

[Felipe Contreras]

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

>  test_expect_success 'basic' '
> -       run_completion "git \"\"" &&
> +       run_completion git "" &&

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"

-- 
Felipe Contreras

Re: [PATCH 3/7] completion: add tests for the __gitcomp_nl() completion helper function

[Felipe Contreras]

On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> 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(+)

Too much code that can be simplified:

--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -85,6 +85,22 @@ test_gitcomp ()
 	test_cmp expected out
 }

+# Test __gitcomp_nl
+# Arguments are:
+# 1: typed text so far (cur)
+# -: the rest are passed to __gitcomp_nl
+test_gitcomp_nl ()
+{
+	local -a COMPREPLY &&
+	sed -e 's/Z$//' > expected &&
+	cur="$1" &&
+	shift &&
+	__gitcomp_nl "$@" &&
+	print_comp &&
+	cp expected out /tmp
+	test_cmp expected out
+}
+
 invalid_variable_name="${foo.bar}"

 test_expect_success '__gitcomp - trailing space - options' '
@@ -134,88 +150,39 @@ test_expect_success '__gitcomp - doesnt fail
because of invalid variable name' '
 	__gitcomp "$invalid_variable_name"
 '

+read -r -d "" refs <<-\EOF
+maint
+master
+next
+pu
+EOF
+
 test_expect_success '__gitcomp_nl - trailing space' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_gitcomp_nl "m" "$refs" <<-EOF
 	maint Z
 	master Z
 	EOF
-	(
-		declare -a COMPREPLY &&
-		refs="$(cat <<-\EOF
-			maint
-			master
-			next
-			pu
-			EOF
-		)" &&
-		cur="m" &&
-		__gitcomp_nl "$refs" &&
-		print_comp
-	) &&
-	test_cmp expected out
 '

 test_expect_success '__gitcomp_nl - prefix' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_gitcomp_nl "--fixup=m" "$refs" "--fixup=" "m" <<-EOF
 	--fixup=maint Z
 	--fixup=master Z
 	EOF
-	(
-		declare -a COMPREPLY &&
-		refs="$(cat <<-\EOF
-			maint
-			master
-			next
-			pu
-			EOF
-		)" &&
-		cur="--fixup=m" &&
-		__gitcomp_nl "$refs" "--fixup=" "m" &&
-		print_comp
-	) &&
-	test_cmp expected out
 '

 test_expect_success '__gitcomp_nl - suffix' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_gitcomp_nl "branch.ma" "$refs" "branch." "ma" "." <<-\EOF
 	branch.maint.Z
 	branch.master.Z
 	EOF
-	(
-		declare -a COMPREPLY &&
-		refs="$(cat <<-\EOF
-			maint
-			master
-			next
-			pu
-			EOF
-		)" &&
-		cur="branch.ma" &&
-		__gitcomp_nl "$refs" "branch." "ma" "." &&
-		print_comp
-	) &&
-	test_cmp expected out
 '

 test_expect_success '__gitcomp_nl - no suffix' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_gitcomp_nl "ma" "$refs" "" "ma" "" <<-\EOF
 	maintZ
 	masterZ
 	EOF
-	(
-		declare -a COMPREPLY &&
-		refs="$(cat <<-\EOF
-			maint
-			master
-			next
-			pu
-			EOF
-		)" &&
-		cur="ma" &&
-		__gitcomp_nl "$refs" "" "ma" "" &&
-		print_comp
-	) &&
-	test_cmp expected out
 '

 test_expect_success '__gitcomp_nl - doesnt fail because of invalid
variable name' '

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/7] completion: make the 'basic' test more tester-friendly

[Felipe Contreras]

On Sun, Nov 18, 2012 at 12:00 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> SZEDER Gábor wrote:
>
>> 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.
> [...]
>> 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.
>
> Looks good.  I wonder if this points to the need for a test_grep
> helper more generally.

It does.

-- 
Felipe Contreras

Re: [PATCH 1/7] completion: make the 'basic' test more tester-friendly

[Felipe Contreras]

On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> 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(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 8fa025f9..b56759f7 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -158,14 +158,22 @@ test_expect_success '__gitcomp - suffix' '
>  test_expect_success 'basic' '
>         run_completion "git \"\"" &&
>         # built-in
> -       grep -q "^add \$" out &&
> +       echo "add " >expected &&
> +       sed -n "/^add \$/p" out >out2 &&
> +       test_cmp expected out2 &&
>         # script
> -       grep -q "^filter-branch \$" out &&
> +       echo "filter-branch " >expected &&
> +       sed -n "/^filter-branch \$/p" out >out2 &&
> +       test_cmp expected out2 &&
>         # plumbing
> -       ! grep -q "^ls-files \$" out &&
> +       >expected &&
> +       sed -n "/^ls-files \$/p" out >out2 &&
> +       test_cmp expected out2 &&
>
>         run_completion "git f" &&
> -       ! grep -q -v "^f" out
> +       >expected &&
> +       sed -n "/^[^f]/p" out >out2 &&
> +       test_cmp expected out2
>  '
>
>  test_expect_success 'double dash "git" itself' '

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

--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -191,18 +191,20 @@ test_expect_success '__gitcomp_nl - doesnt fail
because of invalid variable name

 test_expect_success 'basic' '
        run_completion "git " &&
+       (
        # built-in
-       echo "add " >expected &&
-       sed -n "/^add \$/p" out >out2 &&
-       test_cmp expected out2 &&
+       sed -n "/^add $/p" out &&
        # script
-       echo "filter-branch " >expected &&
-       sed -n "/^filter-branch \$/p" out >out2 &&
-       test_cmp expected out2 &&
+       sed -n "/^filter-branch $/p" out &&
        # plumbing
-       >expected &&
-       sed -n "/^ls-files \$/p" out >out2 &&
-       test_cmp expected out2 &&
+       sed -n "/^ls-files $/p" out
+       ) > actual &&
+
+       sed -e 's/Z$//' > expected <<-EOF &&
+       add Z
+       filter-branch Z
+       EOF
+       test_cmp expected actual &&

        run_completion "git f" &&
        >expected &&

-- 
Felipe Contreras