[PATCH] completion: fix shell expansion of items

[PATCH] completion: fix shell expansion of items

[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

Re: [PATCH] completion: fix shell expansion of items

[Jeff King]

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

Re: [PATCH] completion: fix shell expansion of items

[Junio C Hamano]

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.

Re: [PATCH] completion: fix shell expansion of items

[SZEDER Gábor]

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 
+

Re: [PATCH] completion: fix shell expansion of items

[Jeff King]

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

Re: [PATCH] completion: fix shell expansion of items

[Felipe Contreras]

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

[PATCH] Revert "completion: fix shell expansion of items"

[Jeff King]

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

Re: [PATCH] Revert "completion: fix shell expansion of items"

[Junio C Hamano]

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

Re: [PATCH] Revert "completion: fix shell expansion of items"

[SZEDER Gábor]

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

Re: [PATCH] Revert "completion: fix shell expansion of items"

[Junio C Hamano]

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.

Re: [PATCH] Revert "completion: fix shell expansion of items"

[Jeff King]

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

[PATCH 1/3] t9902: add a few basic completion tests

[Jeff King]

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

[PATCH 2/3] t9902: add completion tests for "odd" filenames

[Jeff King]

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

[PATCH 3/3] completion: improve shell expansion of items

[Jeff King]

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

[PATCH 4/3] completion: quote completions we find

[Jeff King]

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

Re: [PATCH 4/3] completion: quote completions we find

[SZEDER Gábor]

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?

Re: [PATCH 4/3] completion: quote completions we find

[Junio C Hamano]

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?

Re: [PATCH 4/3] completion: quote completions we find

[SZEDER Gábor]

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?

Re: [PATCH 3/3] completion: improve shell expansion of items

[SZEDER Gábor]

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

Re: [PATCH 3/3] completion: improve shell expansion of items

[Jeff King]

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

Re: [PATCH 3/3] completion: improve shell expansion of items

[Jeff King]

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

Re: [PATCH 3/3] completion: improve shell expansion of items

[Jeff King]

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

Re: [PATCH 3/3] completion: improve shell expansion of items

[SZEDER Gábor]

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

[PATCH 1/5] completion: fix non-critical bugs in __gitcomp() tests

[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

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

[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

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

[Junio C Hamano]

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

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

[SZEDER Gábor]

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

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

[Junio C Hamano]

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']?

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

[Jeff King]

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

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

[Junio C Hamano]

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
	}

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

[Jeff King]

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

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

[SZEDER Gábor]

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.

[PATCH 3/5] 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 | 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

[PATCH 4/5] completion: test __gitcomp() and __gitcomp_nl() with expandable words

[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

[POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl()

[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

Re: [POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl()

[SZEDER Gábor]

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

Re: [POC PATCH 5/5] completion: avoid compgen to fix expansion issues in __gitcomp_nl()

[Jeff King]

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