# [PATCH 0/6] Detect non email patches in git-mailinfo 10 messages from 2006-05-23 to 2006-05-26. Participants: Eric W. Biederman, Junio C Hamano. Thread: https://gitlist.dev/t/4267 ## Eric W. Biederman, 2006-05-23 19:42 Subject: [PATCH 0/6] Detect non email patches in git-mailinfo Message-ID: URL: https://gitlist.dev/e/m18xosjznu.fsf%40ebiederm.dsl.xmission.com ``` After looking at a number of additional quit patches I noticed a small problem with using the current git-mailinfo. On patches with out any leading headers git-mailinfo can get confused and loose a bit of information. So far I have only seen this in the quilt from Andi Kleen but it is fairly straight forward to fix. What follows is a small patch series that one small step at a time refactors (and I think simplifies) git-mailinfo so that it can detect and cope with a file without any email headers. Eric ``` ## Eric W. Biederman, 2006-05-23 19:44 Subject: [PATCH 1/6] Make read_one_header_line return a flag not a length. Message-ID: URL: https://gitlist.dev/e/m14pzgjzlg.fsf%40ebiederm.dsl.xmission.com In-Reply-To: ``` Currently we only use the return value from read_one_header line to tell if the line we have read is a header or not. So make it a flag. This paves the way for better email detection. Signed-off-by: Eric W. Biederman --- mailinfo.c | 22 +++++++++++----------- 1 files changed, 11 insertions(+), 11 deletions(-) 40f4ca44ec851e435ce9453c682c71b9c67063b9 diff --git a/mailinfo.c b/mailinfo.c index b276519..83a2986 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -331,7 +331,7 @@ struct header_def { int namelen; }; -static void check_header(char *line, int len, struct header_def *header) +static void check_header(char *line, struct header_def *header) { int i; @@ -349,7 +349,7 @@ static void check_header(char *line, int } } -static void check_subheader_line(char *line, int len) +static void check_subheader_line(char *line) { static struct header_def header[] = { { "Content-Type", handle_subcontent_type }, @@ -357,9 +357,9 @@ static void check_subheader_line(char *l handle_content_transfer_encoding }, { NULL }, }; - check_header(line, len, header); + check_header(line, header); } -static void check_header_line(char *line, int len) +static void check_header_line(char *line) { static struct header_def header[] = { { "From", handle_from }, @@ -370,7 +370,7 @@ static void check_header_line(char *line handle_content_transfer_encoding }, { NULL }, }; - check_header(line, len, header); + check_header(line, header); } static int read_one_header_line(char *line, int sz, FILE *in) @@ -709,8 +709,8 @@ static void handle_multipart_body(void) return; /* We are on boundary line. Start slurping the subhead. */ while (1) { - int len = read_one_header_line(line, sizeof(line), stdin); - if (!len) { + int hdr = read_one_header_line(line, sizeof(line), stdin); + if (!hdr) { if (handle_multipart_one_part() < 0) return; /* Reset per part headers */ @@ -718,7 +718,7 @@ static void handle_multipart_body(void) charset[0] = 0; } else - check_subheader_line(line, len); + check_subheader_line(line); } fclose(patchfile); if (!patch_lines) { @@ -787,15 +787,15 @@ int main(int argc, char **argv) exit(1); } while (1) { - int len = read_one_header_line(line, sizeof(line), stdin); - if (!len) { + int hdr = read_one_header_line(line, sizeof(line), stdin); + if (!hdr) { if (multipart_boundary[0]) handle_multipart_body(); else handle_body(); break; } - check_header_line(line, len); + check_header_line(line); } return 0; } -- 1.3.2.g5041c-dirty ``` ## Eric W. Biederman, 2006-05-23 19:45 Subject: [PATCH 2/6] Move B and Q decoding into check header. Message-ID: URL: https://gitlist.dev/e/m1zmh8ikym.fsf_-_%40ebiederm.dsl.xmission.com In-Reply-To: ``` B and Q decoding is not appropriate for in body headers, so move it up to where we explicitly know we have a real email header. Signed-off-by: Eric W. Biederman --- mailinfo.c | 12 +++++------- 1 files changed, 5 insertions(+), 7 deletions(-) 3cccc5a0728a981cc6f4ea72e81513fd902e29a2 diff --git a/mailinfo.c b/mailinfo.c index 83a2986..bee7b20 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -324,6 +324,7 @@ static void cleanup_space(char *buf) } } +static void decode_header_bq(char *it); typedef int (*header_fn_t)(char *); struct header_def { const char *name; @@ -343,6 +344,10 @@ static void check_header(char *line, str int len = header[i].namelen; if (!strncasecmp(line, header[i].name, len) && line[len] == ':' && isspace(line[len + 1])) { + /* Unwrap inline B and Q encoding, and optionally + * normalize the meta information to utf8. + */ + decode_header_bq(line + len + 2); header[i].func(line + len + 2); break; } @@ -597,13 +602,6 @@ static void handle_info(void) cleanup_space(email); cleanup_space(sub); - /* Unwrap inline B and Q encoding, and optionally - * normalize the meta information to utf8. - */ - decode_header_bq(name); - decode_header_bq(date); - decode_header_bq(email); - decode_header_bq(sub); printf("Author: %s\nEmail: %s\nSubject: %s\nDate: %s\n\n", name, email, sub, date); } -- 1.3.2.g5041c-dirty ``` ## Eric W. Biederman, 2006-05-23 19:47 Subject: [PATCH 3/6] Refactor commit messge handling. Message-ID: URL: https://gitlist.dev/e/m1verwikvj.fsf_-_%40ebiederm.dsl.xmission.com In-Reply-To: ``` - Move handle_info into main so it is called once after everything has been parsed. This allows the removal of a static variable and removes two duplicate calls. - Move parsing of inbody headers into handle_commit. This means we parse the in-body headers after we have decoded the character set, and it removes code duplication between handle_multipart_one_part and handle_body. - Change the flag indicating that we have seen an in body prefix header into another bit in seen. This is a little more general and allows the possibility of parsing in body headers after the body message has begun. Signed-off-by: Eric W. Biederman --- mailinfo.c | 58 ++++++++++++++++++++++------------------------------------ 1 files changed, 22 insertions(+), 36 deletions(-) 3f6fe4d5e86c3d8d1fad75bfeb71f398966813d4 diff --git a/mailinfo.c b/mailinfo.c index bee7b20..3fa9505 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -237,38 +237,41 @@ static int eatspace(char *line) #define SEEN_FROM 01 #define SEEN_DATE 02 #define SEEN_SUBJECT 04 +#define SEEN_PREFIX 0x08 /* First lines of body can have From:, Date:, and Subject: */ -static int handle_inbody_header(int *seen, char *line) +static void handle_inbody_header(int *seen, char *line) { + if (*seen & SEEN_PREFIX) + return; if (!memcmp("From:", line, 5) && isspace(line[5])) { if (!(*seen & SEEN_FROM) && handle_from(line+6)) { *seen |= SEEN_FROM; - return 1; + return; } } if (!memcmp("Date:", line, 5) && isspace(line[5])) { if (!(*seen & SEEN_DATE)) { handle_date(line+6); *seen |= SEEN_DATE; - return 1; + return; } } if (!memcmp("Subject:", line, 8) && isspace(line[8])) { if (!(*seen & SEEN_SUBJECT)) { handle_subject(line+9); *seen |= SEEN_SUBJECT; - return 1; + return; } } if (!memcmp("[PATCH]", line, 7) && isspace(line[7])) { if (!(*seen & SEEN_SUBJECT)) { handle_subject(line); *seen |= SEEN_SUBJECT; - return 1; + return; } } - return 0; + *seen |= SEEN_PREFIX; } static char *cleanup_subject(char *subject) @@ -590,12 +593,7 @@ static void decode_transfer_encoding(cha static void handle_info(void) { char *sub; - static int done_info = 0; - - if (done_info) - return; - done_info = 1; sub = cleanup_subject(subject); cleanup_space(name); cleanup_space(date); @@ -609,7 +607,7 @@ static void handle_info(void) /* We are inside message body and have read line[] already. * Spit out the commit log. */ -static int handle_commit_msg(void) +static int handle_commit_msg(int *seen) { if (!cmitmsg) return 0; @@ -633,6 +631,11 @@ static int handle_commit_msg(void) decode_transfer_encoding(line); if (metainfo_charset) convert_to_utf8(line, charset); + + handle_inbody_header(seen, line); + if (!(*seen & SEEN_PREFIX)) + continue; + fputs(line, cmitmsg); } while (fgets(line, sizeof(line), stdin) != NULL); fclose(cmitmsg); @@ -664,26 +667,16 @@ static void handle_patch(void) * that the first part to contain commit message and a patch, and * handle other parts as pure patches. */ -static int handle_multipart_one_part(void) +static int handle_multipart_one_part(int *seen) { - int seen = 0; int n = 0; - int len; while (fgets(line, sizeof(line), stdin) != NULL) { again: - len = eatspace(line); n++; - if (!len) - continue; if (is_multipart_boundary(line)) break; - if (0 <= seen && handle_inbody_header(&seen, line)) - continue; - seen = -1; /* no more inbody headers */ - line[len] = '\n'; - handle_info(); - if (handle_commit_msg()) + if (handle_commit_msg(seen)) goto again; handle_patch(); break; @@ -695,6 +688,7 @@ static int handle_multipart_one_part(voi static void handle_multipart_body(void) { + int seen = 0; int part_num = 0; /* Skip up to the first boundary */ @@ -709,7 +703,7 @@ static void handle_multipart_body(void) while (1) { int hdr = read_one_header_line(line, sizeof(line), stdin); if (!hdr) { - if (handle_multipart_one_part() < 0) + if (handle_multipart_one_part(&seen) < 0) return; /* Reset per part headers */ transfer_encoding = TE_DONTCARE; @@ -730,18 +724,9 @@ static void handle_body(void) { int seen = 0; - while (fgets(line, sizeof(line), stdin) != NULL) { - int len = eatspace(line); - if (!len) - continue; - if (0 <= seen && handle_inbody_header(&seen, line)) - continue; - seen = -1; /* no more inbody headers */ - line[len] = '\n'; - handle_info(); - handle_commit_msg(); + if (fgets(line, sizeof(line), stdin) != NULL) { + handle_commit_msg(&seen); handle_patch(); - break; } fclose(patchfile); if (!patch_lines) { @@ -791,6 +776,7 @@ int main(int argc, char **argv) handle_multipart_body(); else handle_body(); + handle_info(); break; } check_header_line(line); -- 1.3.2.g5041c-dirty ``` ## Eric W. Biederman, 2006-05-23 19:49 Subject: [PATCH 4/6] In handle_body only read a line if we don't already have one. Message-ID: URL: https://gitlist.dev/e/m1r72kiksz.fsf_-_%40ebiederm.dsl.xmission.com In-Reply-To: ``` This prepares for detecting non-email patches that don't have mail headers. In which case we have already read the first line so handle_body should not ignore it. Signed-off-by: Eric W. Biederman --- mailinfo.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) 3ad0c255a351d771c7f301d4a4e9bfb6fdcbde5f diff --git a/mailinfo.c b/mailinfo.c index 3fa9505..99989c2 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -724,7 +724,7 @@ static void handle_body(void) { int seen = 0; - if (fgets(line, sizeof(line), stdin) != NULL) { + if (line[0] || fgets(line, sizeof(line), stdin) != NULL) { handle_commit_msg(&seen); handle_patch(); } -- 1.3.2.g5041c-dirty ``` ## Eric W. Biederman, 2006-05-23 19:53 Subject: [PATCH 5/6] More accurately detect header lines in read_one_header_line Message-ID: URL: https://gitlist.dev/e/m1mzd8iklr.fsf_-_%40ebiederm.dsl.xmission.com In-Reply-To: ``` Only count lines of the form '^.*: ' and '^From ' as email header lines. Signed-off-by: Eric W. Biederman --- mailinfo.c | 25 +++++++++++++++++-------- 1 files changed, 17 insertions(+), 8 deletions(-) b955444f0bfb4ee9a5cd31686dd7eeec0750e235 diff --git a/mailinfo.c b/mailinfo.c index 99989c2..c642ff4 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -385,20 +385,29 @@ static int read_one_header_line(char *li { int ofs = 0; while (ofs < sz) { + const char *colon; int peek, len; if (fgets(line + ofs, sz - ofs, in) == NULL) - return ofs; + break; len = eatspace(line + ofs); if (len == 0) - return ofs; - peek = fgetc(in); ungetc(peek, in); - if (peek == ' ' || peek == '\t') { - /* Yuck, 2822 header "folding" */ - ofs += len; - continue; + break; + colon = strchr(line, ':'); + if (!colon || !isspace(colon[1])) { + /* Readd the newline */ + line[ofs + len] = '\n'; + line[ofs + len + 1] = '\0'; + break; } - return ofs + len; + ofs += len; + /* Yuck, 2822 header "folding" */ + peek = fgetc(in); ungetc(peek, in); + if (peek != ' ' && peek != '\t') + break; } + /* Count mbox From headers as headers */ + if (!ofs && !memcmp(line, "From ", 5)) + ofs = 1; return ofs; } -- 1.3.2.g5041c-dirty ``` ## Eric W. Biederman, 2006-05-23 19:58 Subject: [PATCH 6/6] Allow in body headers beyond the in body header prefix. Message-ID: URL: https://gitlist.dev/e/m1irnwikcz.fsf_-_%40ebiederm.dsl.xmission.com In-Reply-To: ``` - handle_from is fixed to not mangle it's input line. - Then handle_inbody_header is allowed to look in the body of a commit message for additional headers that we haven't already seen. This allows patches with all of the right information in unfortunate places to be imported. Signed-off-by: Eric W. Biederman --- mailinfo.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-) eca59d2fd60af47170cdbfdebf3384465f0e7635 diff --git a/mailinfo.c b/mailinfo.c index c642ff4..99374b3 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -72,11 +72,14 @@ static int bogus_from(char *line) return 1; } -static int handle_from(char *line) +static int handle_from(char *in_line) { - char *at = strchr(line, '@'); + char line[1000]; + char *at; char *dst; + strcpy(line, in_line); + at = strchr(line, '@'); if (!at) return bogus_from(line); @@ -242,8 +245,6 @@ #define SEEN_PREFIX 0x08 /* First lines of body can have From:, Date:, and Subject: */ static void handle_inbody_header(int *seen, char *line) { - if (*seen & SEEN_PREFIX) - return; if (!memcmp("From:", line, 5) && isspace(line[5])) { if (!(*seen & SEEN_FROM) && handle_from(line+6)) { *seen |= SEEN_FROM; -- 1.3.2.g5041c-dirty ``` ## Junio C Hamano, 2006-05-23 23:44 Subject: Re: [PATCH 0/6] Detect non email patches in git-mailinfo Message-ID: <7vlkss1f32.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vlkss1f32.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Thanks. Merged to "next", this probably would graduate to "master" by the end of the week if not earlier. ``` ## Junio C Hamano, 2006-05-26 07:29 Subject: Re: [PATCH 5/6] More accurately detect header lines in read_one_header_line Message-ID: <7vy7wpnt0t.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vy7wpnt0t.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` ebiederm@xmission.com (Eric W. Biederman) writes: > Only count lines of the form '^.*: ' and '^From ' as email > header lines. I am having trouble with this patch. > diff --git a/mailinfo.c b/mailinfo.c > index 99989c2..c642ff4 100644 > --- a/mailinfo.c > +++ b/mailinfo.c > @@ -385,20 +385,29 @@ static int read_one_header_line(char *li > { > int ofs = 0; > while (ofs < sz) { > + const char *colon; > int peek, len; > if (fgets(line + ofs, sz - ofs, in) == NULL) > + break; > len = eatspace(line + ofs); > if (len == 0) > + break; > + colon = strchr(line, ':'); > + if (!colon || !isspace(colon[1])) { > + /* Readd the newline */ > + line[ofs + len] = '\n'; > + line[ofs + len + 1] = '\0'; > + break; > } Because eatspace() eats the trailing space, although your commit message say lines matching "^.*: " are headers, this does not match the criteria: X-Spam-Checker-Version: SpamAssassin 3.1.1 (2006-03-10) on gitster.siamese.dyndns.org -> X-Spam-Level: X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00 autolearn=ham version=3.1.1 Notice that the field body for this unstructured header X-Spam-Level (an optional field) consists of a single whitespace. It will be gone because of eatspace() when your check sees the line, so the header parsing stops prematurely. Was there a particular reason you needed this change? That is, did you have to parse mail-looking input that does not have a blank line between runs of headers and the body of the message? If so, I'd at least like to remove the || !isspace(colon[1]) from the test. After all, I do not think RFC2822 requires a whitespace after the colon there. ``` ## Eric W. Biederman, 2006-05-26 08:16 Subject: Re: [PATCH 5/6] More accurately detect header lines in read_one_header_line Message-ID: URL: https://gitlist.dev/e/m1d5e1ciav.fsf%40ebiederm.dsl.xmission.com In-Reply-To: <7vr72hns7h.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano writes: > Junio C Hamano writes: > >> Was there a particular reason you needed this change? That is, >> did you have to parse mail-looking input that does not have a >> blank line between runs of headers and the body of the message? Yes. I had patches that had a subject line followed by a blank line, and the problem was that the old check thought the subject was a header line, despite not even having a colon in it. >> If so, I'd at least like to remove the || !isspace(colon[1]) >> from the test. After all, I do not think RFC2822 requires a >> whitespace after the colon there. > > In other words, something like this (tested): Looks good to me, sorry for missing that one. Eric ```