Re: [PATCH] format-patch: Properly escape From_ lines when creating an mbox.

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2010, 14:49 UTC
Message-ID: <7vaar3nds1.fsf@alter.siamese.dyndns.org>
In-Reply-To: <87eiggiy8g.fsf@yoom.home.cworth.org>

Carl Worth <cworth@cworth.org> writes:

Show 10 quoted lines

> On Tue, 08 Jun 2010 20:50:01 -0700, Junio C Hamano <gitster@pobox.com> wrote:
>> Carl Worth <cworth@cworth.org> writes:
>> Especially because your implementation quotes lines that begin with "From "
>> unconditionally (even when the tail end of the line would never be a
>> valid-looking timestamp).  Such an output will confuse existing mailsplit,
>> but the worst part of the story is that somebody who is applying a series
>> of patches will _not_ notice the breakage.  The payload of the second and
>> subsequent messages will likely be concatenated as if it were part of the
>> first message, ignoring cruft between patches, but the resulting tree
>> would likely to be the same as what the sending end intended.

Please disregard the above; I wasn't thinking straight.

If your format-patch quotes ">*From " in the log message, and you unquote it somewhere in mailsplit to mailinfo pipeline, then the only time any funny interaction between the current git and your git would happen is when your git formats a commit with a line in its log that begins with "From " that cannot be a mistaken as a UNIX-From line and you use "am" from the current git on the output; the resulting commit would get an extra ">" left in the message, but that is a small price to pay. There is no other downside I can see (and the upside is that the output from your format-patch won't be split incorrectly, of course).

I find that the change to format-patch not to emit the UNIX-From line when generating one file per commit is somewhat iffy. An upside is that the existing mailsplit-mailinfo pipeline already knows not to split such an input, so the change makes

    git format-patch <revspec> |
    while read path
    do
        git am $path || break
    done

(which is essentially what an old rebase did) do the right thing, even in the presense of a confusing "From " in the log message.

That change however is not without downsides. You may potentially be breaking people's existing scripts in various ways. They may be relying on the presense of the line by:

 - using it to pick up the original commit object name from, just like
   "rebase" does;

 - using it as the "magic" number to protect them from being fed a bad
   input;

 - stripping the first line unconditionally, assuming it is that UNIX-From
   line they shouldn't cut and paste into the MUA.

It is nice at the conceptual level, though. By declaring individual file RFC2822 message (not mbox), it makes it very clear that it is MUA's responsibility to quote "From " in the payload when the output is used by MUA to compose and send a message. IOW, we shouldn't be doing the quote in our output when operating in that mode.

Thanks.