* [PATCH] contrib/completion: suppress stderror in bash completion of git remotes @ 2015-02-09 20:58 Matt Korostoff 2015-02-09 21:00 ` Matt Korostoff 2015-02-09 21:09 ` Junio C Hamano 0 siblings, 2 replies; 11+ messages in thread From: Matt Korostoff @ 2015-02-09 20:58 UTC (permalink / raw) To: git; +Cc: Matt Korostoff 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) ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] contrib/completion: suppress stderror in bash completion of git remotes 2015-02-09 20:58 [PATCH] contrib/completion: suppress stderror in bash completion of git remotes Matt Korostoff @ 2015-02-09 21:00 ` Matt Korostoff 2015-02-09 21:09 ` Junio C Hamano 1 sibling, 0 replies; 11+ messages in thread From: Matt Korostoff @ 2015-02-09 21:00 UTC (permalink / raw) To: git 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 <mkorostoff@gmail.com> 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) > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] contrib/completion: suppress stderror in bash completion of git remotes 2015-02-09 20:58 [PATCH] contrib/completion: suppress stderror in bash completion of git remotes Matt Korostoff 2015-02-09 21:00 ` Matt Korostoff @ 2015-02-09 21:09 ` Junio C Hamano 2015-02-10 0:08 ` Matt Korostoff 2015-02-10 2:10 ` [PATCH] contrib/completion: suppress stderror in bash SZEDER Gábor 1 sibling, 2 replies; 11+ messages in thread From: Junio C Hamano @ 2015-02-09 21:09 UTC (permalink / raw) To: Matt Korostoff; +Cc: git 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*/}" ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] contrib/completion: suppress stderror in bash completion of git remotes 2015-02-09 21:09 ` Junio C Hamano @ 2015-02-10 0:08 ` Matt Korostoff 2015-02-10 2:10 ` [PATCH] contrib/completion: suppress stderror in bash SZEDER Gábor 1 sibling, 0 replies; 11+ messages in thread From: Matt Korostoff @ 2015-02-10 0:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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: > 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*/}" ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] contrib/completion: suppress stderror in bash 2015-02-09 21:09 ` Junio C Hamano 2015-02-10 0:08 ` Matt Korostoff @ 2015-02-10 2:10 ` SZEDER Gábor 2015-02-10 15:25 ` [PATCH] contrib/completion: deprecate __git_remotes in bash completion Matt Korostoff 2015-02-10 18:31 ` [PATCH] contrib/completion: suppress stderror in bash Junio C Hamano 1 sibling, 2 replies; 11+ messages in thread From: SZEDER Gábor @ 2015-02-10 2:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Matt Korostoff, git Hi, Quoting Junio C Hamano <gitster@pobox.com>: > Matt Korostoff <mkorostoff@gmail.com> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] contrib/completion: deprecate __git_remotes in bash completion 2015-02-10 2:10 ` [PATCH] contrib/completion: suppress stderror in bash SZEDER Gábor @ 2015-02-10 15:25 ` Matt Korostoff 2015-02-10 18:31 ` [PATCH] contrib/completion: suppress stderror in bash Junio C Hamano 1 sibling, 0 replies; 11+ messages in thread From: Matt Korostoff @ 2015-02-10 15:25 UTC (permalink / raw) To: git; +Cc: Matt Korostoff Bash auto completion supplies a function __git_remotes which lists the git remotes of a repository by reading the .git/remotes directory. As of git 1.7.6 this is handled natively by the `git remote` command. This function is now deprecated. Signed-off-by: Matt Korostoff <MKorostoff@gmail.com> --- ***** Great point Gabor! I would hesitate a little about removing the function entirely, because users may have external scripts that rely on this function, but certainly for our internal purposes the __git_remotes shell function can be replaced with the native. Here's a short screen cast of the included patch working on my local http://i.imgur.com/6ZSMXCO.gif ***** contrib/completion/git-completion.bash | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 2fece98..8b41871 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -409,14 +409,14 @@ __git_refs_remotes () done } +# Deprecated wrapper function around git remote. Supplied for legacy purposes, +# to avoid breaking any external scripts which rely on this function. +# Originally this function was used to programmatically generate a list of git +# remotes by reading the .git/remotes directory, but as of git 1.7.6 that is +# natively handled by the git remote command __git_remotes () { - local i IFS=$'\n' d="$(__gitdir)" - test -d "$d/remotes" && ls -1 "$d/remotes" - for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do - i="${i#remote.}" - echo "${i/.url*/}" - done + git remote } __git_list_merge_strategies () @@ -558,7 +558,7 @@ __git_complete_remote_or_refspec () ((c++)) done if [ -z "$remote" ]; then - __gitcomp_nl "$(__git_remotes)" + __gitcomp_nl "$(git remote)" return fi if [ $no_complete_refspec = 1 ]; then @@ -927,7 +927,7 @@ _git_archive () return ;; --remote=*) - __gitcomp_nl "$(__git_remotes)" "" "${cur##--remote=}" + __gitcomp_nl "$(git remote)" "" "${cur##--remote=}" return ;; --*) @@ -1397,7 +1397,7 @@ _git_ls_files () _git_ls_remote () { - __gitcomp_nl "$(__git_remotes)" + __gitcomp_nl "$(git remote)" } _git_ls_tree () @@ -1639,7 +1639,7 @@ _git_push () { case "$prev" in --repo) - __gitcomp_nl "$(__git_remotes)" + __gitcomp_nl "$(git remote)" return ;; --recurse-submodules) @@ -1649,7 +1649,7 @@ _git_push () esac case "$cur" in --repo=*) - __gitcomp_nl "$(__git_remotes)" "" "${cur##--repo=}" + __gitcomp_nl "$(git remote)" "" "${cur##--repo=}" return ;; --recurse-submodules=*) @@ -1797,7 +1797,7 @@ _git_config () { case "$prev" in branch.*.remote|branch.*.pushremote) - __gitcomp_nl "$(__git_remotes)" + __gitcomp_nl "$(git remote)" return ;; branch.*.merge) @@ -1809,7 +1809,7 @@ _git_config () return ;; remote.pushdefault) - __gitcomp_nl "$(__git_remotes)" + __gitcomp_nl "$(git remote)" return ;; remote.*.fetch) @@ -1944,7 +1944,7 @@ _git_config () ;; remote.*) local pfx="${cur%.*}." cur_="${cur#*.}" - __gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "." + __gitcomp_nl "$(git remote)" "$pfx" "$cur_" "." __gitcomp_nl_append "pushdefault" "$pfx" "$cur_" return ;; @@ -2250,7 +2250,7 @@ _git_remote () case "$subcommand" in rename|remove|set-url|show|prune) - __gitcomp_nl "$(__git_remotes)" + __gitcomp_nl "$(git remote)" ;; set-head|set-branches) __git_complete_remote_or_refspec -- 1.7.10.2 (Apple Git-33) ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] contrib/completion: suppress stderror in bash 2015-02-10 2:10 ` [PATCH] contrib/completion: suppress stderror in bash SZEDER Gábor 2015-02-10 15:25 ` [PATCH] contrib/completion: deprecate __git_remotes in bash completion Matt Korostoff @ 2015-02-10 18:31 ` Junio C Hamano 2015-02-10 19:16 ` SZEDER Gábor 1 sibling, 1 reply; 11+ messages in thread From: Junio C Hamano @ 2015-02-10 18:31 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Matt Korostoff, git 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)? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] contrib/completion: suppress stderror in bash 2015-02-10 18:31 ` [PATCH] contrib/completion: suppress stderror in bash Junio C Hamano @ 2015-02-10 19:16 ` SZEDER Gábor 2015-03-04 14:04 ` SZEDER Gábor 0 siblings, 1 reply; 11+ messages in thread From: SZEDER Gábor @ 2015-02-10 19:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: Matt Korostoff, git 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. 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] contrib/completion: suppress stderror in bash 2015-02-10 19:16 ` SZEDER Gábor @ 2015-03-04 14:04 ` SZEDER Gábor 2015-03-04 14:10 ` [PATCH 1/2] completion: add a test for __git_remotes() helper function SZEDER Gábor 0 siblings, 1 reply; 11+ messages in thread From: SZEDER Gábor @ 2015-03-04 14:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Matt Korostoff, git Hi, Quoting SZEDER Gábor <szeder@ira.uka.de>: > 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 1/2] completion: add a test for __git_remotes() helper function 2015-03-04 14:04 ` SZEDER Gábor @ 2015-03-04 14:10 ` SZEDER Gábor 2015-03-04 14:10 ` [PATCH 2/2] completion: simplify __git_remotes() SZEDER Gábor 0 siblings, 1 reply; 11+ messages in thread From: SZEDER Gábor @ 2015-03-04 14:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Matt Korostoff, git, SZEDER Gábor The test checks that both remotes under '$GIT_DIR/remotes' and remotes in the config file are listed. Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> --- t/t9902-completion.sh | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index f10a752..7a883d1 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -351,6 +351,25 @@ test_expect_success '__gitcomp_nl - doesnt fail because of invalid variable name __gitcomp_nl "$invalid_variable_name" ' +test_expect_success '__git_remotes - list remotes from $GIT_DIR/remotes and from config file' ' + cat >expect <<-EOF && + remote_from_file_1 + remote_from_file_2 + remote_in_config_1 + remote_in_config_2 + EOF + test_when_finished "rm -rf .git/remotes" && + mkdir -p .git/remotes && + >.git/remotes/remote_from_file_1 && + >.git/remotes/remote_from_file_2 && + test_when_finished "git remote remove remote_in_config_1" && + git remote add remote_in_config_1 git://remote_1 && + test_when_finished "git remote remove remote_in_config_2" && + git remote add remote_in_config_2 git://remote_2 && + __git_remotes >actual && + test_cmp expect actual +' + test_expect_success 'basic' ' run_completion "git " && # built-in -- 2.1.2.1369.g8751039 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] completion: simplify __git_remotes() 2015-03-04 14:10 ` [PATCH 1/2] completion: add a test for __git_remotes() helper function SZEDER Gábor @ 2015-03-04 14:10 ` SZEDER Gábor 0 siblings, 0 replies; 11+ messages in thread From: SZEDER Gábor @ 2015-03-04 14:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Matt Korostoff, git, SZEDER Gábor The __git_remotes() helper function lists the remotes from the config file by processing the output of a 'git config' query. A simple 'git remote' produces the exact same output, so run that instead. Remotes under '$GIT_DIR/remotes' are still listed by running 'ls -1', because 'git remote' unfortunately ignores them. Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> --- contrib/completion/git-completion.bash | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index c21190d..f5ae5e3 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -411,12 +411,9 @@ __git_refs_remotes () __git_remotes () { - local i IFS=$'\n' d="$(__gitdir)" + local d="$(__gitdir)" test -d "$d/remotes" && ls -1 "$d/remotes" - for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do - i="${i#remote.}" - echo "${i/.url*/}" - done + git --git-dir="$d" remote } __git_list_merge_strategies () -- 2.1.2.1369.g8751039 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-03-04 14:11 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-02-09 20:58 [PATCH] contrib/completion: suppress stderror in bash completion of git remotes Matt Korostoff 2015-02-09 21:00 ` Matt Korostoff 2015-02-09 21:09 ` Junio C Hamano 2015-02-10 0:08 ` Matt Korostoff 2015-02-10 2:10 ` [PATCH] contrib/completion: suppress stderror in bash SZEDER Gábor 2015-02-10 15:25 ` [PATCH] contrib/completion: deprecate __git_remotes in bash completion Matt Korostoff 2015-02-10 18:31 ` [PATCH] contrib/completion: suppress stderror in bash Junio C Hamano 2015-02-10 19:16 ` SZEDER Gábor 2015-03-04 14:04 ` SZEDER Gábor 2015-03-04 14:10 ` [PATCH 1/2] completion: add a test for __git_remotes() helper function SZEDER Gábor 2015-03-04 14:10 ` [PATCH 2/2] completion: simplify __git_remotes() SZEDER Gábor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).
