Re: [PATCH v3 1/4] completion: be nicer with zsh

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Feb 2, 2012, 10:12 UTC
Message-ID: <CAMP44s0w1eXWWaMT3WAfLxFHPQvc9dp33cyJ=T2im6g7rsrKhw@mail.gmail.com>
In-Reply-To: <20120202084859.GC3823@burratino>

On Thu, Feb 2, 2012 at 10:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 12 quoted lines

> Felipe Contreras wrote:
>
>> Felipe Contreras (4):
>>   completion: be nicer with zsh
>
> Since I can't find this patch in the mail archive, I'll reply here.
> Luckily the most important bit is above already.
>
> I think I mentioned before that this subject line is what will appear
> in the shortlog and the shortlog is all that some people will see of
> the changelog, so it should include a self-contained description of
> the impact of the patch.

Exactly, and "completion: avoid default value assignment on : true command" tells *nothing* to most people. Why is this patch needed? Do I care about it?

OTOH "completion: be nicer with zsh" explains the purpose of the patch and people that don't care about zsh can happily ignore it if they want, and the ones that care about zsh might want to back port it, or whatever.

Also, if you are looking at a shortlog and looking for patches related to zsh, you would easily see this.

"completion: avoid default value assignment on : true command" almost ensures that the people reading that summary would need to read further to see *why*.

> -- >8 --
> From: Felipe Contreras <felipe.contreras@gmail.com>
> Date: Thu, 2 Feb 2012 03:15:17 +0200
> Subject: completion: avoid default value assignment on : true command

No useful information provided yet. Should I care about this? I guess I need to open the mail and see.

> zsh versions from 4.3.0 to present (4.3.15) do not correctly propagate
> the SH_WORD_SPLIT option into the subshell in ${foo:=$(bar)}
> expressions.  For example, after running

Ok, a bug in zsh, I can skip this. Or if I care about zsh, then I still have to read further, because I still don't know what's the problem.

And BTW, this is extremely detailed. Where's the evidence? Is there a bug report? How can I follow this to find out when it's fixed. (hint: missing link)

Show 11 quoted lines

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

Ok, still reading.

> So avoid the construct, and use an explicit 'test -n "$foo" ||
> foo=$(bar)' instead.

Yeah...

Show 5 quoted lines

> This fixes a bug tht caused all commands to be
> treated as porcelain and show up in "git <TAB><TAB>" completion,
> because the list of all commands was treated as a single word in
> __git_list_porcelain_commands and did not match any of the patterns
> that would usually cause plumbing to be excluded.

Aha! Finally we arrive to the important part.

I guess we have different styles of writing. Personally, I don't see the point of forcing the readers to go through the whole thing. Compare this to mine:

| Subject: [PATCH v3 1/4] completion: be nicer with zsh

At this point most people can skip this.

| And yet another bug in zsh[1] causes a mismatch; zsh seems to have
| problem emulating wordspliting, but only on the command substitution.
|
| Let's avoid it.

A bug in zsh in certain situations... got it.

| I found this issue because __git_compute_porcelain_commands listed all
| commands (not only porcelain).

At this point it might have been better to mention "git <TAB><TAB>" instead, as you did. But not a big deal, the problem is clear; all commands are listed.

Almost all people would stop at this point; either they don't care about zsh, or they care, and they want the patch.

| This is because in zsh the following code:
|
|  for i in $__git_all_commands
|
| would evaluate $__git_all_commands as a single word (with spaces),
| ${=__git_all_commands} should be used to do word splitting expansion
| (unless SH_WORD_SPLIT is used).
|
| sh emulation should take care of that, but the command expantion is
| messing up with that.

And more details for the skeptics, or the ones that want to learn more.

| tl;dr: $__git_porcelain_commands = $__git_all_commands

Wrapping it up, to make clear what happens.

| [1] http://article.gmane.org/gmane.comp.shells.zsh.devel/24296

And then a link to further details, discussion, and possible fixes.

Cheers.

-- 
Felipe Contreras