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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Sep 16, 2016, 22:36 UTC
Message-ID: <128036e3-47b4-b7ae-034f-8d227a62729f@google.com>
In-Reply-To: <xmqqshszimrh.fsf@gitster.mtv.corp.google.com>

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).

Show 5 quoted lines

>                         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.)

To treat them as log messages, we would need to convert them into UTF-8, which may possibly fail, so we would have to figure out how to clean up (we have to clean up because we cannot `die` immediately, at least to preserve the current behavior). Also, we are likely to detect such a failure only while processing a subsequent line - this non-"fail fast" currently is fine, but I'm concerned that it will hinder future development (especially when debugging).

Minor note: the buffer would also need to be more complicated (instead of the current single buffer), either:

o store newlines in that buffer (and we would need to remove all
   newlines before passing to check_header), or
o 2 buffers: one with newlines (for log messages) and one without (for
   check_header).

In light of the above (multiple scissors checks, late detection of failure, more complicated buffer), it seems clearer to me to just change the order of the checks (as in RFC/PATCH 1/3). This necessitates holding on to the old un-decoded buf and len, but this seems easier to me than the above.

>                 strbuf_reset(&mi->in_line_header);
>         }
> 	...