Re: [PATCH] contrib/completion: suppress stderror in bash
Hi,
Quoting SZEDER Gábor :
Hi,
Quoting Junio C Hamano :
SZEDER Gábor writes:
@@ -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*/}"
Do I smell some bitrotting here?
This function just lists all the defined remotes, first by listing the
directories under refs/remotes to get the "legacy" remotes and then
loops over 'git config's output to get the "modern" ones. This
predates the arrival of the 'git remote' command in January 2007, so
it was really a long time ago.
We should just run 'git remote' instead, shouldn't we?
Perhaps. Is it sufficient to just make __git_remotes() a thin
wrapper around, i.e.
__git_remotes ()
{
git remotes
}
or do we need to munge its output further (I didn't look)?
Well, just like in other cases where we run git from the completion
script, we need a '--git-dir="$(__gitdir)"' as well, because the user can
specify the path to a different repo via $GIT_DIR or on the command
line.
Other than that it seems we are OK. Docs say "With no arguments,
shows a list of existing remotes." and as far as I can tell, on
MSysGit, it does so without any funny formatting.
Oh, look what forgotten treasure did I stumble upon in the vaults:
https://github.com/szeder/git/commit/e4e3760c15b485b9ff4768e13050f4b19b5968b8
A two and a half year old commit in my old git repo doing the same...
completely forgotten :)
Unfortunately, however, it's not quite that simple, because 'git
remote' doesn't list remotes under '$GIT_DIR/remotes'. Or at least I
would have expected the following test to work, but it does not:
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 17c6330..6a4c139 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -734,6 +734,15 @@ Pull: refs/heads/master:refs/heads/origin
Pull: refs/heads/next:refs/heads/origin2
EOF
+test_expect_success 'list remote in $GIT_DIR/remotes' '
+ mkdir .git/remotes &&
+ test_when_finished "rm -rf .git/remotes" &&
+ cat remotes_origin >.git/remotes/remote_from_file &&
+ git remote >actual &&
+ echo remote_from_file >expect &&
+ test_cmp expect actual
+'
+
test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
git clone one five &&
origin_url=$(pwd)/one &&
because listing remotes is implemented by for_each_remote(), which
only reads remotes from the config file.
Now, considering how old 'git remote' is, there were plenty of time
for someone to miss this functionality and complain about it, but
since it's still not implemented is probably a good sign that noone
has actually missed it. And I don't think it's worth implementing it
now just to shave off two more lines from the completion script.
Anyway, 'git remote' could still replace that 'git config' query. I
have the patches ready and it seems I got send-email working, so
they'll follow in a minute or two.
Best,
Gábor
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/completion: suppress stderror in bash
Hi,
Quoting Junio C Hamano :
SZEDER Gábor writes:
@@ -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*/}"
Do I smell some bitrotting here?
This function just lists all the defined remotes, first by listing the
directories under refs/remotes to get the "legacy" remotes and then
loops over 'git config's output to get the "modern" ones. This
predates the arrival of the 'git remote' command in January 2007, so
it was really a long time ago.
We should just run 'git remote' instead, shouldn't we?
Perhaps. Is it sufficient to just make __git_remotes() a thin
wrapper around, i.e.
__git_remotes ()
{
git remotes
}
or do we need to munge its output further (I didn't look)?
Well, just like in other cases where we run git from the completion
script, we need a '--git-dir="$(__gitdir)"' as well, because the user can
specify the path to a different repo via $GIT_DIR or on the command
line.
Other than that it seems we are OK. Docs say "With no arguments,
shows a list of existing remotes." and as far as I can tell, on
MSysGit, it does so without any funny formatting.
But beware, I spent most of last year cycling to Mongolia and my git-fu
became a somewhat rusty... and I'm not quite up to speed yet.
Best,
Gábor
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/completion: suppress stderror in bash
SZEDER Gábor writes:
>>> @@ -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*/}"
>
> Do I smell some bitrotting here?
>
> This function just lists all the defined remotes, first by listing the
> directories under refs/remotes to get the "legacy" remotes and then
> loops over 'git config's output to get the "modern" ones. This
> predates the arrival of the 'git remote' command in January 2007, so
> it was really a long time ago.
>
> We should just run 'git remote' instead, shouldn't we?
Perhaps. Is it sufficient to just make __git_remotes() a thin
wrapper around, i.e.
__git_remotes ()
{
git remotes
}
or do we need to munge its output further (I didn't look)?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/completion: suppress stderror in bash
Hi,
Quoting Junio C Hamano :
Matt Korostoff writes:
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*/}"
Do I smell some bitrotting here?
This function just lists all the defined remotes, first by listing the
directories under refs/remotes to get the "legacy" remotes and then
loops over 'git config's output to get the "modern" ones. This
predates the arrival of the 'git remote' command in January 2007, so
it was really a long time ago.
We should just run 'git remote' instead, shouldn't we?
Cheers,
Gábor
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 wrote:
> Matt Korostoff 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/completion: suppress stderror in bash completion of git remotes
Matt Korostoff 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/completion: suppress stderror in bash completion of git remotes
Here are some screen shots demonstrating the issue I'm describing here:
before this patch:
https://cloud.githubusercontent.com/assets/1197335/6108333/1f3b10fa-b040-11e4-9164-3c7769dae110.gif
after this patch:
https://cloud.githubusercontent.com/assets/1197335/6108340/3878cad0-b040-11e4-9994-dcd5c4d62bba.gif
On Mon, Feb 9, 2015 at 3:58 PM, Matt Korostoff wrote:
> 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.
>
> 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*/}"
> --
> 1.7.10.2 (Apple Git-33)
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
