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

From: SZEDER Gábor <szeder@ira.uka.de>
Date: Feb 12, 2016, 21:40 UTC
Message-ID: <20160212224048.Horde.IpOeDKLAMM4a11F2xyIeY4M@webmail.informatik.kit.edu>
In-Reply-To: <xmqqsi0xu2ac.fsf@gitster.mtv.corp.google.com>

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

Show 28 quoted lines

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

Show 52 quoted lines

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