Zsh completion regression

Zsh completion regression

[Stefan Haller]

I'm using zsh   4.3.11.

When I type "git log mas<TAB>", it completes to "git log master\ " (a
backslash, a space, and then the cursor).

Bisecting points to "a31e626 completion: optimize refs completion."
Before this commit, I get "git log master" (with no space at the end).

My completion-fu is not strong enough to dig into this myself, but I'll
be happy to help test, or do whatever else helps.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

Re: Zsh completion regression

[Matthieu Moy]

lists@haller-berlin.de (Stefan Haller) writes:

> I'm using zsh   4.3.11.
>
> When I type "git log mas<TAB>", it completes to "git log master\ " (a
> backslash, a space, and then the cursor).

Same here (although I've been too lazy to bisect myself).

The following patch makes the situation better, but is not really a fix:

--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -525,7 +525,7 @@ __gitcomp ()
 __gitcomp_nl ()
 {
        local s=$'\n' IFS=' '$'\t'$'\n'
-       local cur_="$cur" suffix=" "
+       local cur_="$cur" suffix=""
 
        if [ $# -gt 2 ]; then
                cur_="$3"

With this, the trailing space isn't added, but e.g. "git checkout
master<TAB>" does not add the trailing space, at all.

The problem is a little bit below:

	IFS=$s
	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))

The -S "$suffix" adds a space to the completion, but ZSH escapes the
space (which sounds sensible in general, but is not at all what we
expect). My completion-fu isn't good enough to get any further either
unfortunately.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Zsh completion regression

[SZEDER Gábor]

Hi,


On Thu, Jan 12, 2012 at 03:56:28PM +0100, Matthieu Moy wrote:
> lists@haller-berlin.de (Stefan Haller) writes:
> 
> > I'm using zsh   4.3.11.
> >
> > When I type "git log mas<TAB>", it completes to "git log master\ " (a
> > backslash, a space, and then the cursor).

Stefan, thanks for reporting and bisecting.

> The following patch makes the situation better, but is not really a fix:
> 
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -525,7 +525,7 @@ __gitcomp ()
>  __gitcomp_nl ()
>  {
>         local s=$'\n' IFS=' '$'\t'$'\n'
> -       local cur_="$cur" suffix=" "
> +       local cur_="$cur" suffix=""
>  
>         if [ $# -gt 2 ]; then
>                 cur_="$3"
> 
> With this, the trailing space isn't added,

Yeah, this would be a regression for Bash users, because then they
will need to manually append that space.

> but e.g. "git checkout
> master<TAB>" does not add the trailing space, at all.

I'm not sure what you mean; did you got a trailing space after
'master<TAB>' before a31e6262 (completion: optimize refs completion,
2011-10-15)?
I'm not a zsh user myself, but just tried it and found that in
a31e626^ 'git checkout master<TAB>' doesn't add the trailing space,
and neither does 'git checkout mas<TAB>', which is in sync with what
Stefan reported ("Before this commit, I get "git log master" (with no
space at the end).").

> The problem is a little bit below:
> 
> 	IFS=$s
> 	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))
> 
> The -S "$suffix" adds a space to the completion, but ZSH escapes the
> space (which sounds sensible in general, but is not at all what we
> expect).

So it seems we have two issues here with zsh:

- Spaces appended with 'compgen -S " "' in __gitcomp_nl() are quoted.
  This is the regression caused by a31e6262.
- Spaces appended in __gitcomp_1() (by echo "$word ") are stripped.
  This is a long-standing issue; if those spaces weren't stripped, the
  quoting issue would have been noted earlier, I suspect.

We could fix the regression by not appending a space suffix to
completion words in __gitcomp_nl(), but only when the completion
script is running under zsh to avoid hurting bash users, like this:

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2d02a7f3..49393243 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -601,6 +601,9 @@ __gitcomp_nl ()
 			suffix="$4"
 		fi
 	fi
+	if [ -n "${ZSH_VERSION-}" ] && [ "$suffix" = " " ]; then
+		suffix=""
+	fi
 
 	IFS=$s
 	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))


