{"thread":{"id":"32863","subject":"segfault for git log --graph --no-walk --grep a","startedAt":"2013-02-08T23:52:16Z","lastAt":"2013-02-26T06:47:39Z","messageCount":26,"participants":["Thomas Haller","Junio C Hamano","Jeff King","Johannes Sixt","Torsten Bögershausen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"209048","messageId":"201302090052.22053.thom311@gmail.com","threadId":"32863","inReplyTo":null,"subject":"segfault for git log --graph --no-walk --grep a","fromName":"Thomas Haller","fromEmail":"thom311@gmail.com","sentAt":"2013-02-08T23:52:16Z","receivedAt":"2013-02-08T23:52:16Z","isPatch":false,"sender":{"key":"thom311@gmail.com","avatar":null},"body":"Hallo,\n\n\nI just found that git crashes with a segmentation fault when calling\n$ git log --graph --no-walk --grep pattern\n\nIt happens both for version 1.7.10.4 from Debian (wheezy,amd64) and\na fresh compiled git from github (git.git repository, master).\n\n\nFor the error to occure, the pattern must match the commit message in HEAD.\n\n\n\n\nHow to reproduce:\n git init .\n git commit -m 'text1' --allow-empty\n git commit -m 'text2' --allow-empty\n git log --graph --no-walk --grep 'text2'\n\n\n\nAnd here is coredump I got:\n\nCore was generated by `git log --graph --no-walk --grep text2'.\nProgram terminated with signal 11, Segmentation fault.\n#0 __strlen_sse42 () at ../sysdeps/x86_64/multiarch/strlen-sse4.S:32\n#1 0x00000000004cc13e in commit_match (opt=0x7fffbd0ee500, commit=0x216d1a8) at revision.c:2306\n#2 get_commit_action (revs=0x7fffbd0ee500, commit=0x216d1a8) at revision.c:2338\n#3 0x00000000004984b4 in graph_is_interesting (commit=, graph=) at graph.c:330\n#4 0x0000000000498569 in first_interesting_parent (graph=graph@entry=0x21629c0) at graph.c:369\n#5 0x000000000049965e in graph_update (graph=0x21629c0, commit=) at graph.c:593\n#6 0x00000000004cc7a9 in get_revision (revs=revs@entry=0x7fffbd0ee500) at revision.c:2580\n#7 0x000000000043988a in cmd_log_walk (rev=rev@entry=0x7fffbd0ee500) at builtin/log.c:309\n#8 0x000000000043a398 in cmd_log (argc=9, argv=0x2162930, prefix=0x0) at builtin/log.c:582\n#9 0x0000000000405988 in run_builtin (argv=0x2162930, argc=9, p=0x751438) at git.c:281\n#10 handle_internal_command (argc=9, argv=0x2162930) at git.c:443\n#11 0x0000000000404df2 in run_argv (argv=0x7fffbd0eec00, argcp=0x7fffbd0eec0c) at git.c:489\n#12 main (argc=9, argv=0x2162930) at git.c:564\n\n\nit happens in file revision.c:2306 because \"commit->buffer\" is zero:\n\n retval = grep_buffer(&opt->grep_filter,\n commit->buffer, strlen(commit->buffer));\n\n\n\n\n\nthank you all, for this awesome software.\nThomas\n"},{"id":"209049","messageId":"7vsj56xsg5.fsf@alter.siamese.dyndns.org","threadId":"32863","inReplyTo":"201302090052.22053.thom311@gmail.com","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-09T00:05:14Z","receivedAt":"2013-02-09T00:05:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Haller writes:\n\n> it happens in file revision.c:2306 because \"commit->buffer\" is zero:\n>\n> retval = grep_buffer(&opt->grep_filter,\n> commit->buffer, strlen(commit->buffer));\n\nI think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily\nload missing commit buffers, 2013-01-26); I haven't merged it to any\nof the maintenance releases, but the tip of 'master' should have it\nalready.\n"},{"id":"209050","messageId":"7vobfuxrns.fsf@alter.siamese.dyndns.org","threadId":"32863","inReplyTo":"7vsj56xsg5.fsf@alter.siamese.dyndns.org","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-09T00:22:15Z","receivedAt":"2013-02-09T00:22:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Haller writes:\n>\n>> it happens in file revision.c:2306 because \"commit->buffer\" is zero:\n>>\n>> retval = grep_buffer(&opt->grep_filter,\n>> commit->buffer, strlen(commit->buffer));\n>\n> I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily\n> load missing commit buffers, 2013-01-26); I haven't merged it to any\n> of the maintenance releases, but the tip of 'master' should have it\n> already.\n\nAh, no, this shares the same roots as the breakage the said commit\nfixed, and the solution should use the same idea, but not fixed.\n"},{"id":"209051","messageId":"20130209002710.GA5570@sigill.intra.peff.net","threadId":"32863","inReplyTo":"7vobfuxrns.fsf@alter.siamese.dyndns.org","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-09T00:27:10Z","receivedAt":"2013-02-09T00:27:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 08, 2013 at 04:22:15PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Thomas Haller writes:\n> >\n> >> it happens in file revision.c:2306 because \"commit->buffer\" is zero:\n> >>\n> >> retval = grep_buffer(&opt->grep_filter,\n> >> commit->buffer, strlen(commit->buffer));\n> >\n> > I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily\n> > load missing commit buffers, 2013-01-26); I haven't merged it to any\n> > of the maintenance releases, but the tip of 'master' should have it\n> > already.\n> \n> Ah, no, this shares the same roots as the breakage the said commit\n> fixed, and the solution should use the same idea, but not fixed.\n\nYeah, I think this needs separate treatment. However, this is a perfect\nexample of the \"case-by-case\" I mention in the final two paragraphs\nthere.\n\nWhat's the right encoding to be grepping in? The original, what we will\noutput in, or even something else? IOW, I wonder if this should be using\nlogmsg_reencode in the first place (the obvious reason not to want to do\nso is speed, but logmsg_reencode is written to only have an impact in\nthe case that we do indeed need to reencode).\n\n-Peff\n"},{"id":"209052","messageId":"7vk3qixrc8.fsf@alter.siamese.dyndns.org","threadId":"32863","inReplyTo":"7vobfuxrns.fsf@alter.siamese.dyndns.org","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-09T00:29:11Z","receivedAt":"2013-02-09T00:29:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> Thomas Haller writes:\n>>\n>>> it happens in file revision.c:2306 because \"commit->buffer\" is zero:\n>>>\n>>> retval = grep_buffer(&opt->grep_filter,\n>>> commit->buffer, strlen(commit->buffer));\n>>\n>> I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily\n>> load missing commit buffers, 2013-01-26); I haven't merged it to any\n>> of the maintenance releases, but the tip of 'master' should have it\n>> already.\n>\n> Ah, no, this shares the same roots as the breakage the said commit\n> fixed, and the solution should use the same idea, but not fixed.\n\nPerhaps something along this line...\n\n-- >8 --\nSubject: \"log --grep\": commit's buffer may already have been discarded\n\nFollowing up on be5c9fb9049e (logmsg_reencode: lazily load missing\ncommit buffers, 2013-01-26), extract the part that reads the commit\nbuffer data into a separate helper function, and use it when we\napply the grep filter on the commit during the log walk.\n\nSigned-off-by: Junio C Hamano \n---\n\n The reproduction recipe in original bug report looked like this:\n\n git commit -m 'text1' --allow-empty\n git commit -m 'text2' --allow-empty\n git log --graph --no-walk --grep 'text2'\n\n which does not make any sense to me. We should simply forbid\n combination of --graph (which inherently wants a connected history)\n and --no-walk (which is a way to tell \"This is not about history,\n this is about a single point\").\n\n But that is a separate issue.\n\n commit.c | 19 +++++++++++++++++++\n commit.h | 1 +\n pretty.c | 14 ++------------\n revision.c | 14 +++++++++++---\n 4 files changed, 33 insertions(+), 15 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex e8eb0ae..c0acf0f 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -335,6 +335,25 @@ int parse_commit(struct commit *item)\n \treturn ret;\n }\n \n+char *read_commit_object_data(const struct commit *commit, unsigned long *sizep)\n+{\n+\tchar *msg;\n+\tenum object_type type;\n+\tunsigned long size;\n+\n+\tif (!sizep)\n+\t\tsizep = &size;\n+\n+\tmsg = read_sha1_file(commit->object.sha1, &type, sizep);\n+\tif (!msg)\n+\t\tdie(\"Cannot read commit object %s\",\n+\t\t sha1_to_hex(commit->object.sha1));\n+\tif (type != OBJ_COMMIT)\n+\t\tdie(\"Expected commit for '%s', got %s\",\n+\t\t sha1_to_hex(commit->object.sha1), typename(type));\n+\treturn msg;\n+}\n+\n int find_commit_subject(const char *commit_buffer, const char **subject)\n {\n \tconst char *eol;\ndiff --git a/commit.h b/commit.h\nindex 4138bb4..e314149 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -102,6 +102,7 @@ struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern char *logmsg_reencode(const struct commit *commit,\n \t\t\t const char *output_encoding);\n extern void logmsg_free(char *msg, const struct commit *commit);\n+extern char *read_commit_object_data(const struct commit *commit, unsigned long *size);\n extern void get_commit_format(const char *arg, struct rev_info *);\n extern const char *format_subject(struct strbuf *sb, const char *msg,\n \t\t\t\t const char *line_separator);\ndiff --git a/pretty.c b/pretty.c\nindex eae57ad..004d16d 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -592,18 +592,8 @@ char *logmsg_reencode(const struct commit *commit,\n \tchar *msg = commit->buffer;\n \tchar *out;\n \n-\tif (!msg) {\n-\t\tenum object_type type;\n-\t\tunsigned long size;\n-\n-\t\tmsg = read_sha1_file(commit->object.sha1, &type, &size);\n-\t\tif (!msg)\n-\t\t\tdie(\"Cannot read commit object %s\",\n-\t\t\t sha1_to_hex(commit->object.sha1));\n-\t\tif (type != OBJ_COMMIT)\n-\t\t\tdie(\"Expected commit for '%s', got %s\",\n-\t\t\t sha1_to_hex(commit->object.sha1), typename(type));\n-\t}\n+\tif (!msg)\n+\t\tmsg = read_commit_object_data(commit, NULL);\n \n \tif (!output_encoding || !*output_encoding)\n \t\treturn msg;\ndiff --git a/revision.c b/revision.c\nindex d7562ee..caf8ef3 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2279,9 +2279,16 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t\tstrbuf_addch(&buf, '\\n');\n \t}\n \n-\t/* Copy the commit to temporary if we are using \"fake\" headers */\n-\tif (buf.len)\n+\tif (!commit->buffer) {\n+\t\t/* we may not have commit->buffer */\n+\t\tunsigned long size;\n+\t\tchar *msg = read_commit_object_data(commit, &size);\n+\t\tstrbuf_add(&buf, msg, size);\n+\t\tfree(msg);\n+\t} else if (buf.len) {\n+\t\t/* Copy the commit to temporary if we are using \"fake\" headers */\n \t\tstrbuf_addstr(&buf, commit->buffer);\n+\t}\n \n \tif (opt->grep_filter.header_list && opt->mailmap) {\n \t\tif (!buf.len)\n@@ -2302,9 +2309,10 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t/* Find either in the commit object, or in the temporary */\n \tif (buf.len)\n \t\tretval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);\n-\telse\n+\telse {\n \t\tretval = grep_buffer(&opt->grep_filter,\n \t\t\t\t commit->buffer, strlen(commit->buffer));\n+\t}\n \tstrbuf_release(&buf);\n \treturn retval;\n }\n"},{"id":"209053","messageId":"7vfw16xqvj.fsf@alter.siamese.dyndns.org","threadId":"32863","inReplyTo":"20130209002710.GA5570@sigill.intra.peff.net","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-09T00:39:12Z","receivedAt":"2013-02-09T00:39:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Feb 08, 2013 at 04:22:15PM -0800, Junio C Hamano wrote:\n>\n>> Junio C Hamano writes:\n>> \n>> > Thomas Haller writes:\n>> >\n>> >> it happens in file revision.c:2306 because \"commit->buffer\" is zero:\n>> >>\n>> >> retval = grep_buffer(&opt->grep_filter,\n>> >> commit->buffer, strlen(commit->buffer));\n>> >\n>> > I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily\n>> > load missing commit buffers, 2013-01-26); I haven't merged it to any\n>> > of the maintenance releases, but the tip of 'master' should have it\n>> > already.\n>> \n>> Ah, no, this shares the same roots as the breakage the said commit\n>> fixed, and the solution should use the same idea, but not fixed.\n>\n> Yeah, I think this needs separate treatment. However, this is a perfect\n> example of the \"case-by-case\" I mention in the final two paragraphs\n> there.\n>\n> What's the right encoding to be grepping in? The original, what we will\n> output in, or even something else? IOW, I wonder if this should be using\n> logmsg_reencode in the first place (the obvious reason not to want to do\n> so is speed, but logmsg_reencode is written to only have an impact in\n> the case that we do indeed need to reencode).\n\nYeah, that actually is a good point. We should be using logmsg_reencode\nso that we look for strings in the user's encoding.\n"},{"id":"209054","messageId":"20130209003921.GC5570@sigill.intra.peff.net","threadId":"32863","inReplyTo":"7vk3qixrc8.fsf@alter.siamese.dyndns.org","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-09T00:39:22Z","receivedAt":"2013-02-09T00:39:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 08, 2013 at 04:29:11PM -0800, Junio C Hamano wrote:\n\n> Perhaps something along this line...\n> \n> -- >8 --\n> Subject: \"log --grep\": commit's buffer may already have been discarded\n> \n> Following up on be5c9fb9049e (logmsg_reencode: lazily load missing\n> commit buffers, 2013-01-26), extract the part that reads the commit\n> buffer data into a separate helper function, and use it when we\n> apply the grep filter on the commit during the log walk.\n\nThis obviously makes sense if we don't want to get the route of\nre-encoding for grep. Re-encoding would be a user-visible change, but I\nwonder if it is the right thing to be doing.\n\n> diff --git a/revision.c b/revision.c\n> index d7562ee..caf8ef3 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2279,9 +2279,16 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n> \t\tstrbuf_addch(&buf, '\\n');\n> \t}\n> \n> -\t/* Copy the commit to temporary if we are using \"fake\" headers */\n> -\tif (buf.len)\n> +\tif (!commit->buffer) {\n> +\t\t/* we may not have commit->buffer */\n> +\t\tunsigned long size;\n> +\t\tchar *msg = read_commit_object_data(commit, &size);\n> +\t\tstrbuf_add(&buf, msg, size);\n> +\t\tfree(msg);\n> +\t} else if (buf.len) {\n> +\t\t/* Copy the commit to temporary if we are using \"fake\" headers */\n> \t\tstrbuf_addstr(&buf, commit->buffer);\n> +\t}\n\nHmm. It would be nice to avoid the extra copy when we do not otherwise\nneed to use the strbuf. I would have expected something more like:\n\n const char *msg = commit->buffer;\n if (!msg)\n msg = read_commit_object_data(commit, NULL);\n\n [...]\n\n if (buf.len)\n retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);\n else\n retval = grep_buffer(&opt->grep_filter, msg, strlen(msg));\n\n strbuf_release(&buf);\n if (msg != commit->buffer)\n free(msg);\n return retval;\n\nYou would also need to adjust the other uses of commit->buffer\nthroughout the function to refer to msg.\n\n-Peff\n"},{"id":"209056","messageId":"7va9rexqii.fsf@alter.siamese.dyndns.org","threadId":"32863","inReplyTo":"7vfw16xqvj.fsf@alter.siamese.dyndns.org","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-09T00:47:01Z","receivedAt":"2013-02-09T00:47:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> On Fri, Feb 08, 2013 at 04:22:15PM -0800, Junio C Hamano wrote:\n>>\n>>> Junio C Hamano writes:\n>>> \n>>> > Thomas Haller writes:\n>>> >\n>>> >> it happens in file revision.c:2306 because \"commit->buffer\" is zero:\n>>> >>\n>>> >> retval = grep_buffer(&opt->grep_filter,\n>>> >> commit->buffer, strlen(commit->buffer));\n>>> >\n>>> > I think this has been fixed at be5c9fb9049e (logmsg_reencode: lazily\n>>> > load missing commit buffers, 2013-01-26); I haven't merged it to any\n>>> > of the maintenance releases, but the tip of 'master' should have it\n>>> > already.\n>>> \n>>> Ah, no, this shares the same roots as the breakage the said commit\n>>> fixed, and the solution should use the same idea, but not fixed.\n>>\n>> Yeah, I think this needs separate treatment. However, this is a perfect\n>> example of the \"case-by-case\" I mention in the final two paragraphs\n>> there.\n>>\n>> What's the right encoding to be grepping in? The original, what we will\n>> output in, or even something else? IOW, I wonder if this should be using\n>> logmsg_reencode in the first place (the obvious reason not to want to do\n>> so is speed, but logmsg_reencode is written to only have an impact in\n>> the case that we do indeed need to reencode).\n>\n> Yeah, that actually is a good point. We should be using logmsg_reencode\n> so that we look for strings in the user's encoding.\n\nPerhaps like this. Just like the previous one (which should be\ndiscarded), this makes the function always use the temporary strbuf,\nso doing this upfront actually loses more code than it adds ;-)\n\n revision.c | 29 ++++++++++++-----------------\n 1 file changed, 12 insertions(+), 17 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex d7562ee..07bf728 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2269,6 +2269,9 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n {\n \tint retval;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tchar *message;\n+\tconst char *encoding;\n+\n \tif (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)\n \t\treturn 1;\n \n@@ -2279,32 +2282,24 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t\tstrbuf_addch(&buf, '\\n');\n \t}\n \n-\t/* Copy the commit to temporary if we are using \"fake\" headers */\n-\tif (buf.len)\n-\t\tstrbuf_addstr(&buf, commit->buffer);\n+\tencoding = get_log_output_encoding();\n+\tmessage = logmsg_reencode(commit, encoding);\n+\tstrbuf_addstr(&buf, message);\n+\tif (message != commit->buffer)\n+\t\tfree(message);\n \n \tif (opt->grep_filter.header_list && opt->mailmap) {\n-\t\tif (!buf.len)\n-\t\t\tstrbuf_addstr(&buf, commit->buffer);\n-\n \t\tcommit_rewrite_person(&buf, \"\\nauthor \", opt->mailmap);\n \t\tcommit_rewrite_person(&buf, \"\\ncommitter \", opt->mailmap);\n \t}\n \n \t/* Append \"fake\" message parts as needed */\n-\tif (opt->show_notes) {\n-\t\tif (!buf.len)\n-\t\t\tstrbuf_addstr(&buf, commit->buffer);\n+\tif (opt->show_notes)\n \t\tformat_display_notes(commit->object.sha1, &buf,\n-\t\t\t\t get_log_output_encoding(), 1);\n-\t}\n+\t\t\t\t encoding, 1);\n+\n+\tretval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);\n \n-\t/* Find either in the commit object, or in the temporary */\n-\tif (buf.len)\n-\t\tretval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);\n-\telse\n-\t\tretval = grep_buffer(&opt->grep_filter,\n-\t\t\t\t commit->buffer, strlen(commit->buffer));\n \tstrbuf_release(&buf);\n \treturn retval;\n }\n"},{"id":"209058","messageId":"20130209010524.GB5469@sigill.intra.peff.net","threadId":"32863","inReplyTo":"7va9rexqii.fsf@alter.siamese.dyndns.org","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-09T01:05:24Z","receivedAt":"2013-02-09T01:05:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:\n\n> > Yeah, that actually is a good point. We should be using logmsg_reencode\n> > so that we look for strings in the user's encoding.\n> \n> Perhaps like this. Just like the previous one (which should be\n> discarded), this makes the function always use the temporary strbuf,\n> so doing this upfront actually loses more code than it adds ;-)\n\nI like code simplification, but I worry a little about paying for the\nextra copy in the common case. I did a best-of-five \"git rev-list\n--count --grep=foo HEAD\" before and after your patch, though, and the\ndifference was well within the noise. So maybe it's not worth caring\nabout.\n\n-Peff\n"},{"id":"209060","messageId":"20130209010837.GA10616@sigill.intra.peff.net","threadId":"32863","inReplyTo":"20130209010524.GB5469@sigill.intra.peff.net","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-09T01:08:37Z","receivedAt":"2013-02-09T01:08:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 08, 2013 at 08:05:24PM -0500, Jeff King wrote:\n\n> On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:\n> \n> > > Yeah, that actually is a good point. We should be using logmsg_reencode\n> > > so that we look for strings in the user's encoding.\n> > \n> > Perhaps like this. Just like the previous one (which should be\n> > discarded), this makes the function always use the temporary strbuf,\n> > so doing this upfront actually loses more code than it adds ;-)\n> \n> I like code simplification, but I worry a little about paying for the\n> extra copy in the common case. I did a best-of-five \"git rev-list\n> --count --grep=foo HEAD\" before and after your patch, though, and the\n> difference was well within the noise. So maybe it's not worth caring\n> about.\n\nOh, hold on, I'm incompetent. I measured the wrong build of git. Here\nare the timings for git.git:\n\n [before]\n $ best-of-five git rev-list --count --grep=foo HEAD\n Attempt 1: 0.503\n Attempt 2: 0.5\n Attempt 3: 0.501\n Attempt 4: 0.502\n Attempt 5: 0.5\n\n real 0m0.500s\n user 0m0.488s\n sys 0m0.008s\n\n [after]\n $ best-of-five git rev-list --count --grep=foo HEAD\n Attempt 1: 0.514\n Attempt 2: 0.525\n Attempt 3: 0.517\n Attempt 4: 0.523\n Attempt 5: 0.518\n\n real 0m0.514s\n user 0m0.480s\n sys 0m0.028s\n\nSo not huge, but measurable.\n\n-Peff\n"},{"id":"209293","messageId":"20130211191607.GA21269@sigill.intra.peff.net","threadId":"32863","inReplyTo":"7va9rexqii.fsf@alter.siamese.dyndns.org","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-11T19:16:07Z","receivedAt":"2013-02-11T19:16:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:\n\n> > Yeah, that actually is a good point. We should be using logmsg_reencode\n> > so that we look for strings in the user's encoding.\n> \n> Perhaps like this. Just like the previous one (which should be\n> discarded), this makes the function always use the temporary strbuf,\n> so doing this upfront actually loses more code than it adds ;-)\n\nI didn't see this in What's Cooking or pu. We should probably pick an\napproach and go with it.\n\nI think using logmsg_reencode makes sense. I'd be in favor of avoiding\nthe extra copy in the common case, so something like the patch below. If\nyou feel strongly about the code cleanup at the minor run-time expense,\nI won't argue too much, though.\n\n---\ndiff --git a/revision.c b/revision.c\nindex d7562ee..a08d0a5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2268,7 +2268,10 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n static int commit_match(struct commit *commit, struct rev_info *opt)\n {\n \tint retval;\n+\tconst char *encoding;\n+\tconst char *message;\n \tstruct strbuf buf = STRBUF_INIT;\n+\n \tif (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)\n \t\treturn 1;\n \n@@ -2279,13 +2282,23 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t\tstrbuf_addch(&buf, '\\n');\n \t}\n \n+\t/*\n+\t * We grep in the user's output encoding, under the assumption that it\n+\t * is the encoding they are most likely to write their grep pattern\n+\t * for. In addition, it means we will match the \"notes\" encoding below,\n+\t * so we will not end up with a buffer that has two different encodings\n+\t * in it.\n+\t */\n+\tencoding = get_log_output_encoding();\n+\tmessage = logmsg_reencode(commit, encoding);\n+\n \t/* Copy the commit to temporary if we are using \"fake\" headers */\n \tif (buf.len)\n-\t\tstrbuf_addstr(&buf, commit->buffer);\n+\t\tstrbuf_addstr(&buf, message);\n \n \tif (opt->grep_filter.header_list && opt->mailmap) {\n \t\tif (!buf.len)\n-\t\t\tstrbuf_addstr(&buf, commit->buffer);\n+\t\t\tstrbuf_addstr(&buf, message);\n \n \t\tcommit_rewrite_person(&buf, \"\\nauthor \", opt->mailmap);\n \t\tcommit_rewrite_person(&buf, \"\\ncommitter \", opt->mailmap);\n@@ -2294,18 +2307,18 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t/* Append \"fake\" message parts as needed */\n \tif (opt->show_notes) {\n \t\tif (!buf.len)\n-\t\t\tstrbuf_addstr(&buf, commit->buffer);\n-\t\tformat_display_notes(commit->object.sha1, &buf,\n-\t\t\t\t get_log_output_encoding(), 1);\n+\t\t\tstrbuf_addstr(&buf, message);\n+\t\tformat_display_notes(commit->object.sha1, &buf, encoding, 1);\n \t}\n \n-\t/* Find either in the commit object, or in the temporary */\n+\t/* Find either in the original commit message, or in the temporary */\n \tif (buf.len)\n \t\tretval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);\n \telse\n \t\tretval = grep_buffer(&opt->grep_filter,\n-\t\t\t\t commit->buffer, strlen(commit->buffer));\n+\t\t\t\t message, strlen(message));\n \tstrbuf_release(&buf);\n+\tlogmsg_free(message, commit);\n \treturn retval;\n }\n \n"},{"id":"209296","messageId":"7v621ymxfv.fsf@alter.siamese.dyndns.org","threadId":"32863","inReplyTo":"20130211191607.GA21269@sigill.intra.peff.net","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-11T20:01:56Z","receivedAt":"2013-02-11T20:01:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:\n>\n>> > Yeah, that actually is a good point. We should be using logmsg_reencode\n>> > so that we look for strings in the user's encoding.\n>> \n>> Perhaps like this. Just like the previous one (which should be\n>> discarded), this makes the function always use the temporary strbuf,\n>> so doing this upfront actually loses more code than it adds ;-)\n>\n> I didn't see this in What's Cooking or pu. We should probably pick an\n> approach and go with it.\n>\n> I think using logmsg_reencode makes sense. I'd be in favor of avoiding\n> the extra copy in the common case, so something like the patch below. If\n> you feel strongly about the code cleanup at the minor run-time expense,\n> I won't argue too much, though.\n\nSounds good to me. Care to do the log message while at it?\n\n> ---\n> diff --git a/revision.c b/revision.c\n> index d7562ee..a08d0a5 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2268,7 +2268,10 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n> static int commit_match(struct commit *commit, struct rev_info *opt)\n> {\n> \tint retval;\n> +\tconst char *encoding;\n> +\tconst char *message;\n> \tstruct strbuf buf = STRBUF_INIT;\n> +\n> \tif (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)\n> \t\treturn 1;\n> \n> @@ -2279,13 +2282,23 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n> \t\tstrbuf_addch(&buf, '\\n');\n> \t}\n> \n> +\t/*\n> +\t * We grep in the user's output encoding, under the assumption that it\n> +\t * is the encoding they are most likely to write their grep pattern\n> +\t * for. In addition, it means we will match the \"notes\" encoding below,\n> +\t * so we will not end up with a buffer that has two different encodings\n> +\t * in it.\n> +\t */\n> +\tencoding = get_log_output_encoding();\n> +\tmessage = logmsg_reencode(commit, encoding);\n> +\n> \t/* Copy the commit to temporary if we are using \"fake\" headers */\n> \tif (buf.len)\n> -\t\tstrbuf_addstr(&buf, commit->buffer);\n> +\t\tstrbuf_addstr(&buf, message);\n> \n> \tif (opt->grep_filter.header_list && opt->mailmap) {\n> \t\tif (!buf.len)\n> -\t\t\tstrbuf_addstr(&buf, commit->buffer);\n> +\t\t\tstrbuf_addstr(&buf, message);\n> \n> \t\tcommit_rewrite_person(&buf, \"\\nauthor \", opt->mailmap);\n> \t\tcommit_rewrite_person(&buf, \"\\ncommitter \", opt->mailmap);\n> @@ -2294,18 +2307,18 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n> \t/* Append \"fake\" message parts as needed */\n> \tif (opt->show_notes) {\n> \t\tif (!buf.len)\n> -\t\t\tstrbuf_addstr(&buf, commit->buffer);\n> -\t\tformat_display_notes(commit->object.sha1, &buf,\n> -\t\t\t\t get_log_output_encoding(), 1);\n> +\t\t\tstrbuf_addstr(&buf, message);\n> +\t\tformat_display_notes(commit->object.sha1, &buf, encoding, 1);\n> \t}\n> \n> -\t/* Find either in the commit object, or in the temporary */\n> +\t/* Find either in the original commit message, or in the temporary */\n> \tif (buf.len)\n> \t\tretval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);\n> \telse\n> \t\tretval = grep_buffer(&opt->grep_filter,\n> -\t\t\t\t commit->buffer, strlen(commit->buffer));\n> +\t\t\t\t message, strlen(message));\n> \tstrbuf_release(&buf);\n> +\tlogmsg_free(message, commit);\n> \treturn retval;\n> }\n> \n"},{"id":"209298","messageId":"7vvc9ylh97.fsf@alter.siamese.dyndns.org","threadId":"32863","inReplyTo":"7v621ymxfv.fsf@alter.siamese.dyndns.org","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-11T20:36:52Z","receivedAt":"2013-02-11T20:36:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:\n>>\n>>> > Yeah, that actually is a good point. We should be using logmsg_reencode\n>>> > so that we look for strings in the user's encoding.\n>>> \n>>> Perhaps like this. Just like the previous one (which should be\n>>> discarded), this makes the function always use the temporary strbuf,\n>>> so doing this upfront actually loses more code than it adds ;-)\n>>\n>> I didn't see this in What's Cooking or pu. We should probably pick an\n>> approach and go with it.\n>>\n>> I think using logmsg_reencode makes sense. I'd be in favor of avoiding\n>> the extra copy in the common case, so something like the patch below. If\n>> you feel strongly about the code cleanup at the minor run-time expense,\n>> I won't argue too much, though.\n>\n> Sounds good to me. Care to do the log message while at it?\n\nHeh, how about this? I still need a sign-off from you.\n\nThanks.\n\ncommit 6a76814cd08cac15c1faff5bd97c9e94ac8b6a01\nAuthor: Jeff King \nDate: Mon Feb 11 14:16:07 2013 -0500\n\n log --grep: look for the given string in log output encoding\n \n We used to grep in the raw commit buffer contents, possibly pieces\n of notes encoded in log output encoding appended to it, which was\n insane.\n \n Convert the contents of the commit message also to log output\n encoding before looking for the string. This incidentally fixes a\n possible NULL dereference that can happen when commit->buffer has\n already been freed, which can happen with\n \n \tgit commit -m 'text1' --allow-empty\n \tgit commit -m 'text2' --allow-empty\n \tgit log --graph --no-walk --grep 'text2'\n \n which arguably does not make any sense (--graph inherently wants a\n connected history, and by --no-walk the command line is telling us\n to show discrete points in history without connectivity), and we\n probably should forbid the combination, but that is a separate\n issue.\n"},{"id":"209299","messageId":"20130211204142.GA28248@sigill.intra.peff.net","threadId":"32863","inReplyTo":"7vvc9ylh97.fsf@alter.siamese.dyndns.org","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-11T20:41:42Z","receivedAt":"2013-02-11T20:41:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 11, 2013 at 12:36:52PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Jeff King writes:\n> >\n> >> On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:\n> >>\n> >>> > Yeah, that actually is a good point. We should be using logmsg_reencode\n> >>> > so that we look for strings in the user's encoding.\n> >>> \n> >>> Perhaps like this. Just like the previous one (which should be\n> >>> discarded), this makes the function always use the temporary strbuf,\n> >>> so doing this upfront actually loses more code than it adds ;-)\n> >>\n> >> I didn't see this in What's Cooking or pu. We should probably pick an\n> >> approach and go with it.\n> >>\n> >> I think using logmsg_reencode makes sense. I'd be in favor of avoiding\n> >> the extra copy in the common case, so something like the patch below. If\n> >> you feel strongly about the code cleanup at the minor run-time expense,\n> >> I won't argue too much, though.\n> >\n> > Sounds good to me. Care to do the log message while at it?\n> \n> Heh, how about this? I still need a sign-off from you.\n\nI'm working on the log message and tests right now. There's also a minor\ncode fixup needed to compile with -Wall.\n\n> log --grep: look for the given string in log output encoding\n> \n> We used to grep in the raw commit buffer contents, possibly pieces\n> of notes encoded in log output encoding appended to it, which was\n> insane.\n> \n> Convert the contents of the commit message also to log output\n> encoding before looking for the string. This incidentally fixes a\n> possible NULL dereference that can happen when commit->buffer has\n> already been freed, which can happen with\n> \n> \tgit commit -m 'text1' --allow-empty\n> \tgit commit -m 'text2' --allow-empty\n> \tgit log --graph --no-walk --grep 'text2'\n> \n> which arguably does not make any sense (--graph inherently wants a\n> connected history, and by --no-walk the command line is telling us\n> to show discrete points in history without connectivity), and we\n> probably should forbid the combination, but that is a separate\n> issue.\n\nI'll use bits of that. I had sort of punted on the \"how to reproduce the\nsegfault\" issue entirely because you had noted that it was not a sane\nthing to do. Still, I think it makes sense to mention it with the caveat\nyou give here.\n\n-Peff\n"},{"id":"209300","messageId":"7vr4kmlgdk.fsf@alter.siamese.dyndns.org","threadId":"32863","inReplyTo":"20130211204142.GA28248@sigill.intra.peff.net","subject":"Re: segfault for git log --graph --no-walk --grep a","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-11T20:55:51Z","receivedAt":"2013-02-11T20:55:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Feb 11, 2013 at 12:36:52PM -0800, Junio C Hamano wrote:\n>\n>> Junio C Hamano writes:\n>> \n>> > Jeff King writes:\n>> >\n>> >> On Fri, Feb 08, 2013 at 04:47:01PM -0800, Junio C Hamano wrote:\n>> >>\n>> >>> > Yeah, that actually is a good point. We should be using logmsg_reencode\n>> >>> > so that we look for strings in the user's encoding.\n>> >>> \n>> >>> Perhaps like this. Just like the previous one (which should be\n>> >>> discarded), this makes the function always use the temporary strbuf,\n>> >>> so doing this upfront actually loses more code than it adds ;-)\n>> >>\n>> >> I didn't see this in What's Cooking or pu. We should probably pick an\n>> >> approach and go with it.\n>> >>\n>> >> I think using logmsg_reencode makes sense. I'd be in favor of avoiding\n>> >> the extra copy in the common case, so something like the patch below. If\n>> >> you feel strongly about the code cleanup at the minor run-time expense,\n>> >> I won't argue too much, though.\n>> >\n>> > Sounds good to me. Care to do the log message while at it?\n>> \n>> Heh, how about this? I still need a sign-off from you.\n>\n> I'm working on the log message and tests right now. There's also a minor\n> code fixup needed to compile with -Wall.\n\nThanks; I noticed the constness issue around the message variable as well.\n\n>\n>> log --grep: look for the given string in log output encoding\n>> \n>> We used to grep in the raw commit buffer contents, possibly pieces\n>> of notes encoded in log output encoding appended to it, which was\n>> insane.\n>> \n>> Convert the contents of the commit message also to log output\n>> encoding before looking for the string. This incidentally fixes a\n>> possible NULL dereference that can happen when commit->buffer has\n>> already been freed, which can happen with\n>> \n>> \tgit commit -m 'text1' --allow-empty\n>> \tgit commit -m 'text2' --allow-empty\n>> \tgit log --graph --no-walk --grep 'text2'\n>> \n>> which arguably does not make any sense (--graph inherently wants a\n>> connected history, and by --no-walk the command line is telling us\n>> to show discrete points in history without connectivity), and we\n>> probably should forbid the combination, but that is a separate\n>> issue.\n>\n> I'll use bits of that. I had sort of punted on the \"how to reproduce the\n> segfault\" issue entirely because you had noted that it was not a sane\n> thing to do. Still, I think it makes sense to mention it with the caveat\n> you give here.\n>\n> -Peff\n"},{"id":"209301","messageId":"20130211205958.GA32740@sigill.intra.peff.net","threadId":"32863","inReplyTo":"7v621ymxfv.fsf@alter.siamese.dyndns.org","subject":"[PATCH] log: re-encode commit messages before grepping","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-11T20:59:58Z","receivedAt":"2013-02-11T20:59:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you run \"git log --grep=foo\", we will run your regex on\nthe literal bytes of the commit message. This can provide\nconfusing results if the commit message is not in the same\nencoding as your grep expression (or worse, you have commits\nin multiple encodings, in which case your regex would need\nto be written to match either encoding). On top of this, we\nmight also be grepping in the commit's notes, which are\nalready re-encoded, potentially leading to grepping in a\nbuffer with mixed encodings concatenated. This is insanity,\nbut most people never noticed, because their terminal and\ntheir commit encodings all match.\n\nInstead, let's massage the to-be-grepped commit into a\nstandardized encoding. There is not much point in adding a\nflag for \"this is the encoding I expect my grep pattern to\nmatch\"; the only sane choice is for it to use the log output\nencoding. That is presumably what the user's terminal is\nusing, and it means that the patterns found by the grep will\nmatch the output produced by git.\n\nAs a bonus, this fixes a potential segfault in commit_match\nwhen commit->buffer is NULL, as we now build on logmsg_reencode,\nwhich handles reading the commit buffer from disk if\nnecessary. The segfault can be triggered with:\n\n git commit -m 'text1' --allow-empty\n git commit -m 'text2' --allow-empty\n git log --graph --no-walk --grep 'text2'\n\nwhich arguably does not make any sense (--graph inherently\nwants a connected history, and by --no-walk the command line\nis telling us to show discrete points in history without\nconnectivity), and we probably should forbid the\ncombination, but that is a separate issue.\n\nSigned-off-by: Jeff King \n---\nA few notes:\n\n 1. I suppose we could also use $LANG or one of the $LC_* variables to\n guess at the encoding of the user's pattern. But I think using the\n output encoding makes the most sense, since then the pattern you\n searched for will actually be in the output.\n\n 2. There are still problems with utf8 normalization. E.g., my tests\n represent utf-8 é with \\xc3\\xa9 (the code point for that glyph),\n but it could also be represented by \\x65\\xcc\\x81 (e + combining\n acute). But that is not a new problem; it is an inherent issue with\n grepping utf8. We might in the future want to offer an option to\n normalize utf8 (or possibly the regex library can be taught to\n handle this).\n\n 3. If the commit does need to be re-encoded, we end up doing so here,\n and then potentially again if we actually show the commit. So\n there may be some room to optimize that by stashing the re-encoded\n version somewhere (or it may not make a big difference, I haven't\n measured).\n\n 4. I'm still not clear on why \"--graph --no-walk\" wants to look at\n commit_match after we have already cleared the commit buffer. I\n agree it's nonsensical, but I wonder if it might be a symptom of an\n underlying bug or inefficiency.\n\n revision.c | 27 ++++++++++++++++++-------\n t/t4210-log-i18n.sh | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 78 insertions(+), 7 deletions(-)\n create mode 100755 t/t4210-log-i18n.sh\n\ndiff --git a/revision.c b/revision.c\nindex d7562ee..ef60205 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2268,7 +2268,10 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n static int commit_match(struct commit *commit, struct rev_info *opt)\n {\n \tint retval;\n+\tconst char *encoding;\n+\tchar *message;\n \tstruct strbuf buf = STRBUF_INIT;\n+\n \tif (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)\n \t\treturn 1;\n \n@@ -2279,13 +2282,23 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t\tstrbuf_addch(&buf, '\\n');\n \t}\n \n+\t/*\n+\t * We grep in the user's output encoding, under the assumption that it\n+\t * is the encoding they are most likely to write their grep pattern\n+\t * for. In addition, it means we will match the \"notes\" encoding below,\n+\t * so we will not end up with a buffer that has two different encodings\n+\t * in it.\n+\t */\n+\tencoding = get_log_output_encoding();\n+\tmessage = logmsg_reencode(commit, encoding);\n+\n \t/* Copy the commit to temporary if we are using \"fake\" headers */\n \tif (buf.len)\n-\t\tstrbuf_addstr(&buf, commit->buffer);\n+\t\tstrbuf_addstr(&buf, message);\n \n \tif (opt->grep_filter.header_list && opt->mailmap) {\n \t\tif (!buf.len)\n-\t\t\tstrbuf_addstr(&buf, commit->buffer);\n+\t\t\tstrbuf_addstr(&buf, message);\n \n \t\tcommit_rewrite_person(&buf, \"\\nauthor \", opt->mailmap);\n \t\tcommit_rewrite_person(&buf, \"\\ncommitter \", opt->mailmap);\n@@ -2294,18 +2307,18 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t/* Append \"fake\" message parts as needed */\n \tif (opt->show_notes) {\n \t\tif (!buf.len)\n-\t\t\tstrbuf_addstr(&buf, commit->buffer);\n-\t\tformat_display_notes(commit->object.sha1, &buf,\n-\t\t\t\t get_log_output_encoding(), 1);\n+\t\t\tstrbuf_addstr(&buf, message);\n+\t\tformat_display_notes(commit->object.sha1, &buf, encoding, 1);\n \t}\n \n-\t/* Find either in the commit object, or in the temporary */\n+\t/* Find either in the original commit message, or in the temporary */\n \tif (buf.len)\n \t\tretval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);\n \telse\n \t\tretval = grep_buffer(&opt->grep_filter,\n-\t\t\t\t commit->buffer, strlen(commit->buffer));\n+\t\t\t\t message, strlen(message));\n \tstrbuf_release(&buf);\n+\tlogmsg_free(message, commit);\n \treturn retval;\n }\n \ndiff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh\nnew file mode 100755\nindex 0000000..52a7472\n--- /dev/null\n+++ b/t/t4210-log-i18n.sh\n@@ -0,0 +1,58 @@\n+#!/bin/sh\n+\n+test_description='test log with i18n features'\n+. ./test-lib.sh\n+\n+# two forms of é\n+utf8_e=$(printf '\\303\\251')\n+latin1_e=$(printf '\\351')\n+\n+test_expect_success 'create commits in different encodings' '\n+\ttest_tick &&\n+\tcat >msg <<-EOF &&\n+\tutf8\n+\n+\tt${utf8_e}st\n+\tEOF\n+\tgit add msg &&\n+\tgit -c i18n.commitencoding=utf8 commit -F msg &&\n+\tcat >msg <<-EOF &&\n+\tlatin1\n+\n+\tt${latin1_e}st\n+\tEOF\n+\tgit add msg &&\n+\tgit -c i18n.commitencoding=ISO-8859-1 commit -F msg\n+'\n+\n+test_expect_success 'log --grep searches in log output encoding (utf8)' '\n+\tcat >expect <<-\\EOF &&\n+\tlatin1\n+\tutf8\n+\tEOF\n+\tgit log --encoding=utf8 --format=%s --grep=$utf8_e >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log --grep searches in log output encoding (latin1)' '\n+\tcat >expect <<-\\EOF &&\n+\tlatin1\n+\tutf8\n+\tEOF\n+\tgit log --encoding=ISO-8859-1 --format=%s --grep=$latin1_e >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log --grep does not find non-reencoded values (utf8)' '\n+\t>expect &&\n+\tgit log --encoding=utf8 --format=%s --grep=$latin1_e >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log --grep does not find non-reencoded values (latin1)' '\n+\t>expect &&\n+\tgit log --encoding=ISO-8859-1 --format=%s --grep=$utf8_e >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.8.1.2.11.g1a2f572\n"},{"id":"209305","messageId":"7vmwvalfnn.fsf@alter.siamese.dyndns.org","threadId":"32863","inReplyTo":"20130211205958.GA32740@sigill.intra.peff.net","subject":"Re: [PATCH] log: re-encode commit messages before grepping","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-11T21:11:24Z","receivedAt":"2013-02-11T21:11:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> 1. I suppose we could also use $LANG or one of the $LC_* variables to\n> guess at the encoding of the user's pattern. But I think using the\n> output encoding makes the most sense, since then the pattern you\n> searched for will actually be in the output.\n\nI agree. In addition, if we were to do anything with LANG/LC_CTYPE,\nit should be done at the layer that implements log-output-encoding\n(e.g. lack of configured encoding with nonstandard LANG/LC_CTYPE\nwould use the locale, or something), I think.\n\n> 2. There are still problems with utf8 normalization. E.g., my tests\n> represent utf-8 é with \\xc3\\xa9 (the code point for that glyph),\n> but it could also be represented by \\x65\\xcc\\x81 (e + combining\n> acute). But that is not a new problem; it is an inherent issue with\n> grepping utf8. We might in the future want to offer an option to\n> normalize utf8 (or possibly the regex library can be taught to\n> handle this).\n\nTrue; in either case, this caller (or any other callers) should\ncare. Only grep_buffer() (actually, grep_source_1()) needs to be\ntaught about it.\n\n> 4. I'm still not clear on why \"--graph --no-walk\" wants to look at\n> commit_match after we have already cleared the commit buffer. I\n> agree it's nonsensical, but I wonder if it might be a symptom of an\n> underlying bug or inefficiency.\n\nYeah, that may be something we may want to check, I agree.\n\nThe aded test is also nice. Thanks.\n\n> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh\n> new file mode 100755\n> index 0000000..52a7472\n> --- /dev/null\n> +++ b/t/t4210-log-i18n.sh\n> @@ -0,0 +1,58 @@\n> +#!/bin/sh\n> +\n> +test_description='test log with i18n features'\n> +. ./test-lib.sh\n> +\n> +# two forms of é\n> +utf8_e=$(printf '\\303\\251')\n> +latin1_e=$(printf '\\351')\n> +\n> +test_expect_success 'create commits in different encodings' '\n> +\ttest_tick &&\n> +\tcat >msg <<-EOF &&\n> +\tutf8\n> +\n> +\tt${utf8_e}st\n> +\tEOF\n> +\tgit add msg &&\n> +\tgit -c i18n.commitencoding=utf8 commit -F msg &&\n> +\tcat >msg <<-EOF &&\n> +\tlatin1\n> +\n> +\tt${latin1_e}st\n> +\tEOF\n> +\tgit add msg &&\n> +\tgit -c i18n.commitencoding=ISO-8859-1 commit -F msg\n> +'\n> +\n> +test_expect_success 'log --grep searches in log output encoding (utf8)' '\n> +\tcat >expect <<-\\EOF &&\n> +\tlatin1\n> +\tutf8\n> +\tEOF\n> +\tgit log --encoding=utf8 --format=%s --grep=$utf8_e >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'log --grep searches in log output encoding (latin1)' '\n> +\tcat >expect <<-\\EOF &&\n> +\tlatin1\n> +\tutf8\n> +\tEOF\n> +\tgit log --encoding=ISO-8859-1 --format=%s --grep=$latin1_e >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'log --grep does not find non-reencoded values (utf8)' '\n> +\t>expect &&\n> +\tgit log --encoding=utf8 --format=%s --grep=$latin1_e >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'log --grep does not find non-reencoded values (latin1)' '\n> +\t>expect &&\n> +\tgit log --encoding=ISO-8859-1 --format=%s --grep=$utf8_e >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_done\n"},{"id":"209306","messageId":"20130211211429.GE32740@sigill.intra.peff.net","threadId":"32863","inReplyTo":"7vmwvalfnn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] log: re-encode commit messages before grepping","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-11T21:14:29Z","receivedAt":"2013-02-11T21:14:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 11, 2013 at 01:11:24PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > 1. I suppose we could also use $LANG or one of the $LC_* variables to\n> > guess at the encoding of the user's pattern. But I think using the\n> > output encoding makes the most sense, since then the pattern you\n> > searched for will actually be in the output.\n> \n> I agree. In addition, if we were to do anything with LANG/LC_CTYPE,\n> it should be done at the layer that implements log-output-encoding\n> (e.g. lack of configured encoding with nonstandard LANG/LC_CTYPE\n> would use the locale, or something), I think.\n\nYeah, I had the same thought. I'll leave it to somebody who knows/cares\nmore about i18n. In my world view, utf8 is good enough for everyone. :)\n\n-Peff\n"},{"id":"210239","messageId":"512B22DE.9070603@viscovery.net","threadId":"32863","inReplyTo":"20130211205958.GA32740@sigill.intra.peff.net","subject":"[PATCH ] t4210-log-i18n: spell encoding name \"UTF-8\" correctly","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-02-25T08:37:50Z","receivedAt":"2013-02-25T08:37:50Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt \n\niconv on Windows does not know the encoding name \"utf8\", and does not\nre-encode log messages when this name is given. Request \"UTF-8\" encoding.\n\nSigned-off-by: Johannes Sixt \n---\n I'm not sure whether I'm right to say that \"UTF-8\" is the correct\n spelling. Anyway, 'iconv -l' on my old Linux box lists \"UTF8\", but on\n Windows it does not.\n\n A more correct fix would probably be to use is_encoding_utf8() in more\n places, but it's outside my time budget look after it.\n\n -- Hannes\n\n t/t4210-log-i18n.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh\nindex 52a7472..b1956e2 100755\n--- a/t/t4210-log-i18n.sh\n+++ b/t/t4210-log-i18n.sh\n@@ -15,7 +15,7 @@ test_expect_success 'create commits in different encodings' '\n \tt${utf8_e}st\n \tEOF\n \tgit add msg &&\n-\tgit -c i18n.commitencoding=utf8 commit -F msg &&\n+\tgit -c i18n.commitencoding=UTF-8 commit -F msg &&\n \tcat >msg <<-EOF &&\n \tlatin1\n \n@@ -30,7 +30,7 @@ test_expect_success 'log --grep searches in log output encoding (utf8)' '\n \tlatin1\n \tutf8\n \tEOF\n-\tgit log --encoding=utf8 --format=%s --grep=$utf8_e >actual &&\n+\tgit log --encoding=UTF-8 --format=%s --grep=$utf8_e >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -45,7 +45,7 @@ test_expect_success 'log --grep searches in log output encoding (latin1)' '\n \n test_expect_success 'log --grep does not find non-reencoded values (utf8)' '\n \t>expect &&\n-\tgit log --encoding=utf8 --format=%s --grep=$latin1_e >actual &&\n+\tgit log --encoding=UTF-8 --format=%s --grep=$latin1_e >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n\"Atomic objects are neither active nor radioactive.\" --\nProgramming Languages -- C++, Final Committee Draft (Doc.N3092)\n"},{"id":"210248","messageId":"20130225151916.GA7725@sigill.intra.peff.net","threadId":"32863","inReplyTo":"512B22DE.9070603@viscovery.net","subject":"Re: [PATCH ] t4210-log-i18n: spell encoding name \"UTF-8\" correctly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-25T15:19:17Z","receivedAt":"2013-02-25T15:19:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 25, 2013 at 09:37:50AM +0100, Johannes Sixt wrote:\n\n> From: Johannes Sixt \n> \n> iconv on Windows does not know the encoding name \"utf8\", and does not\n> re-encode log messages when this name is given. Request \"UTF-8\" encoding.\n> \n> Signed-off-by: Johannes Sixt \n> ---\n> I'm not sure whether I'm right to say that \"UTF-8\" is the correct\n> spelling. Anyway, 'iconv -l' on my old Linux box lists \"UTF8\", but on\n> Windows it does not.\n\nUTF-8 is correct according to:\n\n https://en.wikipedia.org/wiki/Utf8#Official_name_and_variants\n\n> A more correct fix would probably be to use is_encoding_utf8() in more\n> places, but it's outside my time budget look after it.\n\nYeah, I wonder if this is a symptom of a deeper issue, which is that\nutf-8 has many synonyms, and we would prefer to canonicalize the\nencoding name before generating an object to avoid inconsistencies (of\ncourse we cannot do so for every imaginable encoding, but utf-8 is a\npretty obvious one we handle already). We _should_ be generating commits\nwith no encoding header at all for utf-8, though.\n\nAnd indeed, it looks like that is the case. commit_tree_extended has:\n\n /* Not having i18n.commitencoding is the same as having utf-8 */\n encoding_is_utf8 = is_encoding_utf8(git_commit_encoding);\n\n [...]\n\n if (!encoding_is_utf8)\n strbuf_addf(&buffer, \"encoding %s\\n\", git_commit_encoding);\n\n\nwhich makes me think that this first hunk...\n\n> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh\n> index 52a7472..b1956e2 100755\n> --- a/t/t4210-log-i18n.sh\n> +++ b/t/t4210-log-i18n.sh\n> @@ -15,7 +15,7 @@ test_expect_success 'create commits in different encodings' '\n> \tt${utf8_e}st\n> \tEOF\n> \tgit add msg &&\n> -\tgit -c i18n.commitencoding=utf8 commit -F msg &&\n> +\tgit -c i18n.commitencoding=UTF-8 commit -F msg &&\n> \tcat >msg <<-EOF &&\n> \tlatin1\n\n...should be a no-op; the utf8 there should never be seen by anybody but\ngit. Can you confirm that is the case?\n\n> @@ -30,7 +30,7 @@ test_expect_success 'log --grep searches in log output encoding (utf8)' '\n> \tlatin1\n> \tutf8\n> \tEOF\n> -\tgit log --encoding=utf8 --format=%s --grep=$utf8_e >actual &&\n> +\tgit log --encoding=UTF-8 --format=%s --grep=$utf8_e >actual &&\n> \ttest_cmp expect actual\n> '\n\nThis one will feed it to iconv, though, because the latin1 commit will\nneed to be re-encoded. I think the simplest thing would just be:\n\ndiff --git a/utf8.c b/utf8.c\nindex 1087870..8d42b50 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -507,6 +507,17 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e\n \n \tif (!in_encoding)\n \t\treturn NULL;\n+\n+\t/*\n+\t * Some platforms do not have the variously spelled variants of\n+\t * UTF-8, so let us feed iconv the most official spelling, which\n+\t * should hopefully be accepted everywhere.\n+\t */\n+\tif (is_encoding_utf8(in_encoding))\n+\t\tin_encoding = \"UTF-8\";\n+\tif (is_encoding_utf8(out_encoding))\n+\t\tout_encoding = \"UTF-8\";\n+\n \tconv = iconv_open(out_encoding, in_encoding);\n \tif (conv == (iconv_t) -1)\n \t\treturn NULL;\n\nDoes that fix the tests for you? It's a larger change, but I think it\nmakes git friendlier all around for people on Windows.\n\n-Peff\n"},{"id":"210271","messageId":"512BB377.2010403@web.de","threadId":"32863","inReplyTo":"512B22DE.9070603@viscovery.net","subject":"Re: [PATCH ] t4210-log-i18n: spell encoding name \"UTF-8\" correctly","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-02-25T18:54:47Z","receivedAt":"2013-02-25T18:54:47Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 25.02.13 09:37, Johannes Sixt wrote:\n> From: Johannes Sixt \n> \n> iconv on Windows does not know the encoding name \"utf8\", and does not\n> re-encode log messages when this name is given. Request \"UTF-8\" encoding.\n> \n> Signed-off-by: Johannes Sixt \n> ---\n> I'm not sure whether I'm right to say that \"UTF-8\" is the correct\n> spelling. Anyway, 'iconv -l' on my old Linux box lists \"UTF8\", but on\n> Windows it does not.\n> \n> A more correct fix would probably be to use is_encoding_utf8() in more\n> places, but it's outside my time budget look after it.\n> \n> -- Hannes\n> \n> t/t4210-log-i18n.sh | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh\n> index 52a7472..b1956e2 100755\n> --- a/t/t4210-log-i18n.sh\n> +++ b/t/t4210-log-i18n.sh\n> @@ -15,7 +15,7 @@ test_expect_success 'create commits in different encodings' '\n> \tt${utf8_e}st\n> \tEOF\n> \tgit add msg &&\n> -\tgit -c i18n.commitencoding=utf8 commit -F msg &&\n> +\tgit -c i18n.commitencoding=UTF-8 commit -F msg &&\n> \tcat >msg <<-EOF &&\n> \tlatin1\n> \n> @@ -30,7 +30,7 @@ test_expect_success 'log --grep searches in log output encoding (utf8)' '\n> \tlatin1\n> \tutf8\n> \tEOF\n> -\tgit log --encoding=utf8 --format=%s --grep=$utf8_e >actual &&\n> +\tgit log --encoding=UTF-8 --format=%s --grep=$utf8_e >actual &&\n> \ttest_cmp expect actual\n> '\n> \n> @@ -45,7 +45,7 @@ test_expect_success 'log --grep searches in log output encoding (latin1)' '\n> \n> test_expect_success 'log --grep does not find non-reencoded values (utf8)' '\n> \t>expect &&\n> -\tgit log --encoding=utf8 --format=%s --grep=$latin1_e >actual &&\n> +\tgit log --encoding=UTF-8 --format=%s --grep=$latin1_e >actual &&\n> \ttest_cmp expect actual\n> '\n> \n> \nHej,\n(beside that I couldn't find t4210 somewhere),\nis it something like the following you are tinking of?\n\n(Not sure if my cut-and-paste stuff applies, its's rather for review)\n\n-- >8 --\n[PATCH] iconv_open(): Use UTF-8 if UTF8 failes\n\nWhen iconv_open() failes with EINVAL, it may be that \"UTF-8\"\nis spelled wrong by mistake.\nFor example, \"UTF8\" is used instead of \"UTF-8\".\nSome iconv implementations tolerate \"UTF8\" or \"utf8\".\nIf not, iconv_open() fails.\nIf is_encoding_utf8() is true change the string to the\noffical string \"UTF-8\" with uppercase letters.\n\nReported-By: Johannes Sixt \nSigned-off-by: Torsten Bögershausen \n---\n utf8.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/utf8.c b/utf8.c\nindex a4ee665..e9850d0 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -487,6 +487,10 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e\n \tif (!in_encoding)\n \t\treturn NULL;\n \tconv = iconv_open(out_encoding, in_encoding);\n+\tif (conv == (iconv_t) -1 && errno == EINVAL) {\n+\t\tconv = iconv_open(is_encoding_utf8(out_encoding) ? \"UTF-8\" : out_encoding,\n+\t\t\t\t\t\t\t\t\t\t\tis_encoding_utf8(in_encoding) ? \"UTF-8\" : in_encoding);\n+\t}\n \tif (conv == (iconv_t) -1)\n \t\treturn NULL;\n \tout = reencode_string_iconv(in, strlen(in), conv);\n-- \n1.8.1.1\n"},{"id":"210274","messageId":"7vmwusxlfm.fsf@alter.siamese.dyndns.org","threadId":"32863","inReplyTo":"20130225151916.GA7725@sigill.intra.peff.net","subject":"Re: [PATCH ] t4210-log-i18n: spell encoding name \"UTF-8\" correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-25T19:06:37Z","receivedAt":"2013-02-25T19:06:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... I think the simplest thing would just be:\n>\n> diff --git a/utf8.c b/utf8.c\n> index 1087870..8d42b50 100644\n> --- a/utf8.c\n> +++ b/utf8.c\n> @@ -507,6 +507,17 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e\n> \n> \tif (!in_encoding)\n> \t\treturn NULL;\n> +\n> +\t/*\n> +\t * Some platforms do not have the variously spelled variants of\n> +\t * UTF-8, so let us feed iconv the most official spelling, which\n> +\t * should hopefully be accepted everywhere.\n> +\t */\n> +\tif (is_encoding_utf8(in_encoding))\n> +\t\tin_encoding = \"UTF-8\";\n> +\tif (is_encoding_utf8(out_encoding))\n> +\t\tout_encoding = \"UTF-8\";\n> +\n> \tconv = iconv_open(out_encoding, in_encoding);\n> \tif (conv == (iconv_t) -1)\n> \t\treturn NULL;\n>\n> Does that fix the tests for you? It's a larger change, but I think it\n> makes git friendlier all around for people on Windows.\n\nYeah, if this is confirmed to work OK (from eyeballing I do not see\na reason why not...) I agree this is the cleanest way forward.\n"},{"id":"210290","messageId":"20130225203100.GA15932@sigill.intra.peff.net","threadId":"32863","inReplyTo":"7vmwusxlfm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH ] t4210-log-i18n: spell encoding name \"UTF-8\" correctly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-25T20:31:00Z","receivedAt":"2013-02-25T20:31:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 25, 2013 at 11:06:37AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... I think the simplest thing would just be:\n> >\n> > diff --git a/utf8.c b/utf8.c\n> > index 1087870..8d42b50 100644\n> > --- a/utf8.c\n> > +++ b/utf8.c\n> > @@ -507,6 +507,17 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e\n> > \n> > \tif (!in_encoding)\n> > \t\treturn NULL;\n> > +\n> > +\t/*\n> > +\t * Some platforms do not have the variously spelled variants of\n> > +\t * UTF-8, so let us feed iconv the most official spelling, which\n> > +\t * should hopefully be accepted everywhere.\n> > +\t */\n> > +\tif (is_encoding_utf8(in_encoding))\n> > +\t\tin_encoding = \"UTF-8\";\n> > +\tif (is_encoding_utf8(out_encoding))\n> > +\t\tout_encoding = \"UTF-8\";\n> > +\n> > \tconv = iconv_open(out_encoding, in_encoding);\n> > \tif (conv == (iconv_t) -1)\n> > \t\treturn NULL;\n> >\n> > Does that fix the tests for you? It's a larger change, but I think it\n> > makes git friendlier all around for people on Windows.\n> \n> Yeah, if this is confirmed to work OK (from eyeballing I do not see\n> a reason why not...) I agree this is the cleanest way forward.\n\nThe only reason I can think of is that you specify \"utf8\", your platform\nunderstands \"utf8\" but not \"UTF-8\", and we rewrite it silently to\n\"UTF-8\". That seems somewhat unlikely, but to be on the safe side, why\ndon't we do it as a fallback? That should be fine performance-wise, as\nit only triggers on the error case.\n\nLike this:\n\n-- >8 --\nSubject: [PATCH] utf8: accept alternate spellings of UTF-8\n\nThe iconv implementation on many platforms will accept\nvariants of UTF-8, including \"UTF8\", \"utf-8\", and \"utf8\",\nbut some do not. We make allowances in our code to treat\nthem all identically, but we sometimes hand the string from\nthe user directly to iconv. In this case, the platform iconv\nmay or may not work.\n\nThere are really four levels of platform iconv support for\nthese synonyms:\n\n 1. All synonyms understood (e.g., glibc).\n\n 2. Only the official \"UTF-8\" understood (e.g., Windows).\n\n 3. Official \"UTF-8\" not understood, but some other synonym\n understood (it's not known whether such a platform exists).\n\n 4. Neither \"UTF-8\" nor any synonym understood (e.g.,\n ancient systems, or ones without utf8 support\n installed).\n\nThis patch teaches git to fall back to using the official\n\"UTF-8\" spelling when iconv_open fails (and the encoding was\none of the synonym spellings). This makes things more\nconvenient to users of type 2 systems, as they can now use\nany of the synonyms for the log output encoding.\n\nType 1 systems are not affected, as iconv already works on\nthe first try.\n\nType 4 systems are not affected, as both attempts already\nfail.\n\nType 3 systems will not benefit from the feature, but\nbecause we only use \"UTF-8\" as a fallback, they will not be\nregressed (i.e., you can continue to use \"utf8\" if your\nplatform supports it). We could try all the various\nsynonyms, but since such systems are not even known to\nexist, it's not worth the effort.\n\nSigned-off-by: Jeff King \n---\nJSixt, can you double-check that this passes t4210 for you?\n\n utf8.c | 20 ++++++++++++++++++--\n 1 file changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/utf8.c b/utf8.c\nindex 1087870..8f6e84b 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -507,9 +507,25 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e\n \n \tif (!in_encoding)\n \t\treturn NULL;\n+\n \tconv = iconv_open(out_encoding, in_encoding);\n-\tif (conv == (iconv_t) -1)\n-\t\treturn NULL;\n+\tif (conv == (iconv_t) -1) {\n+\t\t/*\n+\t\t * Some platforms do not have the variously spelled variants of\n+\t\t * UTF-8, so let's fall back to trying the most official\n+\t\t * spelling. We do so only as a fallback in case the platform\n+\t\t * does understand the user's spelling, but not our official\n+\t\t * one.\n+\t\t */\n+\t\tif (is_encoding_utf8(in_encoding))\n+\t\t\tin_encoding = \"UTF-8\";\n+\t\tif (is_encoding_utf8(out_encoding))\n+\t\t\tout_encoding = \"UTF-8\";\n+\t\tconv = iconv_open(out_encoding, in_encoding);\n+\t\tif (conv == (iconv_t) -1)\n+\t\t\treturn NULL;\n+\t}\n+\n \tout = reencode_string_iconv(in, strlen(in), conv);\n \ticonv_close(conv);\n \treturn out;\n-- \n1.8.1.4.4.g265d2fa\n"},{"id":"210293","messageId":"20130225203652.GB15932@sigill.intra.peff.net","threadId":"32863","inReplyTo":"512BB377.2010403@web.de","subject":"Re: [PATCH ] t4210-log-i18n: spell encoding name \"UTF-8\" correctly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-25T20:36:52Z","receivedAt":"2013-02-25T20:36:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 25, 2013 at 07:54:47PM +0100, Torsten Bögershausen wrote:\n\n> (beside that I couldn't find t4210 somewhere),\n\nIt's newly added in 04deccd.\n\n> diff --git a/utf8.c b/utf8.c\n> index a4ee665..e9850d0 100644\n> --- a/utf8.c\n> +++ b/utf8.c\n> @@ -487,6 +487,10 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e\n> \tif (!in_encoding)\n> \t\treturn NULL;\n> \tconv = iconv_open(out_encoding, in_encoding);\n> +\tif (conv == (iconv_t) -1 && errno == EINVAL) {\n> +\t\tconv = iconv_open(is_encoding_utf8(out_encoding) ? \"UTF-8\" : out_encoding,\n> +\t\t\t\t\t\t\t\t\t\t\tis_encoding_utf8(in_encoding) ? \"UTF-8\" : in_encoding);\n> +\t}\n\nYeah, I think this is pretty close to the patch I just sent. You do the\nfallback, which I think is sane to prevent any possible regression (for\n\"type 3\" systems, as I called them in my commit). I don't know if it is\nworth checking errno; we should get the same failure either way, and I'd\nworry slightly about some odd iconv implementation does not use EINVAL.\n\nAnd of course, I like my indentation and commit message better. :)\n\n-Peff\n"},{"id":"210294","messageId":"512BD0FE.5040108@web.de","threadId":"32863","inReplyTo":"20130225151916.GA7725@sigill.intra.peff.net","subject":"Re: [PATCH ] t4210-log-i18n: spell encoding name \"UTF-8\" correctly","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-02-25T21:00:46Z","receivedAt":"2013-02-25T21:00:46Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 25.02.13 16:19, Jeff King wrote:\n> On Mon, Feb 25, 2013 at 09:37:50AM +0100, Johannes Sixt wrote:\n> \n>> From: Johannes Sixt \n>>\n>> iconv on Windows does not know the encoding name \"utf8\", and does not\n>> re-encode log messages when this name is given. Request \"UTF-8\" encoding.\n>>\n>> Signed-off-by: Johannes Sixt \n>> ---\n>> I'm not sure whether I'm right to say that \"UTF-8\" is the correct\n>> spelling. Anyway, 'iconv -l' on my old Linux box lists \"UTF8\", but on\n>> Windows it does not.\n> \n> UTF-8 is correct according to:\n> \n> https://en.wikipedia.org/wiki/Utf8#Official_name_and_variants\n> \n>> A more correct fix would probably be to use is_encoding_utf8() in more\n>> places, but it's outside my time budget look after it.\n> \n> Yeah, I wonder if this is a symptom of a deeper issue, which is that\n> utf-8 has many synonyms, and we would prefer to canonicalize the\n> encoding name before generating an object to avoid inconsistencies (of\n> course we cannot do so for every imaginable encoding, but utf-8 is a\n> pretty obvious one we handle already). We _should_ be generating commits\n> with no encoding header at all for utf-8, though.\n> \n> And indeed, it looks like that is the case. commit_tree_extended has:\n> \n> /* Not having i18n.commitencoding is the same as having utf-8 */\n> encoding_is_utf8 = is_encoding_utf8(git_commit_encoding);\n> \n> [...]\n> \n> if (!encoding_is_utf8)\n> strbuf_addf(&buffer, \"encoding %s\\n\", git_commit_encoding);\n> \n> \n> which makes me think that this first hunk...\n> \n>> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh\n>> index 52a7472..b1956e2 100755\n>> --- a/t/t4210-log-i18n.sh\n>> +++ b/t/t4210-log-i18n.sh\n>> @@ -15,7 +15,7 @@ test_expect_success 'create commits in different encodings' '\n>> \tt${utf8_e}st\n>> \tEOF\n>> \tgit add msg &&\n>> -\tgit -c i18n.commitencoding=utf8 commit -F msg &&\n>> +\tgit -c i18n.commitencoding=UTF-8 commit -F msg &&\n>> \tcat >msg <<-EOF &&\n>> \tlatin1\n> \n> ...should be a no-op; the utf8 there should never be seen by anybody but\n> git. Can you confirm that is the case?\n> \n>> @@ -30,7 +30,7 @@ test_expect_success 'log --grep searches in log output encoding (utf8)' '\n>> \tlatin1\n>> \tutf8\n>> \tEOF\n>> -\tgit log --encoding=utf8 --format=%s --grep=$utf8_e >actual &&\n>> +\tgit log --encoding=UTF-8 --format=%s --grep=$utf8_e >actual &&\n>> \ttest_cmp expect actual\n>> '\n> \n> This one will feed it to iconv, though, because the latin1 commit will\n> need to be re-encoded. I think the simplest thing would just be:\n> \n> diff --git a/utf8.c b/utf8.c\n> index 1087870..8d42b50 100644\n> --- a/utf8.c\n> +++ b/utf8.c\n> @@ -507,6 +507,17 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e\n> \n> \tif (!in_encoding)\n> \t\treturn NULL;\n> +\n> +\t/*\n> +\t * Some platforms do not have the variously spelled variants of\n> +\t * UTF-8, so let us feed iconv the most official spelling, which\n> +\t * should hopefully be accepted everywhere.\n> +\t */\n> +\tif (is_encoding_utf8(in_encoding))\n> +\t\tin_encoding = \"UTF-8\";\n> +\tif (is_encoding_utf8(out_encoding))\n> +\t\tout_encoding = \"UTF-8\";\n> +\n> \tconv = iconv_open(out_encoding, in_encoding);\n> \tif (conv == (iconv_t) -1)\n> \t\treturn NULL;\n> \n> Does that fix the tests for you? It's a larger change, but I think it\n> makes git friendlier all around for people on Windows.\n> \n> -Peff\n> --\n \nThanks, I'm OK with your version.\n\nAnd a test on cygwin was OK for the new t4210.\n"},{"id":"210316","messageId":"512C5A8B.9070302@viscovery.net","threadId":"32863","inReplyTo":"20130225203100.GA15932@sigill.intra.peff.net","subject":"Re: [PATCH ] t4210-log-i18n: spell encoding name \"UTF-8\" correctly","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-02-26T06:47:39Z","receivedAt":"2013-02-26T06:47:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 2/25/2013 21:31, schrieb Jeff King:\n> Subject: [PATCH] utf8: accept alternate spellings of UTF-8\n> ...\n> JSixt, can you double-check that this passes t4210 for you?\n\nYes, t4210 and all other *i18n* and *log* tests pass with this patch.\n\nThanks,\n-- Hannes\n\n> \n> utf8.c | 20 ++++++++++++++++++--\n> 1 file changed, 18 insertions(+), 2 deletions(-)\n> \n> diff --git a/utf8.c b/utf8.c\n> index 1087870..8f6e84b 100644\n> --- a/utf8.c\n> +++ b/utf8.c\n> @@ -507,9 +507,25 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e\n> \n> \tif (!in_encoding)\n> \t\treturn NULL;\n> +\n> \tconv = iconv_open(out_encoding, in_encoding);\n> -\tif (conv == (iconv_t) -1)\n> -\t\treturn NULL;\n> +\tif (conv == (iconv_t) -1) {\n> +\t\t/*\n> +\t\t * Some platforms do not have the variously spelled variants of\n> +\t\t * UTF-8, so let's fall back to trying the most official\n> +\t\t * spelling. We do so only as a fallback in case the platform\n> +\t\t * does understand the user's spelling, but not our official\n> +\t\t * one.\n> +\t\t */\n> +\t\tif (is_encoding_utf8(in_encoding))\n> +\t\t\tin_encoding = \"UTF-8\";\n> +\t\tif (is_encoding_utf8(out_encoding))\n> +\t\t\tout_encoding = \"UTF-8\";\n> +\t\tconv = iconv_open(out_encoding, in_encoding);\n> +\t\tif (conv == (iconv_t) -1)\n> +\t\t\treturn NULL;\n> +\t}\n> +\n> \tout = reencode_string_iconv(in, strlen(in), conv);\n> \ticonv_close(conv);\n> \treturn out;\n"}]}