Re: [PATCH] contrib/completion: suppress stderror in bash completion of git remotes

From: Matt Korostoff <mkorostoff@gmail.com>
Date: Feb 10, 2015, 00:08 UTC
Message-ID: <CAMZO7d+qL-rzvi0GrAuzAg=02MxByMt0Zc=fMnvK175YQyB8YA@mail.gmail.com>
In-Reply-To: <xmqqy4o6aj1w.fsf@gitster.dls.corp.google.com>

Thank you for your detailed reply Junio. I'll try to address your concerns individually, but let me also offer a general thought that this is probably a good use case to handle even if the root cause is solvable outside of git. That is to say, I would think we'd still want git autocompletion working for users running on imperfect platforms.

> What's "some system"?

My apologies, that was a typo. I meant to write "some systems." I'm not sure what the root cause is, but I can tell you I'm running a rather vanilla development environment (git 1.7.10, bash 3.2, osx 10.8). I wish I could supply a list of the version combinations that result in such an event, but I'm not sure how I would do acquire a list like that.

> Is this a platform's bug (e.g. "test -d" does not work correctly)?

I don't believe so—here's a simple test-of-the-test I threw together https://gist.github.com/MKorostoff/f203e414847d43b21de4 which does not throw this error.

> Is this an configuration error of user's Git repository?

I think I have a pretty generic git configuration (here it is, though note I've had to redact some identifying information https://gist.github.com/MKorostoff/f8358f72b968249a3925). Still, I'd think we would want to handle such a misconfiguration explicitly, rather than throw a seemingly unrelated error during autocompletion

> Is this something else?

It would be very helpful if you could supply a few more details on the type of something you're looking for

> I _think_ you would see the problem if $d/remotes is a directory
> whose contents cannot be listed

I can confirm, that is indeed the behavior. Animated gif example here http://i.imgur.com/qcPxAub.gif

> But I wonder if we rather want the user to notice that
> misconfiguration so that the user can correct it

While I wholeheartedly agree that such user feedback would be valuable, I'm just not sure that it makes sense for this feedback to interrupt the user's typing mid-word.

Sorry if anyone receives this twice, my first attempt to send was rejected for including HTML.

On Mon, Feb 9, 2015 at 4:09 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 46 quoted lines

> Matt Korostoff <mkorostoff@gmail.com> writes:
>
>> In some system configurations there is a bug with the
>> __git_remotes function.  Specifically, there is a problem
>> with line 415, `test -d "$d/remotes" && ls -1 "$d/remotes"`.
>> While `test -d` is meant to prevent listing the remotes
>> directory if it does not exist, in some system, `ls` will
>> run regardless.
>
> What's "some system"?
>
> Is this a platform's bug (e.g. "test -d" does not work correctly)?
>
> Is this an configuration error of user's Git repository?
>
> Is this something else?
>
> I _think_ you would see the problem if $d/remotes is a directory
> whose contents cannot be listed (e.g. "chmod a= $d/remotes"), and
> that would not be a platform's bug (i.e. "test -d" would happily say
> "Yes there is a directory", and "ls" would fail with "Permission
> denied").  But I wonder if we rather want the user to notice that
> misconfiguration so that the user can correct it, instead of hiding
> the error message from "ls".
>
>> This results in an error in which typing `git push or` + `tab`
>> prints out `ls: .git/remotes: No such file or directory`.
>> This can be fixed by simply directing stderror of this line
>> to /dev/null.
>> ---
>>  contrib/completion/git-completion.bash |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 2fece98..72251cc 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -412,7 +412,7 @@ __git_refs_remotes ()
>>  __git_remotes ()
>>  {
>>       local i IFS=$'\n' d="$(__gitdir)"
>> -     test -d "$d/remotes" && ls -1 "$d/remotes"
>> +     test -d "$d/remotes" && ls -1 "$d/remotes" 2>/dev/null
>>       for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
>>               i="${i#remote.}"
>>               echo "${i/.url*/}"