{"thread":{"id":"34786","subject":"[PATCHv2] mailmap: handle mailmap blobs without trailing newlines","startedAt":"2013-08-28T01:41:39Z","lastAt":"2013-08-28T19:39:00Z","messageCount":2,"participants":["Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"226075","messageId":"20130828014138.GA30623@sigill.intra.peff.net","threadId":"34786","inReplyTo":null,"subject":"[PATCHv2] mailmap: handle mailmap blobs without trailing newlines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-28T01:41:39Z","receivedAt":"2013-08-28T01:41:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The read_mailmap_buf function reads each line of the mailmap\nusing strchrnul, like:\n\n    const char *end = strchrnul(buf, '\\n');\n    unsigned long linelen = end - buf + 1;\n\nBut that's off-by-one when we actually hit the NUL byte; our\nline does not have a terminator, and so is only \"end - buf\"\nbytes long. As a result, when we subtract the linelen from\nthe total len, we end up with (unsigned long)-1 bytes left\nin the buffer, and we start reading random junk from memory.\n\nWe could fix it with:\n\n    unsigned long linelen = end - buf + !!*end;\n\nbut let's take a step back for a moment. It's questionable\nin the first place for a function that takes a buffer and\nlength to be using strchrnul. But it works because we only\nhave one caller (and are only likely to ever have this one),\nwhich is handing us data from read_sha1_file. Which means\nthat it's always NUL-terminated.\n\nInstead of tightening the assumptions to make the\nbuffer/length pair work for a caller that doesn't actually\nexist, let's let loosen the assumptions to what the real\ncaller has: a modifiable, NUL-terminated string.\n\nThis makes the code simpler and shorter (because we don't\nhave to correlate strchrnul with the length calculation),\ncorrect (because the code with the off-by-one just goes\naway), and more efficient (we can drop the extra allocation\nwe needed to create NUL-terminated strings for each line,\nand just terminate in place).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is the squashed version to replace what's in\njk/mailmap-incomplete-line.\n\n mailmap.c          | 21 +++++++++------------\n t/t4203-mailmap.sh | 16 +++++++++++++++-\n 2 files changed, 24 insertions(+), 13 deletions(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex b16542f..caa7c6b 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -187,20 +187,17 @@ static void read_mailmap_buf(struct string_list *map,\n \treturn 0;\n }\n \n-static void read_mailmap_buf(struct string_list *map,\n-\t\t\t     const char *buf, unsigned long len,\n-\t\t\t     char **repo_abbrev)\n+static void read_mailmap_string(struct string_list *map, char *buf,\n+\t\t\t\tchar **repo_abbrev)\n {\n-\twhile (len) {\n-\t\tconst char *end = strchrnul(buf, '\\n');\n-\t\tunsigned long linelen = end - buf + 1;\n-\t\tchar *line = xmemdupz(buf, linelen);\n+\twhile (*buf) {\n+\t\tchar *end = strchrnul(buf, '\\n');\n \n-\t\tread_mailmap_line(map, line, repo_abbrev);\n+\t\tif (*end)\n+\t\t\t*end++ = '\\0';\n \n-\t\tfree(line);\n-\t\tbuf += linelen;\n-\t\tlen -= linelen;\n+\t\tread_mailmap_line(map, buf, repo_abbrev);\n+\t\tbuf = end;\n \t}\n }\n \n@@ -224,7 +221,7 @@ static int read_mailmap_blob(struct string_list *map,\n \tif (type != OBJ_BLOB)\n \t\treturn error(\"mailmap is not a blob: %s\", name);\n \n-\tread_mailmap_buf(map, buf, size, repo_abbrev);\n+\tread_mailmap_string(map, buf, repo_abbrev);\n \n \tfree(buf);\n \treturn 0;\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex aae30d9..10c7b12 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -159,7 +159,8 @@ test_expect_success 'setup mailmap blob tests' '\n \tBlob Guy <author@example.com>\n \tBlob Guy <bugs@company.xx>\n \tEOF\n-\tgit add just-bugs both &&\n+\tprintf \"Tricky Guy <author@example.com>\" >no-newline &&\n+\tgit add just-bugs both no-newline &&\n \tgit commit -m \"my mailmaps\" &&\n \techo \"Repo Guy <author@example.com>\" >.mailmap &&\n \techo \"Internal Guy <author@example.com>\" >internal.map\n@@ -243,6 +244,19 @@ test_expect_success 'mailmap.blob defaults to HEAD:.mailmap in bare repo' '\n \t)\n '\n \n+test_expect_success 'mailmap.blob can handle blobs without trailing newline' '\n+\tcat >expect <<-\\EOF &&\n+\tTricky Guy (1):\n+\t      initial\n+\n+\tnick1 (1):\n+\t      second\n+\n+\tEOF\n+\tgit -c mailmap.blob=map:no-newline shortlog HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cleanup after mailmap.blob tests' '\n \trm -f .mailmap\n '\n-- \n1.8.4.2.g87d4a77\n"},{"id":"226105","messageId":"xmqq61upa9m3.fsf@gitster.dls.corp.google.com","threadId":"34786","inReplyTo":"20130828014138.GA30623@sigill.intra.peff.net","subject":"Re: [PATCHv2] mailmap: handle mailmap blobs without trailing newlines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-28T19:39:00Z","receivedAt":"2013-08-28T19:39:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This makes the code simpler and shorter (because we don't\n> have to correlate strchrnul with the length calculation),\n> correct (because the code with the off-by-one just goes\n> away), and more efficient (we can drop the extra allocation\n> we needed to create NUL-terminated strings for each line,\n> and just terminate in place).\n\nNice.  Will queue.  Thanks.\n"}]}