{"thread":{"id":"34418","subject":"Bug in .mailmap handling?","startedAt":"2013-07-12T16:07:21Z","lastAt":"2013-07-12T21:13:13Z","messageCount":8,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"223166","messageId":"51E029B9.20108@googlemail.com","threadId":"34418","inReplyTo":null,"subject":"Bug in .mailmap handling?","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-12T16:07:21Z","receivedAt":"2013-07-12T16:07:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hello,\n\nyou may have noticed I am currently trying to bring the \nmailmap file of git itself up to date. I noticed\nsome behavior, which I did not expect. Have a look yourself:\n\n---\n\t# prepare test environment:\n\tmkdir testmailmap\n\tcd testmailmap/\n\tgit init\n\n\t# do a commit:\n\techo \"asdf\" > test1 \n\tgit add test1\n\tgit commit -a --author=\"A \" -m \"add test1\"\n\n\t# commit with same name, but different email \n\t# (different capitalization does the trick already, \n\t# but here I am going to use a different mail)\n\techo \"asdf\" > test2\n\tgit add test2\n\tgit commit -a --author=\"A \" -m \"add test2\"\n\n\t# how do we know it's the same person?\n\tgit shortlog\n\tA (2):\n\t\t add test1\n\t\t add test2\n\n\t# reports as expected:\n\tgit shortlog -sne\n\t\t 1 A \n\t\t 1 A \n\t\t \n\t# Adding the line to the mailmap should make life easy, so we know\n\t# it's the same person\n\techo \"A \" > .mailmap\n\n\t# Come on, I just wanted to have it reported as one person!\n\tgit shortlog -sne\n\t\t 1 A \n\t\t 1 A \n\t\t \n\t# So let's try another line in the mailmap file, (small 'a')\n\techo \"A \" > .mailmap\n\n\t# We're not there yet?\n\tgit shortlog -sne\n\t\t 1 A \n\t\t 1 A \n\n\t# Now let's write it rather explicit: \n\t# (essentially just write 2 lines into the mailmap file)\n\tcat << EOF > .mailmap\n\tA \n\tA \n\tEOF\n\t\t \n\t# works as expected now\n\tgit shortlog -sne\n\t\t 2 A \n\n\t# works as expected now as well\n\tgit shortlog \n\tA (2):\n\t\t add test1\n\t\t add test2\n"},{"id":"223196","messageId":"7vppunllzf.fsf@alter.siamese.dyndns.org","threadId":"34418","inReplyTo":"51E029B9.20108@googlemail.com","subject":"Re: Bug in .mailmap handling?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-12T17:35:48Z","receivedAt":"2013-07-12T17:35:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Hello,\n>\n> you may have noticed I am currently trying to bring the \n> mailmap file of git itself up to date. I noticed\n> some behavior, which I did not expect. Have a look yourself:\n>\n> ---\n> \t# prepare test environment:\n> \tmkdir testmailmap\n> \tcd testmailmap/\n> \tgit init\n>\n> \t# do a commit:\n> \techo \"asdf\" > test1 \n> \tgit add test1\n> \tgit commit -a --author=\"A \" -m \"add test1\"\n>\n> \t# commit with same name, but different email \n> \t# (different capitalization does the trick already, \n> \t# but here I am going to use a different mail)\n> \techo \"asdf\" > test2\n> \tgit add test2\n> \tgit commit -a --author=\"A \" -m \"add test2\"\n>\n> \t# how do we know it's the same person?\n> \tgit shortlog\n> \tA (2):\n> \t\t add test1\n> \t\t add test2\n\nYou don't, and it is a long known behaviour.\n\n> \t# reports as expected:\n> \tgit shortlog -sne\n> \t\t 1 A \n> \t\t 1 A \n\nYes.\n\n> \t# Adding the line to the mailmap should make life easy, so we know\n> \t# it's the same person\n> \techo \"A \" > .mailmap\n>\n> \t# Come on, I just wanted to have it reported as one person!\n> \tgit shortlog -sne\n> \t\t 1 A \n> \t\t 1 A \n\nErr, where does the lowercase a@ come from in the above? Are we\nmissing some steps before we get here?\n\n> \t# So let's try another line in the mailmap file, (small 'a')\n> \techo \"A \" > .mailmap\n\nThis is \">\", not \">>\", I presume? Otherwise changed_email is mapped\nto two destination, no?\n\n> \t# We're not there yet?\n> \tgit shortlog -sne\n> \t\t 1 A \n> \t\t 1 A \n\nExpected, as long as some hidden set-up you did not describe that\ncaused me to say \"Err, where does the lowercase a@ come from\" is\nthere, i.e. one of the two commits is done by .\n\n> \t# Now let's write it rather explicit: \n> \t# (essentially just write 2 lines into the mailmap file)\n> \tcat << EOF > .mailmap\n> \tA \n> \tA \n> \tEOF\n> \t\t \n> \t# works as expected now\n> \tgit shortlog -sne\n> \t\t 2 A \n\nMakes sense.\n\n> \t# works as expected now as well\n> \tgit shortlog \n> \tA (2):\n> \t\t add test1\n> \t\t add test2\n"},{"id":"223198","messageId":"51E04125.5040003@googlemail.com","threadId":"34418","inReplyTo":"7vppunllzf.fsf@alter.siamese.dyndns.org","subject":"Re: Bug in .mailmap handling?","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-12T17:47:17Z","receivedAt":"2013-07-12T17:47:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The commands are exactly as given in the first mail.\n(I run it multiple times, and this exact behavior comes up)\n\nI think there is some problem with the capitalisation\nof the first (if not all) characters. (Hence the small 'a')\n\nSo either check with these example commands yourself, or take my latest\npatch for the mailmap to reproduce on real names, please.\n\nOn 07/12/2013 07:35 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n> \n>> Hello,\n>>\n>> you may have noticed I am currently trying to bring the \n>> mailmap file of git itself up to date. I noticed\n>> some behavior, which I did not expect. Have a look yourself:\n>>\n>> ---\n>> \t# prepare test environment:\n>> \tmkdir testmailmap\n>> \tcd testmailmap/\n>> \tgit init\n>>\n>> \t# do a commit:\n>> \techo \"asdf\" > test1 \n>> \tgit add test1\n>> \tgit commit -a --author=\"A \" -m \"add test1\"\n>>\n>> \t# commit with same name, but different email \n>> \t# (different capitalization does the trick already, \n>> \t# but here I am going to use a different mail)\n>> \techo \"asdf\" > test2\n>> \tgit add test2\n>> \tgit commit -a --author=\"A \" -m \"add test2\"\n>>\n>> \t# how do we know it's the same person?\n>> \tgit shortlog\n>> \tA (2):\n>> \t\t add test1\n>> \t\t add test2\n> \n> You don't, and it is a long known behaviour.\n> \n>> \t# reports as expected:\n>> \tgit shortlog -sne\n>> \t\t 1 A \n>> \t\t 1 A \n> \n> Yes.\n> \n>> \t# Adding the line to the mailmap should make life easy, so we know\n>> \t# it's the same person\n>> \techo \"A \" > .mailmap\n>>\n>> \t# Come on, I just wanted to have it reported as one person!\n>> \tgit shortlog -sne\n>> \t\t 1 A \n>> \t\t 1 A \n> \n> Err, where does the lowercase a@ come from in the above? Are we\n> missing some steps before we get here?\n> \n>> \t# So let's try another line in the mailmap file, (small 'a')\n>> \techo \"A \" > .mailmap\n> \n> This is \">\", not \">>\", I presume? Otherwise changed_email is mapped\n> to two destination, no?\n> \n>> \t# We're not there yet?\n>> \tgit shortlog -sne\n>> \t\t 1 A \n>> \t\t 1 A \n> \n> Expected, as long as some hidden set-up you did not describe that\n> caused me to say \"Err, where does the lowercase a@ come from\" is\n> there, i.e. one of the two commits is done by .\n> \n>> \t# Now let's write it rather explicit: \n>> \t# (essentially just write 2 lines into the mailmap file)\n>> \tcat << EOF > .mailmap\n>> \tA \n>> \tA \n>> \tEOF\n>> \t\t \n>> \t# works as expected now\n>> \tgit shortlog -sne\n>> \t\t 2 A \n> \n> Makes sense.\n> \n>> \t# works as expected now as well\n>> \tgit shortlog \n>> \tA (2):\n>> \t\t add test1\n>> \t\t add test2\n\n\n"},{"id":"223213","messageId":"7vfvvjjzee.fsf@alter.siamese.dyndns.org","threadId":"34418","inReplyTo":"51E029B9.20108@googlemail.com","subject":"Re: Bug in .mailmap handling?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-12T20:28:57Z","receivedAt":"2013-07-12T20:28:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> \t# Adding the line to the mailmap should make life easy, so we know\n> \t# it's the same person\n> \techo \"A \" > .mailmap\n\nWhile I was looking at this, I noticed this piece of code:\n\ndiff --git a/mailmap.c b/mailmap.c\nindex 2a7b366..418081e 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -122,7 +122,7 @@ static char *parse_name_and_email(char *buffer, char **name,\n \twhile (nend > nstart && isspace(*nend))\n \t\t--nend;\n \n-\t*name = (nstart < nend ? nstart : NULL);\n+\t*name = (nstart <= nend ? nstart : NULL);\n \t*email = left+1;\n \t*(nend+1) = '\\0';\n \t*right++ = '\\0';\n\nThe function is given a buffer \"A ...\", nstart scans\nfrom the beginning of the buffer, skipping whitespaces (there isn't\nany, so nstart points at the buffer), while nend starts from one\nbyte before the first '<' and skips whitespaces backwards and ends\nat the first non-whitespace (i.e. it hits \"A\" at the beginning of\nthe buffer). nstart == nend in this case for a single-letter name,\nand an off-by-one error makes it fail to pick up the name, which\nmakes the entry equivalent to\n\n\t \n\nwithout the name. I do not think this bug affected anything you\nobserved, though.\n\n> \tgit shortlog -sne\n> \t\t 1 A \n> \t\t 1 A \n\nThis is coming from mailmap.c::add_mapping() that downcases the\ne-mail address.\n\nchanged_email@example.org is mapped to a@example.org because of this\ndowncasing, while \"A \" does not have any entry for it\nin the .mailmap file, so it is given back as-is. Hence we see two\ndistinct entries.\n"},{"id":"223214","messageId":"51E06898.3070303@googlemail.com","threadId":"34418","inReplyTo":"7vfvvjjzee.fsf@alter.siamese.dyndns.org","subject":"Re: Bug in .mailmap handling?","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-12T20:35:36Z","receivedAt":"2013-07-12T20:35:36Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 07/12/2013 10:28 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n> \n>> \t# Adding the line to the mailmap should make life easy, so we know\n>> \t# it's the same person\n>> \techo \"A \" > .mailmap\n> \n> While I was looking at this, I noticed this piece of code:\n> \n> diff --git a/mailmap.c b/mailmap.c\n> index 2a7b366..418081e 100644\n> --- a/mailmap.c\n> +++ b/mailmap.c\n> @@ -122,7 +122,7 @@ static char *parse_name_and_email(char *buffer, char **name,\n> \twhile (nend > nstart && isspace(*nend))\n> \t\t--nend;\n> \n> -\t*name = (nstart < nend ? nstart : NULL);\n> +\t*name = (nstart <= nend ? nstart : NULL);\n> \t*email = left+1;\n> \t*(nend+1) = '\\0';\n> \t*right++ = '\\0';\n> \n> The function is given a buffer \"A ...\", nstart scans\n> from the beginning of the buffer, skipping whitespaces (there isn't\n> any, so nstart points at the buffer), while nend starts from one\n> byte before the first '<' and skips whitespaces backwards and ends\n> at the first non-whitespace (i.e. it hits \"A\" at the beginning of\n> the buffer). nstart == nend in this case for a single-letter name,\n> and an off-by-one error makes it fail to pick up the name, which\n> makes the entry equivalent to\n> \n> \t \n> \n> without the name. I do not think this bug affected anything you\n> observed, though.\n> \n>> \tgit shortlog -sne\n>> \t\t 1 A \n>> \t\t 1 A \n> \n> This is coming from mailmap.c::add_mapping() that downcases the\n> e-mail address.\n> \n> changed_email@example.org is mapped to a@example.org because of this\n> downcasing, while \"A \" does not have any entry for it\n> in the .mailmap file, so it is given back as-is. Hence we see two\n> distinct entries.\n> \n\nSo do I understand it right, we're having 2 bugs in here?\n\nOne being triggered by the short name, only one character.\nSo if you want to debug the other bug with a longer name,\nyou can either use a made up name, or run\n git shortlog -sne |grep Knut\nin the git repository having the mailmap file already updated.\nThe way the mailmap file is written, I'd assume only one line\nto be found, as of now 2 lines come up\n 2\tKnut Franke \n 1\tKnut Franke \n\nwhich seems to downcase the whole first email.\n"},{"id":"223215","messageId":"7v8v1bjyyy.fsf@alter.siamese.dyndns.org","threadId":"34418","inReplyTo":"7vfvvjjzee.fsf@alter.siamese.dyndns.org","subject":"Re: Bug in .mailmap handling?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-12T20:38:13Z","receivedAt":"2013-07-12T20:38:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> \tgit shortlog -sne\n>> \t\t 1 A \n>> \t\t 1 A \n>\n> This is coming from mailmap.c::add_mapping() that downcases the\n> e-mail address.\n>\n> changed_email@example.org is mapped to a@example.org because of this\n> downcasing, while \"A \" does not have any entry for it\n> in the .mailmap file, so it is given back as-is. Hence we see two\n> distinct entries.\n\nI think it is wrong for the parser to lose information by\ndowncasing.\n\nIt is perfectly fine to do the comparison case insensitively, to\nallow in the input is mangled using the\nentry for in the .mailmap file; it is\nnot fine to downcase the parsed result, especially the side that is\nused as the \"rewritten result\" (i.e. the tokens earlier on the line),\nas that would mean mangling the output.\n\nLet me see if I can quickly whip up a fix.\n"},{"id":"223218","messageId":"7v4nbzjyel.fsf@alter.siamese.dyndns.org","threadId":"34418","inReplyTo":"7v8v1bjyyy.fsf@alter.siamese.dyndns.org","subject":"Re: Bug in .mailmap handling?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-12T20:50:26Z","receivedAt":"2013-07-12T20:50:26Z","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>> Stefan Beller writes:\n>>\n>>> \tgit shortlog -sne\n>>> \t\t 1 A \n>>> \t\t 1 A \n>>\n>> This is coming from mailmap.c::add_mapping() that downcases the\n>> e-mail address.\n>>\n>> changed_email@example.org is mapped to a@example.org because of this\n>> downcasing, while \"A \" does not have any entry for it\n>> in the .mailmap file, so it is given back as-is. Hence we see two\n>> distinct entries.\n>\n> I think it is wrong for the parser to lose information by\n> downcasing.\n>\n> It is perfectly fine to do the comparison case insensitively, to\n> allow in the input is mangled using the\n> entry for in the .mailmap file; it is\n> not fine to downcase the parsed result, especially the side that is\n> used as the \"rewritten result\" (i.e. the tokens earlier on the line),\n> as that would mean mangling the output.\n>\n> Let me see if I can quickly whip up a fix.\n\nIt might be just the matter of doing this.\n\nI suspect that we could drop the downcasing of old-email, too, but\nthe new-email is supposed to be the rewritten result that appears on\nthe output, and downcasing it is very wrong.\n\nI also suspect that this was an old workaround for the original\nstring-list that did not know how to match items case insensitively,\nwhich we should have removed when we added case sensitivity support\nto the string-list at around 577f63e7 (Merge branch\n'ap/log-mailmap', 2013-01-20).\n\n mailmap.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex 418081e..c64a53d 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -56,9 +56,6 @@ static void add_mapping(struct string_list *map,\n \tif (old_email)\n \t\tfor (p = old_email; *p; p++)\n \t\t\t*p = tolower(*p);\n-\tif (new_email)\n-\t\tfor (p = new_email; *p; p++)\n-\t\t\t*p = tolower(*p);\n \n \tif (old_email == NULL) {\n \t\told_email = new_email;\n"},{"id":"223220","messageId":"1373663593-20346-1-git-send-email-stefanbeller@googlemail.com","threadId":"34418","inReplyTo":"7v4nbzjyel.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Add a testcase for checking case insensitivity of mail map","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-12T21:13:13Z","receivedAt":"2013-07-12T21:13:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Your patch seems to do it.\nI added a test case which doesn't fail, if your patch is added.\n\nSigned-off-by: Stefan Beller \n\n---\n t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++\n 1 file changed, 33 insertions(+)\n\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex 842b754..07152e9 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -409,4 +409,37 @@ test_expect_success 'Blame output (complex mapping)' '\n \ttest_cmp expect actual.fuzz\n '\n \n+test_expect_success 'cleanup after mailmap.blob tests' '\n+\trm -f .mailmap\n+'\n+\n+cat >expect <<\\EOF\n+ 2\tA \n+ 2\tOther Author \n+ 2\tSanta Claus \n+ 1\tA U Thor \n+ 1\tCTO \n+ 1\tSome Dude \n+EOF\n+\n+test_expect_success 'Test case sensitivity of Names' '\n+\t# do a commit:\n+\techo \"asdf\" > test1\n+\tgit add test1\n+\tgit commit -a --author=\"A \" -m \"add test1\"\n+\n+\t# commit with same name, but different email\n+\t# (different capitalization does the trick already,\n+\t# but here I am going to use a different mail)\n+\techo \"asdf\" > test2\n+\tgit add test2\n+\tgit commit -a --author=\"A \" -m \"add test2\"\n+\n+\t# Adding the line to the mailmap should make life easy, so we know\n+\t# it is the same person\n+\techo \"A \" > .mailmap\n+\n+\tgit shortlog -sne HEAD >actual && test_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.3.2.776.gfcf213d\n"}]}