Although it wouldn't append a space at all for zsh users, that's
nothing new, it's just restoring old behavior.  And then later someone
more knowledgable about the quirks of zsh's completion and in
particular zsh's bash completion emulation can come up with a proper
fix to the issue of quoted spaces and stripped trailing spaces.


However.

While playing around with zsh, I noticed that git completion works
without even sourcing git's bash completion script.  As it turned out,
zsh ships its own git completion script[1], and from my cursory tests
it seems to be quite capable: it supports commands, aliases, options,
refs, ref1..ref2, config variables, ...  and I also saw a __git_ps1()
equivalent for zsh.

So, is there any reason why you are still using git's bash completion
under zsh (which has some quirks and lacks some features) instead of
zsh's own?  Perhaps it would make sense to point zsh users to zsh's
git completion and drop zsh compatibility from git's bash completion.
We did similar with vim config files: git included a vim syntax
highlight config file for commit messages under contrib/vim/, but
eventually we dropped it after vim started shipping more capable
git-specific config files (for git config files, rebase instruction
sheets, etc.).


[1] http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=blob;f=Completion/Unix/Command/_git;hb=HEAD


Best,
Gábor

Re: Zsh completion regression

[Matthieu Moy]

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

>> but e.g. "git checkout
>> master<TAB>" does not add the trailing space, at all.
>
> I'm not sure what you mean; did you got a trailing space after
> 'master<TAB>' before a31e6262 (completion: optimize refs completion,
> 2011-10-15)?

No. My above sentence should read "... not add the trailing space, at
all, even for bash users". IOW, your understanding is correct.

> We could fix the regression by not appending a space suffix to
> completion words in __gitcomp_nl(), but only when the completion
> script is running under zsh to avoid hurting bash users, like this:
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 2d02a7f3..49393243 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -601,6 +601,9 @@ __gitcomp_nl ()
>  			suffix="$4"
>  		fi
>  	fi
> +	if [ -n "${ZSH_VERSION-}" ] && [ "$suffix" = " " ]; then
> +		suffix=""
> +	fi
>  
>  	IFS=$s
>  	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))

I hate to see special case for different shells, but if no one finds a
better solution, then yes, this is the way to go. Not having the space
may be irritating, but having the quoted space hurts really much more (I
have to delete the space and the backslash manually to continue).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Zsh completion regression

[SZEDER Gábor]

Hi,


On Sat, Jan 14, 2012 at 03:32:08PM +0100, Matthieu Moy wrote:
> SZEDER Gábor <szeder@ira.uka.de> writes:
> > We could fix the regression by not appending a space suffix to
> > completion words in __gitcomp_nl(), but only when the completion
> > script is running under zsh to avoid hurting bash users, like this:
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index 2d02a7f3..49393243 100755
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -601,6 +601,9 @@ __gitcomp_nl ()
> >  			suffix="$4"
> >  		fi
> >  	fi
> > +	if [ -n "${ZSH_VERSION-}" ] && [ "$suffix" = " " ]; then
> > +		suffix=""
> > +	fi
> >  
> >  	IFS=$s
> >  	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))
> 
> I hate to see special case for different shells,

I agree and thought about that, too.  We are dealing with zsh quirks
in the completion script either by

- having whole functions with different definitions in zsh and
  in bash (currently__git_shopt() and _get_comp_words_by_ref()), or 
- having similar shell-specific cases inside functions (currently
  _git() and _gitk()).

We use the former when the implementations for the two shells differ
significantly, and the latter when the shell-specific parts are small
and most of the function's implementation can be used in both shells.
I think this regression fix fits into the latter category.

> but if no one finds a
> better solution, then yes, this is the way to go. Not having the space
> may be irritating, but having the quoted space hurts really much more (I
> have to delete the space and the backslash manually to continue).

I have no better idea for fixing this regression, and no idea at all
for a proper fix (i.e. to make zsh behave the same way in this respect
as bash).  I can imagine that it's irritating, that's why I aimed for
this minimal regression fix, which, maybe with a Tested-by from you or
Stefan, can even go into maint, so affected users can get it faster.


Best,
Gábor

[PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Matthieu Moy]

Commit a31e626 (completion: optimize refs completion) introduced a
regression for ZSH users: ref names were completed with a quoted trailing
space (i.e. "git checkout ma" completes to "git checkout master\ "). The
space is convenient for bash users since we use "-o nospace", but a
quoted space is worse than nothing. The absence of trailing space for ZSH
is a long-standing issue, that this patch is not fixing. We just fix the
regression by not appending a space when the shell is ZSH.

Original-patch-by: SZEDER Gábor <szeder@ira.uka.de>
Reported-by: Stefan Haller <lists@haller-berlin.de>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 contrib/completion/git-completion.bash |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b0062ba..488e1f4 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -534,6 +534,12 @@ __gitcomp_nl ()
 		fi
 	fi
 
+	# ZSH would quote the trailing space added with -S. bash users
+	# will appreciate the extra space to compensate the use of -o nospace.
+	if [ -n "${ZSH_VERSION-}" ] && [ "$suffix" = " " ]; then
+		suffix=""
+	fi
+
 	IFS=$s
 	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))
 }
-- 
1.7.9.rc0.25.gb9a1f.dirty

Re: Zsh completion regression

[Stefan Haller]

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

> I can imagine that it's irritating, that's why I aimed for
> this minimal regression fix, which, maybe with a Tested-by from you or
> Stefan, can even go into maint, so affected users can get it faster.

The patch works well for me, so feel free to add a Tested-by from me.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

Re: Zsh completion regression

[Stefan Haller]

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

> However.
> 
> While playing around with zsh, I noticed that git completion works
> without even sourcing git's bash completion script.  As it turned out,
> zsh ships its own git completion script[1], and from my cursory tests
> it seems to be quite capable: it supports commands, aliases, options,
> refs, ref1..ref2, config variables, ...  and I also saw a __git_ps1()
> equivalent for zsh.
> 
> So, is there any reason why you are still using git's bash completion
> under zsh (which has some quirks and lacks some features) instead of
> zsh's own?  Perhaps it would make sense to point zsh users to zsh's
> git completion and drop zsh compatibility from git's bash completion.
> We did similar with vim config files: git included a vim syntax
> highlight config file for commit messages under contrib/vim/, but
> eventually we dropped it after vim started shipping more capable
> git-specific config files (for git config files, rebase instruction
> sheets, etc.).

Last time I compared the two, the bash completion script was a lot more
complete and powerful. The zsh script had a few annoying limitations for
things that I use every day, and that worked with the bash script, so I
switched to that. Unfortunately I forgot the details, it's a while ago.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index b0062ba..488e1f4 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -534,6 +534,12 @@ __gitcomp_nl ()
>  		fi
>  	fi
>  
> +	# ZSH would quote the trailing space added with -S. bash users
> +	# will appreciate the extra space to compensate the use of -o nospace.
> +	if [ -n "${ZSH_VERSION-}" ] && [ "$suffix" = " " ]; then
> +		suffix=""
> +	fi

This should take care of the SP set by the beginning of the helper
function

        local cur_="$cur" suffix=" "

but is that the right thing to do if suffix came from "$4"?

