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

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

[Sebastian Schuberth]

This avoids output like

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

while completing branch names.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 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 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
 	fi
 }
@@ -327,7 +327,7 @@ __git_tags ()
 	local dir="$(__gitdir)"
 	if [ -d "$dir" ]; then
 		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
-			refs/tags
+			refs/tags 2>/dev/null
 		return
 	fi
 }
@@ -355,14 +355,14 @@ __git_refs ()
 			;;
 		esac
 		git --git-dir="$dir" for-each-ref --format="%($format)" \
-			$refs
+			$refs 2>/dev/null
 		if [ -n "$track" ]; then
 			# employ the heuristic used by git checkout
 			# Try to find a remote branch that matches the completion word
 			# but only output if the branch name is unique
 			local ref entry
 			git --git-dir="$dir" for-each-ref --shell --format="ref=%(refname:short)" \
-				"refs/remotes/" | \
+				"refs/remotes/" 2>/dev/null | \
 			while read -r entry; do
 				eval "$entry"
 				ref="${ref#*/}"
@@ -1835,7 +1835,7 @@ _git_config ()
 		remote="${remote%.push}"
 		__gitcomp_nl "$(git --git-dir="$(__gitdir)" \
 			for-each-ref --format='%(refname):%(refname)' \
-			refs/heads)"
+			refs/heads 2>/dev/null)"
 		return
 		;;
 	pull.twohead|pull.octopus)
-- 
2.7.0.windows.1

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

[Jeff King]

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.

> 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

Obviously these are pretty small timings from my git.git with ~600 tags,
but you can see that refname:short really does cost more.  On a more
ridiculous example repository I have with about 10 million refs, the
timings are more like 5s, 66s, 5.5s.

Just thought I'd throw that our there in case any completion people feel
like poking around with it.

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Thu, 4 Feb 2016, Jeff King wrote:

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

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

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

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

Maybe we should just shut up the completions in more cases? Dunno...

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

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

[Jeff King]

On Thu, Feb 04, 2016 at 12:26:19PM +0100, Johannes Schindelin wrote:

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

Good, I'm not going crazy! But my search skills are apparently
atrophying. :)

It looks like we just addressed the git-gc issue there. for-each-ref
uses the "rawref" interface, so it gets fed broken things and warns
about them.

I'm tempted to say that it should just silently ignore broken symrefs,
as they're kind-of a normal thing. But I also think Sebastian's patch to
squelch stderr during completion is quite reasonable, too.

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

:short is slow because it checks for ambiguity. So it has to walk the
dwim_ref() rules backwards, checking if each possibility is an existing
ref.

Whereas strip=2 is literally just skipping past the early bits of the
refname string.

-Peff

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

"short" tries to ensure that the result is not ambiguous within the
repository, so when asked to shorten refs/heads/foo, it needs to
check if refs/tags/foo exists.  "strip=2" textually strips two
levels from the top without worrying about ambiguity across
different hierarchies.

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

[Sebastian Schuberth]

On 04.02.2016 11:34, Sebastian Schuberth wrote:

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

The discussion got a bit off the point with the "short" vs. "strip=2" 
stuff, but I guess the patch itself if good to apply?

-- 
Sebastian Schuberth

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

[Junio C Hamano]

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

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

[Jeff King]

On Fri, Feb 12, 2016 at 12:00:43PM -0800, Junio C Hamano wrote:

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

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

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

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

I may have learned about this soon after I started learning bash,
but I admit that this was the first time I found a practical use
case for it ;-).

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

[SZEDER Gábor]

Quoting Junio C Hamano <gitster@pobox.com>:

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

Please no :)

First, we don't have to redirect stderr of every completion function,
it's sufficient to do so only for the two "main" entry point functions
__git_main() and __gitk_main().

But:

  * It would swallow even those errors that we are interested in,
    e.g. (note the missing quotes around $foo):

       $ func () { if [ $foo = y ] ; then echo "foo is y" ; fi ; }
       $ foo=
       $ func 2>/dev/null
       $ func
       bash: [: =: unary operator expected

    Something like this should not happen, it's a bug in the
    completion script that should be fixed, and we should get a bug
    report.

  * I often find myself tracing/debugging the completion script
    through stderr by scattering

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

    and the like all over the place.  If completion functions' stderr
    were redirected, then I would have to disable that redirection
    first to be able do this kind of poor man's tracing.

  * I have a WIP patch series that deals with errors from git
    commands.
    It's a mixed bag of __gitdir()-related cleanups, fixes and
    optimizations, which factors out all git executions into a
    __git() wrapper function and redirects stderr only in that
    function, thereby eliminating most of the 2>/dev/null
    redirections in the completion script.
    It still needs some work to iron out a wrinkle or two around
    corner cases, though.



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

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

[Jeff King]

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

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

[SZEDER Gábor]

Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:

> 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

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

[Junio C Hamano]

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.

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

[Jeff King]

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

-Peff

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

[SZEDER Gábor]

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,

'git for-each-ref' does in fact understand double asterisks:

   $ git for-each-ref --format='%(refname)' '**/master'
   refs/heads/master
   refs/remotes/github/master
   refs/remotes/origin/master
   $ git for-each-ref --format='%(refname)' 'refs/heads/b/**'
   refs/heads/b/r/a/n/c/h

Great, this combined with refname:strip=2 and 3 might open up some
more optimization possibilities...

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

[Jeff King]

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.

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

-Peff

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

[Duy Nguyen]

On Sat, Feb 13, 2016 at 6:46 AM, Jeff King <peff@peff.net> wrote:
> 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).

