Re: [PATCH v4 1/4] completion: work around zsh option propagation bug

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Feb 6, 2012, 23:57 UTC
Message-ID: <CAMP44s1urg0-1GZqLytg0h=qT77_zajwsKwNt9HnVfRZoOECqA@mail.gmail.com>
In-Reply-To: <7vvcnj4kif.fsf@alter.siamese.dyndns.org>

On Tue, Feb 7, 2012 at 1:20 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 35 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> I find that what Jonathan gave you helps them much better:
>>>        ...
>>>        fn () {
>>>                var='one two'
>>>                printf '%s\n' $var
>>>        }
>>>        x=$(fn)
>>>        : ${y=$(fn)}
>>>
>>>    printing "$x" results in two lines as expected, but printing "$y" results
>>>    in a single line because $var is expanded as a single word when evaluating
>>>    fn to compute y.
>>>
>>>    So avoid the construct, and use an explicit 'test -n "$foo" || foo=$(bar)'
>>>    instead.
>>>
>>> So I'll take the first two lines of the message (good bits), and simplify
>>> the "This fixes a bug tht caused..." from the last paragraph (or perhaps
>>> even drop it).
>>
>> I'm not sure about it, because this relies on knowledge of how printf
>> works, and it's not used that often; an example with 'for' would be
>> much more clear IMO.
>
> Meaning, replace the fn() definition with something like:
>
>        fn () {
>                var='one two'
>                for v in $var
>                do
>                        echo "$v"
>                done
>        }

Yes.

Show 8 quoted lines

> I can see that may make the issue easier to see; as you pointed out, it
> requires no implicit knowledge that printf "loops" over the arguments
> and applies the format string as manu times as needed to eat them.
> Let me update the log message before I merge it to 'next'.
>
> My main point was to illustrate the problematic pattern for people who
> write for bash, not for zsh, and that does not change with the above
> improvement, though ;-).

True, but I think that's an addendum. Most likely the people working on this script will be thinking on bash terms, and would not notice if they introduce code that is difficult for zsh, even if it's carefully explained in this commit message; it would be zsh people that find the bug, thus that's why I address the zsh side _first_.

So, IMO it should look like (looks like a fix has been submitted, so I've updated accordingly):

--- completion: work around zsh option propagation bug

Right now when listing commands in zsh (git <TAB><TAB>), all of them will show up, instead of only porcelain ones.

This is caused by a bug in zsh[1] that causes subshells to loose the word splitting option (SH_WORD_SPLIT) since 4.3.0[2]. It will probably be fixed in the next release (4.3.16).

Basically, in zsh, this:

 for i in $__git_all_commands

Should be:

 for i in ${=__git_all_commands}

Otherwise there's no word-splitting expansion (unless SH_WORD_SPLIT is set). sh emulation should take care of that, but the subshell is messing things up.

The visible result is that __git_list_porcelain_commands don't do any filtering.

Specifically, the issue is with subshells in parmeter expansion (e.g. '${var=$(func)'):

       fn () {
               var='one two'
               for v in $var; do
                       echo "$v"
               done
       }

       x=$(fn)
       : ${y=$(fn)}

Printing "$x" results in two lines as expected, but printing "$y" results in a single line because $var is expanded as a single word (there's no word-splitting expansion).

So avoid the construct, and use an explicit '[ -n "$foo" ] || foo=$(bar)' instead.

[1] http://article.gmane.org/gmane.comp.shells.zsh.devel/24296 [2] http://article.gmane.org/gmane.comp.shells.zsh.devel/24338 ---

Cheers.

-- 
Felipe Contreras