Re: [PATCH] git-completion.bash: always swallow error output of for-each-ref

From: SZEDER Gábor <szeder@ira.uka.de>
Date: Feb 13, 2016, 01:07 UTC
Message-ID: <20160213020712.Horde.SM-rQbc5Jx1UwdYxdvNFNJx@webmail.informatik.kit.edu>
In-Reply-To: <20160212234041.GA15688@sigill.intra.peff.net>

Quoting Jeff King <peff@peff.net>:

Show 23 quoted lines

> On Sat, Feb 13, 2016 at 12:21:22AM +0100, SZEDER Gábor wrote:
>
>> I think in this case we should opt for performance instead of correctness,
>> and use Peff's 'refname:strip=2'.  Ambiguous refs will only hurt you, if,
>> well, your repo actually has ambiguous refs AND you happen to want to do
>> something with one of those refs.  I suspect that's rather uncommon, and
>> even then you could simply rename one of those refs.  OTOH, as shown in
>> the ticket, you don't need that many refs to make refs completion
>> unacceptably slow on Windows, and it will bite every time you attempt to
>> complete a ref.
>
> I'm not even sure that this is a correctness tradeoff at all. For
> example, in the function __git_heads(), we are asking for-each-ref to
> tell us about everything under refs/heads/. If you have a refs/heads/foo
> and refs/tags/foo, we don't care; we are trying to print the unqualified
> branch names. And in fact having refname:short print "heads/foo" in this
> case may be actively wrong. For instance, in _git_branch(), you cannot
> use the resulting completion of "heads/foo", as that command wants
> unqualified names in "refs/heads/", and you do not have
> "refs/heads/heads/foo".
>
> So I think switching to :strip is an improvement in both correctness
> _and_ performance.

Right. I was more worried about __git_refs(), because it asks for everything under refs/heads/, refs/tags/ and refs/remotes/, and its output is used in a lot more places and fed to a lot more commands than the output of __git_heads() (or __git_tags(), for that matter). But I thought that a branch-tag ambiguity would cause git to error out complaining, just like in the case of ref-path ambiguity. Successfully avoiding ambiguous refs for many years, I wasn't aware that 'git rev-parse' doesn't barf, but only warns and resolves the ambiguity in favor of the tag.

Show 5 quoted lines

>> Now, if 'git for-each-ref' could understand '**' globbing, not just
>> fnmatch...
>
> I think it does already, since 4917e1e (Makefile: promote wildmatch to
> be the default fnmatch implementation, 2013-05-30).

Things are looking up!

A single 'master' branch and 10 remotes with 10k remote branches each, i.e. a total of 100001 refs, all packed. To uniquely complete 'master ' after 'git checkout m<TAB>' on Linux in current git.git, i.e. with 'refname:short':

   $ cur=m ; time __gitcomp_nl "$(__git_refs '' 1)"

   real  0m7.641s
   user  0m5.888s
   sys   0m1.832s

Using 'refname:strip=2' for both 'git for-each-ref' in __git_refs():

   $ cur=m ; time __gitcomp_nl "$(__git_refs '' 1)"

   real  0m2.848s
   user  0m2.308s
   sys   0m0.596s

Quick'n'dirty PoC using 'refname:strip', '**' globbing and a few more tricks to let 'git for-each-ref' do the filtering instead of the shell loop behind __gitcomp_nl():

   $ cur=m ; time IFS=$'\n' COMPREPLY=( $(__git_refs_PoC '' 1) )

   real  0m0.247s
   user  0m0.208s
   sys   0m0.032s

Not bad for a Friday night, huh? :)