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

[Junio C Hamano <gitster@pobox.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.

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