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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Quoting Johannes Schindelin <Johannes.Schindelin@xxxxxx>:

Hi Peff,

On Thu, 4 Feb 2016, Jeff King wrote:

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 15ebba5..7c0549d 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -317,7 +317,7 @@ __git_heads ()
>  	local dir="$(__gitdir)"
>  	if [ -d "$dir" ]; then
>  		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
> -			refs/heads
> +			refs/heads 2>/dev/null
>  		return

Not really related to your topic, but digging into it caused me to read
b7dd2d2 (for-each-ref: Do not lookup objects when they will not be used,
2009-05-27), which is about making sure for-each-ref is very fast in
completion.

It looks like %(refname:short) is actually kind of expensive:

Yep, this was reported on the Git for Windows bug tracker, too:

	https://github.com/git-for-windows/git/issues/524

$ time git for-each-ref --format='%(refname)' refs/tags  >/dev/null

real    0m0.004s
user    0m0.000s
sys     0m0.004s

$ time git for-each-ref --format='%(refname:short)' refs/tags >/dev/null

real    0m0.009s
user    0m0.004s
sys     0m0.004s

And the timings in the ticket I mentioned above are not pretty small:
0.055s vs 1.341s

It's ironic that 'refname:short' came about because it was faster than
'refname' plus removing 'refs/{heads,tags,remotes}/' in a shell loop, at
least on Linux.

However, 'refname:short' performs a lot more stat() calls per ref to check
ambiguity, especially since many ref races got fixed.  In a repo with a
single master branch:

$ strace git for-each-ref --format='%(refname)' refs/heads/master 2>&1 |grep 'stat("\.git.*\(master\|packed-refs\)'
  stat(".git/refs/heads/master", {st_mode=S_IFREG|0644, st_size=41, ...}) = 0
  lstat(".git/refs/heads/master", {st_mode=S_IFREG|0644, st_size=41, ...}) = 0

$ strace git for-each-ref --format='%(refname:short)' refs/heads/master 2>&1 |grep 'stat("\.git.*\(master\|packed-refs\)'
  stat(".git/refs/heads/master", {st_mode=S_IFREG|0644, st_size=41, ...}) = 0
  lstat(".git/refs/heads/master", {st_mode=S_IFREG|0644, st_size=41, ...}) = 0
lstat(".git/master", 0x7fff6dac9610) = -1 ENOENT (No such file or directory) stat(".git/packed-refs", 0x7fff6dac9460) = -1 ENOENT (No such file or directory) lstat(".git/refs/master", 0x7fff6dac9610) = -1 ENOENT (No such file or directory) stat(".git/packed-refs", 0x7fff6dac9460) = -1 ENOENT (No such file or directory) lstat(".git/refs/tags/master", 0x7fff6dac9610) = -1 ENOENT (No such file or directory) stat(".git/packed-refs", 0x7fff6dac9460) = -1 ENOENT (No such file or directory) lstat(".git/refs/remotes/master", 0x7fff6dac9610) = -1 ENOENT (No such file or directory) stat(".git/packed-refs", 0x7fff6dac9460) = -1 ENOENT (No such file or directory) lstat(".git/refs/remotes/master/HEAD", 0x7fff6dac9610) = -1 ENOENT (No such file or directory) stat(".git/packed-refs", 0x7fff6dac9460) = -1 ENOENT (No such file or directory)

Since stat()s were never a strong side of Windows, I'm afraid 'refname:short'
fired backwards and made things much slower over there.  Ouch.

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.

Now, if 'git for-each-ref' could understand '**' globbing, not just
fnmatch...


The upcoming refname:strip does much better:

$ time git for-each-ref --format='%(refname:strip=2)' refs/tags >/dev/null

real    0m0.004s
user    0m0.000s
sys     0m0.004s

This is funny: after reading the commit message at
https://github.com/git/git/commit/0571979b it eludes me why strip=2 should
be so much faster than short...

Ciao,
Dscho


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]