threads / discuss / 31611

Problem completing remotes when .git/remotes exits

Subject: Problem completing remotes when .git/remotes exits

## tl;dr

6 messages between Sep 19, 2012 and Sep 26, 2012.

replies: 5people: 3as markdown or json

Johannes Sixt· Sep 19, 2012, 19:55 UTC · lore

I have an empty .git/remotes directory. Trying to complete the name of a remote always reports an error:

git@master:1023> git fetch <TAB>ls: invalid option -- ' ' Try `ls --help' for more information.

I have these:

	alias ls='ls $LS_OPTIONS'
and
	LS_OPTIONS='-N --color=tty -T 0'

I instrumented __git_remotes with set -x, which shows:

git@master:1006> git fetch <TAB>+++ __gitdir +++ '[' -z '' ']' +++ '[' -n '' ']' +++ '[' -n '' ']' +++ '[' -d .git ']' +++ echo .git ++ local i 'IFS= ' d=.git ++ test -d .git/remotes ++ ls '-N --color=tty -T 0' -1 .git/remotes ls: invalid option -- ' ' Try `ls --help' for more information. ...

Notice that the expansion of $LS_OPTIONS is not split at the blanks, obviously, because $IFS does not contain a blank at that moment.

The patch below helps, but it looks like a work-around rather than a solution. Ideas?

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index be800e0..824f5b6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -369,8 +369,9 @@ __git_refs_remotes ()
 
 __git_remotes ()
 {
-	local i IFS=$'\n' d="$(__gitdir)"
+	local i d="$(__gitdir)"
 	test -d "$d/remotes" && ls -1 "$d/remotes"
+	local IFS=$'\n'
 	for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
 		i="${i#remote.}"
 		echo "${i/.url*/}"

SZEDER Gábor· Sep 25, 2012, 23:00 UTC · re: Johannes Sixt · lore

Re: Problem completing remotes when .git/remotes exits

Hi,

On Wed, Sep 19, 2012 at 09:55:28PM +0200, Johannes Sixt wrote:

Show 33 quoted lines

> I have an empty .git/remotes directory. Trying to complete the name of
> a remote always reports an error:
> 
> git@master:1023> git fetch <TAB>ls: invalid option -- ' '
> Try `ls --help' for more information.
> 
> I have these:
> 
> 	alias ls='ls $LS_OPTIONS'
> and
> 	LS_OPTIONS='-N --color=tty -T 0'
> 
> I instrumented __git_remotes with set -x, which shows:
> 
> git@master:1006> git fetch <TAB>+++ __gitdir
> +++ '[' -z '' ']'
> +++ '[' -n '' ']'
> +++ '[' -n '' ']'
> +++ '[' -d .git ']'
> +++ echo .git
> ++ local i 'IFS=
> ' d=.git
> ++ test -d .git/remotes
> ++ ls '-N --color=tty -T 0' -1 .git/remotes
> ls: invalid option -- ' '
> Try `ls --help' for more information.
> ...
> 
> Notice that the expansion of $LS_OPTIONS is not split at the blanks,
> obviously, because $IFS does not contain a blank at that moment.
> 
> The patch below helps, but it looks like a work-around rather than a
> solution. Ideas?

I've got two alternative solutions for this issue.

The first one is less intrusive: use the 'command' builtin to tell the shell to ignore shell functions and aliases and just run the ls command.

diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index be800e09..bcde9472 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -370,7 +370,7 @@ __git_refs_remotes ()
 __git_remotes ()
 {
 	local i IFS=$'\n' d="$(__gitdir)"
-	test -d "$d/remotes" && ls -1 "$d/remotes"
+	test -d "$d/remotes" && command 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*/}"


But then it got me thinking...  Notice how much effort we spend just
to get the list of remotes?  We could just run 'git remote' directly
instead...

diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index be800e09..1daeaccf 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -369,12 +369,8 @@ __git_refs_remotes ()
 
 __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
+	local d="$(__gitdir)"
+	git --git-dir="$d" remote
 }
 
 __git_list_merge_strategies ()


I prefer the second one ;)

Best,
Gábor

Junio C Hamano· Sep 25, 2012, 23:43 UTC · re: SZEDER Gábor · lore

Re: Problem completing remotes when .git/remotes exits

SZEDER Gábor <szeder@ira.uka.de> writes:

> -	test -d "$d/remotes" && ls -1 "$d/remotes"
> +	test -d "$d/remotes" && command ls -1 "$d/remotes"

Yuck. For normal scripts, nobody sane would define "alias" for non-interactive environments, but because these things work in an interactive environment, we have to protect ourselves from user aliases. Not just "ls", but "test" we see above may misbehave X-<.

SZEDER Gábor· Sep 26, 2012, 11:43 UTC · re: Junio C Hamano · lore

Re: Problem completing remotes when .git/remotes exits

On Tue, Sep 25, 2012 at 04:43:59PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > -	test -d "$d/remotes" && ls -1 "$d/remotes"
> > +	test -d "$d/remotes" && command ls -1 "$d/remotes"
> 
> Yuck.  For normal scripts, nobody sane would define "alias" for
> non-interactive environments, but because these things work in an
> interactive environment, we have to protect ourselves from user
> aliases.  Not just "ls", but "test" we see above may misbehave X-<.

Right, however, while ls is frequently aliased (my ubuntu box has alias ls='ls --color=auto' in /etc/skel/.bashrc by default, but that's not an issue in this case), I think aliasing test is just crazy. Yeah, it's possible, but if we go down that route, then we should also worry about the [ builtin being aliased:

$ if [ -r nonexisting ] ; then echo found ; fi $ alias [='echo using aliased [' $ if [ -r nonexisting ] ; then echo found ; fi using aliased [ -r nonexisting ] found $

SZEDER Gábor· Sep 26, 2012, 01:09 UTC · re: SZEDER Gábor · lore

Re: Problem completing remotes when .git/remotes exits

On Wed, Sep 26, 2012 at 01:00:45AM +0200, SZEDER Gábor wrote:

> But then it got me thinking...  Notice how much effort we spend just
> to get the list of remotes?  We could just run 'git remote' directly
> instead...

Actually, we can't, because 'git remote' doesn't seem to list remotes stored under .git/remotes. Is that intentional?

Anyway, we could still use 'git remote' to replace at least the config query and the for loop to spare a few lines of code and a subshell.

SZEDER Gábor· Sep 26, 2012, 01:26 UTC · re: SZEDER Gábor · lore

Re: Problem completing remotes when .git/remotes exits

On Wed, Sep 26, 2012 at 03:09:38AM +0200, SZEDER Gábor wrote:

Show 7 quoted lines

> On Wed, Sep 26, 2012 at 01:00:45AM +0200, SZEDER Gábor wrote:
> > But then it got me thinking...  Notice how much effort we spend just
> > to get the list of remotes?  We could just run 'git remote' directly
> > instead...
> 
> Actually, we can't, because 'git remote' doesn't seem to list remotes
> stored under .git/remotes.  Is that intentional?

Looks like a bug, as it seems to have been lost in translation in 211c8968 (Make git-remote a builtin, 2008-02-29).

list_remote() in git-remote.perl looks for remotes in config and in .git/remotes/, too. The builtin implementation uses remote.c:for_each_remote() from the start, which only looks at the config.