The second reason is consistent behavior across platforms.

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

Nope. I think that's the only difference, feature-wise, between
fnmatch and wildmatch.

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

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

[SZEDER Gábor]

Quoting Jeff King <peff@peff.net>:

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


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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

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
> 
> Not bad for a Friday night, huh? :)

Nope, not bad at all. May I have that patch, please? ;-)

Ciao,
Dscho

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

[SZEDER Gábor]

Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:

> 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

I timed this one using a version that already included one from those
"few more tricks", so the change from ':short' to ':strip=2' alone
doesn't bring quite as much:

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

   real  0m3.645s
   user  0m3.140s
   sys   0m0.588s


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

And this one now looks like:

   $ cur=m ; time __gitcomp_direct "$(__git_refs_PoC '' 1)"

The timing results are the same.


> May I have that patch, please? ;-)

It's early days, and when I say proof of concept I mean it :)
For now it only works for refs from the local repository, and only
when the ref to be completed is on its own on the command line (i.e.
not for 'git log master..<TAB>' or 'commit --fixup=<TAB>'), and the
trailing space is hardcoded, and ...  though, arguably, that already
covers the majority of the cases.  I only switched 'git checkout' to
use this optimized version, because that was the worst offender.

So I won't send patches to the list just now, but you or anyone
interested can take a peek at:

   https://github.com/szeder/git.git completion-PoC-refs-speedup

Maybe even run some numbers on Windows?


Gábor

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

[Jeff King]

On Sat, Feb 13, 2016 at 02:07:12AM +0100, SZEDER Gábor wrote:

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

Yeah, switching to :strip would arguably be a regression when completing
all refs. Right now, you'd get "heads/foo" and "tags/foo" as part of
your completion (but _not_ just "foo"), and either works as a
non-ambiguous ref.

With :strip, you'd just get "foo" twice, and if you use the result of
the completion, it will always point to the tag.

So it is arguably worse. I still think it is worth trading off for
performance, but it is worth acknowledging in the commit message there
that it is a tradeoff.

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

Very cool. I look forward to seeing the final patch. :)

I have noticed in my pathological 10-million-ref bare repositories
(don't ask) that the __git_ps1() prompt is quite slow, too. And I
wondered if it could be related.

But I don't think it is. It's just literally that painful to look at the
packed-refs at all, and "git rev-parse HEAD" has to look at them to
resolve.

-Peff

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

Hi Gábor,

On Sat, 13 Feb 2016, SZEDER Gábor wrote:

> Maybe even run some numbers on Windows?

Here you are:

-- snip --
# Junio's 494398473, i.e. `master
$ cur=m ; time __gitcomp_nl "$(__git_refs '' 1)"

real    0m8.260s
user    0m0.265s
sys     0m0.216s

# Gábor's 6fc6f416, i.e. with strip=2
$ cur=m ; time __gitcomp_nl "$(__git_refs '' 1)"

real    0m2.072s
user    0m0.295s
sys     0m0.203s

# Gábor's 47b146d7, i.e. `completion-PoC-refs-speedup`
$ cur=m ; time __gitcomp_direct "$(__git_refs_PoC '' 1)"

real    0m1.574s
user    0m0.030s
sys     0m0.015s
-- snap --

This is with a fairly normal, real-world repository:

-- snip --
$ git for-each-ref | wc -l
10303
-- snap --

Ciao,
Dscho

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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.

Thanks.

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

[Sebastian Schuberth]

On Wed, Feb 24, 2016 at 12:30 AM, Junio C Hamano <gitster@pobox.com> wrote:

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

Thanks for the follow-up, I was about to ask for a status update on
this. As my patch it ready now, and we don't know how long we'd have
to wait for the other solution, I'd vote for applying my patch.

-- 
Sebastian Schuberth