Re: [PATCH for maint] git-completion: fix zsh support

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 6, 2011, 09:34 UTC
Message-ID: <BANLkTim8=D3ivFoOsbAvDBSRbAu+0us-2g@mail.gmail.com>
In-Reply-To: <20110506052744.GA15132@elie>

On Fri, May 6, 2011 at 8:27 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 22 quoted lines

> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> Maybe simplest would be to use Szeder's fix + make the zsh version of
>>> _get_comp_words_by_ref not overwrite "words" at all?
>>
>> I do not use zsh myself, but it appears to me that these three-patch
>> series can graduate and if real zsh users find problems after using it
>> they can be fixed independenty in-tree.
>>
>> Would that risk too many patch ping-pong among zsh users on 'master'?
>> The "don't declare 'local words' in zsh" patch seems to be the right
>> work-around for the peculiar semantics of "words" array, at least to me.
>
> Gábor's patches already work.  I don't think they will cause breakage
> or patch ping-pong.
>
> I was trying to imagine Felipe's objection and all I could think of
> was that it is not so appealing that _get_comp_words_by_ref is not
> actually writing to "words".  For example, the following on top of
> sg/completion-updates (= 3bee6a4) will print a greeting and the words
> being completed when you press tab, rather than <foo> <bar> <baz>:

[...]

Show 20 quoted lines

> In practice it works great since "words" already has the right
> content, but maybe the "typeset -h" suggestion was motivated by a
> desire to have something easier to explain.
>
> I don't think that's a very strong reason to prevent the fix from
> graduating, though I suppose I would be happy to see something like
> the following on top at some point.
>
> -- 8< --
> Subject: completion: do not pretend to assign to special variable $words on zsh
>
> The special variable $words already has essentially the same meaning
> as bash's COMP_WORDS on zsh.  While assigning one to the other appears
> to work okay, as a no-op, that is actually an illusion --- the value
> of $words can be changed in the _get_comp_words_by_ref function where
> it is assigned, but after that function returns, $words is back to
> normal.
>
> Guard against future breakage by adding a comment mentioning this and
> removing the redundant assignment.

I don't really object to Gábor's patches. The third one actually fixes the issue.

The thing is that it's a *workaround*, and I prefer to keep workarounds to the minimal. So I prefer my patch that introduces only 2 new lines and doesn't change the semantics of the rest of the code. Moreover, it's already fixed in zsh's master, so at some point we might want to remove it, and it's easier to remove 2 non-intrusive lines.

But anyway, my end goal is to get this into the 'maint' branch. It's up to Junio how to get the fix there (if at all), but I think it's easier to just apply my patch there.

Cheers.

-- 
Felipe Contreras