[PATCH] completion: ignore chpwd_functions when cding

[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 <bt@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
-- 
2.1.2

Re: [PATCH] completion: ignore chpwd_functions when cding

[Junio C Hamano]

Brandon Turner <bt@brandonturner.net> writes:

> 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 <bt@brandonturner.net>
> ---

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.

> 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

[PATCH v2] 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.

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 <bt@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
-- 
2.1.2

[PATCH v3] completion: ignore chpwd_functions when cding on 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 <bt@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
-- 
2.1.2

Re: [PATCH] completion: ignore chpwd_functions when cding

[Brandon Turner]

On Wed, Oct 8, 2014 at 1:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>
> 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

>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.

Re: [PATCH v3] completion: ignore chpwd_functions when cding on zsh

[Øystein Walle]

Brandon Turner <bt <at> brandonturner.net> writes:

> 
> 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 more
simply by using cd -q.

Øsse

Re: [PATCH v3] completion: ignore chpwd_functions when cding on zsh

[Junio C Hamano]

Øystein Walle <oystwa@gmail.com> writes:

> Brandon Turner <bt <at> brandonturner.net> writes:
>
>> 
>> 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 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?

[PATCH v4] completion: ignore chpwd_functions when cding on 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 <bt@brandonturner.net>
---
As Øystein 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 -- ${=1} && _ret=0
 }
 
+__git_ls_files_helper ()
+{
+	(
+		test -n "${CDPATH+set}" && unset CDPATH
+		cd -q "$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
-- 
2.1.2

Re: [PATCH v3] completion: ignore chpwd_functions when cding on zsh

[Øystein Walle]

Junio C Hamano <gitster <at> pobox.com> writes:

> 
> Øystein Walle <oystwa <at> gmail.com> writes:
> 
> > Brandon Turner <bt <at> brandonturner.net> writes:
> >
> >> 
> >> 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 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?
> 

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).

Øsse

Re: [PATCH v4] completion: ignore chpwd_functions when cding on zsh

[Øystein Walle]

Brandon Turner <bt <at> brandonturner.net> writes:

> +__git_ls_files_helper ()
> +{
> +	(
> +		test -n "${CDPATH+set}" && unset CDPATH
> +		cd -q "$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
> +}
> +

(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='hello world'
    $ str2='goodbye moon'
    $ printf '%s\n' $str1 $str2
    hello world
    goodbye moon

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

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

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

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.

Øsse

Re: [PATCH v4] completion: ignore chpwd_functions when cding on zsh

[Junio C Hamano]

Øystein Walle <oystwa@gmail.com> writes:

> 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.

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/completion/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" == "--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" == "--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
 }
 
 

Re: [PATCH v4] completion: ignore chpwd_functions when cding on zsh

[Junio C Hamano]

Brandon Turner <bt@brandonturner.net> writes:

> As Øystein 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 Øystein 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 <gitster@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/completion/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" == "--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" == "--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
 }
 
 
-- 
2.1.2-464-g5e996a3

Re: [PATCH v4] completion: ignore chpwd_functions when cding on zsh

[Brandon Turner]

On Thu, Oct 9, 2014 at 3:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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 Øystein.

Re: [PATCH v4] completion: ignore chpwd_functions when cding on zsh

[Junio C Hamano]

Brandon Turner <bt@brandonturner.net> writes:

> On Thu, Oct 9, 2014 at 3:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> 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 Øystein.

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 --
From: Junio C Hamano <gitster@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=UTF-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 Øystein 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 <gitster@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/completion/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" == "--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" == "--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
 }
 
 
@@ -504,7 +500,7 @@ __git_complete_index_file ()
 		;;
 	esac
 
-	__gitcomp_file "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_"
+	__gitcomp_file "$(__git_index_files "$1" ${pfx:+"$pfx"})" "$pfx" "$cur_"
 }
 
 __git_complete_file ()
-- 
2.1.2-466-g338ee7a

Re: [PATCH v4] completion: ignore chpwd_functions when cding on zsh

[Brandon Turner]

On Thu, Oct 9, 2014 at 5:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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. :-)

Re: [PATCH v4] completion: ignore chpwd_functions when cding on zsh

[Øystein Walle]

Brandon Turner <bt <at> brandonturner.net> writes:

> 
> On Thu, Oct 9, 2014 at 5:11 PM, Junio C Hamano <gitster <at> pobox.com> wrote:
> > 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. 
> 

Sorry, for the late reply, guys...

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

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

Øsse