As far as I can see, "$4" is used to append "." in very limited cases, and
nobody explicitly passes SP as "$4" when calling this, so it may be easier
to read if you moved this before that "if we have 3 or more args, use the
fourth one as the suffix" block, i.e. something like this?

 contrib/completion/git-completion.bash |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b0062ba..4ad75ed 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -525,7 +525,10 @@ __gitcomp ()
 __gitcomp_nl ()
 {
 	local s=$'\n' IFS=' '$'\t'$'\n'
-	local cur_="$cur" suffix=" "
+	local cur_="$cur"
+	# Because we use '-o nospace' under bash, we need to compensate
+	# for it by appending SP after completed word ourselves.
+	local suffix="${BASH_VERSION+ }"
 
 	if [ $# -gt 2 ]; then
 		cur_="$3"

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Matthieu Moy]

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

> but is that the right thing to do if suffix came from "$4"?
>
> As far as I can see, "$4" is used to append "." in very limited cases, and
> nobody explicitly passes SP as "$4" when calling this, so it may be easier
> to read if you moved this before that "if we have 3 or more args, use the
> fourth one as the suffix" block, i.e. something like this?

Why not, but in case someone explicitely passes " " as $4 in the future,
it's likely to be better to strip it for the same reason we strip it here.

I don't care much either way in this case.

> +	# Because we use '-o nospace' under bash, we need to compensate
> +	# for it by appending SP after completed word ourselves.
> +	local suffix="${BASH_VERSION+ }"

Not sure why you reworded the comment, but I don't think it's a good
idea to remove the "ZSH would quote the trailing space added with -S"
that I had added, because this is really the reason we do a special case
here. Your version is misleading, because we use -o nospace for ZSH too.

So, overall, I prefer my version ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> but is that the right thing to do if suffix came from "$4"?
>>
>> As far as I can see, "$4" is used to append "." in very limited cases, and
>> nobody explicitly passes SP as "$4" when calling this, so it may be easier
>> to read if you moved this before that "if we have 3 or more args, use the
>> fourth one as the suffix" block, i.e. something like this?
>
> Why not, but in case someone explicitely passes " " as $4 in the future,
> it's likely to be better to strip it for the same reason we strip it here.

I doubt that would be sufficent or appropriate. If some caller _WANTS_ to
add a SP, shouldn't we be devising a way to tell zsh to add it without
quoting, instead of silently stripping?

> I don't care much either way in this case.
>
>> +	# Because we use '-o nospace' under bash, we need to compensate
>> +	# for it by appending SP after completed word ourselves.
>> +	local suffix="${BASH_VERSION+ }"
>
> Not sure why you reworded the comment, but I don't think it's a good
> idea to remove the "ZSH would quote the trailing space added with -S"
> that I had added, because this is really the reason we do a special case
> here. Your version is misleading, because we use -o nospace for ZSH too.

Ok, use of "-o nospace" in Zsh is what I missed. I thought the issue was
about the nospace emulation.

So does that mean we would be forcing zsh users to add SP themselves?  I
wonder if we can do better than that.

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> but is that the right thing to do if suffix came from "$4"?
>>>
>>> As far as I can see, "$4" is used to append "." in very limited cases, and
>>> nobody explicitly passes SP as "$4" when calling this, so it may be easier
>>> to read if you moved this before that "if we have 3 or more args, use the
>>> fourth one as the suffix" block, i.e. something like this?
>>
>> Why not, but in case someone explicitely passes " " as $4 in the future,
>> it's likely to be better to strip it for the same reason we strip it here.
>
> I doubt that would be sufficent or appropriate. If some caller _WANTS_ to
> add a SP, shouldn't we be devising a way to tell zsh to add it without
> quoting,

Yes, this is the point. But up to now, nobody found such a way so we're
just trying to work around it in the less painfull way for the user.

If someone _wants_ to add a SP, then he still can't do it portably with
your patch, because the space will be quoted for bash users, and not for
ZSH users, so one of them will be unhappy.

> So does that mean we would be forcing zsh users to add SP themselves? 

Yes, but we already do so. From my commit message:

  The absence of trailing space for ZSH is a long-standing issue, that
  this patch is not fixing.

;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>> I doubt that would be sufficent or appropriate. If some caller _WANTS_ to
>> add a SP, shouldn't we be devising a way to tell zsh to add it without
>> quoting,
>
> Yes, this is the point. But up to now, nobody found such a way so we're
> just trying to work around it in the less painfull way for the user.

Ok, I take it that the original patch is meant as a small step in making
it a usable state, by not adding useless "quoted SP". In the ideal world
it may be better to add SP but we do not know how without zsh interfering
with our attempt to do so and adding an unwanted quoting, so we are taking
the second best approach that we at least know works.

Which is fine by me, and as you said, the completion script always asked
zsh users to add SP themselves, so it is not even a regression.

The real reason I am continuing this thread is to keep it alive so that a
zsh guru would jump in from somewhere and show us "here is how to tell Zsh
not to quote $suffix"; that does not seem to be happening yet, so let's
use your patch as-is.

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Felipe Contreras]

