{"thread":{"id":"27309","subject":"[PATCH v3 (for maint)] git-completion: fix zsh support","startedAt":"2011-05-09T20:45:13Z","lastAt":"2011-05-10T11:43:50Z","messageCount":19,"participants":["Felipe Contreras","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"167517","messageId":"1304973913-5376-1-git-send-email-felipe.contreras@gmail.com","threadId":"27309","inReplyTo":null,"subject":"[PATCH v3 (for maint)] git-completion: fix zsh support","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2011-05-09T20:45:13Z","receivedAt":"2011-05-09T20:45:13Z","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, sort\nof like 'COMP_WORDS' in bash. This was not isolated correctly in zsh's\nbash completion, so by trying to set it as 'local' in git's completion,\nunexpected results occur; assignations are not propagated to upper\nlevels in the call stack.\n\nThis is now fixed in the latest master branch of zsh[1] by simply\ndefining 'words' as hidden (typeset -h), which removes the special\nmeaning inside the emulated bash function. It probably won't be released\nuntil version 4.3.12.\n\nIn the meantime, we can workaround the issue by doing the same; defining\nwords as hidden (typeset -h) as soon as possible.\n\nRight now 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\n[1] http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=e880604f029088f32fb1ecc39213d720ae526aaa\n\nComments-by: SZEDER Gábor \nComments-by: Jonathan Nieder \nSigned-off-by: Felipe Contreras \n---\n contrib/completion/git-completion.bash | 8 ++++++++\n 1 files changed, 8 insertions(+), 0 deletions(-)\n\nThis patch is meant for the maintenance branch, so Szeder's patches are not\nrequired.\n\nv2: fix _gitk() too as Szeder suggested.\n\nv3: improve commit message as Jonathan Nieder suggested.\n Also, improve comments.\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 840ae38..763f145 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2710,6 +2710,10 @@ _git ()\n \tif [[ -n ${ZSH_VERSION-} ]]; then\n \t\temulate -L bash\n \t\tsetopt KSH_TYPESET\n+\n+\t\t# workaround zsh's bashinit's bug that leaves 'words' as a\n+\t\t# special variable in versions < 4.3.12\n+\t\ttypeset -h words\n \tfi\n \n \tlocal cur words cword\n@@ -2761,6 +2765,10 @@ _gitk ()\n \tif [[ -n ${ZSH_VERSION-} ]]; then\n \t\temulate -L bash\n \t\tsetopt KSH_TYPESET\n+\n+\t\t# workaround zsh's bashinit's bug that leaves 'words' as a\n+\t\t# special variable in versions < 4.3.12\n+\t\ttypeset -h words\n \tfi\n \n \t__git_has_doubledash && return\n-- \n1.7.5.1.1.g638e6\n"},{"id":"167518","messageId":"20110509211316.GB22908@elie","threadId":"27309","inReplyTo":"1304973913-5376-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3 (for maint)] git-completion: fix zsh support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-09T21:13:16Z","receivedAt":"2011-05-09T21:13:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> It turns out 'words' is a special variable used by zsh completion, sort\n> of like 'COMP_WORDS' in bash.\n\nI was not aware that COMP_WORDS was special (rather than just\nprepopulated). Is it?\n\n> This was not isolated correctly in zsh's\n> bash completion, so by trying to set it as 'local' in git's completion,\n> unexpected results occur; assignations are not propagated to upper\n> levels in the call stack.\n\nDoes the call stack grow up or down? I suspect this means:\n\n\tzsh's bash completion emulation layer does not sufficiently\n\tinsulate us from that reality. In particular, the variable\n\tkeeps the \"special\" attribute (even after a declaration \"local\n\twords\"), so assignments within a function are undone whenever\n\tthe function returns.\n\n\tIn particular, until 3bee6a473 (completion: don't declare\n\t'local words' to make zsh happy, 2011-04-28), the \"words\" array\n\twould be cleared in _git by declaring \"local words\" and its new\n\tvalue would never be propagated from _get_comp_words_by_ref so\n\tit remained empty and the completion script could not tell that\n\tthere were existing subcommand names on the command line (so\n\t\"git log m\" would complete subcommand names).\n\n\tAnd even after 3bee6a473 we do not have the ability to modify\n\twords. (... explanation of impact of the change goes here ...)\n\nI am not a great writer so that is probably more verbose than needed.\nSo it might be better for me to describe the goals of a commit message:\n\n 1) the text should be specific about what the commit fixes, so\n someone reading it later (e.g., after bisecting) does not come\n around and accidentally break it\n \n 2) in particular, the text should be specific about the observable\n symptoms, so it can be easier to check if a later change has\n broken it.\n\n> This is now fixed in the latest master branch of zsh[1] by simply\n> defining 'words' as hidden (typeset -h), which removes the special\n> meaning inside the emulated bash function. It probably won't be released\n> until version 4.3.12.\n>\n> In the meantime, we can workaround the issue by doing the same; defining\n> words as hidden (typeset -h) as soon as possible.\n\nIt might make sense to reverse the order of these: first explain the\nfix in the context of the problem being solved, and then add a note\nmentioning that the fix will not be needed for long and that the\nmethod is the same as what zsh is planning to use.\n\nMeanwhile this doesn't address the risk that functions called by the\ncompletion script will use $words. Outside the context of the commit\nmessage I think you've said something about that (e.g., that the zsh\ndevelopers prefer this fix --- a reference would be nice so we could\nsteal their rationale). Maybe the best thing to say would be \"that is\na risk, but let's wait and see\", to give future readers more\nconfidence that that was considered but it is ok to fix it if it comes\nup?\n\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -2710,6 +2710,10 @@ _git ()\n> \tif [[ -n ${ZSH_VERSION-} ]]; then\n> \t\temulate -L bash\n> \t\tsetopt KSH_TYPESET\n> +\n> +\t\t# workaround zsh's bashinit's bug that leaves 'words' as a\n> +\t\t# special variable in versions < 4.3.12\n> +\t\ttypeset -h words\n\nI don't think the comment clarifies much. What is the intended\nmessage to the reader? For example if it is \"don't remove this line\nunless you use zsh 4.3.12 or greater\", I'd say something like\n\n\t\t# bashcompinit versions after 4.3.12 already hide the\n\t\t# special \"words\" variable already. We do it\n\t\t# again ourselves to support older zsh versions.\n\nHope that helps,\nJonathan\n"},{"id":"167539","messageId":"BANLkTikkhryMa69DSx4EAYjw+aar4icKcQ@mail.gmail.com","threadId":"27309","inReplyTo":"20110509211316.GB22908@elie","subject":"Re: [PATCH v3 (for maint)] git-completion: fix zsh support","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2011-05-09T22:08:17Z","receivedAt":"2011-05-09T22:08:17Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, May 10, 2011 at 12:13 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> It turns out 'words' is a special variable used by zsh completion, sort\n>> of like 'COMP_WORDS' in bash.\n>\n> I was not aware that COMP_WORDS was special (rather than just\n> prepopulated).  Is it?\n\nI didn't mean to suggest it was special, only what 'words' was used\nfor. I'll remove that section as it doesn't really matter.\n\n>> This was not isolated correctly in zsh's\n>> bash completion, so by trying to set it as 'local' in git's completion,\n>> unexpected results occur; assignations are not propagated to upper\n>> levels in the call stack.\n>\n> Does the call stack grow up or down?  I suspect this means:\n\nI'll change that to \"outer levels\".\n\n>        zsh's bash completion emulation layer does not sufficiently\n>        insulate us from that reality.  In particular, the variable\n>        keeps the \"special\" attribute (even after a declaration \"local\n>        words\"), so assignments within a function are undone whenever\n>        the function returns.\n\nThat explains less.\n\n>        In particular, until 3bee6a473 (completion: don't declare\n>        'local words' to make zsh happy, 2011-04-28), the \"words\" array\n>        would be cleared in _git by declaring \"local words\" and its new\n>        value would never be propagated from _get_comp_words_by_ref so\n>        it remained empty and the completion script could not tell that\n>        there were existing subcommand names on the command line (so\n>        \"git log m\" would complete subcommand names).\n\nI don't see the point in explaining in excruciating detail all the\nseries of steps in which an unset variable causes problems; the\nvariable doesn't get set, thus one can assume there are problems. And\nthe problem is already explained to be that completion is completely\nbroken.\n\n>        And even after 3bee6a473 we do not have the ability to modify\n>        words.  (... explanation of impact of the change goes here ...)\n>\n> I am not a great writer so that is probably more verbose than needed.\n> So it might be better for me to describe the goals of a commit message:\n>\n>  1) the text should be specific about what the commit fixes, so\n>    someone reading it later (e.g., after bisecting) does not come\n>    around and accidentally break it\n\nSee the title: fix zsh support\n\n>  2) in particular, the text should be specific about the observable\n>    symptoms, so it can be easier to check if a later change has\n>    broken it.\n\nFrom the title: zsh completion doesn't work at all. Which is repeated again:\n---\nRight now zsh is completely broken...\n---\n\nIf zsh completion is completely broken, chance are it has to do with\nthis. And I did explain the most obvious test; check if the value of\n'words' doesn't get propagated to the upper layers in the call stack.\n\n>> This is now fixed in the latest master branch of zsh[1] by simply\n>> defining 'words' as hidden (typeset -h), which removes the special\n>> meaning inside the emulated bash function. It probably won't be released\n>> until version 4.3.12.\n>>\n>> In the meantime, we can workaround the issue by doing the same; defining\n>> words as hidden (typeset -h) as soon as possible.\n>\n> It might make sense to reverse the order of these: first explain the\n> fix in the context of the problem being solved, and then add a note\n> mentioning that the fix will not be needed for long and that the\n> method is the same as what zsh is planning to use.\n\nThat's what I did initially, but everyone doubted my solution. Now I\nwant to start making sure people see it's a good solution.\n\n> Meanwhile this doesn't address the risk that functions called by the\n> completion script will use $words.\n\nIt does.\n\n> Outside the context of the commit\n> message I think you've said something about that (e.g., that the zsh\n> developers prefer this fix --- a reference would be nice so we could\n> steal their rationale).\n\nI did. If you don't like their commit message, talk to them.\n\n> Maybe the best thing to say would be \"that is\n> a risk, but let's wait and see\", to give future readers more\n> confidence that that was considered but it is ok to fix it if it comes\n> up?\n\nI don't see any risk.\n\n>> --- a/contrib/completion/git-completion.bash\n>> +++ b/contrib/completion/git-completion.bash\n>> @@ -2710,6 +2710,10 @@ _git ()\n>>       if [[ -n ${ZSH_VERSION-} ]]; then\n>>               emulate -L bash\n>>               setopt KSH_TYPESET\n>> +\n>> +             # workaround zsh's bashinit's bug that leaves 'words' as a\n>> +             # special variable in versions < 4.3.12\n>> +             typeset -h words\n>\n> I don't think the comment clarifies much.  What is the intended\n> message to the reader?  For example if it is \"don't remove this line\n> unless you use zsh 4.3.12 or greater\", I'd say something like\n>\n>                # bashcompinit versions after 4.3.12 already hide the\n>                # special \"words\" variable already.  We do it\n>                # again ourselves to support older zsh versions.\n\nI think it's perfectly clear. Specially if you compare it against the\ntwo lines above that have no explanation at all. Double standards\nmuch?\n\n-- \nFelipe Contreras\n"},{"id":"167541","messageId":"1304979299-6496-1-git-send-email-felipe.contreras@gmail.com","threadId":"27309","inReplyTo":"BANLkTikkhryMa69DSx4EAYjw+aar4icKcQ@mail.gmail.com","subject":"[PATCH v4 (for maint)] git-completion: fix zsh support","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2011-05-09T22:14:59Z","receivedAt":"2011-05-09T22:14:59Z","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. This\nwas not isolated correctly in zsh's bash completion emulation, so by\ntrying to set it as 'local' in git's completion, unexpected results\noccur; assignations are not propagated to outer levels in the call\nstack.\n\nThis is now fixed in the latest master branch of zsh[1] by simply\ndefining 'words' as hidden (typeset -h), which removes the special\nmeaning inside the emulated bash function. It probably won't be released\nuntil version 4.3.12.\n\nIn the meantime, we can workaround the issue by doing the same; defining\nwords as hidden (typeset -h) as soon as possible.\n\nRight now 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 bash-completion[2] scripts, and\nrelies on the 'words' variable to behave like any normal variable.\n\n[1] http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=e880604f029088f32fb1ecc39213d720ae526aaa\n[2] http://bash-completion.alioth.debian.org/\n\nComments-by: SZEDER Gábor \nComments-by: Jonathan Nieder \nSigned-off-by: Felipe Contreras \n---\n contrib/completion/git-completion.bash | 8 ++++++++\n 1 files changed, 8 insertions(+), 0 deletions(-)\n\nThis patch is meant for the maintenance branch, so Szeder's patches are not\nrequired.\n\nv2: fix _gitk() too as Szeder suggested.\n\nv3: improve commit message as Jonathan Nieder suggested.\n Also, improve comments.\n\nv4: more commit message improvements\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 840ae38..763f145 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2710,6 +2710,10 @@ _git ()\n \tif [[ -n ${ZSH_VERSION-} ]]; then\n \t\temulate -L bash\n \t\tsetopt KSH_TYPESET\n+\n+\t\t# workaround zsh's bashinit's bug that leaves 'words' as a\n+\t\t# special variable in versions < 4.3.12\n+\t\ttypeset -h words\n \tfi\n \n \tlocal cur words cword\n@@ -2761,6 +2765,10 @@ _gitk ()\n \tif [[ -n ${ZSH_VERSION-} ]]; then\n \t\temulate -L bash\n \t\tsetopt KSH_TYPESET\n+\n+\t\t# workaround zsh's bashinit's bug that leaves 'words' as a\n+\t\t# special variable in versions < 4.3.12\n+\t\ttypeset -h words\n \tfi\n \n \t__git_has_doubledash && return\n-- \n1.7.5.1.1.g638e6\n"},{"id":"167547","messageId":"20110509225307.GG22908@elie","threadId":"27309","inReplyTo":"1304979299-6496-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v4 (for maint)] git-completion: fix zsh support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-09T22:53:07Z","receivedAt":"2011-05-09T22:53:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> v4: more commit message improvements\n\nThanks. You seem to have ignored most of my review; I'll try to find\ntime to clarify the commit message if the consensus is that this\napproach is a good idea.\n\nAn alternative possibility if we need this fixed in the v1.7.5.x\nseries (do we?) would be cherry-picking the fix from\nsg/completion-updates on top of maint.\n\nTo clarify the trade-offs:\n\n - in terms of lines of code, the fix itself in sg/completion-updates\n and this fix are about the same size. But the sg/completion-updates\n version relies on a code cleanup.\n\n - the fix in sg/completion-updates is less likely to be broken by\n future changes in the bashcompinit library.\n\n - this fix is conceptually simpler. In a way, the fix in\n sg/completion-updates only works by accident.\n"},{"id":"167551","messageId":"BANLkTim1Di2ABtrv8ncVPT6+vCvq5pD_kw@mail.gmail.com","threadId":"27309","inReplyTo":"20110509225307.GG22908@elie","subject":"Re: [PATCH v4 (for maint)] git-completion: fix zsh support","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2011-05-09T23:13:29Z","receivedAt":"2011-05-09T23:13:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, May 10, 2011 at 1:53 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> v4: more commit message improvements\n>\n> Thanks.  You seem to have ignored most of my review;\n\nI did not ignore it, I replied to your comments. A review doesn't\nmeant that I should do what you say.\n\n> I'll try to find\n> time to clarify the commit message if the consensus is that this\n> approach is a good idea.\n>\n> An alternative possibility if we need this fixed in the v1.7.5.x\n> series (do we?) would be cherry-picking the fix from\n> sg/completion-updates on top of maint.\n\nWhich cannot be done; you need the rest of the cleanups. Otherwise you\nwould have to make many many intrusive changes.\n\n> To clarify the trade-offs:\n>\n>  - in terms of lines of code, the fix itself in sg/completion-updates\n>   and this fix are about the same size.  But the sg/completion-updates\n>   version relies on a code cleanup.\n\nIt's not:\n\nfc: 1 files changed, 2 insertions(+), 0 deletions(-)\nsg: 1 files changed, 8 insertions(+), 0 deletions(-)\n\nThat is if I remove the comments I added; Gabor's version doesn't have any.\n\n>  - the fix in sg/completion-updates is less likely to be broken by\n>   future changes in the bashcompinit library.\n\nHow exactly?\n\n>  - this fix is conceptually simpler.  In a way, the fix in\n>   sg/completion-updates only works by accident.\n\nYou are missing other advantages:\n\n- thix fix is less intrusive\n- this fix would be easier to remove in the future (kind of comes from\nthe previous one)\n- this fix has been acknowledge by zsh developers\n- this fix can be applies both on 'maint' and on top of Gabor's\nreorganization without changes\n\nI really don't understand how adding two lines of code that make\nsomething from totally broken to working properly can be so difficult.\n\n-- \nFelipe Contreras\n"},{"id":"167553","messageId":"7v1v07o3rs.fsf@alter.siamese.dyndns.org","threadId":"27309","inReplyTo":"20110509225307.GG22908@elie","subject":"Re: [PATCH v4 (for maint)] git-completion: fix zsh support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-09T23:25:27Z","receivedAt":"2011-05-09T23:25:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> An alternative possibility if we need this fixed in the v1.7.5.x\n> series (do we?) would be cherry-picking the fix from\n> sg/completion-updates on top of maint.\n\nAs contrib/ material, I personally don't think we would care strongly\nenough to bother.\n\n> To clarify the trade-offs:\n>\n> - in terms of lines of code, the fix itself in sg/completion-updates\n> and this fix are about the same size. But the sg/completion-updates\n> version relies on a code cleanup.\n>\n> - the fix in sg/completion-updates is less likely to be broken by\n> future changes in the bashcompinit library.\n>\n> - this fix is conceptually simpler. In a way, the fix in\n> sg/completion-updates only works by accident.\n\nHmm, zsh does not want to see \"word\" getting localized by the user because\nit has a special semantics associated with it. Szeder avoids localizing\nit. Felipe sidesteps the issue by stripping the funny special semantics\nfrom the variable. I guess both have a similar degree of conceptual\nsimplicity.\n\nOne big thing going for this patch is that this is the blessed solution\nzsh folks themselves like to use, no? The repeated mention \"zsh bashinit\nbug\" in the code seems to suggest that it is the case.\n\nI do not mind reverting sg/completion-updates from 'next' (please remind\nme that I need to resurrect your \"private shopt shim\" separately if we go\nthis route) and applying a cleaned-up version of this one.\n\nThanks.\n"},{"id":"167554","messageId":"20110509232804.GB23781@elie","threadId":"27309","inReplyTo":"BANLkTim1Di2ABtrv8ncVPT6+vCvq5pD_kw@mail.gmail.com","subject":"Re: [PATCH v4 (for maint)] git-completion: fix zsh support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-09T23:28:04Z","receivedAt":"2011-05-09T23:28:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n> On Tue, May 10, 2011 at 1:53 AM, Jonathan Nieder wrote:\n\n>>  - the fix in sg/completion-updates is less likely to be broken by\n>>   future changes in the bashcompinit library.\n>\n> How exactly?\n\nBecause there remains the possibility that functions from bashcompinit\nwill make use of the $words variable. I have said this about three\ntimes. It is not very likely, assuming the zsh developers want to keep\nsupporting that fix (and I think they should), but the chance is there.\n\n>>  - this fix is conceptually simpler.  In a way, the fix in\n>>   sg/completion-updates only works by accident.\n>\n> You are missing other advantages:\n\nSorry, I should have prefaced the above with \"in my opinion\". And to\nbe clear, I am not saying this fix should not be applied; I am just\nexplaining the trade-offs as I understand them.\n\nThe reason I asked for another opinion is that I find it hard to be\nobjective in this case, because of another consideration I didn't\nmention: each moment I have been spending on this is an exercise in\nfrustration.\n\nI even _like_ your fix (just like I _like_ Gábor's cleanup and see\nno reason to contemplate the possibility of backing it out). So it's\na shame.\n"},{"id":"167556","messageId":"20110509233506.GC23781@elie","threadId":"27309","inReplyTo":"7v1v07o3rs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 (for maint)] git-completion: fix zsh support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-09T23:35:06Z","receivedAt":"2011-05-09T23:35:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> One big thing going for this patch is that this is the blessed solution\n> zsh folks themselves like to use, no? The repeated mention \"zsh bashinit\n> bug\" in the code seems to suggest that it is the case.\n>\n> I do not mind reverting sg/completion-updates from 'next' (please remind\n> me that I need to resurrect your \"private shopt shim\" separately if we go\n> this route) and applying a cleaned-up version of this one.\n\nThanks, will spend some time later today to do so.\n\n> Thanks.\n\nSorry for all the noise. I wish I knew a way to work on patches like\nthis one without causing so much animosity.\n"},{"id":"167561","messageId":"BANLkTikUe_Kwdi+F3vK-7kC9WNU-x_8LDw@mail.gmail.com","threadId":"27309","inReplyTo":"20110509232804.GB23781@elie","subject":"Re: [PATCH v4 (for maint)] git-completion: fix zsh support","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2011-05-09T23:58:32Z","receivedAt":"2011-05-09T23:58:32Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, May 10, 2011 at 2:28 AM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>> On Tue, May 10, 2011 at 1:53 AM, Jonathan Nieder wrote:\n>\n>>>  - the fix in sg/completion-updates is less likely to be broken by\n>>>   future changes in the bashcompinit library.\n>>\n>> How exactly?\n>\n> Because there remains the possibility that functions from bashcompinit\n> will make use of the $words variable.  I have said this about three\n> times.  It is not very likely, assuming the zsh developers want to keep\n> supporting that fix (and I think they should), but the chance is there.\n\nAnd I even wrote a test to show you that's not the case:\nhttp://article.gmane.org/gmane.comp.version-control.git/172963\n\nNow, can you modify my test to explain how *exactly* zsh folks can\nscrew my patch up?\n\n>>>  - this fix is conceptually simpler.  In a way, the fix in\n>>>   sg/completion-updates only works by accident.\n>>\n>> You are missing other advantages:\n>\n> Sorry, I should have prefaced the above with \"in my opinion\".  And to\n> be clear, I am not saying this fix should not be applied; I am just\n> explaining the trade-offs as I understand them.\n>\n> The reason I asked for another opinion is that I find it hard to be\n> objective in this case, because of another consideration I didn't\n> mention: each moment I have been spending on this is an exercise in\n> frustration.\n\nWell, don't :) Just ask yourself this question: is the patch good\nenough? If not, send your own version.\n\nPersonally I think the latest version of the patch clearly explains\nwhat it is doing, and why. It even has comments on the code, which the\nother alternative doesn't.\n\n-- \nFelipe Contreras\n"},{"id":"167564","messageId":"20110510020421.GA25814@elie","threadId":"27309","inReplyTo":"BANLkTikkhryMa69DSx4EAYjw+aar4icKcQ@mail.gmail.com","subject":"Re: [PATCH v3 (for maint)] git-completion: fix zsh support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-10T02:04:21Z","receivedAt":"2011-05-10T02:04:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSorry I missed this message before.\n\nFelipe Contreras wrote:\n> On Tue, May 10, 2011 at 12:13 AM, Jonathan Nieder wrote:\n\n>>        zsh's bash completion emulation layer does not sufficiently\n>>        insulate us from that reality.  In particular, the variable\n>>        keeps the \"special\" attribute (even after a declaration \"local\n>>        words\"), so assignments within a function are undone whenever\n>>        the function returns.\n>\n> That explains less.\n\nI believe you. Could you give a hint of what it misses, so that it\ncan be fixed?\n\nWhat I was trying to say is that (in the previous paragraph) it turns\nout 'words' is a special variable used by zsh completion and (in the\nquoted text)\n\n - we would prefer to use that variable for something else\n\n - zsh's bashcompinit library can and should help us to do so\n\n - it would do so by using typeset -h to hide the variable, an\n important effect of which is to clear the attribute called\n \"special\"\n\n - even declaring \"local words\" does not clear that attribute\n\n - the pertinent effect here is that (in the words of the zshcompwid\n manpage) as long as parameters like words remain special, \"except\n for compstate, the parameters are reset on each function exit\n (including nested function calls from within the completion widget)\n to the values they had when the function was entered\".\n\n>>        In particular, until 3bee6a473 (completion: don't declare\n>>        'local words' to make zsh happy, 2011-04-28), the \"words\" array\n>>        would be cleared in _git by declaring \"local words\" and its new\n>>        value would never be propagated from _get_comp_words_by_ref so\n>>        it remained empty and the completion script could not tell that\n>>        there were existing subcommand names on the command line (so\n>>        \"git log m\" would complete subcommand names).\n>\n> I don't see the point in explaining in excruciating detail all the\n> series of steps in which an unset variable causes problems; the\n> variable doesn't get set, thus one can assume there are problems.\n\nAm I daft? I guess so. I really do have sympathy for the person who\nruns into this code, and wanting to check while making some change\nwith unrelated purpose that it is still fixed, fires up zsh and runs\n\n\tgit a\n\n. Does it complete? Yep, check, moving on. Oops.\n\nThere's some flamebait in the rest of your responses, so I'm snipping\nthe rest of my explanations. If you have sincere questions about my\nfeedback in the future, I'd be glad to answer them.\n"},{"id":"167565","messageId":"20110510025529.GA26619@elie","threadId":"27309","inReplyTo":"1304979299-6496-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v5 0/2] git-completion: fix zsh support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-10T02:55:29Z","receivedAt":"2011-05-10T02:55:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nAs promised, here are two patches. No doubt I have done all sorts\nof horrible things to ruin them. Sorry about that; thoughts,\nimprovements, bug reports welcome.\n\nFelipe Contreras (1):\n completion: suppress zsh's special 'words' variable\n\nJonathan Nieder (1):\n completion: move private shopt shim for zsh to __git_ namespace\n\n contrib/completion/git-completion.bash | 14 ++++++++++----\n 1 files changed, 10 insertions(+), 4 deletions(-)\n"},{"id":"167566","messageId":"20110510025916.GB26619@elie","threadId":"27309","inReplyTo":"20110510025529.GA26619@elie","subject":"[PATCH 1/2] completion: suppress zsh's special 'words' variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-10T02:59:16Z","receivedAt":"2011-05-10T02:59:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Felipe Contreras \n\nAfter git's tab completion script gained zsh support in\nv1.7.4-rc0~169^2 (completion: make compatible with zsh, 2010-09-06)\nit was broken moments later. More precisely, the completion does not\nnotice when it has seen a subcommand name, so all words complete as\noptions to the git wrapper or subcommand names. For example, typing\n\"git log origi\" gives no completions because there are no \"git\norigi...\" commands.\n\nThe cause: it turns out 'words' is one of the special parameters used\nby the zsh completion system, used to hold the words from the command\nit is completing. As a result (in the words of zshcompwid(1)):\n\n\t[...] the parameters are reset on each function exit\n\t(including nested function calls from within the completion\n\twidget) to the values they had when the function was entered.\n\nEach function in git's completion script using the 'words' array\n\n - declares \"local words\", causing the array to be cleared (but not\n resetting the special attribute);\n\n - calls \"_get_comp_words_by_ref -n := words\" to fill it with a\n modified version of COMP_WORDS with ':' and '=' no longer treated\n as word separators (see v1.7.4-rc0~11^2~2, 2010-12-02). Within\n _get_comp_words_by_ref all is well, and when the function returns,\n words is reset to its former value;\n\n - examines $words and finds it empty.\n\nFix it by suppressing the special 'words' variable with typeset -h\nso it can be used as an ordinary array. The only risk is that the\ncompletion script might call a function that wants to inspect the\n'words' variable, expecting the zsh-specific meaning; luckily the next\nversion of zsh's bashcompinit (e880604f, 29140: hide the \"words\"\nspecial variable so that it may be used as an ordinary variable by\nbash completions, 2011-05-04) will also use 'typeset -h words' when\ncalling completion functions so\n\n - soon this fix will be redundant :)\n - anyone else using the bashcompinit library is risking the same\n problem, so presumably other functions from that library are\n carefully written to only look at $COMP_WORDS and not $words.\n\nThis fixes a regression introduced by v1.7.4-rc0~11^2~2 (2010-12-02).\n\nReported-by: Stefan Haller \nImproved-by: SZEDER Gábor \nSigned-off-by: Felipe Contreras \nSigned-off-by: Jonathan Nieder \n---\n contrib/completion/git-completion.bash | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex b81f444..da586e5 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2608,6 +2608,7 @@ _git ()\n \tif [[ -n ${ZSH_VERSION-} ]]; then\n \t\temulate -L bash\n \t\tsetopt KSH_TYPESET\n+\t\ttypeset -h words\n \tfi\n \n \tlocal cur words cword prev\n@@ -2659,6 +2660,7 @@ _gitk ()\n \tif [[ -n ${ZSH_VERSION-} ]]; then\n \t\temulate -L bash\n \t\tsetopt KSH_TYPESET\n+\t\ttypeset -h words\n \tfi\n \n \tlocal cur words cword prev\n-- \n1.7.5.1\n"},{"id":"167567","messageId":"20110510030027.GC26619@elie","threadId":"27309","inReplyTo":"20110510025529.GA26619@elie","subject":"[PATCH 2/2] completion: move private shopt shim for zsh to __git_ namespace","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-10T03:00:27Z","receivedAt":"2011-05-10T03:00:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Most zsh users probably probably do not expect a custom shopt function\nto enter their environment just because they ran \"source\n~/.git-completion.sh\".\n\nSuch namespace pollution makes development of other scripts confusing\n(because it makes the bash-specific shopt utility seem to be available\nin zsh) and makes git's tab completion script brittle (since any other\nshell snippet implementing some other subset of shopt will break it).\nRename the shopt shim to the more innocuous __git_shopt to be a good\ncitizen (with two underscores to avoid confusion with completion rules\nfor a hypothetical \"git shopt\" command).\n\nSigned-off-by: Jonathan Nieder \nAcked-by: SZEDER Gábor \n---\nThanks again.\n\n contrib/completion/git-completion.bash | 12 ++++++++----\n 1 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex da586e5..a236234 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -628,12 +628,12 @@ __git_refs_remotes ()\n __git_remotes ()\n {\n \tlocal i ngoff IFS=$'\\n' d=\"$(__gitdir)\"\n-\tshopt -q nullglob || ngoff=1\n-\tshopt -s nullglob\n+\t__git_shopt -q nullglob || ngoff=1\n+\t__git_shopt -s nullglob\n \tfor i in \"$d/remotes\"/*; do\n \t\techo ${i#$d/remotes/}\n \tdone\n-\t[ \"$ngoff\" ] && shopt -u nullglob\n+\t[ \"$ngoff\" ] && __git_shopt -u nullglob\n \tfor i in $(git --git-dir=\"$d\" config --get-regexp 'remote\\..*\\.url' 2>/dev/null); do\n \t\ti=\"${i#remote.}\"\n \t\techo \"${i/.url*/}\"\n@@ -2701,7 +2701,7 @@ complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \\\n fi\n \n if [[ -n ${ZSH_VERSION-} ]]; then\n-\tshopt () {\n+\t__git_shopt () {\n \t\tlocal option\n \t\tif [ $# -ne 2 ]; then\n \t\t\techo \"USAGE: $0 (-q|-s|-u)