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

[Jeff King <peff@peff.net>]

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