Re: [PATCH v4 1/4] completion: work around zsh option propagation bug
On Fri, Feb 3, 2012 at 10:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 32 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Right now when listing commands in zsh (git <TAB><TAB>), all of them
>> will show up, instead of only porcelain ones.
>
> Jonathan's rewrite goes straight to the root cause instead, which is
> another way to describe the problem.
>
> Explaining user-visible symptoms at the beginning like you did is a good
> strategy that I would want to see more contributors follow, though.
>
>> 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
>> up with that. So __git_list_porcelain_commands does not do any
>> filtering.
>
> Let me step back a bit and see if we are on the same page wrt the root
> cause of the problem and for whom we are explaining the change.
>
> The adaptation of the bash completion script to zsh is done by asking zsh
> to obey POSIXy word splitting rules to honor $IFS that is in effect when
> the words are split. However zsh does not do a good job at it in some
> cases, and your patch works it around by avoiding a construct known to be
> troublesome to zsh.Troublesome to zsh emulation, yeah.
> Am I correct so far? If so, especially if the first sentence of the above
> paragraph is correct, then how would it help others to teach "this is the
> right way to do a word-split if we were writing in native zsh" when we are
> not?
Because without that explanation it's quite difficult to know what part of the code would behave differently in zsh, and how. Most people are not familiar with shell features, and would have no idea what "word splitting" means in a practical context.
> While it probably is a good description to have in a bug report given to
> zsh folks, it is useless for people who read the history of Git.
Of course it's not. It tells you that there is indeed an issue in zsh, and not in the way we are using it, as it has been acknowledged by zsh developers.
> The readers need to read the solution described in order to understand why
> the updated construct is written in an unnatural (to people who write to
> POSIXy shells) way, or to avoid reintroducing a similar problem elsewhere
> in the future.
Doesn't this explain that?
--- sh emulation should take care of that, but the subshell is messing up with that. ---
Granted, for people not familiar with shell features "subshell" should be accompanied with $(foo).
Show 19 quoted lines
> 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.
Cheers.
--
Felipe Contreras
Felipe Contreras
