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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 7, 2012 at 1:20 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Felipe Contreras <felipe.contreras@xxxxxxxxx> 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.

> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]