On Sat, Jan 14, 2012 at 8:55 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> Commit a31e626 (completion: optimize refs completion) introduced a
> regression for ZSH users: ref names were completed with a quoted trailing
> space (i.e. "git checkout ma" completes to "git checkout master\ "). The
> space is convenient for bash users since we use "-o nospace", but a
> quoted space is worse than nothing. The absence of trailing space for ZSH
> is a long-standing issue, that this patch is not fixing. We just fix the
> regression by not appending a space when the shell is ZSH.

I have this issue with the script from v1.7.8.3, and I think it
started with a zsh update.

-- 
Felipe Contreras

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Felipe Contreras]

On Tue, Jan 17, 2012 at 9:18 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Jan 14, 2012 at 8:55 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>> Commit a31e626 (completion: optimize refs completion) introduced a
>> regression for ZSH users: ref names were completed with a quoted trailing
>> space (i.e. "git checkout ma" completes to "git checkout master\ "). The
>> space is convenient for bash users since we use "-o nospace", but a
>> quoted space is worse than nothing. The absence of trailing space for ZSH
>> is a long-standing issue, that this patch is not fixing. We just fix the
>> regression by not appending a space when the shell is ZSH.
>
> I have this issue with the script from v1.7.8.3, and I think it
> started with a zsh update.

Yeah, works fine with zsh 4.3.11, not 4.3.14 or 15.

-- 
Felipe Contreras

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Tue, Jan 17, 2012 at 9:18 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Sat, Jan 14, 2012 at 8:55 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>>> Commit a31e626 (completion: optimize refs completion) introduced a
>>> regression for ZSH users: ref names were completed with a quoted trailing
>>> space (i.e. "git checkout ma" completes to "git checkout master\ "). The
>>> space is convenient for bash users since we use "-o nospace", but a
>>> quoted space is worse than nothing. The absence of trailing space for ZSH
>>> is a long-standing issue, that this patch is not fixing. We just fix the
>>> regression by not appending a space when the shell is ZSH.
>>
>> I have this issue with the script from v1.7.8.3, and I think it
>> started with a zsh update.
>
> Yeah, works fine with zsh 4.3.11, not 4.3.14 or 15.

As I was planning to queue Matthieu's patch as-is as a regression fix
before v1.7.9-rc2, I would appreciate if you can clarify this report a
bit. Do you mean with the patch more recent versions of zsh still does not
like the workaround and adds quoted space at the end?

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Felipe Contreras]

On Tue, Jan 17, 2012 at 10:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Tue, Jan 17, 2012 at 9:18 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>> On Sat, Jan 14, 2012 at 8:55 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>>>> Commit a31e626 (completion: optimize refs completion) introduced a
>>>> regression for ZSH users: ref names were completed with a quoted trailing
>>>> space (i.e. "git checkout ma" completes to "git checkout master\ "). The
>>>> space is convenient for bash users since we use "-o nospace", but a
>>>> quoted space is worse than nothing. The absence of trailing space for ZSH
>>>> is a long-standing issue, that this patch is not fixing. We just fix the
>>>> regression by not appending a space when the shell is ZSH.
>>>
>>> I have this issue with the script from v1.7.8.3, and I think it
>>> started with a zsh update.
>>
>> Yeah, works fine with zsh 4.3.11, not 4.3.14 or 15.
>
> As I was planning to queue Matthieu's patch as-is as a regression fix
> before v1.7.9-rc2, I would appreciate if you can clarify this report a
> bit. Do you mean with the patch more recent versions of zsh still does not
> like the workaround and adds quoted space at the end?

I am saying I am seeing the issue (or at least the same symptom) even
before a31e626 with recent zsh versions. I will try a patched version
of the script and a31e626, but I'm guessing the result would be the
same.

-- 
Felipe Contreras

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> As I was planning to queue Matthieu's patch as-is as a regression fix
>> before v1.7.9-rc2, I would appreciate if you can clarify this report a
>> bit. Do you mean with the patch more recent versions of zsh still does not
>> like the workaround and adds quoted space at the end?
>
> I am saying I am seeing the issue (or at least the same symptom) even
> before a31e626 with recent zsh versions. I will try a patched version
> of the script and a31e626, but I'm guessing the result would be the
> same.

