[PATCH] completion: ignore chpwd_functions when cding

[Brandon Turner]

Software, such as RVM (ruby version manager), may set chpwd functions
that result in an endless loop when cding. chpwd functions should be
ignored.

Signed-off-by: Brandon Turner <***@brandonturner.net>
---
For an example of this bug, see:
https://github.com/wayneeseguin/rvm/issues/3076

contrib/completion/git-completion.bash | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 06bf262..996de31 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -283,7 +283,8 @@ __git_ls_files_helper ()
{
(
test -n "${CDPATH+set}" && unset CDPATH
- cd "$1"
+ (( ${#chpwd_functions} )) && chpwd_functions=()
+ builtin cd "$1"
if [ "$2" == "--committable" ]; then
git diff-index --name-only --relative HEAD
else

[Junio C Hamano]

Post by Brandon Turner
Software, such as RVM (ruby version manager), may set chpwd functions
that result in an endless loop when cding. chpwd functions should be
ignored.
---

Can you mention that this is abomination limited only to zsh
somewhere in the log message? Or does bash share the same glitch?

If this is limited to zsh, I wonder if we can take advantage of the
fact that we have git-completion.bash and git-completion.zsh to
avoid contaminating shared part of the code.

Thanks.

Post by Brandon Turner
https://github.com/wayneeseguin/rvm/issues/3076
contrib/completion/git-completion.bash | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 06bf262..996de31 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -283,7 +283,8 @@ __git_ls_files_helper ()
{
(
test -n "${CDPATH+set}" && unset CDPATH
- cd "$1"
+ (( ${#chpwd_functions} )) && chpwd_functions=()
+ builtin cd "$1"
if [ "$2" == "--committable" ]; then
git diff-index --name-only --relative HEAD
else

[Brandon Turner]

Post by Junio C Hamano
Can you mention that this is abomination limited only to zsh
somewhere in the log message? Or does bash share the same glitch?
If this is limited to zsh, I wonder if we can take advantage of the
fact that we have git-completion.bash and git-completion.zsh to
avoid contaminating shared part of the code.

I'm going to submit two versions of the patch:
v2 - addresses the log message, but the patch still applies to bash
and zsh
v3 - moves the change to git-completion.zsh as an override, bash is
unaffected

Post by Junio C Hamano
From my testing, bash is unaffected, so v3 would be an ok fix. Since

we cannot control what functions users add to chpwd_functions, v2 might
make more sense.

[Brandon Turner]

Software, such as RVM (ruby version manager), may set chpwd functions
that result in an endless loop when cding. chpwd functions should be
ignored.

As I've only seen this so far on ZSH, I'm applying this change only to
the git-completion.zsh overrides.

Signed-off-by: Brandon Turner <***@brandonturner.net>
---
This applies the patch to zsh only using git-completion.zsh.

For more details on the RVM bug, see:
https://github.com/wayneeseguin/rvm/issues/3076
contrib/completion/git-completion.zsh | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh
index 9f6f0fa..12ac984 100644
--- a/contrib/completion/git-completion.zsh
+++ b/contrib/completion/git-completion.zsh
@@ -93,6 +93,21 @@ __gitcomp_file ()
compadd -Q -p "${2-}" -f -- ${=1} && _ret=0
}

+__git_ls_files_helper ()
+{
+ (
+ test -n "${CDPATH+set}" && unset CDPATH
+ (( ${#chpwd_functions} )) && chpwd_functions=()
+ builtin cd "$1"
+ if [ "$2" == "--committable" ]; then
+ git diff-index --name-only --relative HEAD
+ else
+ # NOTE: $2 is not quoted in order to support multiple options
+ git ls-files --exclude-standard $2
+ fi
+ ) 2>/dev/null
+}
+
__git_zsh_bash_func ()
{
emulate -L ksh

[Øystein Walle]

Post by Brandon Turner
=20
Software, such as RVM (ruby version manager), may set chpwd functions
that result in an endless loop when cding. chpwd functions should be
ignored.

Now that it has moved to the zsh-specific script you can achieve this m=
ore
simply by using cd -q.

=C3=98sse

[Junio C Hamano]

=20
Software, such as RVM (ruby version manager), may set chpwd function=

s

that result in an endless loop when cding. chpwd functions should b=

e

ignored.

Now that it has moved to the zsh-specific script you can achieve this=

more

simply by using cd -q.

;-)

Is the way we defeat CDPATH for POSIX shells sufficient, or does it
also need to be customized for zsh?

[Brandon Turner]

Software, such as RVM (ruby version manager), may set chpwd functions
that result in an endless loop when cding. chpwd functions should be
ignored.

As I've only seen this so far on ZSH, I'm applying this change only to
the git-completion.zsh overrides.

Signed-off-by: Brandon Turner <***@brandonturner.net>
---
As =C3=98ystein pointed out, on zsh we can use "cd -q" to ignore
chpwd_functions.

Junio - from my testing, unsetting CDPATH is sufficient on zsh.

contrib/completion/git-completion.zsh | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/contrib/completion/git-completion.zsh b/contrib/completion=
/git-completion.zsh
index 9f6f0fa..04ed348 100644
--- a/contrib/completion/git-completion.zsh
+++ b/contrib/completion/git-completion.zsh
@@ -93,6 +93,20 @@ __gitcomp_file ()
compadd -Q -p "${2-}" -f -- ${=3D1} && _ret=3D0
}
=20
+__git_ls_files_helper ()
+{
+ (
+ test -n "${CDPATH+set}" && unset CDPATH
+ cd -q "$1"
+ if [ "$2" =3D=3D "--committable" ]; then
+ git diff-index --name-only --relative HEAD
+ else
+ # NOTE: $2 is not quoted in order to support multiple options
+ git ls-files --exclude-standard $2
+ fi
+ ) 2>/dev/null
+}
+
__git_zsh_bash_func ()
{
emulate -L ksh
--=20
2.1.2

[Øystein Walle]

Post by Brandon Turner
+__git_ls_files_helper ()
+{
+ (
+ test -n "${CDPATH+set}" && unset CDPATH
+ cd -q "$1"
+ if [ "$2" =3D=3D "--committable" ]; then
+ git diff-index --name-only --relative HEAD
+ else
+ # NOTE: $2 is not quoted in order to support multiple options
+ git ls-files --exclude-standard $2
+ fi
+ ) 2>/dev/null
+}
+

(Sorry about this; I should've caught it the first time around). Zsh
does not split string expansions into several words by default. For
example:

$ str1=3D'hello world'
$ str2=3D'goodbye moon'
$ printf '%s\n' $str1 $str2
hello world
goodbye moon

This can be enabled on a "per-expansion basis" by using =3D while
expanding:=20

$ str1=3D'hello world'
$ str2=3D'goodbye moon'
$ printf '%s\n' $=3Dstr1 $str2
hello
world
goodbye moon

So the $2 in your patch should be $=3D2.

BUT: Over a year ago Git learned the -C argument. Couldn't we use that
here? That way we would not have to unset CDPATH and can get rid of the
subshell and cd -q. If we allow the other functions to use several
arguments to pass options with we can get rid of the whole seperation
between bash and zsh altogether.

=C3=98sse

[Junio C Hamano]

BUT: Over a year ago Git learned the -C argument. Couldn't we use tha=

t

here? That way we would not have to unset CDPATH and can get rid of t=

he

subshell and cd -q. If we allow the other functions to use several
arguments to pass options with we can get rid of the whole seperation
between bash and zsh altogether.

Wow, that is an excellent suggestion. It would look like the
attached, right?

By stepping away further and further from the originally proposed
solution and trying to identify the real problem that needs to be
solved, you reached a better solution ;-).

contrib/completion/git-completion.bash | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completio=
n/git-completion.bash
index 5ea5b82..f22de9d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -281,16 +281,12 @@ __gitcomp_file ()
# argument, and using the options specified in the second argument.
__git_ls_files_helper ()
{
- (
- test -n "${CDPATH+set}" && unset CDPATH
- cd "$1"
- if [ "$2" =3D=3D "--committable" ]; then
- git diff-index --name-only --relative HEAD
- else
- # NOTE: $2 is not quoted in order to support multiple options
- git ls-files --exclude-standard $2
- fi
- ) 2>/dev/null
+ if [ "$2" =3D=3D "--committable" ]; then
+ git -C "$1" diff-index --name-only --relative HEAD
+ else
+ # NOTE: $2 is not quoted in order to support multiple options
+ git -C "$1" ls-files --exclude-standard $2
+ fi 2>/dev/null
}
=20
=20

[Junio C Hamano]

Post by Brandon Turner
As =C3=98ystein pointed out, on zsh we can use "cd -q" to ignore
chpwd_functions.
Junio - from my testing, unsetting CDPATH is sufficient on zsh.

Let's do this instead, though.

Bugs are mine; as I do not use zsh myself, some testing is very much
appreciated.

Thanks.

-- >8 --
Subject: [PATCH] completion: use "git -C $there" instead of (cd $there =
&& git ...)

We have had "git -C $there" to first go to a different directory
and run a Git command without changing the arguments for quite some
time. Use it instead of (cd $there && git ...) in the completion
script.

This allows us to lose the work-around for misfeatures of modern
interactive-minded shells that make "cd" unusable in scripts (e.g.
end users' $CDPATH taking us to unexpected places in any POSIX
shell, and chpwd functions spewing unwanted output in zsh).

Based on =C3=98ystein Walle's idea, which was raised during the
discussion on the solution by Brandon Turner for a problem zsh users
had with RVM which mucks with chpwd_functions in users' environments
(https://github.com/wayneeseguin/rvm/issues/3076).

Signed-off-by: Junio C Hamano <***@pobox.com>
---
contrib/completion/git-completion.bash | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completio=
n/git-completion.bash
index dba3c15..42f7308 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -263,16 +263,12 @@ __gitcomp_file ()
# argument, and using the options specified in the second argument.
__git_ls_files_helper ()
{
- (
- test -n "${CDPATH+set}" && unset CDPATH
- cd "$1"
- if [ "$2" =3D=3D "--committable" ]; then
- git diff-index --name-only --relative HEAD
- else
- # NOTE: $2 is not quoted in order to support multiple options
- git ls-files --exclude-standard $2
- fi
- ) 2>/dev/null
+ if [ "$2" =3D=3D "--committable" ]; then
+ git -C "$1" diff-index --name-only --relative HEAD
+ else
+ # NOTE: $2 is not quoted in order to support multiple options
+ git -C "$1" ls-files --exclude-standard $2
+ fi 2>/dev/null
}
=20
=20
--=20
2.1.2-464-g5e996a3

[Brandon Turner]

Post by Junio C Hamano
Bugs are mine; as I do not use zsh myself, some testing is very much
appreciated.

I've tested this patch in zsh and it fixes the original problem. I've
also tested various scenarios in bash and zsh (CDPATH set, different
places within repos, etc.) and see no problems.

Thanks for all of the help Junio and =C3=98ystein.

[Junio C Hamano]

Post by Junio C Hamano
Bugs are mine; as I do not use zsh myself, some testing is very much
appreciated.

I've tested this patch in zsh and it fixes the original problem. I'v=

e

also tested various scenarios in bash and zsh (CDPATH set, different
places within repos, etc.) and see no problems.
Thanks for all of the help Junio and =C3=98ystein.

Actually the patch was slightly wrong. It did not quite matter as
"cd ''" is a no-op, but "git -C '' cmd" is not that lenient (which
may be something we may want to fix) and breaks t9902 by exposing
an existing breakage in the callchain.

Here is a replacement.

-- >8 --
=46rom: Junio C Hamano <***@pobox.com>
Date: Thu, 9 Oct 2014 13:45:21 -0700
Subject: [PATCH] completion: use "git -C $there" instead of (cd $there =
&& git
...)
MIME-Version: 1.0
Content-Type: text/plain; charset=3DUTF-8
Content-Transfer-Encoding: 8bit

We have had "git -C $there" to first go to a different directory
and run a Git command without changing the arguments for quite some
time. Use it instead of (cd $there && git ...) in the completion
script.

This allows us to lose the work-around for misfeatures of modern
interactive-minded shells that make "cd" unusable in scripts (e.g.
end users' $CDPATH taking us to unexpected places in any POSIX
shell, and chpwd functions spewing unwanted output in zsh).

Based on =C3=98ystein Walle's idea, which was raised during the
discussion on the solution by Brandon Turner for a problem zsh users
had with RVM which mucks with chpwd_functions in users' environments
(https://github.com/wayneeseguin/rvm/issues/3076).

As $root variable, which is used to direct where to chdir to, is set
to "." based on if $2 to __git_index_files is set (not if it is empty),
the only caller of the function is fixed not to pass the optional $2
when it does not want us to switch to a different directory. Otherwise
we would end up doing "git -C '' command...", which would not work.

Maybe we would want "git -C '' command..." to mean "do not chdir
anywhere", but that is a spearate topic.

Signed-off-by: Junio C Hamano <***@pobox.com>
---
contrib/completion/git-completion.bash | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completio=
n/git-completion.bash
index dba3c15..6077925 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -263,16 +263,12 @@ __gitcomp_file ()
# argument, and using the options specified in the second argument.
__git_ls_files_helper ()
{
- (
- test -n "${CDPATH+set}" && unset CDPATH
- cd "$1"
- if [ "$2" =3D=3D "--committable" ]; then
- git diff-index --name-only --relative HEAD
- else
- # NOTE: $2 is not quoted in order to support multiple options
- git ls-files --exclude-standard $2
- fi
- ) 2>/dev/null
+ if [ "$2" =3D=3D "--committable" ]; then
+ git -C "$1" diff-index --name-only --relative HEAD
+ else
+ # NOTE: $2 is not quoted in order to support multiple options
+ git -C "$1" ls-files --exclude-standard $2
+ fi 2>/dev/null
}
=20
=20
@@ -504,7 +500,7 @@ __git_complete_index_file ()
;;
esac
=20
- __gitcomp_file "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_"
+ __gitcomp_file "$(__git_index_files "$1" ${pfx:+"$pfx"})" "$pfx" "$cu=
r_"
}
=20
__git_complete_file ()
--=20
2.1.2-466-g338ee7a

[Brandon Turner]

Post by Junio C Hamano
Actually the patch was slightly wrong. It did not quite matter as
"cd ''" is a no-op, but "git -C '' cmd" is not that lenient (which
may be something we may want to fix) and breaks t9902 by exposing
an existing breakage in the callchain.
Here is a replacement.

I did some more testing on this iteration as well - looks good. :-)

[Øystein Walle]

=20
On Thu, Oct 9, 2014 at 5:11 PM, Junio C Hamano <gitster <at> pobox.co=

Post by Junio C Hamano
Actually the patch was slightly wrong. It did not quite matter as
"cd ''" is a no-op, but "git -C '' cmd" is not that lenient (which
may be something we may want to fix) and breaks t9902 by exposing
an existing breakage in the callchain.
Here is a replacement.

=20
I did some more testing on this iteration as well - looks good.=20
=20

Sorry, for the late reply, guys...

I've tested it too and it seems to work just fine.

As for my comments about using $=3D2 instead of $2: When zsh runs throu=
gh
this code it does so while emulating ksh. Thus the reliance on splittin=
g
unquoted expansions is not a problem. I was unaware of this until ten
minutes ago.=20

=C3=98sse

[Øystein Walle]

Post by Junio C Hamano
=20
=20

=20
Software, such as RVM (ruby version manager), may set chpwd functi=

ons

Post by Junio C Hamano

that result in an endless loop when cding. chpwd functions should=

be

Post by Junio C Hamano

ignored.

Now that it has moved to the zsh-specific script you can achieve th=

is more

Post by Junio C Hamano

simply by using cd -q.

=20
=20
=20
Is the way we defeat CDPATH for POSIX shells sufficient, or does it
also need to be customized for zsh?
=20

That is fine (to the best of my knowledge). If the current directory is=
not
part of CDPATH at all then Zsh will try the current directory first, so=
if
anything Zsh should fail more seldom here than others (but not *never*,=
so
the hack is still needed).

=C3=98sse

[Brandon Turner]

Software, such as RVM (ruby version manager), may set chpwd functions
that result in an endless loop when cding. chpwd functions should be
ignored.

I have only noticed the RVM bug on ZSH, bash seems unaffected. However
this change seems safe to apply to both bash and zsh as we cannot
control what functions users add to chpwd_functions.

Signed-off-by: Brandon Turner <***@brandonturner.net>
---
This addresses Junio's request to update the log message. The patch
still applies to bash and zsh.

For more information on the RVM bug, see:
https://github.com/wayneeseguin/rvm/issues/3076
contrib/completion/git-completion.bash | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 06bf262..996de31 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -283,7 +283,8 @@ __git_ls_files_helper ()
{
(
test -n "${CDPATH+set}" && unset CDPATH
- cd "$1"
+ (( ${#chpwd_functions} )) && chpwd_functions=()
+ builtin cd "$1"
if [ "$2" == "--committable" ]; then
git diff-index --name-only --relative HEAD
else