Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations

From: Jonathan Tan <jonathantanmy@google.com>
Date: Sep 17, 2016, 00:22 UTC
Message-ID: <d5eab197-e079-4c95-2661-9a576b9b5707@google.com>
In-Reply-To: <xmqq7fabigzf.fsf@gitster.mtv.corp.google.com>

On 09/16/2016 04:04 PM, Junio C Hamano wrote:

Show 15 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
>> I'm concerned about what happens if check_header fails - we would then
>> have some lines which need to be treated as log messages. (At least,
>> they are currently treated that way.)
>
> I actually think we should refactor check_header() further so that
> in-body header processing does not even see things that shouldn't be
> changed.  The current check_header() should be used only for real
> mail headers, and then a reduced version of check_header() that is
> called for in-body will _ONLY_ handle the header lines that are
> handled by the first "search for the interesting parts" loop.
>
> And of course we would update your "does it look like rfc2822?" to
> match what are handled by the "interesting parts" loop.  That I
> think would match the current behaviour much better, I would think.

There would be a bit of code duplication in that this "does it look like rfc2822" function would also need to account for duplicate headers (e.g. 2 "Subject:" lines in the in-body headers) because check_header would reject the 2nd one, but that is minor. (Alternatively, we could just allow duplicate headers in the in-body headers.)

> The ">From " and "[PATCH]" cases in check_header() should not even
> be there.  We should handle them inside handle_commit_msg(), as
> these two cases should never appear in the real header part of a
> message.

> And if we clean it up like that, I do not think we would ever need
> to worry about "ah, it looked like a header but it is not after
> all".  And not having to worry about it is a good thing and should
> be one of the primary goals in this conversion, I whould think.

Yes, this makes sense. I'll go ahead and make a patch set implementing this (unless anyone has any objections).