Re: [PATCH/WIP v3 06/31] am: detect mbox patches

On Thu, Jun 25, 2015 at 9:40 PM, Paul Tan [off-list ref] wrote:

On Wed, Jun 24, 2015 at 11:10 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

quoted

+     else if (l1.len && l2.len && l3.len && is_email(paths->items->string))
+             ret = PATCH_FORMAT_MBOX;

Maybe we can do better than this by folding the `is_email() function into this here function, reusing the same strbuf to read the lines and keeping track of the email header lines we saw... I would really like to avoid opening the same file twice just to figure out whether it is in email format.

Okay, how about every time we call a strbuf_getline(), we save the
line to a string_list as well? Like string_list_getline_crlf() below:
[...]

Hmm, on second thought, I don't think it's worth the code complexity.
While I agree it's desirable to not open the file twice, I don't think
detecting the patch format is so IO intensive that it needs to be
optimized to that extent.

Instead, we should probably just modify is_email() to take a FILE*,
and then fseek(fp, 0L, SEEK_SET) to the beginning.

I think the logic of is_email() is complex and so it should not be
folded into the detect_patch_format() function, especially since we
may add detection of other patch formats in the future, and may need
more complex heuristics.

Regards,
Paul