Re: [PATCHv2] mailmap: handle mailmap blobs without trailing newlines

2013-08-28 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 This makes the code simpler and shorter (because we don't
 have to correlate strchrnul with the length calculation),
 correct (because the code with the off-by-one just goes
 away), and more efficient (we can drop the extra allocation
 we needed to create NUL-terminated strings for each line,
 and just terminate in place).

Nice.  Will queue.  Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCHv2] mailmap: handle mailmap blobs without trailing newlines

2013-08-27 Thread Jeff King
The read_mailmap_buf function reads each line of the mailmap
using strchrnul, like:

const char *end = strchrnul(buf, '\n');
unsigned long linelen = end - buf + 1;

But that's off-by-one when we actually hit the NUL byte; our
line does not have a terminator, and so is only end - buf
bytes long. As a result, when we subtract the linelen from
the total len, we end up with (unsigned long)-1 bytes left
in the buffer, and we start reading random junk from memory.

We could fix it with:

unsigned long linelen = end - buf + !!*end;

but let's take a step back for a moment. It's questionable
in the first place for a function that takes a buffer and
length to be using strchrnul. But it works because we only
have one caller (and are only likely to ever have this one),
which is handing us data from read_sha1_file. Which means
that it's always NUL-terminated.

Instead of tightening the assumptions to make the
buffer/length pair work for a caller that doesn't actually
exist, let's let loosen the assumptions to what the real
caller has: a modifiable, NUL-terminated string.

This makes the code simpler and shorter (because we don't
have to correlate strchrnul with the length calculation),
correct (because the code with the off-by-one just goes
away), and more efficient (we can drop the extra allocation
we needed to create NUL-terminated strings for each line,
and just terminate in place).

Signed-off-by: Jeff King p...@peff.net
---
This is the squashed version to replace what's in
jk/mailmap-incomplete-line.

 mailmap.c  | 21 +
 t/t4203-mailmap.sh | 16 +++-
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index b16542f..caa7c6b 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -187,20 +187,17 @@ static void read_mailmap_buf(struct string_list *map,
return 0;
 }
 
-static void read_mailmap_buf(struct string_list *map,
-const char *buf, unsigned long len,
-char **repo_abbrev)
+static void read_mailmap_string(struct string_list *map, char *buf,
+   char **repo_abbrev)
 {
-   while (len) {
-   const char *end = strchrnul(buf, '\n');
-   unsigned long linelen = end - buf + 1;
-   char *line = xmemdupz(buf, linelen);
+   while (*buf) {
+   char *end = strchrnul(buf, '\n');
 
-   read_mailmap_line(map, line, repo_abbrev);
+   if (*end)
+   *end++ = '\0';
 
-   free(line);
-   buf += linelen;
-   len -= linelen;
+   read_mailmap_line(map, buf, repo_abbrev);
+   buf = end;
}
 }
 
@@ -224,7 +221,7 @@ static int read_mailmap_blob(struct string_list *map,
if (type != OBJ_BLOB)
return error(mailmap is not a blob: %s, name);
 
-   read_mailmap_buf(map, buf, size, repo_abbrev);
+   read_mailmap_string(map, buf, repo_abbrev);
 
free(buf);
return 0;
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index aae30d9..10c7b12 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -159,7 +159,8 @@ test_expect_success 'setup mailmap blob tests' '
Blob Guy aut...@example.com
Blob Guy b...@company.xx
EOF
-   git add just-bugs both 
+   printf Tricky Guy aut...@example.com no-newline 
+   git add just-bugs both no-newline 
git commit -m my mailmaps 
echo Repo Guy aut...@example.com .mailmap 
echo Internal Guy aut...@example.com internal.map
@@ -243,6 +244,19 @@ test_expect_success 'mailmap.blob defaults to 
HEAD:.mailmap in bare repo' '
)
 '
 
+test_expect_success 'mailmap.blob can handle blobs without trailing newline' '
+   cat expect -\EOF 
+   Tricky Guy (1):
+ initial
+
+   nick1 (1):
+ second
+
+   EOF
+   git -c mailmap.blob=map:no-newline shortlog HEAD actual 
+   test_cmp expect actual
+'
+
 test_expect_success 'cleanup after mailmap.blob tests' '
rm -f .mailmap
 '
-- 
1.8.4.2.g87d4a77
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html