Re: [PATCH] contrib/completion: suppress stderror in bash completion of git remotes
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*/}"
Matt Korostoff
