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