Re: [PATCH] contrib/completion: suppress stderror in bash
- From
SZEDER Gábor <szeder@ira.uka.de>
- Date
- Mar 4, 2015, 14:04 UTC
- Message-ID
- <20150304150447.Horde.t4rJ5Q_QdRg1uznQKWetMg1@webmail.informatik.kit.edu>
- In-Reply-To
- <20150210201648.Horde.QMVdqmJIJnvLkf0U46NdYA1@webmail.informatik.kit.edu>
Hi,
Quoting SZEDER Gábor <szeder@ira.uka.de>:
Show 44 quoted lines
> Hi,
>
> Quoting Junio C Hamano <gitster@pobox.com>:
>
>> SZEDER Gábor <szeder@ira.uka.de> 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