{"thread":{"id":"33520","subject":"git log - crash and core dump","startedAt":"2013-04-16T16:55:59Z","lastAt":"2013-04-19T03:03:30Z","messageCount":28,"participants":["Ivan Lyapunov","Antoine Pelisse","René Scharfe","Junio C Hamano","Jeff King","John Keeping","Konstantin Khomoutov","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"214497","messageId":"CANKwXW1EXLiWgdVM4+k_11wu1Nyixp05PUXmQYP_gUXQKek_OA@mail.gmail.com","threadId":"33520","inReplyTo":null,"subject":"git log - crash and core dump","fromName":"Ivan Lyapunov","fromEmail":"dront78@gmail.com","sentAt":"2013-04-16T16:55:59Z","receivedAt":"2013-04-16T16:55:59Z","isPatch":false,"sender":{"key":"dront78@gmail.com","avatar":"https://gravatar.com/avatar/ef7ba136ecd3bbd34601fe7af87d7c052935e5fbdee6ff634709a8c68c4b595e?d=mp&s=160"},"body":"git version 1.8.2.1 crashes on my ArchLinux x86_64 on git log command\ngdb bt is attached\n\ngit log | less\ndoes not crash in same repo\n\nI cannot share a repo for a debug purposes since it's private repo of\nmy employer\nbut I can perform any suitable tests on repo to help this bug to be fixed\n\n#0 0x00007ffff722b3e6 in ____strtoull_l_internal () from /usr/lib/libc.so.6\n#1 0x00000000004b31d4 in pp_user_info (pp=pp@entry=0x7fffffffd310,\nwhat=what@entry=0x521379 \"Author\", sb=sb@entry=0x7fffffffd290,\n line=line@entry=0x7b3a45 \"Ivan Lyapunov -\n<> 1354083115 +0400\\ncommitter Ivan Lyapunov \n1354083115 +0400\\n\\n- small merge fixes\",\nencoding=encoding@entry=0x505400 \"UTF-8\") at pretty.c:441\n#2 0x00000000004b533a in pp_header (sb=0x7fffffffd290,\nmsg_p=0x7fffffffd228, commit=0x7c1e10, encoding=0x505400 \"UTF-8\",\npp=0x7fffffffd310) at pretty.c:1415\n#3 pretty_print_commit (pp=pp@entry=0x7fffffffd310,\ncommit=commit@entry=0x7c1e10, sb=sb@entry=0x7fffffffd290) at\npretty.c:1545\n#4 0x00000000004a0b45 in show_log (opt=opt@entry=0x7fffffffd4d0) at\nlog-tree.c:683\n#5 0x00000000004a1616 in log_tree_commit\n(opt=opt@entry=0x7fffffffd4d0, commit=commit@entry=0x7c1e10) at\nlog-tree.c:859\n#6 0x0000000000438b03 in cmd_log_walk (rev=rev@entry=0x7fffffffd4d0)\nat builtin/log.c:310\n#7 0x00000000004395dd in cmd_log (argc=1, argv=0x7fffffffdd30,\nprefix=0x0) at builtin/log.c:582\n#8 0x000000000040562d in run_builtin (argv=0x7fffffffdd30, argc=1,\np=0x754d18 ) at git.c:282\n#9 handle_internal_command (argc=1, argv=0x7fffffffdd30) at git.c:444\n#10 0x0000000000404a6f in run_argv (argv=0x7fffffffdbd0,\nargcp=0x7fffffffdbdc) at git.c:490\n#11 main (argc=1, argv=0x7fffffffdd30) at git.c:565\n"},{"id":"214500","messageId":"E09BD6FE-D531-4CB1-B452-F3FF000CD8C3@gmail.com","threadId":"33520","inReplyTo":"CANKwXW1EXLiWgdVM4+k_11wu1Nyixp05PUXmQYP_gUXQKek_OA@mail.gmail.com","subject":"Re: git log - crash and core dump","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-04-16T17:29:34Z","receivedAt":"2013-04-16T17:29:34Z","isPatch":false,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"Hey\n\n> #0 0x00007ffff722b3e6 in ____strtoull_l_internal () from /usr/lib/libc.so.6\n> #1 0x00000000004b31d4 in pp_user_info (pp=pp@entry=0x7fffffffd310,\n> what=what@entry=0x521379 \"Author\", sb=sb@entry=0x7fffffffd290,\n> line=line@entry=0x7b3a45 \"Ivan Lyapunov -\n> <> 1354083115 +0400\\ncommitter Ivan Lyapunov \n> 1354083115 +0400\\n\\n- small merge fixes\",\n> encoding=encoding@entry=0x505400 \"UTF-8\") at pretty.c:441\n\nClearly the author line is messed up after name and email. It means we won't be able to parse the time, and return a null pointer to it (which we run through strtoll after, with a crash). \nI thought that bug was already fixed though and we're now checking for null time also ? I think I can submit a fix for that when I'm back home. \n\nThanks for reporting !\n\nCheers, Antoine \n"},{"id":"214509","messageId":"516D93C4.1000100@lsrfire.ath.cx","threadId":"33520","inReplyTo":"CANKwXW1EXLiWgdVM4+k_11wu1Nyixp05PUXmQYP_gUXQKek_OA@mail.gmail.com","subject":"Re: git log - crash and core dump","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-04-16T18:09:08Z","receivedAt":"2013-04-16T18:09:08Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.04.2013 18:55, schrieb Ivan Lyapunov:\n> git version 1.8.2.1 crashes on my ArchLinux x86_64 on git log command\n> gdb bt is attached\n> \n> git log | less\n> does not crash in same repo\n> \n> I cannot share a repo for a debug purposes since it's private repo of\n> my employer\n> but I can perform any suitable tests on repo to help this bug to be fixed\n> \n> #0 0x00007ffff722b3e6 in ____strtoull_l_internal () from /usr/lib/libc.so.6\n> #1 0x00000000004b31d4 in pp_user_info (pp=pp@entry=0x7fffffffd310,\n> what=what@entry=0x521379 \"Author\", sb=sb@entry=0x7fffffffd290,\n> line=line@entry=0x7b3a45 \"Ivan Lyapunov -\n> <> 1354083115 +0400\\ncommitter Ivan Lyapunov \n\nSo this is the author information, correct?\n\n\tIvan Lyapunov -<> 1354083115 +0400\n |author name| |---author email----| ^^^ |--time--| |tz-|\n\nHow did you manage to add the \"-<>\" after the email address?\n\nWhat does git log in version 1.8.1 or earlier show for this commit?\n\n> 1354083115 +0400\\n\\n- small merge fixes\",\n> encoding=encoding@entry=0x505400 \"UTF-8\") at pretty.c:441\n> #2 0x00000000004b533a in pp_header (sb=0x7fffffffd290,\n> msg_p=0x7fffffffd228, commit=0x7c1e10, encoding=0x505400 \"UTF-8\",\n> pp=0x7fffffffd310) at pretty.c:1415\n> #3 pretty_print_commit (pp=pp@entry=0x7fffffffd310,\n> commit=commit@entry=0x7c1e10, sb=sb@entry=0x7fffffffd290) at\n> pretty.c:1545\n> #4 0x00000000004a0b45 in show_log (opt=opt@entry=0x7fffffffd4d0) at\n> log-tree.c:683\n> #5 0x00000000004a1616 in log_tree_commit\n> (opt=opt@entry=0x7fffffffd4d0, commit=commit@entry=0x7c1e10) at\n> log-tree.c:859\n> #6 0x0000000000438b03 in cmd_log_walk (rev=rev@entry=0x7fffffffd4d0)\n> at builtin/log.c:310\n> #7 0x00000000004395dd in cmd_log (argc=1, argv=0x7fffffffdd30,\n> prefix=0x0) at builtin/log.c:582\n> #8 0x000000000040562d in run_builtin (argv=0x7fffffffdd30, argc=1,\n> p=0x754d18 ) at git.c:282\n> #9 handle_internal_command (argc=1, argv=0x7fffffffdd30) at git.c:444\n> #10 0x0000000000404a6f in run_argv (argv=0x7fffffffdbd0,\n> argcp=0x7fffffffdbdc) at git.c:490\n> #11 main (argc=1, argv=0x7fffffffdd30) at git.c:565\n\nDoes this patch help?\n\n pretty.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex d3a82d2..713eefc 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,\n \tconst char *mailbuf, *namebuf;\n \tsize_t namelen, maillen;\n \tint max_length = 78; /* per rfc2822 */\n-\tunsigned long time;\n-\tint tz;\n+\tunsigned long time = 0;\n+\tint tz = 0;\n \n \tif (pp->fmt == CMIT_FMT_ONELINE)\n \t\treturn;\n@@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,\n \tstrbuf_add(&name, namebuf, namelen);\n \n \tnamelen = name.len + mail.len + 3; /* ' ' + '<' + '>' */\n-\ttime = strtoul(ident.date_begin, &date, 10);\n-\ttz = strtol(date, NULL, 10);\n+\tif (ident.date_begin) {\n+\t\ttime = strtoul(ident.date_begin, &date, 10);\n+\t\ttz = strtol(date, NULL, 10);\n+\t}\n \n \tif (pp->fmt == CMIT_FMT_EMAIL) {\n \t\tstrbuf_addstr(sb, \"From: \");\n"},{"id":"214525","messageId":"7v61zml0ow.fsf@alter.siamese.dyndns.org","threadId":"33520","inReplyTo":"516D93C4.1000100@lsrfire.ath.cx","subject":"Re: git log - crash and core dump","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-16T19:45:03Z","receivedAt":"2013-04-16T19:45:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Does this patch help?\n>\n> pretty.c | 10 ++++++----\n> 1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/pretty.c b/pretty.c\n> index d3a82d2..713eefc 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,\n> \tconst char *mailbuf, *namebuf;\n> \tsize_t namelen, maillen;\n> \tint max_length = 78; /* per rfc2822 */\n> -\tunsigned long time;\n> -\tint tz;\n> +\tunsigned long time = 0;\n> +\tint tz = 0;\n> \n> \tif (pp->fmt == CMIT_FMT_ONELINE)\n> \t\treturn;\n> @@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,\n> \tstrbuf_add(&name, namebuf, namelen);\n> \n> \tnamelen = name.len + mail.len + 3; /* ' ' + '<' + '>' */\n> -\ttime = strtoul(ident.date_begin, &date, 10);\n> -\ttz = strtol(date, NULL, 10);\n> +\tif (ident.date_begin) {\n> +\t\ttime = strtoul(ident.date_begin, &date, 10);\n> +\t\ttz = strtol(date, NULL, 10);\n> +\t}\n> \n> \tif (pp->fmt == CMIT_FMT_EMAIL) {\n> \t\tstrbuf_addstr(sb, \"From: \");\n\nLooks like a sensible change. split_ident_line() decided that the\ngiven input was mangled and decided there is no valid date (the\ninput had <> where the timestamp string was required), so the\nupdated code leaves the time/tz unspecified.\n\nIt still is curious how a malformed line was created in the first\nplace. I wouldn't worry if a private tool used hash-object to\ncreate such a commit, but if it is something that is commonly used\n(e.g. \"git commit\"), others may suffer from the same and the tool\nneeds to be tightened a bit.\n"},{"id":"214541","messageId":"516DBE2E.4060201@lsrfire.ath.cx","threadId":"33520","inReplyTo":"7v61zml0ow.fsf@alter.siamese.dyndns.org","subject":"Re: git log - crash and core dump","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-04-16T21:10:06Z","receivedAt":"2013-04-16T21:10:06Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"First, lest I forget again: Thank you, Ivan, for the very useful\nbug report!\n\nAm 16.04.2013 21:45, schrieb Junio C Hamano:\n> René Scharfe writes:\n> \n>> Does this patch help?\n>>\n>> pretty.c | 10 ++++++----\n>> 1 file changed, 6 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/pretty.c b/pretty.c\n>> index d3a82d2..713eefc 100644\n>> --- a/pretty.c\n>> +++ b/pretty.c\n>> @@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,\n>> \tconst char *mailbuf, *namebuf;\n>> \tsize_t namelen, maillen;\n>> \tint max_length = 78; /* per rfc2822 */\n>> -\tunsigned long time;\n>> -\tint tz;\n>> +\tunsigned long time = 0;\n>> +\tint tz = 0;\n>> \n>> \tif (pp->fmt == CMIT_FMT_ONELINE)\n>> \t\treturn;\n>> @@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,\n>> \tstrbuf_add(&name, namebuf, namelen);\n>> \n>> \tnamelen = name.len + mail.len + 3; /* ' ' + '<' + '>' */\n>> -\ttime = strtoul(ident.date_begin, &date, 10);\n>> -\ttz = strtol(date, NULL, 10);\n>> +\tif (ident.date_begin) {\n>> +\t\ttime = strtoul(ident.date_begin, &date, 10);\n>> +\t\ttz = strtol(date, NULL, 10);\n>> +\t}\n>> \n>> \tif (pp->fmt == CMIT_FMT_EMAIL) {\n>> \t\tstrbuf_addstr(sb, \"From: \");\n> \n> Looks like a sensible change. split_ident_line() decided that the\n> given input was mangled and decided there is no valid date (the\n> input had <> where the timestamp string was required), so the\n> updated code leaves the time/tz unspecified.\n\nWe'd need update pretty.c::format_person_part() and\nbuiltin/blame.c::get_ac_line() as well, though.\n\nHow about making split_ident_line() a bit friendlier be letting it\nprovide the epoch as default time stamp instead of NULL? We shouldn't\ndo that if we'd like to be able to tell a missing/broken time stamp\napart from a commit that was actually made back in 1970 (e.g. an\nimported one). Or if we'd like to not show a time stamp in git log\noutput at all in that case.\n\n-- >8 --\nSubject: ident: let split_ident_line() provide a default time stamp\n\nIf a commit has a broken time stamp, split_ident_line() sets\ndate_begin, date_end, tz_begin and tz_end to NULL. Not all callers\nare prepared to handle that case and segfault.\n\nInstead of fixing them and having to be careful while implementing\nthe next caller, provide a string consisting of the number zero as\ndefault value, representing the UNIX epoch. That's the value that\ngit log showed before it was converted to use split_ident_line().\n\nReported-by: Ivan Lyapunov \nSigned-off-by: Rene Scharfe \n---\n ident.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/ident.c b/ident.c\nindex 1c123e6..ee840f4 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -191,6 +191,8 @@ static void strbuf_addstr_without_crud(struct strbuf *sb, const char *src)\n \tsb->buf[sb->len] = '\\0';\n }\n \n+static const char zero_string[] = \"0\";\n+\n /*\n * Reverse of fmt_ident(); given an ident line, split the fields\n * to allow the caller to parse it.\n@@ -254,10 +256,10 @@ int split_ident_line(struct ident_split *split, const char *line, int len)\n \treturn 0;\n \n person_only:\n-\tsplit->date_begin = NULL;\n-\tsplit->date_end = NULL;\n-\tsplit->tz_begin = NULL;\n-\tsplit->tz_end = NULL;\n+\tsplit->date_begin = zero_string;\n+\tsplit->date_end = zero_string + strlen(zero_string);\n+\tsplit->tz_begin = zero_string;\n+\tsplit->tz_end = zero_string + strlen(zero_string);\n \treturn 0;\n }\n \n-- \n1.8.2.1\n"},{"id":"214543","messageId":"CALWbr2xpweZOYo6szAJL5axdBBrV-c1-X_yuh6FAZQbq4iVcTw@mail.gmail.com","threadId":"33520","inReplyTo":"7v61zml0ow.fsf@alter.siamese.dyndns.org","subject":"Re: git log - crash and core dump","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-04-16T21:24:43Z","receivedAt":"2013-04-16T21:24:43Z","isPatch":false,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Tue, Apr 16, 2013 at 9:45 PM, Junio C Hamano wrote:\n> It still is curious how a malformed line was created in the first\n> place. I wouldn't worry if a private tool used hash-object to\n> create such a commit, but if it is something that is commonly used\n> (e.g. \"git commit\"), others may suffer from the same and the tool\n> needs to be tightened a bit.\n\nI already happened to see one like that, and it was clearly imported\nthrough remote-hg. I've not been able to reproduce though, and the\nparser in git-fast-import seemed already robust enough to me to not\nallow this kind of messed-up line. I will see if I can find some time\nto reproduce/investigate this deeper.\n"},{"id":"214546","messageId":"20130416213414.GA11229@sigill.intra.peff.net","threadId":"33520","inReplyTo":"7v61zml0ow.fsf@alter.siamese.dyndns.org","subject":"Re: git log - crash and core dump","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-16T21:34:14Z","receivedAt":"2013-04-16T21:34:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 16, 2013 at 12:45:03PM -0700, Junio C Hamano wrote:\n\n> René Scharfe writes:\n> \n> > Does this patch help?\n> >\n> > pretty.c | 10 ++++++----\n> > 1 file changed, 6 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/pretty.c b/pretty.c\n> > index d3a82d2..713eefc 100644\n> > --- a/pretty.c\n> > +++ b/pretty.c\n> > @@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,\n> > \tconst char *mailbuf, *namebuf;\n> > \tsize_t namelen, maillen;\n> > \tint max_length = 78; /* per rfc2822 */\n> > -\tunsigned long time;\n> > -\tint tz;\n> > +\tunsigned long time = 0;\n> > +\tint tz = 0;\n> > \n> > \tif (pp->fmt == CMIT_FMT_ONELINE)\n> > \t\treturn;\n> > @@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,\n> > \tstrbuf_add(&name, namebuf, namelen);\n> > \n> > \tnamelen = name.len + mail.len + 3; /* ' ' + '<' + '>' */\n> > -\ttime = strtoul(ident.date_begin, &date, 10);\n> > -\ttz = strtol(date, NULL, 10);\n> > +\tif (ident.date_begin) {\n> > +\t\ttime = strtoul(ident.date_begin, &date, 10);\n> > +\t\ttz = strtol(date, NULL, 10);\n> > +\t}\n> > \n> > \tif (pp->fmt == CMIT_FMT_EMAIL) {\n> > \t\tstrbuf_addstr(sb, \"From: \");\n> \n> Looks like a sensible change. split_ident_line() decided that the\n> given input was mangled and decided there is no valid date (the\n> input had <> where the timestamp string was required), so the\n> updated code leaves the time/tz unspecified.\n\nHmm. This seemed oddly familiar to me, and indeed:\n\n http://thread.gmane.org/gmane.comp.version-control.git/216870/focus=217077\n\nWe need to fix blame, too, and there is a question of how \"cat-file -p\"\nbehaves.\n\n-Peff\n"},{"id":"214551","messageId":"7vzjwyi0ba.fsf@alter.siamese.dyndns.org","threadId":"33520","inReplyTo":"516DBE2E.4060201@lsrfire.ath.cx","subject":"Re: git log - crash and core dump","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-16T22:21:29Z","receivedAt":"2013-04-16T22:21:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> How about making split_ident_line() a bit friendlier be letting it\n> provide the epoch as default time stamp instead of NULL? \n\nTwo knee-jerk concerns I have without going back to the callers:\n\n * Would that \"0\" ever be given to the approxidate parser, which\n rejects ancient dates in numbers-since-epoch format without @\n prefix?\n\n * Does any existing caller use the NULL as a sign to see the input\n was without date and act on that information?\n\n\n> -- >8 --\n> Subject: ident: let split_ident_line() provide a default time stamp\n>\n> If a commit has a broken time stamp, split_ident_line() sets\n> date_begin, date_end, tz_begin and tz_end to NULL. Not all callers\n> are prepared to handle that case and segfault.\n>\n> Instead of fixing them and having to be careful while implementing\n> the next caller, provide a string consisting of the number zero as\n> default value, representing the UNIX epoch. That's the value that\n> git log showed before it was converted to use split_ident_line().\n>\n> Reported-by: Ivan Lyapunov \n> Signed-off-by: Rene Scharfe \n> ---\n> ident.c | 10 ++++++----\n> 1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/ident.c b/ident.c\n> index 1c123e6..ee840f4 100644\n> --- a/ident.c\n> +++ b/ident.c\n> @@ -191,6 +191,8 @@ static void strbuf_addstr_without_crud(struct strbuf *sb, const char *src)\n> \tsb->buf[sb->len] = '\\0';\n> }\n> \n> +static const char zero_string[] = \"0\";\n> +\n> /*\n> * Reverse of fmt_ident(); given an ident line, split the fields\n> * to allow the caller to parse it.\n> @@ -254,10 +256,10 @@ int split_ident_line(struct ident_split *split, const char *line, int len)\n> \treturn 0;\n> \n> person_only:\n> -\tsplit->date_begin = NULL;\n> -\tsplit->date_end = NULL;\n> -\tsplit->tz_begin = NULL;\n> -\tsplit->tz_end = NULL;\n> +\tsplit->date_begin = zero_string;\n> +\tsplit->date_end = zero_string + strlen(zero_string);\n> +\tsplit->tz_begin = zero_string;\n> +\tsplit->tz_end = zero_string + strlen(zero_string);\n> \treturn 0;\n> }\n"},{"id":"214566","messageId":"CANKwXW2h1+CGw2M3CmeXzXAX+wdCPgs5Ff_bGvTgYVpkwpijUw@mail.gmail.com","threadId":"33520","inReplyTo":"7vzjwyi0ba.fsf@alter.siamese.dyndns.org","subject":"Re: git log - crash and core dump","fromName":"Ivan Lyapunov","fromEmail":"dront78@gmail.com","sentAt":"2013-04-17T02:50:36Z","receivedAt":"2013-04-17T02:50:36Z","isPatch":false,"sender":{"key":"dront78@gmail.com","avatar":"https://gravatar.com/avatar/ef7ba136ecd3bbd34601fe7af87d7c052935e5fbdee6ff634709a8c68c4b595e?d=mp&s=160"},"body":"Looks like I missed a lot of fun I'm sleeping ;)\n\nThe current repo is a product of our core crossplatform team, working\non Linux/MacOSX/Win32 environment. The Linux environment also\nseparates on Ubuntu/ArchLinux distros, and probably most of repo bugs\ncoming from ArchLinux, since other environments are stable in time. I\ncan't say exact git verison the bugs was introduced, but we have a\ncommits, dated by 0 secs since the Epoch and invalid Author fields\nalso. Also git fsck --full got the following stats on repo\n\nChecking object directories: 100% (256/256), done.\nerror in commit 7eeb541987af8a589c6ebee53346c48a13142233: invalid\nauthor/committer line - missing space before date\nerror in commit c23b0a487143e5d5d96cdc5354975e95114241ee: invalid\nauthor/committer line - missing space before date\nerror in commit c7fa421863e073996b5d1ba6beb6001b9d146cba: invalid\nauthor/committer line - missing space before date\nerror in commit 131155bd75c588bcd251b719c483d1d5bcb78504: invalid\nauthor/committer line - missing space before date\nerror in commit 0888e7ffe6ae0aaf1b6d1ba67d05715487f88a52: invalid\nauthor/committer line - missing space before date\nerror in commit 3cdeddd15c251a13fb3e79844ed3ea0e02cb611a: invalid\nauthor/committer line - missing space before date\nerror in commit 21f9fe1565d89da845fc7080495c922103bacf24: invalid\nauthor/committer line - missing space before date\nerror in commit f557d3427ba1bb33a1c1fd2c7936efa7e7c70281: invalid\nauthor/committer line - missing space before date\nerror in commit c625943779c72b41b08b41730e56126b89cbb7b4: invalid\nauthor/committer line - missing space before date\nerror in commit a83fc863991aae2bdad148a5897ed4315792dd82: invalid\nauthor/committer line - missing space before date\nerror in commit 207321f773e695b2ae88884c34620bc663383f90: invalid\nauthor/committer line - missing space before date\nerror in commit 67368e9eda9892acd6c6ebf03dd6f22b6de2db8a: invalid\nauthor/committer line - missing space before date\nerror in commit 525a5d508a7f466a1339752e921517f4db8c4af6: invalid\nauthor/committer line - missing space before date\nerror in commit 38215e27f74caa342e3353c4cd548fcf8c1df3dc: invalid\nauthor/committer line - missing space before date\nerror in commit 1ee0167194eb34caca2c20ce5c74d062fc898718: invalid\nauthor/committer line - missing space before date\nerror in commit 3274c469b981285f9a4d0b0a62afbb8f4d3e93ae: invalid\nauthor/committer line - missing space before date\nerror in commit f37ab83f71ca93f42256e05efdd4244eb321efaf: invalid\nauthor/committer line - missing space before date\nerror in commit 9bb6f7d63bb5a37b8afc3ae090bd6f34deb68633: invalid\nauthor/committer line - missing space before date\nChecking objects: 100% (9576/9576), done.\n\nAnd I haven't find a way to fix without loosing a commits history, so\nwe left them as it is. I will check the approved patches and writeback\na little bit later. Thanks a lot for looking this\n\nIvan\n\n2013/4/17 Junio C Hamano :\n> René Scharfe writes:\n>\n>> How about making split_ident_line() a bit friendlier be letting it\n>> provide the epoch as default time stamp instead of NULL?\n>\n> Two knee-jerk concerns I have without going back to the callers:\n>\n> * Would that \"0\" ever be given to the approxidate parser, which\n> rejects ancient dates in numbers-since-epoch format without @\n> prefix?\n>\n> * Does any existing caller use the NULL as a sign to see the input\n> was without date and act on that information?\n>\n>\n>> -- >8 --\n>> Subject: ident: let split_ident_line() provide a default time stamp\n>>\n>> If a commit has a broken time stamp, split_ident_line() sets\n>> date_begin, date_end, tz_begin and tz_end to NULL. Not all callers\n>> are prepared to handle that case and segfault.\n>>\n>> Instead of fixing them and having to be careful while implementing\n>> the next caller, provide a string consisting of the number zero as\n>> default value, representing the UNIX epoch. That's the value that\n>> git log showed before it was converted to use split_ident_line().\n>>\n>> Reported-by: Ivan Lyapunov \n>> Signed-off-by: Rene Scharfe \n>> ---\n>> ident.c | 10 ++++++----\n>> 1 file changed, 6 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/ident.c b/ident.c\n>> index 1c123e6..ee840f4 100644\n>> --- a/ident.c\n>> +++ b/ident.c\n>> @@ -191,6 +191,8 @@ static void strbuf_addstr_without_crud(struct strbuf *sb, const char *src)\n>> sb->buf[sb->len] = '\\0';\n>> }\n>>\n>> +static const char zero_string[] = \"0\";\n>> +\n>> /*\n>> * Reverse of fmt_ident(); given an ident line, split the fields\n>> * to allow the caller to parse it.\n>> @@ -254,10 +256,10 @@ int split_ident_line(struct ident_split *split, const char *line, int len)\n>> return 0;\n>>\n>> person_only:\n>> - split->date_begin = NULL;\n>> - split->date_end = NULL;\n>> - split->tz_begin = NULL;\n>> - split->tz_end = NULL;\n>> + split->date_begin = zero_string;\n>> + split->date_end = zero_string + strlen(zero_string);\n>> + split->tz_begin = zero_string;\n>> + split->tz_end = zero_string + strlen(zero_string);\n>> return 0;\n>> }\n"},{"id":"214576","messageId":"CANKwXW0WkZXJMi+6T8ymvTi5-JpRfrn_xKmjGw-FsXpaCGuK5g@mail.gmail.com","threadId":"33520","inReplyTo":"CANKwXW2h1+CGw2M3CmeXzXAX+wdCPgs5Ff_bGvTgYVpkwpijUw@mail.gmail.com","subject":"Re: git log - crash and core dump","fromName":"Ivan Lyapunov","fromEmail":"dront78@gmail.com","sentAt":"2013-04-17T05:22:01Z","receivedAt":"2013-04-17T05:22:01Z","isPatch":false,"sender":{"key":"dront78@gmail.com","avatar":"https://gravatar.com/avatar/ef7ba136ecd3bbd34601fe7af87d7c052935e5fbdee6ff634709a8c68c4b595e?d=mp&s=160"},"body":"I checked René Scharfe's patch and it works - no more git log crash.\nAlso I checked a broken commits by git show and the most of them\ncreated by me. I can confirm I never used anyting else except console\ngit commit or netbeans gui to commit, which is just git gui wrapper\ntool. Several commits is a just branch merge commits with no changes\nat all. The modification time across broken commits is around from\nnear end of Oct to end of Nov 2012 and there are good commits between\nbroken maded on the same machine. I hope it helps to reproduce the\nproblem. Also it would be good if there are some way to fix this\nerrors in repo, since we found the same bugs in our Eclipse-based\nAndroid project repo\nIvan\n\n2013/4/17 Ivan Lyapunov :\n> Looks like I missed a lot of fun I'm sleeping ;)\n>\n> The current repo is a product of our core crossplatform team, working\n> on Linux/MacOSX/Win32 environment. The Linux environment also\n> separates on Ubuntu/ArchLinux distros, and probably most of repo bugs\n> coming from ArchLinux, since other environments are stable in time. I\n> can't say exact git verison the bugs was introduced, but we have a\n> commits, dated by 0 secs since the Epoch and invalid Author fields\n> also. Also git fsck --full got the following stats on repo\n>\n> Checking object directories: 100% (256/256), done.\n> error in commit 7eeb541987af8a589c6ebee53346c48a13142233: invalid\n> author/committer line - missing space before date\n> error in commit c23b0a487143e5d5d96cdc5354975e95114241ee: invalid\n> author/committer line - missing space before date\n> error in commit c7fa421863e073996b5d1ba6beb6001b9d146cba: invalid\n> author/committer line - missing space before date\n> error in commit 131155bd75c588bcd251b719c483d1d5bcb78504: invalid\n> author/committer line - missing space before date\n> error in commit 0888e7ffe6ae0aaf1b6d1ba67d05715487f88a52: invalid\n> author/committer line - missing space before date\n> error in commit 3cdeddd15c251a13fb3e79844ed3ea0e02cb611a: invalid\n> author/committer line - missing space before date\n> error in commit 21f9fe1565d89da845fc7080495c922103bacf24: invalid\n> author/committer line - missing space before date\n> error in commit f557d3427ba1bb33a1c1fd2c7936efa7e7c70281: invalid\n> author/committer line - missing space before date\n> error in commit c625943779c72b41b08b41730e56126b89cbb7b4: invalid\n> author/committer line - missing space before date\n> error in commit a83fc863991aae2bdad148a5897ed4315792dd82: invalid\n> author/committer line - missing space before date\n> error in commit 207321f773e695b2ae88884c34620bc663383f90: invalid\n> author/committer line - missing space before date\n> error in commit 67368e9eda9892acd6c6ebf03dd6f22b6de2db8a: invalid\n> author/committer line - missing space before date\n> error in commit 525a5d508a7f466a1339752e921517f4db8c4af6: invalid\n> author/committer line - missing space before date\n> error in commit 38215e27f74caa342e3353c4cd548fcf8c1df3dc: invalid\n> author/committer line - missing space before date\n> error in commit 1ee0167194eb34caca2c20ce5c74d062fc898718: invalid\n> author/committer line - missing space before date\n> error in commit 3274c469b981285f9a4d0b0a62afbb8f4d3e93ae: invalid\n> author/committer line - missing space before date\n> error in commit f37ab83f71ca93f42256e05efdd4244eb321efaf: invalid\n> author/committer line - missing space before date\n> error in commit 9bb6f7d63bb5a37b8afc3ae090bd6f34deb68633: invalid\n> author/committer line - missing space before date\n> Checking objects: 100% (9576/9576), done.\n>\n> And I haven't find a way to fix without loosing a commits history, so\n> we left them as it is. I will check the approved patches and writeback\n> a little bit later. Thanks a lot for looking this\n>\n> Ivan\n>\n> 2013/4/17 Junio C Hamano :\n>> René Scharfe writes:\n>>\n>>> How about making split_ident_line() a bit friendlier be letting it\n>>> provide the epoch as default time stamp instead of NULL?\n>>\n>> Two knee-jerk concerns I have without going back to the callers:\n>>\n>> * Would that \"0\" ever be given to the approxidate parser, which\n>> rejects ancient dates in numbers-since-epoch format without @\n>> prefix?\n>>\n>> * Does any existing caller use the NULL as a sign to see the input\n>> was without date and act on that information?\n>>\n>>\n>>> -- >8 --\n>>> Subject: ident: let split_ident_line() provide a default time stamp\n>>>\n>>> If a commit has a broken time stamp, split_ident_line() sets\n>>> date_begin, date_end, tz_begin and tz_end to NULL. Not all callers\n>>> are prepared to handle that case and segfault.\n>>>\n>>> Instead of fixing them and having to be careful while implementing\n>>> the next caller, provide a string consisting of the number zero as\n>>> default value, representing the UNIX epoch. That's the value that\n>>> git log showed before it was converted to use split_ident_line().\n>>>\n>>> Reported-by: Ivan Lyapunov \n>>> Signed-off-by: Rene Scharfe \n>>> ---\n>>> ident.c | 10 ++++++----\n>>> 1 file changed, 6 insertions(+), 4 deletions(-)\n>>>\n>>> diff --git a/ident.c b/ident.c\n>>> index 1c123e6..ee840f4 100644\n>>> --- a/ident.c\n>>> +++ b/ident.c\n>>> @@ -191,6 +191,8 @@ static void strbuf_addstr_without_crud(struct strbuf *sb, const char *src)\n>>> sb->buf[sb->len] = '\\0';\n>>> }\n>>>\n>>> +static const char zero_string[] = \"0\";\n>>> +\n>>> /*\n>>> * Reverse of fmt_ident(); given an ident line, split the fields\n>>> * to allow the caller to parse it.\n>>> @@ -254,10 +256,10 @@ int split_ident_line(struct ident_split *split, const char *line, int len)\n>>> return 0;\n>>>\n>>> person_only:\n>>> - split->date_begin = NULL;\n>>> - split->date_end = NULL;\n>>> - split->tz_begin = NULL;\n>>> - split->tz_end = NULL;\n>>> + split->date_begin = zero_string;\n>>> + split->date_end = zero_string + strlen(zero_string);\n>>> + split->tz_begin = zero_string;\n>>> + split->tz_end = zero_string + strlen(zero_string);\n>>> return 0;\n>>> }\n"},{"id":"214577","messageId":"7vli8hhgmn.fsf@alter.siamese.dyndns.org","threadId":"33520","inReplyTo":"7vzjwyi0ba.fsf@alter.siamese.dyndns.org","subject":"Re: git log - crash and core dump","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-17T05:26:40Z","receivedAt":"2013-04-17T05:26:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> René Scharfe writes:\n>\n>> How about making split_ident_line() a bit friendlier be letting it\n>> provide the epoch as default time stamp instead of NULL? \n>\n> Two knee-jerk concerns I have without going back to the callers:\n>\n> * Would that \"0\" ever be given to the approxidate parser, which\n> rejects ancient dates in numbers-since-epoch format without @\n> prefix?\n>\n> * Does any existing caller use the NULL as a sign to see the input\n> was without date and act on that information?\n\nI looked at all the callers (there aren't that many), and none of\nthem did \"Do this on a person-only ident, and do that on an ident\nwith timestamp\". So for the callers that ignore timestamp, your\npatch will be a no-op, and for others that assume there is a\ntimestamp, it will turn a crash/segv into output with funny\ntimestamp.\n\nSo I think the patch is a right thing to do (we would need in-code\ncomment to warn new callers about the semantics, though).\n\nThanks.\n"},{"id":"214582","messageId":"20130417063942.GA27703@sigill.intra.peff.net","threadId":"33520","inReplyTo":"7vli8hhgmn.fsf@alter.siamese.dyndns.org","subject":"Re: git log - crash and core dump","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-17T06:39:42Z","receivedAt":"2013-04-17T06:39:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 16, 2013 at 10:26:40PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > René Scharfe writes:\n> >\n> >> How about making split_ident_line() a bit friendlier be letting it\n> >> provide the epoch as default time stamp instead of NULL? \n> >\n> > Two knee-jerk concerns I have without going back to the callers:\n> >\n> > * Would that \"0\" ever be given to the approxidate parser, which\n> > rejects ancient dates in numbers-since-epoch format without @\n> > prefix?\n> >\n> > * Does any existing caller use the NULL as a sign to see the input\n> > was without date and act on that information?\n> \n> I looked at all the callers (there aren't that many), and none of\n> them did \"Do this on a person-only ident, and do that on an ident\n> with timestamp\". So for the callers that ignore timestamp, your\n> patch will be a no-op, and for others that assume there is a\n> timestamp, it will turn a crash/segv into output with funny\n> timestamp.\n\nWhat about sane_ident_split in builtin/commit.c? It explicitly rejects a\nNULL date. The logic in determine_author_info is a little hard to follow\n(it assembles the ident line with fmt_ident and then reparses it), but I\nbelieve it should be catching a bogus line from \"commit -c\", or from\nGIT_AUTHOR_DATE in the environment.\n\nAs a side note, when determine_author_info sees a bogus ident, it\nappears to just silently ignore it, which is probably a bad thing.\nShouldn't we by complaining? Or am I mis-reading the code?\n\n-Peff\n"},{"id":"214587","messageId":"20130417082701.GP2278@serenity.lan","threadId":"33520","inReplyTo":"CANKwXW0WkZXJMi+6T8ymvTi5-JpRfrn_xKmjGw-FsXpaCGuK5g@mail.gmail.com","subject":"Re: git log - crash and core dump","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-04-17T08:27:01Z","receivedAt":"2013-04-17T08:27:01Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Apr 17, 2013 at 09:22:01AM +0400, Ivan Lyapunov wrote:\n> I checked René Scharfe's patch and it works - no more git log crash.\n> Also I checked a broken commits by git show and the most of them\n> created by me. I can confirm I never used anyting else except console\n> git commit or netbeans gui to commit, which is just git gui wrapper\n> tool.\n\nDoesn't NetBeans use JGit[1]? That makes it a bit more than a wrapper\nfor the Git command line tools.\n\n[1] http://eclipse.org/jgit/\n"},{"id":"214592","messageId":"CANKwXW10tA+qNZ21YYcZxFLL4fWVRhq42nrkSogtmwAMTgPAzQ@mail.gmail.com","threadId":"33520","inReplyTo":"20130417082701.GP2278@serenity.lan","subject":"Re: git log - crash and core dump","fromName":"Ivan Lyapunov","fromEmail":"dront78@gmail.com","sentAt":"2013-04-17T09:14:48Z","receivedAt":"2013-04-17T09:14:48Z","isPatch":false,"sender":{"key":"dront78@gmail.com","avatar":"https://gravatar.com/avatar/ef7ba136ecd3bbd34601fe7af87d7c052935e5fbdee6ff634709a8c68c4b595e?d=mp&s=160"},"body":"netbeans use some integrated git wrapper and I don't know about JGit\nsource base or not. In Eclipse we use Egit. Also all broken commits\nlimited to november 2012, but we still continue to use the same\ndevelopment environments without any problems\nIvan\n\n2013/4/17 John Keeping :\n> On Wed, Apr 17, 2013 at 09:22:01AM +0400, Ivan Lyapunov wrote:\n>> I checked René Scharfe's patch and it works - no more git log crash.\n>> Also I checked a broken commits by git show and the most of them\n>> created by me. I can confirm I never used anyting else except console\n>> git commit or netbeans gui to commit, which is just git gui wrapper\n>> tool.\n>\n> Doesn't NetBeans use JGit[1]? That makes it a bit more than a wrapper\n> for the Git command line tools.\n>\n> [1] http://eclipse.org/jgit/\n"},{"id":"214593","messageId":"20130417134311.56ea1b191270647dc9ede1df@domain007.com","threadId":"33520","inReplyTo":"CANKwXW10tA+qNZ21YYcZxFLL4fWVRhq42nrkSogtmwAMTgPAzQ@mail.gmail.com","subject":"Re: git log - crash and core dump","fromName":"Konstantin Khomoutov","fromEmail":"flatworm@users.sourceforge.net","sentAt":"2013-04-17T09:43:11Z","receivedAt":"2013-04-17T09:43:11Z","isPatch":false,"sender":{"key":"flatworm@users.sourceforge.net","avatar":null},"body":"On Wed, 17 Apr 2013 13:14:48 +0400\nIvan Lyapunov wrote:\n\n> netbeans use some integrated git wrapper and I don't know about JGit\n> source base or not. In Eclipse we use Egit. Also all broken commits\n> limited to november 2012, but we still continue to use the same\n> development environments without any problems\n\nDoes \"the same\" also mean \"of the same version\"?\nI mean that if, say, you managed to update netbeans after November, 2012\nthat would explain a lot as the update might just silently pull\na fix for the Git-interfacing code. The same might apply to Git itself\nas well.\n"},{"id":"214598","messageId":"CANKwXW2gLo7o7DmQQvmGtMDUOjQVHAJZYcyRS5N11q+KQY9Dcw@mail.gmail.com","threadId":"33520","inReplyTo":"CANKwXW1heci+D5ZO3aF+dMN9davRawuZuKz0bf2n3iRiMjjgHg@mail.gmail.com","subject":"Re: git log - crash and core dump","fromName":"Ivan Lyapunov","fromEmail":"dront78@gmail.com","sentAt":"2013-04-17T10:23:40Z","receivedAt":"2013-04-17T10:23:40Z","isPatch":false,"sender":{"key":"dront78@gmail.com","avatar":"https://gravatar.com/avatar/ef7ba136ecd3bbd34601fe7af87d7c052935e5fbdee6ff634709a8c68c4b595e?d=mp&s=160"},"body":"missed a list sorry for dup\n\nI ment the same because git changes the version too. Also\nnetbeans/eclipse upgrade are not synced, because of different\nproducts. So the same ment only names, not versions. But in deep\nsearch another repos I found the broken commits in Eclipse repo dated\nby 13 march 2013. Can them produced because of previous broken\ncommits? And probably you can be right about java git wrappers can\nproduce this issues, I'll try to make a broken repo from scratch\nlater.\nIvan\n\n2013/4/17 Ivan Lyapunov :\n> I ment the same because git changes the version too. Also\n> netbeans/eclipse upgrade are not synced, because of different\n> products. So the same ment only names, not versions. But in deep\n> search another repos I found the broken commits in Eclipse repo dated\n> by 13 march 2013. Can them produced because of previous broken\n> commits? And probably you can be right about java git wrappers can\n> produce this issues, I'll try to make a broken repo from scratch\n> later.\n> Ivan\n"},{"id":"214624","messageId":"7v1ua9f3lg.fsf@alter.siamese.dyndns.org","threadId":"33520","inReplyTo":"20130417063942.GA27703@sigill.intra.peff.net","subject":"Re: git log - crash and core dump","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-17T17:51:07Z","receivedAt":"2013-04-17T17:51:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> What about sane_ident_split in builtin/commit.c? It explicitly rejects a\n> NULL date. The logic in determine_author_info is a little hard to follow\n> (it assembles the ident line with fmt_ident and then reparses it), but I\n> believe it should be catching a bogus line from \"commit -c\", or from\n> GIT_AUTHOR_DATE in the environment.\n\nYeah, you are of course right. I noticed that \"fmt_ident then parse\"\nsequence a bit funny, too. It seems to manually parse to prepare\nwhat it feeds fmt_ident.\n\n> As a side note, when determine_author_info sees a bogus ident, it\n> appears to just silently ignore it, which is probably a bad thing.\n\nTrue, too.\n"},{"id":"214625","messageId":"516EE300.7020200@lsrfire.ath.cx","threadId":"33520","inReplyTo":"20130417063942.GA27703@sigill.intra.peff.net","subject":"Re: git log - crash and core dump","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-04-17T17:59:28Z","receivedAt":"2013-04-17T17:59:28Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.04.2013 08:39, schrieb Jeff King:\n> On Tue, Apr 16, 2013 at 10:26:40PM -0700, Junio C Hamano wrote:\n>\n>> Junio C Hamano writes:\n>>\n>>> René Scharfe writes:\n>>>\n>>>> How about making split_ident_line() a bit friendlier be letting it\n>>>> provide the epoch as default time stamp instead of NULL?\n>>>\n>>> Two knee-jerk concerns I have without going back to the callers:\n>>>\n>>> * Would that \"0\" ever be given to the approxidate parser, which\n>>> rejects ancient dates in numbers-since-epoch format without @\n>>> prefix?\n>>>\n>>> * Does any existing caller use the NULL as a sign to see the input\n>>> was without date and act on that information?\n>>\n>> I looked at all the callers (there aren't that many), and none of\n>> them did \"Do this on a person-only ident, and do that on an ident\n>> with timestamp\". So for the callers that ignore timestamp, your\n>> patch will be a no-op, and for others that assume there is a\n>> timestamp, it will turn a crash/segv into output with funny\n>> timestamp.\n>\n> What about sane_ident_split in builtin/commit.c? It explicitly rejects a\n> NULL date. The logic in determine_author_info is a little hard to follow\n> (it assembles the ident line with fmt_ident and then reparses it), but I\n> believe it should be catching a bogus line from \"commit -c\", or from\n> GIT_AUTHOR_DATE in the environment.\n\nRight, so let's keep the NULLs and fix the individual cases. A quick\n\"git grep -W -e date_begin -e date_end -e tz_begin -e tz_end\" reveals\nthat there are only the ones we talked about: blame, pretty, commit\nand -- of course -- ident. And only the first two need fixing.\n\n> As a side note, when determine_author_info sees a bogus ident, it\n> appears to just silently ignore it, which is probably a bad thing.\n> Shouldn't we by complaining? Or am I mis-reading the code?\n\nThe code looks complicated, but I just tried it: fmt_ident() dies if you \ngive it an invalid date.\n\nRené\n"},{"id":"214626","messageId":"20130417180248.GA1839@sigill.intra.peff.net","threadId":"33520","inReplyTo":"516EE300.7020200@lsrfire.ath.cx","subject":"Re: git log - crash and core dump","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-17T18:02:48Z","receivedAt":"2013-04-17T18:02:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 17, 2013 at 07:59:28PM +0200, René Scharfe wrote:\n\n> >What about sane_ident_split in builtin/commit.c? It explicitly rejects a\n> >NULL date. The logic in determine_author_info is a little hard to follow\n> >(it assembles the ident line with fmt_ident and then reparses it), but I\n> >believe it should be catching a bogus line from \"commit -c\", or from\n> >GIT_AUTHOR_DATE in the environment.\n> \n> Right, so let's keep the NULLs and fix the individual cases. A quick\n> \"git grep -W -e date_begin -e date_end -e tz_begin -e tz_end\" reveals\n> that there are only the ones we talked about: blame, pretty, commit\n> and -- of course -- ident. And only the first two need fixing.\n\nYes, that matches the list I came up with in February.\n\nI think we also need to do something about \"git cat-file -p\", which does\nnot use the split_ident_line parser (but has its own problems with the\nhome-grown parser).\n\n> >As a side note, when determine_author_info sees a bogus ident, it\n> >appears to just silently ignore it, which is probably a bad thing.\n> >Shouldn't we by complaining? Or am I mis-reading the code?\n> \n> The code looks complicated, but I just tried it: fmt_ident() dies if\n> you give it an invalid date.\n\nIt does seem like determine_author_info can be greatly simplified, but I\nhaven't looked all that closely.\n\n-Peff\n"},{"id":"214628","messageId":"516EEAFF.6000209@lsrfire.ath.cx","threadId":"33520","inReplyTo":"516EE300.7020200@lsrfire.ath.cx","subject":"[PATCH] pretty: handle broken commit headers gracefully","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-04-17T18:33:35Z","receivedAt":"2013-04-17T18:33:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Centralize the parsing of the date and time zone strings in the new\nhelper function show_ident_date() and make sure it checks the pointers\nprovided by split_ident_line() for NULL before use.\n\nReported-by: Ivan Lyapunov \nSigned-off-by: Rene Scharfe \n---\nThe first test case passes on v1.8.1, i.e. it also showed the epoch.\nThe second one passes on v1.8.1 and on master because there already\nis a NULL check in format_person_part().\n\n pretty.c | 45 ++++++++++++++++++++++++---------------------\n t/t4211-log-corrupt.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 21 deletions(-)\n create mode 100755 t/t4211-log-corrupt.sh\n\ndiff --git a/pretty.c b/pretty.c\nindex d3a82d2..c116248 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -393,6 +393,19 @@ static void add_rfc2047(struct strbuf *sb, const char *line, size_t len,\n \tstrbuf_addstr(sb, \"?=\");\n }\n \n+static const char *show_ident_date(const struct ident_split *ident,\n+\t\t\t\t enum date_mode mode)\n+{\n+\tunsigned long date = 0;\n+\tint tz = 0;\n+\n+\tif (ident->date_begin && ident->date_end)\n+\t\tdate = strtoul(ident->date_begin, NULL, 10);\n+\tif (ident->tz_begin && ident->tz_end)\n+\t\ttz = strtol(ident->tz_begin, NULL, 10);\n+\treturn show_date(date, tz, mode);\n+}\n+\n void pp_user_info(const struct pretty_print_context *pp,\n \t\t const char *what, struct strbuf *sb,\n \t\t const char *line, const char *encoding)\n@@ -401,12 +414,10 @@ void pp_user_info(const struct pretty_print_context *pp,\n \tstruct strbuf mail;\n \tstruct ident_split ident;\n \tint linelen;\n-\tchar *line_end, *date;\n+\tchar *line_end;\n \tconst char *mailbuf, *namebuf;\n \tsize_t namelen, maillen;\n \tint max_length = 78; /* per rfc2822 */\n-\tunsigned long time;\n-\tint tz;\n \n \tif (pp->fmt == CMIT_FMT_ONELINE)\n \t\treturn;\n@@ -438,8 +449,6 @@ void pp_user_info(const struct pretty_print_context *pp,\n \tstrbuf_add(&name, namebuf, namelen);\n \n \tnamelen = name.len + mail.len + 3; /* ' ' + '<' + '>' */\n-\ttime = strtoul(ident.date_begin, &date, 10);\n-\ttz = strtol(date, NULL, 10);\n \n \tif (pp->fmt == CMIT_FMT_EMAIL) {\n \t\tstrbuf_addstr(sb, \"From: \");\n@@ -472,13 +481,16 @@ void pp_user_info(const struct pretty_print_context *pp,\n \n \tswitch (pp->fmt) {\n \tcase CMIT_FMT_MEDIUM:\n-\t\tstrbuf_addf(sb, \"Date: %s\\n\", show_date(time, tz, pp->date_mode));\n+\t\tstrbuf_addf(sb, \"Date: %s\\n\",\n+\t\t\t show_ident_date(&ident, pp->date_mode));\n \t\tbreak;\n \tcase CMIT_FMT_EMAIL:\n-\t\tstrbuf_addf(sb, \"Date: %s\\n\", show_date(time, tz, DATE_RFC2822));\n+\t\tstrbuf_addf(sb, \"Date: %s\\n\",\n+\t\t\t show_ident_date(&ident, DATE_RFC2822));\n \t\tbreak;\n \tcase CMIT_FMT_FULLER:\n-\t\tstrbuf_addf(sb, \"%sDate: %s\\n\", what, show_date(time, tz, pp->date_mode));\n+\t\tstrbuf_addf(sb, \"%sDate: %s\\n\", what,\n+\t\t\t show_ident_date(&ident, pp->date_mode));\n \t\tbreak;\n \tdefault:\n \t\t/* notin' */\n@@ -688,8 +700,6 @@ static size_t format_person_part(struct strbuf *sb, char part,\n {\n \t/* currently all placeholders have same length */\n \tconst int placeholder_len = 2;\n-\tint tz;\n-\tunsigned long date = 0;\n \tstruct ident_split s;\n \tconst char *name, *mail;\n \tsize_t maillen, namelen;\n@@ -716,30 +726,23 @@ static size_t format_person_part(struct strbuf *sb, char part,\n \tif (!s.date_begin)\n \t\tgoto skip;\n \n-\tdate = strtoul(s.date_begin, NULL, 10);\n-\n \tif (part == 't') {\t/* date, UNIX timestamp */\n \t\tstrbuf_add(sb, s.date_begin, s.date_end - s.date_begin);\n \t\treturn placeholder_len;\n \t}\n \n-\t/* parse tz */\n-\ttz = strtoul(s.tz_begin + 1, NULL, 10);\n-\tif (*s.tz_begin == '-')\n-\t\ttz = -tz;\n-\n \tswitch (part) {\n \tcase 'd':\t/* date */\n-\t\tstrbuf_addstr(sb, show_date(date, tz, dmode));\n+\t\tstrbuf_addstr(sb, show_ident_date(&s, dmode));\n \t\treturn placeholder_len;\n \tcase 'D':\t/* date, RFC2822 style */\n-\t\tstrbuf_addstr(sb, show_date(date, tz, DATE_RFC2822));\n+\t\tstrbuf_addstr(sb, show_ident_date(&s, DATE_RFC2822));\n \t\treturn placeholder_len;\n \tcase 'r':\t/* date, relative */\n-\t\tstrbuf_addstr(sb, show_date(date, tz, DATE_RELATIVE));\n+\t\tstrbuf_addstr(sb, show_ident_date(&s, DATE_RELATIVE));\n \t\treturn placeholder_len;\n \tcase 'i':\t/* date, ISO 8601 */\n-\t\tstrbuf_addstr(sb, show_date(date, tz, DATE_ISO8601));\n+\t\tstrbuf_addstr(sb, show_ident_date(&s, DATE_ISO8601));\n \t\treturn placeholder_len;\n \t}\n \ndiff --git a/t/t4211-log-corrupt.sh b/t/t4211-log-corrupt.sh\nnew file mode 100755\nindex 0000000..ec5099b\n--- /dev/null\n+++ b/t/t4211-log-corrupt.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description='git log with invalid commit headers'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit foo &&\n+\n+\tgit cat-file commit HEAD |\n+\tsed \"/^author /s/>/>-<>/\" >broken_email.commit &&\n+\tgit hash-object -w -t commit broken_email.commit >broken_email.hash &&\n+\tgit update-ref refs/heads/broken_email $(cat broken_email.hash)\n+'\n+\n+test_expect_success 'git log with broken author email' '\n+\t{\n+\t\techo commit $(cat broken_email.hash)\n+\t\techo \"Author: A U Thor \"\n+\t\techo \"Date: Thu Jan 1 00:00:00 1970 +0000\"\n+\t\techo\n+\t\techo \" foo\"\n+\t} >expect.out &&\n+\t: >expect.err &&\n+\n+\tgit log broken_email >actual.out 2>actual.err &&\n+\n+\ttest_cmp expect.out actual.out &&\n+\ttest_cmp expect.err actual.err\n+'\n+\n+test_expect_success 'git log --format with broken author email' '\n+\techo \"A U Thor+author@example.com+\" >expect.out &&\n+\t: >expect.err &&\n+\n+\tgit log --format=\"%an+%ae+%ad\" broken_email >actual.out 2>actual.err &&\n+\n+\ttest_cmp expect.out actual.out &&\n+\ttest_cmp expect.err actual.err\n+'\n+\n+test_done\n-- \n1.8.2.1\n"},{"id":"214629","messageId":"516EEB12.5050209@lsrfire.ath.cx","threadId":"33520","inReplyTo":"516EE300.7020200@lsrfire.ath.cx","subject":"[PATCH] blame: handle broken commit headers gracefully","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-04-17T18:33:54Z","receivedAt":"2013-04-17T18:33:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"split_ident_line() can leave us with the pointers date_begin, date_end,\ntz_begin and tz_end all set to NULL. Check them before use and supply\nthe same fallback values as in the case of a negative return code from\nsplit_ident_line().\n\nThe \"(unknown)\" is not actually shown in the output, though, because it\nwill be converted to a number (zero) eventually.\n\nSigned-off-by: Rene Scharfe \n---\nMinimal patch, test case missing. It's a bit sad that the old commit\nparser of blame handled Ivan's specific corruption (extra \"-<>\" after\nemail) gracefully because it used the spaces as cutting points instead\nof \"<\" and \">\".\n\n builtin/blame.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 86100e9..7770781 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1375,10 +1375,15 @@ static void get_ac_line(const char *inbuf, const char *what,\n \tmaillen = ident.mail_end - ident.mail_begin;\n \tmailbuf = ident.mail_begin;\n \n-\t*time = strtoul(ident.date_begin, NULL, 10);\n+\tif (ident.date_begin && ident.date_end)\n+\t\t*time = strtoul(ident.date_begin, NULL, 10);\n+\telse\n+\t\t*time = 0;\n \n-\tlen = ident.tz_end - ident.tz_begin;\n-\tstrbuf_add(tz, ident.tz_begin, len);\n+\tif (ident.tz_begin && ident.tz_end)\n+\t\tstrbuf_add(tz, ident.tz_begin, ident.tz_end - ident.tz_begin);\n+\telse\n+\t\tstrbuf_addstr(tz, \"(unknown)\");\n \n \t/*\n \t * Now, convert both name and e-mail using mailmap\n-- \n1.8.2.1\n"},{"id":"214632","messageId":"516EF2AD.9090403@lsrfire.ath.cx","threadId":"33520","inReplyTo":"20130417180248.GA1839@sigill.intra.peff.net","subject":"Re: git log - crash and core dump","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-04-17T19:06:21Z","receivedAt":"2013-04-17T19:06:21Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.04.2013 20:02, schrieb Jeff King:\n> I think we also need to do something about \"git cat-file -p\", which does\n> not use the split_ident_line parser (but has its own problems with the\n> home-grown parser).\n\nAh, while it prints commit object contents verbatim, it formats the date\nof tags. And it does it without help from tag.c (or ident.c), which in\nturn does its own parsing as well. So it looks like we have two more\ncandidates for conversion to split_ident_line() here.\n\nRené\n"},{"id":"214639","messageId":"20130417210048.GA635@sigill.intra.peff.net","threadId":"33520","inReplyTo":"516EF2AD.9090403@lsrfire.ath.cx","subject":"[PATCH] cat-file: print tags raw for \"cat-file -p\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-17T21:00:48Z","receivedAt":"2013-04-17T21:00:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 17, 2013 at 09:06:21PM +0200, René Scharfe wrote:\n\n> Am 17.04.2013 20:02, schrieb Jeff King:\n> >I think we also need to do something about \"git cat-file -p\", which does\n> >not use the split_ident_line parser (but has its own problems with the\n> >home-grown parser).\n> \n> Ah, while it prints commit object contents verbatim, it formats the date\n> of tags. And it does it without help from tag.c (or ident.c), which in\n> turn does its own parsing as well. So it looks like we have two more\n> candidates for conversion to split_ident_line() here.\n\nI think we should apply the patch below to just drop the date formatting\nfrom cat-file, along with your two patches. This is the 4/4 from the\nseries I posted in February:\n\n http://thread.gmane.org/gmane.comp.version-control.git/216870/focus=217081\n\nbut there I claimed that \"git tag -v\" might be affected. Upon looking\ncloser, it is not; we accidentally dropped the pretty-printing of the\ndate from it many years ago (and nobody seemed to care).\n\nThe other patches from that series aren't necessary. The 1/4 is replaced\nby your patches (which do roughly the same thing, but add nice tests and\nseem to refactor a bit more). The 2/4 and 3/4 patches were about adding\nnew fsck checks for tags, but I think there is some refactoring\nnecessary there. They can wait for now.\n\n-- >8 --\nSubject: [PATCH] cat-file: print tags raw for \"cat-file -p\"\n\nWhen \"cat-file -p\" prints commits, it shows them in their\nraw format, since git's format is already human-readable.\nFor tags, however, we print the whole thing raw except for\none thing: we convert the timestamp on the tagger line into a\nhuman-readable date.\n\nThis dates all the way back to a0f15fa (Pretty-print tagger\ndates, 2006-03-01). At that time there was no other way to\npretty-print a tag. These days, however, neither of those\nmatters much. The normal way to pretty-print a tag is with\n\"git show\", which is much more flexible than \"cat-file -p\".\n\nCommit a0f15fa also built \"verify-tag --verbose\" (and\nsubsequently \"tag -v\") around the \"cat-file -p\" output.\nHowever, that behavior was lost in commit 62e09ce (Make git\ntag a builtin, 2007-07-20), and we went back to printing\nthe raw tag contents. Nobody seems to have noticed the bug\nsince then (and it is arguably a saner behavior anyway, as\nit shows the actual bytes for which we verified the\nsignature).\n\nLet's drop the tagger-date formatting for \"cat-file -p\". It\nmakes us more consistent with cat-file's commit\npretty-printer, and as a bonus, we can drop the hand-rolled\ntag parsing code in cat-file (which happened to behave\ninconsistently with the tag pretty-printing code elsewhere).\n\nThis is a change of output format, so it's possible that\nsome callers could considered this a regression. However,\nthe original behavior was arguably a bug (due to the\ninconsistency with commits), likely nobody was relying on it\n(even we do not use it ourselves these days), and anyone\nrelying on the \"-p\" pretty-printer should be able to expect\na change in the output format (i.e., while \"cat-file\" is\nplumbing, the output format of \"-p\" was never guaranteed to\nbe stable).\n\nSigned-off-by: Jeff King \n---\n builtin/cat-file.c | 71 -----------------------------------------------------\n t/t1006-cat-file.sh | 5 +---\n 2 files changed, 1 insertion(+), 75 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 40f87b4..045cee7 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -16,73 +16,6 @@\n #define BATCH 1\n #define BATCH_CHECK 2\n \n-static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long size)\n-{\n-\t/* the parser in tag.c is useless here. */\n-\tconst char *endp = buf + size;\n-\tconst char *cp = buf;\n-\n-\twhile (cp < endp) {\n-\t\tchar c = *cp++;\n-\t\tif (c != '\\n')\n-\t\t\tcontinue;\n-\t\tif (7 <= endp - cp && !memcmp(\"tagger \", cp, 7)) {\n-\t\t\tconst char *tagger = cp;\n-\n-\t\t\t/* Found the tagger line. Copy out the contents\n-\t\t\t * of the buffer so far.\n-\t\t\t */\n-\t\t\twrite_or_die(1, buf, cp - buf);\n-\n-\t\t\t/*\n-\t\t\t * Do something intelligent, like pretty-printing\n-\t\t\t * the date.\n-\t\t\t */\n-\t\t\twhile (cp < endp) {\n-\t\t\t\tif (*cp++ == '\\n') {\n-\t\t\t\t\t/* tagger to cp is a line\n-\t\t\t\t\t * that has ident and time.\n-\t\t\t\t\t */\n-\t\t\t\t\tconst char *sp = tagger;\n-\t\t\t\t\tchar *ep;\n-\t\t\t\t\tunsigned long date;\n-\t\t\t\t\tlong tz;\n-\t\t\t\t\twhile (sp < cp && *sp != '>')\n-\t\t\t\t\t\tsp++;\n-\t\t\t\t\tif (sp == cp) {\n-\t\t\t\t\t\t/* give up */\n-\t\t\t\t\t\twrite_or_die(1, tagger,\n-\t\t\t\t\t\t\t cp - tagger);\n-\t\t\t\t\t\tbreak;\n-\t\t\t\t\t}\n-\t\t\t\t\twhile (sp < cp &&\n-\t\t\t\t\t !('0' <= *sp && *sp <= '9'))\n-\t\t\t\t\t\tsp++;\n-\t\t\t\t\twrite_or_die(1, tagger, sp - tagger);\n-\t\t\t\t\tdate = strtoul(sp, &ep, 10);\n-\t\t\t\t\ttz = strtol(ep, NULL, 10);\n-\t\t\t\t\tsp = show_date(date, tz, 0);\n-\t\t\t\t\twrite_or_die(1, sp, strlen(sp));\n-\t\t\t\t\txwrite(1, \"\\n\", 1);\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (cp < endp && *cp == '\\n')\n-\t\t\t/* end of header */\n-\t\t\tbreak;\n-\t}\n-\t/* At this point, we have copied out the header up to the end of\n-\t * the tagger line and cp points at one past \\n. It could be the\n-\t * next header line after the tagger line, or it could be another\n-\t * \\n that marks the end of the headers. We need to copy out the\n-\t * remainder as is.\n-\t */\n-\tif (cp < endp)\n-\t\twrite_or_die(1, cp, endp - cp);\n-}\n-\n static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n {\n \tunsigned char sha1[20];\n@@ -133,10 +66,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \t\tbuf = read_sha1_file(sha1, &type, &size);\n \t\tif (!buf)\n \t\t\tdie(\"Cannot read object %s\", obj_name);\n-\t\tif (type == OBJ_TAG) {\n-\t\t\tpprint_tag(sha1, buf, size);\n-\t\t\treturn 0;\n-\t\t}\n \n \t\t/* otherwise just spit out the data */\n \t\tbreak;\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 9820f70..9cc5c6b 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -135,14 +135,11 @@ tag_size=$(strlen \"$tag_content\")\n tag_content=\"$tag_header_without_timestamp 0000000000 +0000\n \n $tag_description\"\n-tag_pretty_content=\"$tag_header_without_timestamp Thu Jan 1 00:00:00 1970 +0000\n-\n-$tag_description\"\n \n tag_sha1=$(echo_without_newline \"$tag_content\" | git mktag)\n tag_size=$(strlen \"$tag_content\")\n \n-run_tests 'tag' $tag_sha1 $tag_size \"$tag_content\" \"$tag_pretty_content\" 1\n+run_tests 'tag' $tag_sha1 $tag_size \"$tag_content\" \"$tag_content\" 1\n \n test_expect_success \\\n \"Reach a blob from a tag pointing to it\" \\\n-- \n1.8.2.11.g42401f0\n"},{"id":"214640","messageId":"20130417210713.GB635@sigill.intra.peff.net","threadId":"33520","inReplyTo":"516EEB12.5050209@lsrfire.ath.cx","subject":"Re: [PATCH] blame: handle broken commit headers gracefully","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-17T21:07:13Z","receivedAt":"2013-04-17T21:07:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 17, 2013 at 08:33:54PM +0200, René Scharfe wrote:\n\n> Minimal patch, test case missing. It's a bit sad that the old commit\n> parser of blame handled Ivan's specific corruption (extra \"-<>\" after\n> email) gracefully because it used the spaces as cutting points instead\n> of \"<\" and \">\".\n\nThat may mean there is room for improvement in split_ident_line to\nbe more resilient in removing cruft. With something like:\n\n Name -<> 123456789 -0000\n\nit would obviously be nice to find the date timestamp there, but I\nwonder what the \"email\" field should return? The full broken string, or\njust \"email@host\"? One way is convenient for overlooking problems in\nbroken commits, but I would worry about code paths that are using\nsplit_ident_line to verify the quality of the string (like\ndetermine_author_info). It's possible we would need a strict and a\nforgiving mode.\n\n-Peff\n"},{"id":"214642","messageId":"516F12A1.9050106@lsrfire.ath.cx","threadId":"33520","inReplyTo":"20130417210713.GB635@sigill.intra.peff.net","subject":"Re: [PATCH] blame: handle broken commit headers gracefully","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-04-17T21:22:41Z","receivedAt":"2013-04-17T21:22:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.04.2013 23:07, schrieb Jeff King:\n> On Wed, Apr 17, 2013 at 08:33:54PM +0200, René Scharfe wrote:\n>\n>> Minimal patch, test case missing. It's a bit sad that the old commit\n>> parser of blame handled Ivan's specific corruption (extra \"-<>\" after\n>> email) gracefully because it used the spaces as cutting points instead\n>> of \"<\" and \">\".\n>\n> That may mean there is room for improvement in split_ident_line to\n> be more resilient in removing cruft. With something like:\n>\n> Name -<> 123456789 -0000\n>\n> it would obviously be nice to find the date timestamp there, but I\n> wonder what the \"email\" field should return? The full broken string, or\n> just \"email@host\"? One way is convenient for overlooking problems in\n> broken commits, but I would worry about code paths that are using\n> split_ident_line to verify the quality of the string (like\n> determine_author_info). It's possible we would need a strict and a\n> forgiving mode.\n\nYou can have both; the necessary data is in the struct ident_split: Just \ncheck that *mail_end == '>' and mail_end + 1 == date_begin etc.\n\nRené\n"},{"id":"214645","messageId":"7vsj2oesa6.fsf@alter.siamese.dyndns.org","threadId":"33520","inReplyTo":"20130417210713.GB635@sigill.intra.peff.net","subject":"Re: [PATCH] blame: handle broken commit headers gracefully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-17T21:55:29Z","receivedAt":"2013-04-17T21:55:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Apr 17, 2013 at 08:33:54PM +0200, René Scharfe wrote:\n>\n>> Minimal patch, test case missing. It's a bit sad that the old commit\n>> parser of blame handled Ivan's specific corruption (extra \"-<>\" after\n>> email) gracefully because it used the spaces as cutting points instead\n>> of \"<\" and \">\".\n>\n> That may mean there is room for improvement in split_ident_line to\n> be more resilient in removing cruft. With something like:\n>\n> Name -<> 123456789 -0000\n>\n> it would obviously be nice to find the date timestamp there, but I\n> wonder what the \"email\" field should return? The full broken string, or\n> just \"email@host\"?\n\nOr you can imagine nastier input strings, like\n\n Name <>- 123456789 -0000\n Name - 123456789 -0000\n Name 56789 -0000\n\nI am afraid that at some point \"we should salvage as much as we\ncan\", which is a worthy goal, becomes a losing proposition.\n"},{"id":"214734","messageId":"20130418165600.GA24492@sigill.intra.peff.net","threadId":"33520","inReplyTo":"7vsj2oesa6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame: handle broken commit headers gracefully","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-18T16:56:01Z","receivedAt":"2013-04-18T16:56:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 17, 2013 at 02:55:29PM -0700, Junio C Hamano wrote:\n\n> Or you can imagine nastier input strings, like\n> \n> Name <>- 123456789 -0000\n> Name - 123456789 -0000\n> Name 56789 -0000\n> \n> I am afraid that at some point \"we should salvage as much as we\n> can\", which is a worthy goal, becomes a losing proposition.\n\nGood point. In the worst cases, even if you cleaned things up, you might\neven need to allocate a new string (like your middle one), which would\nmake calling split_ident_line a lot more annoying. Probably not worth\nthe effort.\n\n-Peff\n"},{"id":"214792","messageId":"CAPig+cSn1fQGwqrP6aoVbf9D_JzB30s3c_N_v8sc2VFPUyxfzg@mail.gmail.com","threadId":"33520","inReplyTo":"20130417210048.GA635@sigill.intra.peff.net","subject":"Re: [PATCH] cat-file: print tags raw for \"cat-file -p\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-04-19T03:03:30Z","receivedAt":"2013-04-19T03:03:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 17, 2013 at 5:00 PM, Jeff King wrote:\n> Subject: [PATCH] cat-file: print tags raw for \"cat-file -p\"\n>\n> When \"cat-file -p\" prints commits, it shows them in their\n> raw format, since git's format is already human-readable.\n> For tags, however, we print the whole thing raw except for\n> one thing: we convert the timestamp on the tagger line into a\n> human-readable date.\n> [...]\n> Let's drop the tagger-date formatting for \"cat-file -p\". It\n> makes us more consistent with cat-file's commit\n> pretty-printer, and as a bonus, we can drop the hand-rolled\n> tag parsing code in cat-file (which happened to behave\n> inconsistently with the tag pretty-printing code elsewhere).\n>\n> This is a change of output format, so it's possible that\n> some callers could considered this a regression. However,\n\ns/considered/consider/\n\n> the original behavior was arguably a bug (due to the\n> inconsistency with commits), likely nobody was relying on it\n> (even we do not use it ourselves these days), and anyone\n> relying on the \"-p\" pretty-printer should be able to expect\n> a change in the output format (i.e., while \"cat-file\" is\n> plumbing, the output format of \"-p\" was never guaranteed to\n> be stable).\n>\n> Signed-off-by: Jeff King \n"}]}