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

From: Jeff King <peff@peff.net>
Date: Sep 27, 2012, 06:28 UTC
Message-ID: <20120927062855.GA22890@sigill.intra.peff.net>
In-Reply-To: <20120927003751.GI10144@goldbirke>

On Thu, Sep 27, 2012 at 02:37:51AM +0200, SZEDER Gábor wrote:

Show 19 quoted lines

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