possible bug in autocompletion

possible bug in autocompletion

[Jeroen Meijer]

We have a tag name with some special characters. The tag name is 
'Build%20V%20${bamboo.custom.jiraversion.name}%20Build%20721'. In 
somecases where autocompletion is used an error is given, such as 'bash: 
Build%20V%20${bamboo.custom.jiraversion.name}%20Build%20721: bad 
substitution'. This can be invoked by typing 'git checkout B' and then 
pressing tab.
Of course; the tag is useless but still I guess this is a bug in the 
autocompletion of git.

Regards,

Meijuh

Re: possible bug in autocompletion

[Jeff King]

On Tue, Jul 17, 2012 at 11:10:39AM +0200, Jeroen Meijer wrote:

> We have a tag name with some special characters. The tag name is
> 'Build%20V%20${bamboo.custom.jiraversion.name}%20Build%20721'. In
> somecases where autocompletion is used an error is given, such as
> 'bash: Build%20V%20${bamboo.custom.jiraversion.name}%20Build%20721:
> bad substitution'. This can be invoked by typing 'git checkout B' and
> then pressing tab.

Hrm. Weird. It is the "${}" in your tag name that causes the problem,
and it all boils down to bash trying to do parameter expansion on the
contents of "compgen -W". You can see it in a much simpler example:

  $ echo '${foo.bar}' ;# no expansion, works
  ${foo.bar}

  $ echo "${foo.bar}" ;# expansion, bash rightfully complains
  bash: ${foo.bar}: bad substitution

  $ compgen -W '${foo.bar}' f
  bash: ${foo.bar}: bad substitution

In the final command, we use single-quotes so there is no expansion
before the command execution. So it happens internally to compgen.
documentation for compgen says:

  -W wordlist
          The  wordlist is split using the characters in the IFS special vari‐
          able as delimiters, and each resultant word is expanded.  The possi‐
          ble  completions  are  the members of the resultant list which match
          the word being completed.

Which seems kind of crazy to me. It means that we need to be quoting
everything we feed to compgen to avoid accidental expansion. But I guess
bash is not likely to change anytime soon, so we probably need to work
around it.

> Of course; the tag is useless but still I guess this is a bug in the
> autocompletion of git.

Yeah, that tag is crazy. But this can happen anywhere that we feed
arbitrary data to compgen. Try this:

  echo content >'${foo.bar}' &&
  git add . &&
  git commit

  git show HEAD:<tab>

which generates the same error. Or even a file named "foo$bar", which is
much more likely; that will not generate an error, but it will expand
$bar and produce erroneous results. I think we also have issues with
files with single and double quotes in them.

