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

> 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

> On Thu, Feb 04, 2016 at 11:34:59AM +0100, Sebastian Schuberth wrote:
> 
> > 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.

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

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

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

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

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

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

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

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

>  $ 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,
>
> On Sat, 13 Feb 2016, SZEDER Gábor wrote:
>
>>  $ 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

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

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

> 
> Quoting SZEDER Gábor <szeder@ira.uka.de>:
> 
> >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,

> On Sat, Feb 13, 2016 at 12:43:00AM +0100, SZEDER Gábor wrote:
>
>>
>> Quoting SZEDER Gábor <szeder@ira.uka.de>:
>>
>> >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,
>
> 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.

> On Thu, Feb 04, 2016 at 11:34:59AM +0100, Sebastian Schuberth wrote:
>
>> 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.

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

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

> 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.
>
>> @@ -320,7 +320,7 @@ __git_heads ()
>>  			refs/heads
>>  		return
>>  	fi
>> -}
>> +} 2>/dev/null
>
> Today I learned about yet another fun corner of POSIX shell.

> Jeff King <peff@peff.net> writes:
>
>> On Thu, Feb 04, 2016 at 11:34:59AM +0100, Sebastian Schuberth wrote:
>>
>>> 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.
>
> 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(-)
>
> diff --git a/contrib/completion/git-completion.bash  
> b/contrib/completion/git-completion.bash
> index ba4137d..637c42d 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -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'"

> On Fri, Feb 12, 2016 at 10:40:48PM +0100, SZEDER Gábor wrote:
>
>>  * 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...
>
>>  * 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

> Jeff King <peff@peff.net> writes:
>
>> On Fri, Feb 12, 2016 at 10:40:48PM +0100, SZEDER Gábor wrote:
>>
>>>  * 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...
>>
>>>  * 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
>
> Likewise on both counts.

> This avoids output like
>
>      warning: ignoring broken ref refs/remotes/origin/HEAD
>
> while completing branch names.
>
> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>