{"thread":{"id":"27266","subject":"[PATCH for maint branch] git-completion: fix zsh support","startedAt":"2011-05-05T14:24:18Z","lastAt":"2011-05-09T13:51:55Z","messageCount":14,"participants":["Felipe Contreras","Junio C Hamano","SZEDER Gábor","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"167109","messageId":"1304605458-1483-1-git-send-email-felipe.contreras@gmail.com","threadId":"27266","inReplyTo":null,"subject":"[PATCH for maint branch] git-completion: fix zsh support","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2011-05-05T14:24:18Z","receivedAt":"2011-05-05T14:24:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"It turns out 'words' is a special variable used by zsh completion.\n\nThere's probably a bug in zsh's bashcompinit:\nhttp://article.gmane.org/gmane.comp.shells.zsh.devel/22546\n\nBut in the meantime we can workaround it this way.\n\nCurrently zsh is completely broken after commit da48616 (bash: get\n--pretty=m completion to work with bash v4), which introduced\n_get_comp_words_by_ref() that comes from debian's bash_completion\nscripts, and relies on the 'words' variable to behave like any normal\nvariable.\n\nSigned-off-by: Felipe Contreras \n---\n contrib/completion/git-completion.bash | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\nThis patch is meant for the maintenance branch, so Szeder's patches are not\nneeded.\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 840ae38..8d5eae7 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2710,6 +2710,9 @@ _git ()\n \tif [[ -n ${ZSH_VERSION-} ]]; then\n \t\temulate -L bash\n \t\tsetopt KSH_TYPESET\n+\n+\t\t# 'words' has special meaning in zsh; override that\n+\t\ttypeset -h words\n \tfi\n \n \tlocal cur words cword\n-- \n1.7.5.1.1.g638e6\n"},{"id":"167133","messageId":"7vtyd9830w.fsf@alter.siamese.dyndns.org","threadId":"27266","inReplyTo":"1304605458-1483-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH for maint branch] git-completion: fix zsh support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-05T17:34:39Z","receivedAt":"2011-05-05T17:34:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> It turns out 'words' is a special variable used by zsh completion.\n>\n> There's probably a bug in zsh's bashcompinit:\n> http://article.gmane.org/gmane.comp.shells.zsh.devel/22546\n>\n> But in the meantime we can workaround it this way.\n>\n> Currently zsh is completely broken after commit da48616 (bash: get\n> --pretty=m completion to work with bash v4), which introduced\n> _get_comp_words_by_ref() that comes from debian's bash_completion\n> scripts, and relies on the 'words' variable to behave like any normal\n> variable.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> contrib/completion/git-completion.bash | 3 +++\n> 1 files changed, 3 insertions(+), 0 deletions(-)\n>\n> This patch is meant for the maintenance branch, so Szeder's patches are not\n> needed.\n\nDo you mean sg/completion-upadates that is in 'next' need to be reverted\nand replaced with this one? If that is the case I would love to have an\nAcked-by in this patch.\n\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 840ae38..8d5eae7 100755\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -2710,6 +2710,9 @@ _git ()\n> \tif [[ -n ${ZSH_VERSION-} ]]; then\n> \t\temulate -L bash\n> \t\tsetopt KSH_TYPESET\n> +\n> +\t\t# 'words' has special meaning in zsh; override that\n> +\t\ttypeset -h words\n> \tfi\n> \n> \tlocal cur words cword\n"},{"id":"167155","messageId":"20110505185907.GD1377@goldbirke","threadId":"27266","inReplyTo":"1304605458-1483-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH for maint branch] git-completion: fix zsh support","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2011-05-05T18:59:07Z","receivedAt":"2011-05-05T18:59:07Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nOn Thu, May 05, 2011 at 05:24:18PM +0300, Felipe Contreras wrote:\n> It turns out 'words' is a special variable used by zsh completion.\n> \n> There's probably a bug in zsh's bashcompinit:\n> http://article.gmane.org/gmane.comp.shells.zsh.devel/22546\n> \n> But in the meantime we can workaround it this way.\n\nI would prefer some details about this \"workaround it this way\" in the\ncommit message (i.e. that you used \"typedef -h\", what it does in zsh,\nand why it fixes the issue; my zsh-fu is, well, not particularly\nlarge, so I have no idea), so when someone later runs 'git log --\ncontrib/completion' then he will easier understand what's going on\nwithout the need to look at the patch.\n\n> Currently zsh is completely broken after commit da48616 (bash: get\n> --pretty=m completion to work with bash v4), which introduced\n> _get_comp_words_by_ref() that comes from debian's bash_completion\n> scripts, and relies on the 'words' variable to behave like any normal\n> variable.\n> \n> Signed-off-by: Felipe Contreras \n> ---\n> contrib/completion/git-completion.bash | 3 +++\n> 1 files changed, 3 insertions(+), 0 deletions(-)\n> \n> This patch is meant for the maintenance branch, so Szeder's patches are not\n> needed.\n\nMy first two patches are cleanups, they definitely worth\nkeeping, even when this patch alone would fix the zsh issues.\n\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 840ae38..8d5eae7 100755\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -2710,6 +2710,9 @@ _git ()\n> \tif [[ -n ${ZSH_VERSION-} ]]; then\n> \t\temulate -L bash\n> \t\tsetopt KSH_TYPESET\n> +\n> +\t\t# 'words' has special meaning in zsh; override that\n> +\t\ttypeset -h words\n\nYou have to do the same in _gitk(), too. Although _gitk() itself\ndoesn't use $words, it invokes __git_has_doubledash(), which does,\nhence it's still broken.\n\n> \tfi\n> \n> \tlocal cur words cword\n> -- \n> 1.7.5.1.1.g638e6\n> \n"},{"id":"167164","messageId":"BANLkTikFnSuyfq6NT3wrWdE3AdkVXLmskw@mail.gmail.com","threadId":"27266","inReplyTo":"7vtyd9830w.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH for maint branch] git-completion: fix zsh support","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2011-05-05T19:29:37Z","receivedAt":"2011-05-05T19:29:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 5, 2011 at 8:34 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> It turns out 'words' is a special variable used by zsh completion.\n>>\n>> There's probably a bug in zsh's bashcompinit:\n>> http://article.gmane.org/gmane.comp.shells.zsh.devel/22546\n>>\n>> But in the meantime we can workaround it this way.\n>>\n>> Currently zsh is completely broken after commit da48616 (bash: get\n>> --pretty=m completion to work with bash v4), which introduced\n>> _get_comp_words_by_ref() that comes from debian's bash_completion\n>> scripts, and relies on the 'words' variable to behave like any normal\n>> variable.\n>>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>>  contrib/completion/git-completion.bash |    3 +++\n>>  1 files changed, 3 insertions(+), 0 deletions(-)\n>>\n>> This patch is meant for the maintenance branch, so Szeder's patches are not\n>> needed.\n>\n> Do you mean sg/completion-upadates that is in 'next' need to be reverted\n> and replaced with this one?  If that is the case I would love to have an\n> Acked-by in this patch.\n\nAre those patches going to be applied on the 'maint' branch? The first\ntwo are general cleanups, the third one is actually fixing stuff, but\nit depends on the previous ones. My approach doesn't require any other\npatches so it can be applied directly into maint. It can also be\napplied on top of the two cleanup patches from Szeder.\n\n-- \nFelipe Contreras\n"},{"id":"167169","messageId":"1304625144-5206-1-git-send-email-felipe.contreras@gmail.com","threadId":"27266","inReplyTo":"20110505185907.GD1377@goldbirke","subject":"[PATCH for maint] git-completion: fix zsh support","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2011-05-05T19:52:24Z","receivedAt":"2011-05-05T19:52:24Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"It turns out 'words' is a special variable used by zsh completion.\n\nThere's probably a bug in zsh's bashcompinit:\nhttp://article.gmane.org/gmane.comp.shells.zsh.devel/22546\n\nBut in the meantime we can workaround it by using 'typedef -h', which\ngets rid of any special meaning.\n\nCurrently zsh is completely broken after commit da48616 (bash: get\n--pretty=m completion to work with bash v4), which introduced\n_get_comp_words_by_ref() that comes from debian's bash_completion\nscripts, and relies on the 'words' variable to behave like any normal\nvariable.\n\nComments-by: SZEDER Gábor \nSigned-off-by: Felipe Contreras \n---\n contrib/completion/git-completion.bash | 6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\nThis patch is meant for the maintenance branch, so Szeder's patches are not\nrequired as dependency (althought they are good by themselves).\n\nv2: fix _gitk() too as Szeder suggested.\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 840ae38..74c0b4d 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2710,6 +2710,9 @@ _git ()\n \tif [[ -n ${ZSH_VERSION-} ]]; then\n \t\temulate -L bash\n \t\tsetopt KSH_TYPESET\n+\n+\t\t# 'words' has special meaning in zsh; override that\n+\t\ttypeset -h words\n \tfi\n \n \tlocal cur words cword\n@@ -2761,6 +2764,9 @@ _gitk ()\n \tif [[ -n ${ZSH_VERSION-} ]]; then\n \t\temulate -L bash\n \t\tsetopt KSH_TYPESET\n+\n+\t\t# 'words' has special meaning in zsh; override that\n+\t\ttypeset -h words\n \tfi\n \n \t__git_has_doubledash && return\n-- \n1.7.5.1.1.g638e6\n"},{"id":"167198","messageId":"20110505232503.GA7507@elie","threadId":"27266","inReplyTo":"1304625144-5206-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH for maint] git-completion: fix zsh support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-05T23:25:03Z","receivedAt":"2011-05-05T23:25:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nFelipe Contreras wrote:\n\n> It turns out 'words' is a special variable used by zsh completion.\n>\n> There's probably a bug in zsh's bashcompinit:\n> http://article.gmane.org/gmane.comp.shells.zsh.devel/22546\n>\n> But in the meantime we can workaround it by using 'typedef -h', which\n> gets rid of any special meaning.\n\nAs I mentioned before (sorry to come in late; I assume you forgot to\ncc the previous participants in the discussion?), I do not think this\nis a good fix.\n\nThe point here is that 'words' is a special variable used by zsh\ncompletion, and we are using facilities from zsh completion. So\nif you set 'typeset -h', then the zsh completion functions will\nuse _our_ copy of \"words\".\n\nNow in practice our copy of words matches zsh's anyway, so nothing\ngoes wrong. But that could easily change in the future.\n\n> Currently zsh is completely broken after commit da48616 (bash: get\n> --pretty=m completion to work with bash v4), which introduced\n> _get_comp_words_by_ref() that comes from debian's bash_completion\n> scripts\n\nThe bash_completion project does not originate in Debian fwiw; it\nwas originally from Ian Macdonald iirc and available from\n\n http://www.caliban.org/bash/index.shtml\n\nand then it was abandoned. The bash-completion project on alioth is\nnot Debian-specific, either.\n\nMaybe simplest would be to use Szeder's fix + make the zsh version of\n_get_comp_words_by_ref not overwrite \"words\" at all?\n\nHope that helps,\nJonathan\n"},{"id":"167206","messageId":"20110506012823.GA9114@elie","threadId":"27266","inReplyTo":"1304625144-5206-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH for maint] git-completion: fix zsh support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-06T01:28:24Z","receivedAt":"2011-05-06T01:28:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nFelipe Contreras wrote:\n\n> Currently zsh is completely broken after commit da48616 (bash: get\n> --pretty=m completion to work with bash v4)\n\nI forgot to say: thanks to you and Gábor for working on this! I've\nbeen grumpy lately but that is not your fault. I am very happy to\nsee someone using the zsh support and keeping an eye on bugs and code\ncleanliness.\n"},{"id":"167213","messageId":"7v7ha44ej8.fsf@alter.siamese.dyndns.org","threadId":"27266","inReplyTo":"20110505232503.GA7507@elie","subject":"Re: [PATCH for maint] git-completion: fix zsh support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-06T04:51:55Z","receivedAt":"2011-05-06T04:51:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Maybe simplest would be to use Szeder's fix + make the zsh version of\n> _get_comp_words_by_ref not overwrite \"words\" at all?\n\nI do not use zsh myself, but it appears to me that these three-patch\nseries can graduate and if real zsh users find problems after using it\nthey can be fixed independenty in-tree.\n\nWould that risk too many patch ping-pong among zsh users on 'master'?\nThe \"don't declare 'local words' in zsh\" patch seems to be the right\nwork-around for the peculiar semantics of \"words\" array, at least to me.\n"},{"id":"167218","messageId":"20110506052744.GA15132@elie","threadId":"27266","inReplyTo":"7v7ha44ej8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH for maint] git-completion: fix zsh support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-06T05:27:45Z","receivedAt":"2011-05-06T05:27:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> Maybe simplest would be to use Szeder's fix + make the zsh version of\n>> _get_comp_words_by_ref not overwrite \"words\" at all?\n>\n> I do not use zsh myself, but it appears to me that these three-patch\n> series can graduate and if real zsh users find problems after using it\n> they can be fixed independenty in-tree.\n>\n> Would that risk too many patch ping-pong among zsh users on 'master'?\n> The \"don't declare 'local words' in zsh\" patch seems to be the right\n> work-around for the peculiar semantics of \"words\" array, at least to me.\n\nGábor's patches already work. I don't think they will cause breakage\nor patch ping-pong.\n\nI was trying to imagine Felipe's objection and all I could think of\nwas that it is not so appealing that _get_comp_words_by_ref is not\nactually writing to \"words\". For example, the following on top of\nsg/completion-updates (= 3bee6a4) will print a greeting and the words\nbeing completed when you press tab, rather than :\n\n-- >8 --\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -469,7 +469,8 @@ _get_comp_words_by_ref ()\n \t\t\tprev=${COMP_WORDS[COMP_CWORD-1]}\n \t\t\t;;\n \t\twords)\n-\t\t\twords=(\"${COMP_WORDS[@]}\")\n+\t\t\twords=(foo bar baz)\n+\t\t\techo >&2 Greetings\n \t\t\t;;\n \t\tcword)\n \t\t\tcword=$COMP_CWORD\n@@ -2614,6 +2615,9 @@ _git ()\n \n \tlocal cur cword prev\n \t_get_comp_words_by_ref -n =: cur words cword prev\n+\n+\tprintf >&2 \"WORDS: <%s>\\n\" \"${words[@]}\"\n+\n \twhile [ $c -lt $cword ]; do\n \t\ti=\"${words[c]}\"\n \t\tcase \"$i\" in\n-- 8< --\n\nIn practice it works great since \"words\" already has the right\ncontent, but maybe the \"typeset -h\" suggestion was motivated by a\ndesire to have something easier to explain.\n\nI don't think that's a very strong reason to prevent the fix from\ngraduating, though I suppose I would be happy to see something like\nthe following on top at some point.\n\n-- 8< --\nSubject: completion: do not pretend to assign to special variable $words on zsh\n\nThe special variable $words already has essentially the same meaning\nas bash's COMP_WORDS on zsh. While assigning one to the other appears\nto work okay, as a no-op, that is actually an illusion --- the value\nof $words can be changed in the _get_comp_words_by_ref function where\nit is assigned, but after that function returns, $words is back to\nnormal.\n\nGuard against future breakage by adding a comment mentioning this and\nremoving the redundant assignment.\n\nSigned-off-by: Jonathan Nieder \n---\n contrib/completion/git-completion.bash | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 10c1b83..8dfd97f 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -469,7 +469,10 @@ _get_comp_words_by_ref ()\n \t\t\tprev=${COMP_WORDS[COMP_CWORD-1]}\n \t\t\t;;\n \t\twords)\n-\t\t\twords=(\"${COMP_WORDS[@]}\")\n+\t\t\t# Nothing to do --- zsh's $words already contains\n+\t\t\t# the list of words being completed (and if we\n+\t\t\t# try to assign to it, the value wouldn't propagate\n+\t\t\t# from this function anyway).\n \t\t\t;;\n \t\tcword)\n \t\t\tcword=$COMP_CWORD\n-- \n1.7.5.1\n"},{"id":"167237","messageId":"BANLkTikk1wfaC4Aic4iyJZXbZ5kkuEDxaA@mail.gmail.com","threadId":"27266","inReplyTo":"20110505232503.GA7507@elie","subject":"Re: [PATCH for maint] git-completion: fix zsh support","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2011-05-06T09:27:45Z","receivedAt":"2011-05-06T09:27:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, May 6, 2011 at 2:25 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>> It turns out 'words' is a special variable used by zsh completion.\n>>\n>> There's probably a bug in zsh's bashcompinit:\n>> http://article.gmane.org/gmane.comp.shells.zsh.devel/22546\n>>\n>> But in the meantime we can workaround it by using 'typedef -h', which\n>> gets rid of any special meaning.\n>\n> As I mentioned before (sorry to come in late; I assume you forgot to\n> cc the previous participants in the discussion?), I do not think this\n> is a good fix.\n>\n> The point here is that 'words' is a special variable used by zsh\n> completion, and we are using facilities from zsh completion.  So\n> if you set 'typeset -h', then the zsh completion functions will\n> use _our_ copy of \"words\".\n\nNo, the scope remains local.\n\n>From the manual:\n---\nHide: only useful for special parameters (those marked `' in the\ntable in zshparam(1)), and for local parameters with the same name as\na special parameter, though harmless for others. A special parameter\nwith this attribute will not retain its special effect when made\nlocal. Thus after `typeset -h PATH', a function containing `typeset\nPATH' will create an ordinary local parameter without the usual\nbehaviour of PATH. Alterna‐ tively, the local parameter may itself be\ngiven this attribute; hence inside a function `typeset -h PATH'\ncreates an ordinary local parameter and the special PATH parameter is\nnot altered in any way. It is also possible to create a local\nparameter using `typeset +h special', where the local copy of special\nwill retain its special properties regardless of having the -h\nattribute. Global special parameters loaded from shell modules (cur‐\nrently those in zsh/mapfile and zsh/parameter) are automatically given\nthe -h attribute to avoid name clashes.\n---\n\n> Now in practice our copy of words matches zsh's anyway, so nothing\n> goes wrong.  But that could easily change in the future.\n\nIt doesn't matter if we change the value of 'words'. See the attached\ntest. The result is:\n\n complete: words=foo (before)\n foo: cur=foo words=blah cwords=1\n foo bar: cur=foo words=blah cwords=1\n complete: words=foo (after)\n\nSee? No change.\n\nIn fact, if you follow the link I posted, that's precisely the fix the\nzsh guys were pushing for. And the it is already merged:\nhttp://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=e880604f029088f32fb1ecc39213d720ae526aaa\n\nI also discussed the workaround with zsh guys:\nhttp://article.gmane.org/gmane.comp.shells.zsh.devel/22557\n\n>> Currently zsh is completely broken after commit da48616 (bash: get\n>> --pretty=m completion to work with bash v4), which introduced\n>> _get_comp_words_by_ref() that comes from debian's bash_completion\n>> scripts\n>\n> The bash_completion project does not originate in Debian fwiw; it\n> was originally from Ian Macdonald iirc and available from\n>\n>  http://www.caliban.org/bash/index.shtml\n>\n> and then it was abandoned.  The bash-completion project on alioth is\n> not Debian-specific, either.\n\nWell, it's hosted on debian.org, and I haven't seen it used anywhere\nelse. I just don't know how else to identify that project.\n\n> Maybe simplest would be to use Szeder's fix + make the zsh version of\n> _get_comp_words_by_ref not overwrite \"words\" at all?\n\nI think the simplest fix is the one I'm proposing, which zsh guys agree with.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"167238","messageId":"BANLkTim8=D3ivFoOsbAvDBSRbAu+0us-2g@mail.gmail.com","threadId":"27266","inReplyTo":"20110506052744.GA15132@elie","subject":"Re: [PATCH for maint] git-completion: fix zsh support","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2011-05-06T09:34:59Z","receivedAt":"2011-05-06T09:34:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, May 6, 2011 at 8:27 AM, Jonathan Nieder wrote:\n> Junio C Hamano wrote:\n>> Jonathan Nieder writes:\n>\n>>> Maybe simplest would be to use Szeder's fix + make the zsh version of\n>>> _get_comp_words_by_ref not overwrite \"words\" at all?\n>>\n>> I do not use zsh myself, but it appears to me that these three-patch\n>> series can graduate and if real zsh users find problems after using it\n>> they can be fixed independenty in-tree.\n>>\n>> Would that risk too many patch ping-pong among zsh users on 'master'?\n>> The \"don't declare 'local words' in zsh\" patch seems to be the right\n>> work-around for the peculiar semantics of \"words\" array, at least to me.\n>\n> Gábor's patches already work.  I don't think they will cause breakage\n> or patch ping-pong.\n>\n> I was trying to imagine Felipe's objection and all I could think of\n> was that it is not so appealing that _get_comp_words_by_ref is not\n> actually writing to \"words\".  For example, the following on top of\n> sg/completion-updates (= 3bee6a4) will print a greeting and the words\n> being completed when you press tab, rather than :\n\n[...]\n\n> In practice it works great since \"words\" already has the right\n> content, but maybe the \"typeset -h\" suggestion was motivated by a\n> desire to have something easier to explain.\n>\n> I don't think that's a very strong reason to prevent the fix from\n> graduating, though I suppose I would be happy to see something like\n> the following on top at some point.\n>\n> -- 8< --\n> Subject: completion: do not pretend to assign to special variable $words on zsh\n>\n> The special variable $words already has essentially the same meaning\n> as bash's COMP_WORDS on zsh.  While assigning one to the other appears\n> to work okay, as a no-op, that is actually an illusion --- the value\n> of $words can be changed in the _get_comp_words_by_ref function where\n> it is assigned, but after that function returns, $words is back to\n> normal.\n>\n> Guard against future breakage by adding a comment mentioning this and\n> removing the redundant assignment.\n\nI don't really object to Gábor's patches. The third one actually fixes\nthe issue.\n\nThe thing is that it's a *workaround*, and I prefer to keep\nworkarounds to the minimal. So I prefer my patch that introduces only\n2 new lines and doesn't change the semantics of the rest of the code.\nMoreover, it's already fixed in zsh's master, so at some point we\nmight want to remove it, and it's easier to remove 2 non-intrusive\nlines.\n\nBut anyway, my end goal is to get this into the 'maint' branch. It's\nup to Junio how to get the fix there (if at all), but I think it's\neasier to just apply my patch there.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"167239","messageId":"20110506095920.GA1635@elie","threadId":"27266","inReplyTo":"BANLkTikk1wfaC4Aic4iyJZXbZ5kkuEDxaA@mail.gmail.com","subject":"Re: [PATCH for maint] git-completion: fix zsh support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-06T09:59:20Z","receivedAt":"2011-05-06T09:59:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> No, the scope remains local.\n\nIs this local in the sense of typeset used in ksh functions declared\nas \"function f\" or local in the sense of typeset used in ksh functions\ndeclared as \"f ()\"? If the latter, I think you misunderstood me --- I\nnever meant to suggest otherwise.\n\n> In fact, if you follow the link I posted, that's precisely the fix the\n> zsh guys were pushing for. And the it is already merged:\n> http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=e880604f029088f32fb1ecc39213d720ae526aaa\n\nNow this changes things (since it amounts to a guarantee that the\nbash completion emulation has already extracted all it needs from\n$words before calling a completion function). What happens if someone\ncalls \"compgen -F\" after words is hidden?\n\n> Well, it's hosted on debian.org, and I haven't seen it used anywhere\n> else. I just don't know how else to identify that project.\n\nHere you go: http://pkgs.fedoraproject.org/gitweb/?p=bash-completion.git;a=tree\n\nIt's called bash-completion or bash_completion. It really has\nbasically nothing to do with Debian; they just are using Debian\ninfrastructure. But hey, call it what you want. I don't like\narguing.\n\n> I think the simplest fix is the one I'm proposing\n\nI'm happy with it as long as the zsh people are committed to making\nsure it continues to work (which it sounds like they are, luckily).\nI suppose it is intended to obsolete the third patch from Gábor's\nseries? Could you provide an explanation for the commit log,\nsomething to the effect that\n\n - the words array has special meaning\n - that produces such-and-such puzzling symptoms\n - zsh 4.3.12 (?) will fix it by using \"typeset -h\" to hide it when\n running completion functions designed for bash\n - we can make the same fix to work correctly with earlier versions of\n zsh, and that's what this patch does\n\n?\n\nSorry I have been so dense --- hopefully I understand it roughly\nnow.\n\nRegards,\nJonathan\n"},{"id":"167240","messageId":"20110506100543.GA2016@elie","threadId":"27266","inReplyTo":"20110506095920.GA1635@elie","subject":"Re: [PATCH for maint] git-completion: fix zsh support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-06T10:06:03Z","receivedAt":"2011-05-06T10:06:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> What happens if someone\n> calls \"compgen -F\" after words is hidden?\n\nFor avoidance of doubt: here \"after words is hidden\" means \"from a\nscope in which words is hidden\".\n"},{"id":"167499","messageId":"BANLkTi=gNO6YyWKhTSX0=irEme-9vwyH2Q@mail.gmail.com","threadId":"27266","inReplyTo":"20110506095920.GA1635@elie","subject":"Re: [PATCH for maint] git-completion: fix zsh support","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2011-05-09T13:51:55Z","receivedAt":"2011-05-09T13:51:55Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, May 6, 2011 at 12:59 PM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> No, the scope remains local.\n>\n> Is this local in the sense of typeset used in ksh functions declared\n> as \"function f\" or local in the sense of typeset used in ksh functions\n> declared as \"f ()\"?  If the latter, I think you misunderstood me --- I\n> never meant to suggest otherwise.\n\nI'm sorry, I don't know what you mean.\n\nBy \"local\" I mean that all the layers bellow in the call stack will\nuse a separate \"words\" variable than the layers above.\n\n>> In fact, if you follow the link I posted, that's precisely the fix the\n>> zsh guys were pushing for. And the it is already merged:\n>> http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=e880604f029088f32fb1ecc39213d720ae526aaa\n>\n> Now this changes things (since it amounts to a guarantee that the\n> bash completion emulation has already extracted all it needs from\n> $words before calling a completion function).  What happens if someone\n> calls \"compgen -F\" after words is hidden?\n\nI fail to see an example of that. Why don't you try?\n\n>> Well, it's hosted on debian.org, and I haven't seen it used anywhere\n>> else. I just don't know how else to identify that project.\n>\n> Here you go: http://pkgs.fedoraproject.org/gitweb/?p=bash-completion.git;a=tree\n\nThat's just packaging:\nURL: http://bash-completion.alioth.debian.org/\n\nhttp://pkgs.fedoraproject.org/gitweb/?p=bash-completion.git;a=blob;f=bash-completion.spec;h=855462dd092a50959d9576f6bc02c01332dd63a2;hb=HEAD#l12\n\n>> I think the simplest fix is the one I'm proposing\n>\n> I'm happy with it as long as the zsh people are committed to making\n> sure it continues to work (which it sounds like they are, luckily).\n> I suppose it is intended to obsolete the third patch from Gábor's\n> series?  Could you provide an explanation for the commit log,\n> something to the effect that\n>\n>  - the words array has special meaning\n>  - that produces such-and-such puzzling symptoms\n>  - zsh 4.3.12 (?) will fix it by using \"typeset -h\" to hide it when\n>   running completion functions designed for bash\n>  - we can make the same fix to work correctly with earlier versions of\n>   zsh, and that's what this patch does\n>\n> ?\n\nI thought that's what I explained more or less. I will add the missing\npoint and the new developments on the commit message and resend.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}