{"thread":{"id":"27157","subject":"[PATCH] blame: Improve parsing for emails with spaces","startedAt":"2011-04-21T22:07:36Z","lastAt":"2011-04-29T19:13:55Z","messageCount":6,"participants":["Josh Stone","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"166209","messageId":"1303423656-32002-1-git-send-email-jistone@redhat.com","threadId":"27157","inReplyTo":null,"subject":"[PATCH] blame: Improve parsing for emails with spaces","fromName":"Josh Stone","fromEmail":"jistone@redhat.com","sentAt":"2011-04-21T22:07:36Z","receivedAt":"2011-04-21T22:07:36Z","isPatch":true,"sender":{"key":"jistone@redhat.com","avatar":null},"body":"One of my git repositories has some old commits where the authors\nobfuscated their email address as . To\nhandle this, blame needs to look for the leading '<' when scanning\nto split the \"name \", rather then only a space delimiter.\n\nSigned-off-by: Josh Stone \n---\n builtin/blame.c | 2 +-\n t/annotate-tests.sh | 12 +++++++++++-\n t/t8002-blame.sh | 2 +-\n 3 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex f6b03f7..41525f1 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1377,7 +1377,7 @@ static void get_ac_line(const char *inbuf, const char *what,\n \ttimepos = tmp;\n \n \t*tmp = 0;\n-\twhile (person < tmp && *tmp != ' ')\n+\twhile (person < tmp && !(*tmp == ' ' && tmp[1] == '<'))\n \t\ttmp--;\n \tif (tmp <= person)\n \t\treturn;\ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex d34208c..abb1885 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -1,5 +1,5 @@\n # This file isn't used as a test script directly, instead it is\n-# sourced from t8001-annotate.sh and t8001-blame.sh.\n+# sourced from t8001-annotate.sh and t8002-blame.sh.\n \n check_count () {\n \thead=\n@@ -124,3 +124,13 @@ test_expect_success \\\n test_expect_success \\\n 'some edit' \\\n 'check_count A 1 B 1 B1 1 B2 1 \"A U Thor\" 1 C 1 D 1'\n+\n+test_expect_success \\\n+ 'an obfuscated email added' \\\n+ 'sed -e \"1i No robots allowed\" < file > file.new &&\n+ mv file.new file &&\n+ GIT_AUTHOR_NAME=\"E\" GIT_AUTHOR_EMAIL=\"E at test dot git\" git commit -a -m \"norobots\"'\n+\n+test_expect_success \\\n+ 'obfuscated email parsed' \\\n+ 'check_count A 1 B 1 B1 1 B2 1 \"A U Thor\" 1 C 1 D 1 E 1'\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex d3a51e1..e2896cf 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -8,7 +8,7 @@ PROG='git blame -c'\n \n PROG='git blame -c -e'\n test_expect_success 'Blame --show-email works' '\n- check_count \"\" 1 \"\" 1 \"\" 1 \"\" 1 \"\" 1 \"\" 1 \"\" 1\n+ check_count \"\" 1 \"\" 1 \"\" 1 \"\" 1 \"\" 1 \"\" 1 \"\" 1 \"\" 1\n '\n \n test_done\n-- \n1.7.4.4\n"},{"id":"166735","messageId":"20110429131103.GB4540@sigill.intra.peff.net","threadId":"27157","inReplyTo":"1303423656-32002-1-git-send-email-jistone@redhat.com","subject":"Re: [PATCH] blame: Improve parsing for emails with spaces","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-29T13:11:03Z","receivedAt":"2011-04-29T13:11:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 21, 2011 at 03:07:36PM -0700, Josh Stone wrote:\n\n> One of my git repositories has some old commits where the authors\n> obfuscated their email address as . To\n> handle this, blame needs to look for the leading '<' when scanning\n> to split the \"name \", rather then only a space delimiter.\n\nHmm. That address seems bogus, and I wonder if we should be rejecting it\nat commit time. Still, it is something we may run across in existing\nrepositories, so handling it more gracefully makes sense.\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index f6b03f7..41525f1 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1377,7 +1377,7 @@ static void get_ac_line(const char *inbuf, const char *what,\n> \ttimepos = tmp;\n> \n> \t*tmp = 0;\n> -\twhile (person < tmp && *tmp != ' ')\n> +\twhile (person < tmp && !(*tmp == ' ' && tmp[1] == '<'))\n> \t\ttmp--;\n> \tif (tmp <= person)\n> \t\treturn;\n\nThe fix looks fine. It's a little gross that we are parsing the commit\nheaders ourselves here, as opposed to using some library function\nprovided for commits. I think we could eliminate some code by using\nthe parser in pretty.c, but it might need some refactoring to do so\nefficiently.\n\nLooking over the other places we parse author info, I don't think the\nsame problem exists elsewhere. Most other places parse from left to\nright, and just go to the closing \">\".\n\n-Peff\n"},{"id":"166766","messageId":"7vvcxxvsz4.fsf@alter.siamese.dyndns.org","threadId":"27157","inReplyTo":"20110429131103.GB4540@sigill.intra.peff.net","subject":"Re: [PATCH] blame: Improve parsing for emails with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-29T17:59:43Z","receivedAt":"2011-04-29T17:59:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Apr 21, 2011 at 03:07:36PM -0700, Josh Stone wrote:\n>\n>> One of my git repositories has some old commits where the authors\n>> obfuscated their email address as . To\n>> handle this, blame needs to look for the leading '<' when scanning\n>> to split the \"name \", rather then only a space delimiter.\n\nGiven that we enclose the e-mail inside \"<>\" pair and excise \"<\" from\nauthor names in fmt_ident(), I think it makes sense to look for \" <\" like\nthis patch does.\n\n> Hmm. That address seems bogus, and I wonder if we should be rejecting it\n> at commit time. Still, it is something we may run across in existing\n> repositories, so handling it more gracefully makes sense.\n\nPerhaps but within reason. \n\nWhat new types of breakages are we proposing to tolerate, what breakages\nare we declaring not worth fixing, and what is the price of not loosening\nthis? Without this patch, such a broken commit will result in the author\nemail shown somewhat broken, but the original is already broken to begin\nwith, and also the entry for the blamed line will come with its commit\nobject name anyway, so I do not think it is such a big deal.\n\nIt would be an entirely different issue if the command barfed and refused\nto blame the file.\n\n> Looking over the other places we parse author info, I don't think the\n> same problem exists elsewhere. Most other places parse from left to\n> right, and just go to the closing \">\".\n\nBecause fmt_ident() removes \"<\" or \">\" from given email address, I think\nit is sufficient.\n\nIt would be nice to have an abstraction around the parsing in a way\nsimilar to fmt_ident() is an abstraction around the formatting, but that\nwould be a separate topic.\n"},{"id":"166767","messageId":"7vr58lvsj4.fsf@alter.siamese.dyndns.org","threadId":"27157","inReplyTo":"7vvcxxvsz4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame: Improve parsing for emails with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-29T18:09:19Z","receivedAt":"2011-04-29T18:09:19Z","isPatch":true,"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 Thu, Apr 21, 2011 at 03:07:36PM -0700, Josh Stone wrote:\n>>\n>>> One of my git repositories has some old commits where the authors\n>>> obfuscated their email address as . To\n>>> handle this, blame needs to look for the leading '<' when scanning\n>>> to split the \"name \", rather then only a space delimiter.\n>\n> Given that we enclose the e-mail inside \"<>\" pair and excise \"<\" from\n> author names in fmt_ident(), I think it makes sense to look for \" <\" like\n> this patch does.\n\nWill queue. Thanks.\n\nAuthor: Josh Stone \nDate: Thu Apr 21 15:07:36 2011 -0700\n\n blame: tolerate bogus e-mail addresses a bit better\n \n The names and e-mails are sanitized by fmt_ident() when creating commits,\n so that they do not contain \"<\" nor \">\", and the \"committer\" and \"author\"\n lines in the commit object will always be in the form:\n \n (\"author\" | \"committer\") name SP \"<\" email \">\" SP timestamp SP zone\n \n When parsing the email part out, the current code looks for SP starting\n from the end of the email part, but the author could obfuscate the address\n as \"author at example dot com\".\n \n We should instead look for SP followed by \"<\", to match the logic of the\n side that formats these lines.\n \n Signed-off-by: Josh Stone \n Signed-off-by: Junio C Hamano \n"},{"id":"166768","messageId":"4DBB00CC.3030400@redhat.com","threadId":"27157","inReplyTo":"7vvcxxvsz4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame: Improve parsing for emails with spaces","fromName":"Josh Stone","fromEmail":"jistone@redhat.com","sentAt":"2011-04-29T18:17:48Z","receivedAt":"2011-04-29T18:17:48Z","isPatch":true,"sender":{"key":"jistone@redhat.com","avatar":null},"body":"On 04/29/2011 10:59 AM, Junio C Hamano wrote:\n> It would be an entirely different issue if the command barfed and refused\n> to blame the file.\n\nYeah - the command doesn't fail outright. Anything that builds on\nblame/annotate would get bad data, but for me it's more of a formatting\nissue. In my actual bad commit, the author's \"name > Hmm. That address seems bogus, and I wonder if we should be rejecting it\n> > at commit time. Still, it is something we may run across in existing\n> > repositories, so handling it more gracefully makes sense.\n> \n> Perhaps but within reason. \n> \n> What new types of breakages are we proposing to tolerate, what breakages\n> are we declaring not worth fixing, and what is the price of not loosening\n> this? Without this patch, such a broken commit will result in the author\n> email shown somewhat broken, but the original is already broken to begin\n> with, and also the entry for the blamed line will come with its commit\n> object name anyway, so I do not think it is such a big deal.\n\nI'm pretty sure such an address would make a non-rfc822-compliant \"from\"\nheader when used with format-patch. But given that it is obviously a\nbogus address, I don't think there's much we can do anyway, and anyone\nlooking at will say \"Oh, that's wrong\". So it's probably not a big deal.\n\n-Peff\n"}]}