From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, peff@peff.net
Subject: Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations
Date: Fri, 16 Sep 2016 16:04:36 -0700 [thread overview]
Message-ID: <xmqq7fabigzf.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <128036e3-47b4-b7ae-034f-8d227a62729f@google.com> (Jonathan Tan's message of "Fri, 16 Sep 2016 15:36:05 -0700")
Jonathan Tan <jonathantanmy@google.com> writes:
> On 09/16/2016 01:59 PM, Junio C Hamano wrote:
>> if (mi->in_line_header->len) {
>> /* we have read the beginning of one in-line header */
>> if (line->len && isspace(*line->buf) &&
>> !(mi->use_scissors && is_scissors_line(line))) {
>
> Minor note: this means that the scissors check appears twice in the
> code, once here and once below (for the non-header case).
Yes. I actually was wondering if it is even more sensible to always
have the scissors check at the very beginning. Even if we saw a
half-written in-body header already in the message, when we see a
scissors line, we clear the slate and restart as if the line after
the scissors is the first line in the body of the message.
>> append to mi->in_line_header strbuf;
>> return 0;
>> }
>> /* otherwise we know mi->in_line_header is now complete */
>> check_header(mi, mi->in_line_header, ...);
>
> (Sorry - should have also noticed this in your original e-mail.)
>
> 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.
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.
Thanks.
next prev parent reply other threads:[~2016-09-16 23:04 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-02 19:58 [PATCH] sequencer: support folding in rfc2822 footer Jonathan Tan
2016-09-03 2:23 ` Junio C Hamano
2016-09-06 22:08 ` Jonathan Tan
2016-09-06 23:30 ` Jonathan Tan
2016-09-07 6:38 ` Jeff King
2016-09-16 17:37 ` [RFC/PATCH 0/3] handle multiline in-body headers Jonathan Tan
2016-09-16 18:29 ` Junio C Hamano
2016-09-16 17:37 ` [RFC/PATCH 1/3] mailinfo: refactor commit message processing Jonathan Tan
2016-09-16 19:12 ` Junio C Hamano
2016-09-16 21:46 ` Jeff King
2016-09-16 17:37 ` [RFC/PATCH 2/3] mailinfo: correct malformed test example Jonathan Tan
2016-09-16 19:19 ` Junio C Hamano
2016-09-16 22:42 ` Jonathan Tan
2016-09-16 22:55 ` Junio C Hamano
2016-09-17 0:31 ` Jonathan Tan
2016-09-17 3:48 ` Junio C Hamano
2016-09-16 17:37 ` [RFC/PATCH 3/3] mailinfo: handle in-body header continuations Jonathan Tan
2016-09-16 20:17 ` Junio C Hamano
2016-09-16 20:49 ` Jonathan Tan
2016-09-16 20:59 ` Junio C Hamano
2016-09-16 22:36 ` Jonathan Tan
2016-09-16 23:04 ` Junio C Hamano [this message]
2016-09-17 0:22 ` Jonathan Tan
2016-09-16 21:51 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq7fabigzf.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).