{"thread":{"id":"7481","subject":"Re: [PATCH] git-mailinfo fixes for patch munging","startedAt":"2007-03-30T16:18:45Z","lastAt":"2007-03-30T21:32:10Z","messageCount":3,"participants":["Junio C Hamano","Don Zickus"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"38398","messageId":"20070330161845.GI11029@redhat.com","threadId":"7481","inReplyTo":null,"subject":"[PATCH] git-mailinfo fixes for patch munging","fromName":"Don Zickus","fromEmail":"dzickus@redhat.com","sentAt":"2007-03-30T16:18:45Z","receivedAt":"2007-03-30T16:18:45Z","isPatch":true,"sender":{"key":"dzickus@redhat.com","avatar":null},"body":"Don't translate the patch to UTF-8, instead preserve the data as is.  Also\nallow overwriting the primary mail headers (addresses Linus's concern).  \n\nI also revert a test case that was included in the original patch.  Now it\nmakes sense why it was the way it was. :)\n\nCheers,\nDon\n\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex d94578c..71b6457 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -294,14 +294,14 @@ static char *header[MAX_HDR_PARSED] = {\n \t\"From\",\"Subject\",\"Date\",\n };\n \n-static int check_header(char *line, char **hdr_data)\n+static int check_header(char *line, char **hdr_data, int overwrite)\n {\n \tint i;\n \n \t/* search for the interesting parts */\n \tfor (i = 0; header[i]; i++) {\n \t\tint len = strlen(header[i]);\n-\t\tif (!hdr_data[i] &&\n+\t\tif ((!hdr_data[i] || overwrite) &&\n \t\t    !strncasecmp(line, header[i], len) &&\n \t\t    line[len] == ':' && isspace(line[len + 1])) {\n \t\t\t/* Unwrap inline B and Q encoding, and optionally\n@@ -614,6 +614,7 @@ static int find_boundary(void)\n \n static int handle_boundary(void)\n {\n+\tchar newline[]=\"\\n\";\n again:\n \tif (!memcmp(line+content_top->boundary_len, \"--\", 2)) {\n \t\t/* we hit an end boundary */\n@@ -628,7 +629,7 @@ again:\n \t\t\t\t\t\"can't recover\\n\");\n \t\t\texit(1);\n \t\t}\n-\t\thandle_filter(\"\\n\");\n+\t\thandle_filter(newline);\n \n \t\t/* skip to the next boundary */\n \t\tif (!find_boundary())\n@@ -643,7 +644,7 @@ again:\n \n \t/* slurp in this section's info */\n \twhile (read_one_header_line(line, sizeof(line), fin))\n-\t\tcheck_header(line, p_hdr_data);\n+\t\tcheck_header(line, p_hdr_data, 1);\n \n \t/* eat the blank line after section info */\n \treturn (fgets(line, sizeof(line), fin) != NULL);\n@@ -699,10 +700,14 @@ static int handle_commit_msg(char *line)\n \t\t\tif (!*cp)\n \t\t\t\treturn 0;\n \t\t}\n-\t\tif ((still_looking = check_header(cp, s_hdr_data)) != 0)\n+\t\tif ((still_looking = check_header(cp, s_hdr_data, 0)) != 0)\n \t\t\treturn 0;\n \t}\n \n+\t/* normalize the log message to UTF-8. */\n+\tif (metainfo_charset)\n+\t\tconvert_to_utf8(line, charset);\n+\n \tif (patchbreak(line)) {\n \t\tfclose(cmitmsg);\n \t\tcmitmsg = NULL;\n@@ -767,12 +772,8 @@ static void handle_body(void)\n \t\t\t\treturn;\n \t\t}\n \n-\t\t/* Unwrap transfer encoding and optionally\n-\t\t * normalize the log message to UTF-8.\n-\t\t */\n+\t\t/* Unwrap transfer encoding */\n \t\tdecode_transfer_encoding(line);\n-\t\tif (metainfo_charset)\n-\t\t\tconvert_to_utf8(line, charset);\n \n \t\tswitch (transfer_encoding) {\n \t\tcase TE_BASE64:\n@@ -875,7 +876,7 @@ int mailinfo(FILE *in, FILE *out, int ks, const char *encoding,\n \n \t/* process the email header */\n \twhile (read_one_header_line(line, sizeof(line), fin))\n-\t\tcheck_header(line, p_hdr_data);\n+\t\tcheck_header(line, p_hdr_data, 1);\n \n \thandle_body();\n \thandle_info();\ndiff --git a/t/t5100/patch0005 b/t/t5100/patch0005\nindex e7d6f66..7d24b24 100644\n--- a/t/t5100/patch0005\n+++ b/t/t5100/patch0005\n@@ -61,7 +61,7 @@ diff --git a/git-cvsimport-script b/git-cvsimport-script\n  \t\tpush(@old,$fn);\n \n -- \n-David KÃ¥gedal\n+David Kågedal\n -\n To unsubscribe from this list: send the line \"unsubscribe git\" in\n the body of a message to majordomo@vger.kernel.org\n"},{"id":"38378","messageId":"7vmz1uzaxd.fsf@assigned-by-dhcp.cox.net","threadId":"7481","inReplyTo":"20070330161845.GI11029@redhat.com","subject":"Re: [PATCH] git-mailinfo fixes for patch munging","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-30T21:19:42Z","receivedAt":"2007-03-30T21:19:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Don Zickus <dzickus@redhat.com> writes:\n\n> Don't translate the patch to UTF-8, instead preserve the data as is.  Also\n> allow overwriting the primary mail headers (addresses Linus's concern).  \n>\n> I also revert a test case that was included in the original patch.  Now it\n> makes sense why it was the way it was. :)\n>\n> Cheers,\n> Don\n\nThanks.  Sign-off would have been nice.\n\n> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\n> index d94578c..71b6457 100644\n> --- a/builtin-mailinfo.c\n> +++ b/builtin-mailinfo.c\n> @@ -294,14 +294,14 @@ static char *header[MAX_HDR_PARSED] = {\n>  \t\"From\",\"Subject\",\"Date\",\n>  };\n>  \n> -static int check_header(char *line, char **hdr_data)\n> +static int check_header(char *line, char **hdr_data, int overwrite)\n>  {\n>  \tint i;\n>  \n>  \t/* search for the interesting parts */\n>  \tfor (i = 0; header[i]; i++) {\n>  \t\tint len = strlen(header[i]);\n> -\t\tif (!hdr_data[i] &&\n> +\t\tif ((!hdr_data[i] || overwrite) &&\n>  \t\t    !strncasecmp(line, header[i], len) &&\n>  \t\t    line[len] == ':' && isspace(line[len + 1])) {\n>  \t\t\t/* Unwrap inline B and Q encoding, and optionally\n\nThis check_header is called from each multi-part boundary with\noverwrite=1, so if you have two parts and you have From: or\nSubject: in the multi-part header (not in-body), wouldn't they\noverwrite what we already have?  That is not desired, I would\nthink.\n\nFor non multi-part case, what we traditionally have done is:\n\n\t* Take Subject:, Date:, and From: from RFC2822 headers\n          to prime the title and authorship information.\n\n\t* The first lines of the body of the message (i.e. after\n          the blank line that separates 2822 headers and the\n          body) can look like the above header lines to\n          override.\n\n\t* A line that does not look like an overriding in-body\n          header line is the first line of the commit log\n          message.  After that, nothing is taken as an\n          overriding in-body header.\n\nFor a multi-part, I think we only processed the first part as\nthe commit log message, potentially starting with the overriding\nin-body headers.  In other words, in-body headers are what the\nuser *types* to override what the MUA says in RFC2822 headers.\nAs the stuff that follow the multi-part boundary (like\ncontent-type and transfer encoding) are of the MUA kind, I\nsuspect we do not want it to override what the sender said in\nthe earlier parts of the message.\n\n> @@ -614,6 +614,7 @@ static int find_boundary(void)\n>  \n>  static int handle_boundary(void)\n>  {\n> +\tchar newline[]=\"\\n\";\n>  again:\n>  \tif (!memcmp(line+content_top->boundary_len, \"--\", 2)) {\n>  \t\t/* we hit an end boundary */\n> @@ -628,7 +629,7 @@ again:\n>  \t\t\t\t\t\"can't recover\\n\");\n>  \t\t\texit(1);\n>  \t\t}\n> -\t\thandle_filter(\"\\n\");\n> +\t\thandle_filter(newline);\n>  \n>  \t\t/* skip to the next boundary */\n>  \t\tif (!find_boundary())\n\nThese two hunks certainly do not hurt, but why?  Is this about\nthe constness of the first parameter to handle_filter() and its\ncall chain?\n\nHaving said that, the result of the patch is much better.  \n\nIn fact, I couldn't \"git am\" this patch (the part that reverts\nthe test vector) with the current tip of 'master' because of the\nbreakage you are fixing with it ;-).\n\nNow I can.  So I'd probably take this patch for now.\n"},{"id":"38380","messageId":"20070330213210.GL11029@redhat.com","threadId":"7481","inReplyTo":"7vmz1uzaxd.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-mailinfo fixes for patch munging","fromName":"Don Zickus","fromEmail":"dzickus@redhat.com","sentAt":"2007-03-30T21:32:10Z","receivedAt":"2007-03-30T21:32:10Z","isPatch":true,"sender":{"key":"dzickus@redhat.com","avatar":null},"body":"On Fri, Mar 30, 2007 at 02:19:42PM -0700, Junio C Hamano wrote:\n> Don Zickus <dzickus@redhat.com> writes:\n> \n> > Don't translate the patch to UTF-8, instead preserve the data as is.  Also\n> > allow overwriting the primary mail headers (addresses Linus's concern).  \n> >\n> > I also revert a test case that was included in the original patch.  Now it\n> > makes sense why it was the way it was. :)\n> >\n> > Cheers,\n> > Don\n> \n> Thanks.  Sign-off would have been nice.\n\nDoh. Sorry.  Should I repost with fix below?\n\n> This check_header is called from each multi-part boundary with\n> overwrite=1, so if you have two parts and you have From: or\n> Subject: in the multi-part header (not in-body), wouldn't they\n> overwrite what we already have?  That is not desired, I would\n> think.\n\nHmm.  I guess I never thought about that case.  You are right, that check\ncan be changed to a zero (because the rfc2822 are checked elsewhere).\n\n> > @@ -614,6 +614,7 @@ static int find_boundary(void)\n> >  \n> >  static int handle_boundary(void)\n> >  {\n> > +\tchar newline[]=\"\\n\";\n> >  again:\n> >  \tif (!memcmp(line+content_top->boundary_len, \"--\", 2)) {\n> >  \t\t/* we hit an end boundary */\n> > @@ -628,7 +629,7 @@ again:\n> >  \t\t\t\t\t\"can't recover\\n\");\n> >  \t\t\texit(1);\n> >  \t\t}\n> > -\t\thandle_filter(\"\\n\");\n> > +\t\thandle_filter(newline);\n> >  \n> >  \t\t/* skip to the next boundary */\n> >  \t\tif (!find_boundary())\n> \n> These two hunks certainly do not hurt, but why?  Is this about\n> the constness of the first parameter to handle_filter() and its\n> call chain?\n\nYeah, I SEGFAULT'd when trying to convert_to_utf8() a fixed string. :-)\n\nCheers,\nDon\n"}]}