Something like this seems to fix it for me:

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ffedce7..2d20824 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -261,7 +261,12 @@ __gitcomp ()
 __gitcomp_nl ()
 {
 	local IFS=$'\n'
-	COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
+	local words=$1
+	words=${words//\\/\\\\}
+	words=${words//\$/\\\$}
+	words=${words//\'/\\\'}
+	words=${words//\"/\\\"}
+	COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$words" -- "${3-$cur}"))
 }
 
 __git_heads ()

but it is awfully ugly. Maybe completion experts can offer a better
solution.

-Peff

Re: possible bug in autocompletion

[Felipe Contreras]

On Tue, Jul 17, 2012 at 2:12 PM, Jeff King <peff@peff.net> wrote:

> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -261,7 +261,12 @@ __gitcomp ()
>  __gitcomp_nl ()
>  {
>         local IFS=$'\n'
> -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> +       local words=$1
> +       words=${words//\\/\\\\}
> +       words=${words//\$/\\\$}
> +       words=${words//\'/\\\'}
> +       words=${words//\"/\\\"}
> +       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$words" -- "${3-$cur}"))
>  }

What about something like this?

local words
printf -v words "%q" "$w"
COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$words" -- "${3-$cur}"))

Cheers.

-- 
Felipe Contreras

Re: possible bug in autocompletion

[Jeff King]

On Wed, Sep 19, 2012 at 07:08:09PM +0200, Felipe Contreras wrote:

> On Tue, Jul 17, 2012 at 2:12 PM, Jeff King <peff@peff.net> wrote:
> 
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -261,7 +261,12 @@ __gitcomp ()
> >  __gitcomp_nl ()
> >  {
> >         local IFS=$'\n'
> > -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> > +       local words=$1
> > +       words=${words//\\/\\\\}
> > +       words=${words//\$/\\\$}
> > +       words=${words//\'/\\\'}
> > +       words=${words//\"/\\\"}
> > +       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$words" -- "${3-$cur}"))
> >  }
> 
> What about something like this?
> 
> local words
> printf -v words "%q" "$w"
> COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$words" -- "${3-$cur}"))

Thanks, I didn't know about bash's internal printf magic. That is a much
more elegant solution.

Care to wrap it up in a patch?

-Peff

Re: possible bug in autocompletion

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Tue, Jul 17, 2012 at 2:12 PM, Jeff King <peff@peff.net> wrote:
>
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -261,7 +261,12 @@ __gitcomp ()
>>  __gitcomp_nl ()
>>  {
>>         local IFS=$'\n'
>> -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
>> +       local words=$1
>> +       words=${words//\\/\\\\}
>> +       words=${words//\$/\\\$}
>> +       words=${words//\'/\\\'}
>> +       words=${words//\"/\\\"}
>> +       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$words" -- "${3-$cur}"))
>>  }
>
> What about something like this?
>
> local words
> printf -v words "%q" "$w"
> COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$words" -- "${3-$cur}"))

Both "printf -v" and "%q" are brilliant ;-)

Re: possible bug in autocompletion

[Felipe Contreras]

On Wed, Sep 19, 2012 at 7:43 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Sep 19, 2012 at 07:08:09PM +0200, Felipe Contreras wrote:
>
>> On Tue, Jul 17, 2012 at 2:12 PM, Jeff King <peff@peff.net> wrote:
>>
>> > --- a/contrib/completion/git-completion.bash
>> > +++ b/contrib/completion/git-completion.bash
>> > @@ -261,7 +261,12 @@ __gitcomp ()
>> >  __gitcomp_nl ()
>> >  {
>> >         local IFS=$'\n'
>> > -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
>> > +       local words=$1
>> > +       words=${words//\\/\\\\}
>> > +       words=${words//\$/\\\$}
>> > +       words=${words//\'/\\\'}
>> > +       words=${words//\"/\\\"}
>> > +       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$words" -- "${3-$cur}"))
>> >  }
>>
>> What about something like this?
>>
>> local words
>> printf -v words "%q" "$w"
>> COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$words" -- "${3-$cur}"))
>
> Thanks, I didn't know about bash's internal printf magic. That is a much
> more elegant solution.
>
> Care to wrap it up in a patch?

I'm trying to, but unfortunately "\n" gets converted to "\\n", so it
doesn't get separated to words. Any ideas?

-- 
Felipe Contreras

Re: possible bug in autocompletion

[Felipe Contreras]

On Wed, Sep 19, 2012 at 8:16 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Wed, Sep 19, 2012 at 7:43 PM, Jeff King <peff@peff.net> wrote:
>> On Wed, Sep 19, 2012 at 07:08:09PM +0200, Felipe Contreras wrote:
>>
>>> On Tue, Jul 17, 2012 at 2:12 PM, Jeff King <peff@peff.net> wrote:
>>>
>>> > --- a/contrib/completion/git-completion.bash
>>> > +++ b/contrib/completion/git-completion.bash
>>> > @@ -261,7 +261,12 @@ __gitcomp ()
>>> >  __gitcomp_nl ()
>>> >  {
>>> >         local IFS=$'\n'
>>> > -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
>>> > +       local words=$1
>>> > +       words=${words//\\/\\\\}
>>> > +       words=${words//\$/\\\$}
>>> > +       words=${words//\'/\\\'}
>>> > +       words=${words//\"/\\\"}
>>> > +       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$words" -- "${3-$cur}"))
>>> >  }
>>>
>>> What about something like this?
>>>
>>> local words
>>> printf -v words "%q" "$w"
>>> COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$words" -- "${3-$cur}"))
>>
>> Thanks, I didn't know about bash's internal printf magic. That is a much
>> more elegant solution.
>>
>> Care to wrap it up in a patch?
>
> I'm trying to, but unfortunately "\n" gets converted to "\\n", so it
> doesn't get separated to words. Any ideas?

Actually, this seems to do the trick:

	local words IFS=$'\n'
	printf -v words "%q" "$1"
	COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "${words//\\n/$IFS}"
-- "${3-$cur}"))

I don't know how to do $'\n' in the middle of double-quotes, but $IFS works.

-- 
Felipe Contreras

Re: possible bug in autocompletion

[Jeff King]

On Wed, Sep 19, 2012 at 08:23:01PM +0200, Felipe Contreras wrote:

> >> Care to wrap it up in a patch?
> >
> > I'm trying to, but unfortunately "\n" gets converted to "\\n", so it
> > doesn't get separated to words. Any ideas?
> 
> Actually, this seems to do the trick:
> 
> 	local words IFS=$'\n'
> 	printf -v words "%q" "$1"
> 	COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "${words//\\n/$IFS}"
> -- "${3-$cur}"))
> 
> I don't know how to do $'\n' in the middle of double-quotes, but $IFS works.

I don't think you can search-and-replace backslash-escaped characters
and always get the correct result.  For example, in this string:

  \\n

Your regex would match the "\n", missing the context that the backslash
is actually the second-half of an escaped pair. Searching for "[^\\]\\n"
would similarly miss that:

  \\\n

should be converted. IOW, I think backslash-escaping fundamentally has
to either be parsed left-to-right, or quoted backslashes peeled in order
(if I were not such a bad computer scientist, I could probably come up
with some proof involving Chomsky's hierarchy of grammars).

I think the real problem is that we are feeding "printf %q" an input
where we want _most_ of the constructs quoted, but not some (namely
newline, which is syntactically significant, and which we accept will
break our completion). So something like the manual quoting I posted
earlier in the thread is actually much closer to what we want.

On a related note, I notice that even with either of our patches, doing
this:

  echo content >'${foo.bar}'
  git add .
  git commit -m whatever
  git show HEAD:<tab>

will yield this completion:

  git show HEAD:${foo.bar}

which is correct, but not useful. It needs to be quoted _again_ to avoid
interpolation when you actually quote the command. Bash's filename
completion handles this automatically. E.g., if you do:

  git add <tab>

you will get:

  git add \$\{foo.bar\}

I have no idea if that internal to bash's filename completion, or if
there is some easy facility offered to programmable completions to do
the same thing.  I don't think this is a high priority, but it would be
nice to handle it. And moreover, I am really wondering if we are missing
some solution that bash is providing to help us with the quoting issues.
Surely we are not the first completion script to come up against this.

-Peff

Re: possible bug in autocompletion

[Felipe Contreras]

On Wed, Sep 19, 2012 at 9:55 PM, Jeff King <peff@peff.net> wrote:

> I have no idea if that internal to bash's filename completion, or if
> there is some easy facility offered to programmable completions to do
> the same thing.  I don't think this is a high priority, but it would be
> nice to handle it. And moreover, I am really wondering if we are missing
> some solution that bash is providing to help us with the quoting issues.
> Surely we are not the first completion script to come up against this.

I found a much easier solution:

-       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
+       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$(quote "$1")"
-- "${3-$cur}"))

But what about the people that don't have bash-completion?

BTW:

quote()
{
    local quoted=${1//\'/\'\\\'\'}
    printf "'%s'" "$quoted"
}

-- 
Felipe Contreras

Re: possible bug in autocompletion

[Jeff King]

On Thu, Sep 20, 2012 at 01:08:29AM +0200, Felipe Contreras wrote:

> On Wed, Sep 19, 2012 at 9:55 PM, Jeff King <peff@peff.net> wrote:
> 
> > I have no idea if that internal to bash's filename completion, or if
> > there is some easy facility offered to programmable completions to do
> > the same thing.  I don't think this is a high priority, but it would be
> > nice to handle it. And moreover, I am really wondering if we are missing
> > some solution that bash is providing to help us with the quoting issues.
> > Surely we are not the first completion script to come up against this.
> 
> I found a much easier solution:
> 
> -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> +       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$(quote "$1")"
> -- "${3-$cur}"))

Oh, nice. :)

> But what about the people that don't have bash-completion?
> 
> BTW:
> 
> quote()
> {
>     local quoted=${1//\'/\'\\\'\'}
>     printf "'%s'" "$quoted"
> }

That is short and obvious enough that we could probably just
cut-and-paste it into our script as _git_quote (and it is basically a
cleaner version of the thing that I posted).

-Peff