git-completion.bash: always swallow error output of for-each-ref

This avoids output like

warning: ignoring broken ref refs/remotes/origin/HEAD

while completing branch names.

Quoted 5 lines · Sebastian Schuberth

This avoids output like

warning: ignoring broken ref refs/remotes/origin/HEAD

while completing branch names.

Hmm. I feel like this case (HEAD points to a branch, then fetch --prune deletes it) came up recently and we discussed quieting that warning. But now I cannot seem to find it.

Anyway, I this is a reasonable workaround. Errors from bash completion scripts are almost always going to be useless and get in the way of reading your own prompt.

contrib/completion/git-completion.bash __git_heads () · line 321

Quoted 11 lines

contrib/completion/git-completion.bash +1 -1

index 15ebba5..7c0549d 100644
@@ -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:

$ 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

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

Quoted 5 lines

$ 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

Quoted 4 lines

The upcoming refname:strip does much better:

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

Quoted 3 lines

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...

Hi Peff,

Quoted 18 lines · Jeff King

Review

Quoted 1 line

(quoted diff) +0 -0

b/contrib/completion/git-completion.bash

contrib/completion/git-completion.bash __git_heads () · line 321

Quoted 10 lines

index 15ebba5..7c0549d 100644

contrib/completion/git-completion.bash +1 -1

@@ -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.

Quoted 1 line

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

Quoted 6 lines

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

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

Quoted 5 lines

$ 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

Quoted 7 lines

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

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...

Quoted 9 lines

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.

Quoted 2 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).

Quoted 2 lines

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.

Quoted 2 lines

I think it does already, since 4917e1e (Makefile: promote wildmatch to be the default fnmatch implementation, 2013-05-30).

Things are looking up!
[...vast improvement in times...]

 $ 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? :)

Hi Gábor,

Quoted 13 lines · SZEDER Gábor

 $ 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

Quoted 9 lines

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

May I have that patch, please? ;-)

Maybe even run some numbers on Windows?

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

Quoting SZEDER Gábor <[email protected]>:

Quoted 2 lines

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

Oh, look, though the manpage says:

<pattern>...
If one or more patterns are given, only refs are shown that match against at least one pattern, either using fnmatch(3) or literally,

Quoted 8 lines · SZEDER Gábor

Quoting SZEDER Gábor <[email protected]>:

Quoted 2 lines

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

Oh, look, though the manpage says:

Quoted 3 lines

<pattern>...
If one or more patterns are given, only refs are shown that match against at least one pattern, either using fnmatch(3) or literally,

Yeah, we might want to update that. Wildmatch is basically fnmatch() compatible, but it understands "**" (which I _think_ is the reason we picked it up in the first place).

I think we dropped it into place by
default because "**" is otherwise meaningless for fnmatch.

I don't think there are any other differences between the two, but Duy probably knows offhand.

It looks like we mention fnmatch() in a few places in the documentation, and AFAIK each of these is now outdated.

Quoted 3 lines

Hmm. I feel like this case (HEAD points to a branch, then fetch --prune deletes it) came up recently and we discussed quieting that warning. But now I cannot seem to find it.

I am pretty certain that it came up in my patch series:

	http://thread.gmane.org/gmane.comp.version-control.git/278538

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...

Quoted 5 lines · Sebastian Schuberth

This avoids output like

warning: ignoring broken ref refs/remotes/origin/HEAD

while completing branch names.

Hmm. I feel like this case (HEAD points to a branch, then fetch --prune deletes it) came up recently and we discussed quieting that warning. But now I cannot seem to find it.

Anyway, I this is a reasonable workaround. Errors from bash completion scripts are almost always going to be useless and get in the way of reading your own prompt.

Quoted 3 lines

Anyway, I this is a reasonable workaround. Errors from bash completion scripts are almost always going to be useless and get in the way of reading your own prompt.

I think that is absolutely the right stance to take, but then I wonder if it is a sensible execution to sprinkle 2>/dev/null everywhere.

For example, couldn't we do something like this instead?

This is just for illustration and does not remove all 2>/dev/null and replace them with a single redirection that covers the entire shell function body, but something along this line smells a lot more pleasant. I dunno.

I agree it's a lot more pleasant, assuming there are no cases where we would want to pass through an error. But I really cannot think of one. Even explosive "woah, your git repo is totally corrupted" messages probably should be suppressed in the prompt.

contrib/completion/git-completion.bash __git_heads () · line 323

Quoted 6 lines

