* Zsh completion regression @ 2012-01-12 11:52 Stefan Haller 2012-01-12 14:56 ` Matthieu Moy 0 siblings, 1 reply; 21+ messages in thread From: Stefan Haller @ 2012-01-12 11:52 UTC (permalink / raw) To: git; +Cc: SZEDER Gábor 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/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: Zsh completion regression 2012-01-12 11:52 Zsh completion regression Stefan Haller @ 2012-01-12 14:56 ` Matthieu Moy 2012-01-14 13:23 ` SZEDER Gábor 0 siblings, 1 reply; 21+ messages in thread From: Matthieu Moy @ 2012-01-12 14:56 UTC (permalink / raw) To: Stefan Haller; +Cc: git, SZEDER Gábor 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/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: Zsh completion regression 2012-01-12 14:56 ` Matthieu Moy @ 2012-01-14 13:23 ` SZEDER Gábor 2012-01-14 14:32 ` Matthieu Moy 2012-01-14 21:36 ` Stefan Haller 0 siblings, 2 replies; 21+ messages in thread From: SZEDER Gábor @ 2012-01-14 13:23 UTC (permalink / raw) To: Matthieu Moy; +Cc: Stefan Haller, git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: Zsh completion regression 2012-01-14 13:23 ` SZEDER Gábor @ 2012-01-14 14:32 ` Matthieu Moy 2012-01-14 15:20 ` SZEDER Gábor 2012-01-14 21:36 ` Stefan Haller 1 sibling, 1 reply; 21+ messages in thread From: Matthieu Moy @ 2012-01-14 14:32 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Stefan Haller, git 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/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: Zsh completion regression 2012-01-14 14:32 ` Matthieu Moy @ 2012-01-14 15:20 ` SZEDER Gábor 2012-01-14 18:55 ` [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) Matthieu Moy 2012-01-14 21:36 ` Zsh completion regression Stefan Haller 0 siblings, 2 replies; 21+ messages in thread From: SZEDER Gábor @ 2012-01-14 15:20 UTC (permalink / raw) To: Matthieu Moy; +Cc: Stefan Haller, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-14 15:20 ` SZEDER Gábor @ 2012-01-14 18:55 ` Matthieu Moy 2012-01-15 2:29 ` Junio C Hamano 2012-01-17 19:18 ` Felipe Contreras 2012-01-14 21:36 ` Zsh completion regression Stefan Haller 1 sibling, 2 replies; 21+ messages in thread From: Matthieu Moy @ 2012-01-14 18:55 UTC (permalink / raw) To: git, gitster; +Cc: 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-14 18:55 ` [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) Matthieu Moy @ 2012-01-15 2:29 ` Junio C Hamano 2012-01-16 11:49 ` Matthieu Moy 2012-01-17 19:18 ` Felipe Contreras 1 sibling, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2012-01-15 2:29 UTC (permalink / raw) To: Matthieu Moy; +Cc: git 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" ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-15 2:29 ` Junio C Hamano @ 2012-01-16 11:49 ` Matthieu Moy 2012-01-16 22:47 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Matthieu Moy @ 2012-01-16 11:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-16 11:49 ` Matthieu Moy @ 2012-01-16 22:47 ` Junio C Hamano 2012-01-17 12:21 ` Matthieu Moy 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2012-01-16 22:47 UTC (permalink / raw) To: Matthieu Moy; +Cc: git 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-16 22:47 ` Junio C Hamano @ 2012-01-17 12:21 ` Matthieu Moy 2012-01-17 18:46 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Matthieu Moy @ 2012-01-17 12:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-17 12:21 ` Matthieu Moy @ 2012-01-17 18:46 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2012-01-17 18:46 UTC (permalink / raw) To: Matthieu Moy; +Cc: git 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-14 18:55 ` [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) Matthieu Moy 2012-01-15 2:29 ` Junio C Hamano @ 2012-01-17 19:18 ` Felipe Contreras 2012-01-17 20:03 ` Felipe Contreras 1 sibling, 1 reply; 21+ messages in thread From: Felipe Contreras @ 2012-01-17 19:18 UTC (permalink / raw) To: Matthieu Moy; +Cc: git, gitster 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-17 19:18 ` Felipe Contreras @ 2012-01-17 20:03 ` Felipe Contreras 2012-01-17 20:11 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Felipe Contreras @ 2012-01-17 20:03 UTC (permalink / raw) To: Matthieu Moy; +Cc: git, gitster 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-17 20:03 ` Felipe Contreras @ 2012-01-17 20:11 ` Junio C Hamano 2012-01-17 23:04 ` Felipe Contreras 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2012-01-17 20:11 UTC (permalink / raw) To: Felipe Contreras; +Cc: Matthieu Moy, git 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? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-17 20:11 ` Junio C Hamano @ 2012-01-17 23:04 ` Felipe Contreras 2012-01-17 23:42 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Felipe Contreras @ 2012-01-17 23:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Matthieu Moy, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-17 23:04 ` Felipe Contreras @ 2012-01-17 23:42 ` Junio C Hamano 2012-01-18 8:16 ` Matthieu Moy 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2012-01-17 23:42 UTC (permalink / raw) To: Felipe Contreras; +Cc: Matthieu Moy, git 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-17 23:42 ` Junio C Hamano @ 2012-01-18 8:16 ` Matthieu Moy 2012-01-25 1:39 ` Felipe Contreras 0 siblings, 1 reply; 21+ messages in thread From: Matthieu Moy @ 2012-01-18 8:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: Felipe Contreras, git 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/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-18 8:16 ` Matthieu Moy @ 2012-01-25 1:39 ` Felipe Contreras 2012-01-25 4:06 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Felipe Contreras @ 2012-01-25 1:39 UTC (permalink / raw) To: Matthieu Moy; +Cc: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) 2012-01-25 1:39 ` Felipe Contreras @ 2012-01-25 4:06 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2012-01-25 4:06 UTC (permalink / raw) To: Felipe Contreras; +Cc: Matthieu Moy, git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: Zsh completion regression 2012-01-14 15:20 ` SZEDER Gábor 2012-01-14 18:55 ` [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) Matthieu Moy @ 2012-01-14 21:36 ` Stefan Haller 1 sibling, 0 replies; 21+ messages in thread From: Stefan Haller @ 2012-01-14 21:36 UTC (permalink / raw) To: SZEDER Gábor, Matthieu Moy; +Cc: git 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/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: Zsh completion regression 2012-01-14 13:23 ` SZEDER Gábor 2012-01-14 14:32 ` Matthieu Moy @ 2012-01-14 21:36 ` Stefan Haller 1 sibling, 0 replies; 21+ messages in thread From: Stefan Haller @ 2012-01-14 21:36 UTC (permalink / raw) To: SZEDER Gábor, Matthieu Moy; +Cc: git 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/ ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2012-01-25 4:06 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-12 11:52 Zsh completion regression Stefan Haller 2012-01-12 14:56 ` Matthieu Moy 2012-01-14 13:23 ` SZEDER Gábor 2012-01-14 14:32 ` Matthieu Moy 2012-01-14 15:20 ` SZEDER Gábor 2012-01-14 18:55 ` [PATCH] bash-completion: don't add quoted space for ZSH (fix regression) Matthieu Moy 2012-01-15 2:29 ` Junio C Hamano 2012-01-16 11:49 ` Matthieu Moy 2012-01-16 22:47 ` Junio C Hamano 2012-01-17 12:21 ` Matthieu Moy 2012-01-17 18:46 ` Junio C Hamano 2012-01-17 19:18 ` Felipe Contreras 2012-01-17 20:03 ` Felipe Contreras 2012-01-17 20:11 ` Junio C Hamano 2012-01-17 23:04 ` Felipe Contreras 2012-01-17 23:42 ` Junio C Hamano 2012-01-18 8:16 ` Matthieu Moy 2012-01-25 1:39 ` Felipe Contreras 2012-01-25 4:06 ` Junio C Hamano 2012-01-14 21:36 ` Zsh completion regression Stefan Haller 2012-01-14 21:36 ` Stefan Haller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).