Re: Problem completing remotes when .git/remotes exits
Hi, On Wed, Sep 19, 2012 at 09:55:28PM +0200, Johannes Sixt wrote: > I have an empty .git/remotes directory. Trying to complete the name of > a remote always reports an error: > > git@master:1023> git fetch 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 +++ __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 -- 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: Problem completing remotes when .git/remotes exits
SZEDER Gábor 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-<. -- 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: 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. -- 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: Problem completing remotes when .git/remotes exits
On Wed, Sep 26, 2012 at 03:09:38AM +0200, SZEDER Gábor wrote: > 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. -- 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: Problem completing remotes when .git/remotes exits
On Tue, Sep 25, 2012 at 04:43:59PM -0700, Junio C Hamano wrote: > SZEDER Gábor 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 $ -- 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