contrib/completion/git-completion.bash +1 -1

@@ -320,7 +320,7 @@ __git_heads ()
 			refs/heads
 		return
 	fi
-}
+} 2>/dev/null

Today I learned about yet another fun corner of POSIX shell.

Quoted 8 lines · Jeff King

Quoted 5 lines · Sebastian Schuberth

This avoids output like

warning: ignoring broken ref refs/remotes/origin/HEAD

while completing branch names.

Quoted 7 lines

Hmm. I feel like this case (HEAD points to a branch, then fetch --prune deletes it) came up recently and we discussed quieting that warning. But now I cannot seem to find it.

Anyway, I this is a reasonable workaround. Errors from bash completion scripts are almost always going to be useless and get in the way of reading your own prompt.

I think that is absolutely the right stance to take, but then I wonder if it is a sensible execution to sprinkle 2>/dev/null everywhere.

For example, couldn't we do something like this instead?

This is just for illustration and does not remove all 2>/dev/null and replace them with a single redirection that covers the entire shell function body, but something along this line smells a lot more pleasant. I dunno.

 contrib/completion/git-completion.bash | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

contrib/completion/git-completion.bash +5 -5

index ba4137d..637c42d 100644
@@ -47,14 +47,14 @@ __gitdir ()
 		elif [ -d .git ]; then
 			echo .git
 		else
-			git rev-parse --git-dir 2>/dev/null
+			git rev-parse --git-dir
 		fi
 	elif [ -d "$1/.git" ]; then
 		echo "$1/.git"
 	else
 		echo "$1"
 	fi
-}
+} 2>/dev/null
 
 # The following function is based on code from:
 #

@@ -320,7 +320,7 @@ __git_heads ()
 			refs/heads
 		return
 	fi
-}
+} 2>/dev/null
 
 __git_tags ()
 {

@@ -330,7 +330,7 @@ __git_tags ()
 			refs/tags
 		return
 	fi
-}
+} 2>/dev/null
 
 # __git_refs accepts 0, 1 (to pass to __gitdir), or 2 arguments
 # presence of 2nd argument means use the guess heuristic employed

@@ -389,7 +389,7 @@ __git_refs ()
 			"refs/remotes/$dir/" 2>/dev/null | sed -e "s#^$dir/##"
 		;;
 	esac
-}
+} 2>/dev/null
 
 # __git_refs2 requires 1 argument (to pass to __git_refs)
 __git_refs2 ()

* It would swallow even those errors that we are interested in, e.g. (note the missing quotes around $foo):
[...]
* I often find myself tracing/debugging the completion script through stderr by scattering

      echo >&2 "foo: '$foo'"

* I have a WIP patch series that deals with errors from git commands.

Quoted 7 lines · SZEDER Gábor

* It would swallow even those errors that we are interested in, e.g. (note the missing quotes around $foo):
[...]
* I often find myself tracing/debugging the completion script through stderr by scattering

      echo >&2 "foo: '$foo'"

One alternative to deal with these would be to add a flag to conditionally turn off stderr, and then leave it on during normal operation and disable it (letting everything through, including whatever random cruft git commands produce) for debugging.

But...

Quoted 2 lines

* I have a WIP patch series that deals with errors from git commands.

I'm happy to wait and see what this patch looks like (and generally happy to defer to you on maintenance issues for completion, as you are much more likely than me to be the one fixing things later on :) ).

-Peff

Quoted 20 lines · Jeff King

Quoted 7 lines · SZEDER Gábor

* It would swallow even those errors that we are interested in, e.g. (note the missing quotes around $foo):
[...]
* I often find myself tracing/debugging the completion script through stderr by scattering

      echo >&2 "foo: '$foo'"

One alternative to deal with these would be to add a flag to conditionally turn off stderr, and then leave it on during normal operation and disable it (letting everything through, including whatever random cruft git commands produce) for debugging.

But...

Quoted 2 lines

* I have a WIP patch series that deals with errors from git commands.

Quoted 5 lines

I'm happy to wait and see what this patch looks like (and generally happy to defer to you on maintenance issues for completion, as you are much more likely than me to be the one fixing things later on :) ).

-Peff

Likewise on both counts.

So, have we decided to wait, or we'd rather apply the band-aid in the meantime? I can go either way, just double checking as I noticed this thread while updating my leftover bits list.

This avoids output like

warning: ignoring broken ref refs/remotes/origin/HEAD

while completing branch names.

Signed-off-by Sebastian Schuberth <[email protected]>