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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 3, 2012, 20:23 UTC
Message-ID: <7v1uqbpsyh.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1328214625-3576-2-git-send-email-felipe.contreras@gmail.com>

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.

Show 12 quoted lines

> 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.

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?

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. Unless you are writing zsh-native completion script and this patch is not about git-completion.bash, that is.

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. 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).

Thanks.