OK, so the issue the patch addresses may not be a regression in the
upcoming v1.7.9 we want to fix quickly, but if you tried the patch we
might see improvements, or we might not see any improvements.

I'll hold onto Matthieu's patch for now, then. It would be nice to get
this minor detail fixed before the release, though, as any fix would be
of limited scope, affecting zsh users for which it is already broken.

Thanks.

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Matthieu Moy]

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

> OK, so the issue the patch addresses may not be a regression in the
> upcoming v1.7.9 we want to fix quickly,

I'm running ZSH 4.3.10 (Debian stable), and for me it is a regression.
It seems there is another bug elsewhere affecting more recent ZSH (I
don't have a recent ZSH version installed to test), but fixing the
regression for old ZSH is still worth it. I'm not even sur the issue
with recent ZSH is related.

At worse, my patch is not intrusive and can easily be reworked later.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Felipe Contreras]

On Wed, Jan 18, 2012 at 10:16 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> OK, so the issue the patch addresses may not be a regression in the
>> upcoming v1.7.9 we want to fix quickly,
>
> I'm running ZSH 4.3.10 (Debian stable), and for me it is a regression.
> It seems there is another bug elsewhere affecting more recent ZSH (I
> don't have a recent ZSH version installed to test), but fixing the
> regression for old ZSH is still worth it. I'm not even sur the issue
> with recent ZSH is related.
>
> At worse, my patch is not intrusive and can easily be reworked later.

I believe I have found a more generic and simpler fix that works for
both the regression in v1.7.9, and users of zsh >= 4.3.12.

Patch sent.

-- 
Felipe Contreras

Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression)

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> At worse, my patch is not intrusive and can easily be reworked later.
>
> I believe I have found a more generic and simpler fix that works for
> both the regression in v1.7.9, and users of zsh >= 4.3.12.
>
> Patch sent.

Matthieu, care to take a look?

It looks fairly straightforward and would have absolutely no impact on
bash users.

Thanks, Felipe.

-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Subject: [PATCH] git-completion: workaround zsh COMPREPLY bug

zsh adds a backslash (foo\ ) for each item in the COMPREPLY array if IFS
doesn't contain spaces. This issue has been reported[1], but there is no
solution yet.

This wasn't a problem due to another bug[2], which was fixed in zsh
version 4.3.12. After this change, 'git checkout ma<tab>' would resolve
to 'git checkout master\ '.

Aditionally, the introduction of __gitcomp_nl in commit a31e626
(completion: optimize refs completion) in git also made the problem
apparent, as Matthieu Moy reported.

The simplest and most generic solution is to hide all the changes we do
to IFS, so that "foo \nbar " is recognized by zsh as "foo bar". This
works on versions of git before and after the introduction of
__gitcomp_nl (a31e626), and versions of zsh before and after 4.3.12.

Once zsh is fixed, we should conditionally disable this workaround to
have the same benefits as bash users.

[1] http://www.zsh.org/mla/workers/2012/msg00053.html
[2] http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=2e25dfb8fd38dbef0a306282ffab1d343ce3ad8d

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b0062ba..c83c734 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2631,6 +2631,10 @@ _git ()
 		# workaround zsh's bug that leaves 'words' as a special
 		# variable in versions < 4.3.12
 		typeset -h words
+
+		# another workaround for zsh because it would quote spaces in
+		# the COMPREPLY array if IFS doesn't contain spaces
+		typeset -h IFS
 	fi
 
 	local cur words cword prev
@@ -2687,6 +2691,10 @@ _gitk ()
 		# workaround zsh's bug that leaves 'words' as a special
 		# variable in versions < 4.3.12
 		typeset -h words
+
+		# another workaround for zsh because it would quote spaces in
+		# the COMPREPLY array if IFS doesn't contain spaces
+		typeset -h IFS
 	fi
 
 	local cur words cword prev