On Fri, Feb 3, 2012 at 10:23 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Felipe Contreras <felipe.contreras@xxxxxxxxx> 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).
> 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
--
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