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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx> wrote:
> Matt Korostoff <mkorostoff@xxxxxxxxx> 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*/}"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]