From: SZEDER Gábor Date: Fri, 12 Feb 2016 21:40:48 GMT Subject: Re: [PATCH] git-completion.bash: always swallow error output of for-each-ref Message-ID: <20160212224048.Horde.IpOeDKLAMM4a11F2xyIeY4M@webmail.informatik.kit.edu> In-Reply-To: Quoting Junio C Hamano : > Jeff King 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 ()