Re: [PATCH] git-am: don't ignore --keep (-k) option

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 27, 2009, 21:11 UTC
Message-ID: <7vfx7zk70c.fsf@alter.siamese.dyndns.org>
In-Reply-To: <87y6lr203s.fsf@meyering.net>

Jim Meyering <jim@meyering.net> writes:

Show 7 quoted lines

>> At the level of "what does each line of the code do", this is a fix, but
>> as we do a lot more than just stripping "[PATCH] " from the beginning of
>> the Subject: line these days, I think we are better off declaring defeat
>> in this particular codepath and not doing anything here.
>
> Sounds fine to me.
> Glad you're keeping everything in perspective.

What the case statement tries to do is _wrong_; "mailinfo -k" keeps the original prefix and all the case statement does is to add an extra [PATCH] that did not exist anywhere in the original on top of that.

What is funny is that the case statement has been trying to do a wrong thing from day-one, ever since the script was introduced in d1c5f2a (Add git-am, applymbox replacement., 2005-10-07). That version uses $keep to hold -k or empty, gives that to mailinfo for producing $dotest/info, and it has the same case statement that switches on $keep_subject nobody sets to add an extra "[PATCH]" in front. Luckily, due to the typo you found, nobody was bitten by the bug, and your patch will break things for people by enabling it ;-).

Thanks for noticing this one. It began an innocent bug nobody noticed, but it is embarrassing that we carefully _maintained_ that code nobody triggers for four years.