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

[Jonathan Tan <jonathantanmy@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).

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