[PATCH v2 0/9] mailmap fixes

[PATCH v2 0/9] mailmap fixes

[Eric Sunshine]

This is a re-roll of jc/mailmap-case-insensitivity [1] in 'pu', which
fixes an off-by-one parsing bug and a case-losing bug in mailmap.

Changes since v1:

* Replace v1 [4/4] with new [1/9] and [3/9] which explicitly demonstrate
  each bug being fixed.

* Fix several crashes and issues in mailmap debug code (new patches
  [5/9] through [8/9].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/230258/

Eric Sunshine (6):
  t4203: demonstrate loss of single-character name in mailmap entry
  t4203: demonstrate loss of uppercase characters in canonical email
  mailmap: debug: fix out-of-order fprintf() arguments
  mailmap: debug: fix malformed fprintf() format directive
  mailmap: debug: eliminate -Wformat field precision type warning
  mailmap: debug: avoid passing NULL to fprintf() '%s' directive

Junio C Hamano (3):
  mailmap: do not lose single-letter names
  mailmap: do not downcase mailmap entries
  mailmap: style fixes

 mailmap.c          | 67 +++++++++++++++++++++++++++++++-----------------------
 t/t4203-mailmap.sh | 18 +++++++++++++++
 2 files changed, 57 insertions(+), 28 deletions(-)

-- 
1.8.3.2.804.g0da7a53

[PATCH v2 1/9] t4203: demonstrate loss of single-character name in mailmap entry

[Eric Sunshine]

A bug in mailmap.c:parse_name_and_email() causes it to overlook the
single-character name in "A <user@host>" and parse it only as
"<user@host>". Demonstrate this problem.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4203-mailmap.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 842b754..27f8f86 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -247,6 +247,15 @@ test_expect_success 'cleanup after mailmap.blob tests' '
 	rm -f .mailmap
 '
 
+test_expect_failure 'single-character name' '
+	echo "     1	A <author@example.com>" >expect &&
+	echo "     1	nick1 <bugs@company.xx>" >>expect &&
+	echo "A <author@example.com>" >.mailmap &&
+	test_when_finished "rm .mailmap" &&
+	git shortlog -es HEAD >actual &&
+	test_cmp expect actual
+'
+
 # Extended mailmap configurations should give us the following output for shortlog
 cat >expect <<\EOF
 A U Thor <author@example.com> (1):
-- 
1.8.3.2.804.g0da7a53

[PATCH v2 2/9] mailmap: do not lose single-letter names

[Eric Sunshine]

From: Junio C Hamano <gitster@pobox.com>

In parse_name_and_email() function, there is this line:

	*name = (nstart < nend ? nstart : NULL);

When the function is given a buffer "A <A@example.org> <old@x.z>",
nstart scans from the beginning of the buffer, skipping whitespaces
(there isn't any, so nstart points at the buffer), while nend starts
from one byte before the first '<' and skips whitespaces backwards
and stops at the first non-whitespace (i.e. it hits "A" at the
beginning of the buffer).  nstart == nend in this case for a
single-letter name, and an off-by-one error makes it fail to pick up
the name, which makes the entry equivalent to

	<A@example.org> <old@x.z>

without the name.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 mailmap.c          | 2 +-
 t/t4203-mailmap.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 2a7b366..418081e 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -122,7 +122,7 @@ static char *parse_name_and_email(char *buffer, char **name,
 	while (nend > nstart && isspace(*nend))
 		--nend;
 
-	*name = (nstart < nend ? nstart : NULL);
+	*name = (nstart <= nend ? nstart : NULL);
 	*email = left+1;
 	*(nend+1) = '\0';
 	*right++ = '\0';
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 27f8f86..8583724 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -247,7 +247,7 @@ test_expect_success 'cleanup after mailmap.blob tests' '
 	rm -f .mailmap
 '
 
-test_expect_failure 'single-character name' '
+test_expect_success 'single-character name' '
 	echo "     1	A <author@example.com>" >expect &&
 	echo "     1	nick1 <bugs@company.xx>" >>expect &&
 	echo "A <author@example.com>" >.mailmap &&
-- 
1.8.3.2.804.g0da7a53

[PATCH v2 3/9] t4203: demonstrate loss of uppercase characters in canonical email

[Eric Sunshine]

The email addresses read from .mailmap are downcased before being
inserted into the mailmap data structure, which undesirably loses
information.  It is impossible, for instance, to map <first.last@host>
to <First.Last@host>. Demonstrate this problem.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4203-mailmap.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 8583724..ffe6a11 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -256,6 +256,15 @@ test_expect_success 'single-character name' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'preserve canonical email case' '
+	echo "     1	A U Thor <AUTHOR@example.com>" >expect &&
+	echo "     1	nick1 <bugs@company.xx>" >>expect &&
+	echo "<AUTHOR@example.com> <author@example.com>" >.mailmap &&
+	test_when_finished "rm .mailmap" &&
+	git shortlog -es HEAD >actual &&
+	test_cmp expect actual
+'
+
 # Extended mailmap configurations should give us the following output for shortlog
 cat >expect <<\EOF
 A U Thor <author@example.com> (1):
-- 
1.8.3.2.804.g0da7a53

[PATCH v2 4/9] mailmap: do not downcase mailmap entries

[Eric Sunshine]

From: Junio C Hamano <gitster@pobox.com>

The email addresses in the records read from the .mailmap file are
downcased very early, and then used to match against e-mail
addresses in the input.  Because we do use case insensitive version
of string list to manage these entries, there is no need to do this,
and worse yet, downcasing the rewritten/canonical e-mail read from
the .mailmap file loses information.

Stop doing that, and also make the string list used to keep multiple
names for an mailmap entry case insensitive (the code that uses the
list, lookup_prefix(), expects a case insensitive match).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 mailmap.c          | 20 ++++++++------------
 t/t4203-mailmap.sh |  2 +-
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 418081e..a7e92db 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -51,14 +51,6 @@ static void add_mapping(struct string_list *map,
 {
 	struct mailmap_entry *me;
 	int index;
-	char *p;
-
-	if (old_email)
-		for (p = old_email; *p; p++)
-			*p = tolower(*p);
-	if (new_email)
-		for (p = new_email; *p; p++)
-			*p = tolower(*p);
 
 	if (old_email == NULL) {
 		old_email = new_email;
@@ -68,13 +60,17 @@ static void add_mapping(struct string_list *map,
 	if ((index = string_list_find_insert_index(map, old_email, 1)) < 0) {
 		/* mailmap entry exists, invert index value */
 		index = -1 - index;
+		me = (struct mailmap_entry *)map->items[index].util;
 	} else {
 		/* create mailmap entry */
-		struct string_list_item *item = string_list_insert_at_index(map, index, old_email);
-		item->util = xcalloc(1, sizeof(struct mailmap_entry));
-		((struct mailmap_entry *)item->util)->namemap.strdup_strings = 1;
+		struct string_list_item *item;
+
+		item = string_list_insert_at_index(map, index, old_email);
+		me = xcalloc(1, sizeof(struct mailmap_entry));
+		me->namemap.strdup_strings = 1;
+		me->namemap.cmp = strcasecmp;
+		item->util = me;
 	}
-	me = (struct mailmap_entry *)map->items[index].util;
 
 	if (old_name == NULL) {
 		debug_mm("mailmap: adding (simple) entry for %s at index %d\n", old_email, index);
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index ffe6a11..c32df80 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -256,7 +256,7 @@ test_expect_success 'single-character name' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'preserve canonical email case' '
+test_expect_success 'preserve canonical email case' '
 	echo "     1	A U Thor <AUTHOR@example.com>" >expect &&
 	echo "     1	nick1 <bugs@company.xx>" >>expect &&
 	echo "<AUTHOR@example.com> <author@example.com>" >.mailmap &&
-- 
1.8.3.2.804.g0da7a53

[PATCH v2 5/9] mailmap: debug: fix out-of-order fprintf() arguments

[Eric Sunshine]

Resolve segmentation fault due to arguments passed in wrong order.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 mailmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mailmap.c b/mailmap.c
index a7e92db..0516354 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -309,7 +309,7 @@ int map_user(struct string_list *map,
 	struct mailmap_entry *me;
 
 	debug_mm("map_user: map '%.*s' <%.*s>\n",
-		 *name, *namelen, *emaillen, *email);
+		 *namelen, *name, *emaillen, *email);
 
 	item = lookup_prefix(map, *email, *emaillen);
 	if (item != NULL) {
-- 
1.8.3.2.804.g0da7a53

Re: [PATCH v2 5/9] mailmap: debug: fix out-of-order fprintf() arguments

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Resolve segmentation fault due to arguments passed in wrong order.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

Thanks.  Shows that the debugging support hasn't been in use for
quite a long time X-<.

> ---
>  mailmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mailmap.c b/mailmap.c
> index a7e92db..0516354 100644
> --- a/mailmap.c
> +++ b/mailmap.c
> @@ -309,7 +309,7 @@ int map_user(struct string_list *map,
>  	struct mailmap_entry *me;
>  
>  	debug_mm("map_user: map '%.*s' <%.*s>\n",
> -		 *name, *namelen, *emaillen, *email);
> +		 *namelen, *name, *emaillen, *email);
>  
>  	item = lookup_prefix(map, *email, *emaillen);
>  	if (item != NULL) {

[PATCH v2 6/9] mailmap: debug: fix malformed fprintf() format directive

[Eric Sunshine]

Resolve segmentation fault due to size_t variable being consumed
by '%s'.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 mailmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mailmap.c b/mailmap.c
index 0516354..62d998a 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -337,7 +337,7 @@ int map_user(struct string_list *map,
 				*name = mi->name;
 				*namelen = strlen(*name);
 		}
-		debug_mm("map_user:  to '%.*s' <.*%s>\n", *namelen, *name,
+		debug_mm("map_user:  to '%.*s' <%.*s>\n", *namelen, *name,
 				 *emaillen, *email);
 		return 1;
 	}
-- 
1.8.3.2.804.g0da7a53

[PATCH v2 7/9] mailmap: debug: eliminate -Wformat field precision type warning

[Eric Sunshine]

The compiler complains that '*' in fprintf() format directive "%.*s"
should have type int, but we pass size_t. Fix this.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 mailmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 62d998a..4cc6e81 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -309,7 +309,7 @@ int map_user(struct string_list *map,
 	struct mailmap_entry *me;
 
 	debug_mm("map_user: map '%.*s' <%.*s>\n",
-		 *namelen, *name, *emaillen, *email);
+		 (int)*namelen, *name, (int)*emaillen, *email);
 
 	item = lookup_prefix(map, *email, *emaillen);
 	if (item != NULL) {
@@ -337,8 +337,8 @@ int map_user(struct string_list *map,
 				*name = mi->name;
 				*namelen = strlen(*name);
 		}
-		debug_mm("map_user:  to '%.*s' <%.*s>\n", *namelen, *name,
-				 *emaillen, *email);
+		debug_mm("map_user:  to '%.*s' <%.*s>\n", (int)*namelen, *name,
+				 (int)*emaillen, *email);
 		return 1;
 	}
 	debug_mm("map_user:  --\n");
-- 
1.8.3.2.804.g0da7a53

[PATCH v2 8/9] mailmap: debug: avoid passing NULL to fprintf() '%s' directive

[Eric Sunshine]

POSIX does not state the behavior of format directive '%s' when passed a
NULL pointer. Some implementations interpolate literal "(null)"; others
may crash. Callers of debug_mm() often pass NULL as indication of either
a missing name or email address.  Instead, let's always supply a proper
string pointer, and make it a bit more descriptive: "(none)"

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 mailmap.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 4cc6e81..928e6e5 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -5,8 +5,10 @@
 #define DEBUG_MAILMAP 0
 #if DEBUG_MAILMAP
 #define debug_mm(...) fprintf(stderr, __VA_ARGS__)
+#define debug_str(X) ((X) ? (X) : "(none)")
 #else
 static inline void debug_mm(const char *format, ...) {}
+static inline const char *debug_str(const char *s) { return s; }
 #endif
 
 const char *git_mailmap_file;
@@ -29,7 +31,7 @@ struct mailmap_entry {
 static void free_mailmap_info(void *p, const char *s)
 {
 	struct mailmap_info *mi = (struct mailmap_info *)p;
-	debug_mm("mailmap: -- complex: '%s' -> '%s' <%s>\n", s, mi->name, mi->email);
+	debug_mm("mailmap: -- complex: '%s' -> '%s' <%s>\n", s, debug_str(mi->name), debug_str(mi->email));
 	free(mi->name);
 	free(mi->email);
 }
@@ -38,7 +40,8 @@ static void free_mailmap_entry(void *p, const char *s)
 {
 	struct mailmap_entry *me = (struct mailmap_entry *)p;
 	debug_mm("mailmap: removing entries for <%s>, with %d sub-entries\n", s, me->namemap.nr);
-	debug_mm("mailmap: - simple: '%s' <%s>\n", me->name, me->email);
+	debug_mm("mailmap: - simple: '%s' <%s>\n", debug_str(me->name), debug_str(me->email));
+
 	free(me->name);
 	free(me->email);
 
@@ -94,7 +97,7 @@ static void add_mapping(struct string_list *map,
 	}
 
 	debug_mm("mailmap:  '%s' <%s> -> '%s' <%s>\n",
-		 old_name, old_email, new_name, new_email);
+		 debug_str(old_name), old_email, debug_str(new_name), debug_str(new_email));
 }
 
 static char *parse_name_and_email(char *buffer, char **name,
@@ -309,7 +312,7 @@ int map_user(struct string_list *map,
 	struct mailmap_entry *me;
 
 	debug_mm("map_user: map '%.*s' <%.*s>\n",
-		 (int)*namelen, *name, (int)*emaillen, *email);
+		 (int)*namelen, debug_str(*name), (int)*emaillen, debug_str(*email));
 
 	item = lookup_prefix(map, *email, *emaillen);
 	if (item != NULL) {
@@ -337,8 +340,8 @@ int map_user(struct string_list *map,
 				*name = mi->name;
 				*namelen = strlen(*name);
 		}
-		debug_mm("map_user:  to '%.*s' <%.*s>\n", (int)*namelen, *name,
-				 (int)*emaillen, *email);
+		debug_mm("map_user:  to '%.*s' <%.*s>\n", (int)*namelen, debug_str(*name),
+				 (int)*emaillen, debug_str(*email));
 		return 1;
 	}
 	debug_mm("map_user:  --\n");
-- 
1.8.3.2.804.g0da7a53

[PATCH v2 9/9] mailmap: style fixes

[Eric Sunshine]

From: Junio C Hamano <gitster@pobox.com>

Wrap overlong lines and format the multi-line comments to match our
coding style.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 mailmap.c | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 928e6e5..44614fc 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -31,7 +31,8 @@ struct mailmap_entry {
 static void free_mailmap_info(void *p, const char *s)
 {
 	struct mailmap_info *mi = (struct mailmap_info *)p;
-	debug_mm("mailmap: -- complex: '%s' -> '%s' <%s>\n", s, debug_str(mi->name), debug_str(mi->email));
+	debug_mm("mailmap: -- complex: '%s' -> '%s' <%s>\n",
+		 s, debug_str(mi->name), debug_str(mi->email));
 	free(mi->name);
 	free(mi->email);
 }
@@ -39,8 +40,10 @@ static void free_mailmap_info(void *p, const char *s)
 static void free_mailmap_entry(void *p, const char *s)
 {
 	struct mailmap_entry *me = (struct mailmap_entry *)p;
-	debug_mm("mailmap: removing entries for <%s>, with %d sub-entries\n", s, me->namemap.nr);
-	debug_mm("mailmap: - simple: '%s' <%s>\n", debug_str(me->name), debug_str(me->email));
+	debug_mm("mailmap: removing entries for <%s>, with %d sub-entries\n",
+		 s, me->namemap.nr);
+	debug_mm("mailmap: - simple: '%s' <%s>\n",
+		 debug_str(me->name), debug_str(me->email));
 
 	free(me->name);
 	free(me->email);
@@ -50,7 +53,8 @@ static void free_mailmap_entry(void *p, const char *s)
 }
 
 static void add_mapping(struct string_list *map,
-			char *new_name, char *new_email, char *old_name, char *old_email)
+			char *new_name, char *new_email,
+			char *old_name, char *old_email)
 {
 	struct mailmap_entry *me;
 	int index;
@@ -76,7 +80,8 @@ static void add_mapping(struct string_list *map,
 	}
 
 	if (old_name == NULL) {
-		debug_mm("mailmap: adding (simple) entry for %s at index %d\n", old_email, index);
+		debug_mm("mailmap: adding (simple) entry for %s at index %d\n",
+			 old_email, index);
 		/* Replace current name and new email for simple entry */
 		if (new_name) {
 			free(me->name);
@@ -88,7 +93,8 @@ static void add_mapping(struct string_list *map,
 		}
 	} else {
 		struct mailmap_info *mi = xcalloc(1, sizeof(struct mailmap_info));
-		debug_mm("mailmap: adding (complex) entry for %s at index %d\n", old_email, index);
+		debug_mm("mailmap: adding (complex) entry for %s at index %d\n",
+			 old_email, index);
 		if (new_name)
 			mi->name = xstrdup(new_name);
 		if (new_email)
@@ -97,11 +103,12 @@ static void add_mapping(struct string_list *map,
 	}
 
 	debug_mm("mailmap:  '%s' <%s> -> '%s' <%s>\n",
-		 debug_str(old_name), old_email, debug_str(new_name), debug_str(new_email));
+		 debug_str(old_name), old_email,
+		 debug_str(new_name), debug_str(new_email));
 }
 
 static char *parse_name_and_email(char *buffer, char **name,
-		char **email, int allow_empty_email)
+				  char **email, int allow_empty_email)
 {
 	char *left, *right, *nstart, *nend;
 	*name = *email = NULL;
@@ -305,21 +312,25 @@ static struct string_list_item *lookup_prefix(struct string_list *map,
 }
 
 int map_user(struct string_list *map,
-			 const char **email, size_t *emaillen,
-			 const char **name, size_t *namelen)
+	     const char **email, size_t *emaillen,
+	     const char **name, size_t *namelen)
 {
 	struct string_list_item *item;
 	struct mailmap_entry *me;
 
 	debug_mm("map_user: map '%.*s' <%.*s>\n",
-		 (int)*namelen, debug_str(*name), (int)*emaillen, debug_str(*email));
+		 (int)*namelen, debug_str(*name),
+		 (int)*emaillen, debug_str(*email));
 
 	item = lookup_prefix(map, *email, *emaillen);
 	if (item != NULL) {
 		me = (struct mailmap_entry *)item->util;
 		if (me->namemap.nr) {
-			/* The item has multiple items, so we'll look up on name too */
-			/* If the name is not found, we choose the simple entry      */
+			/*
+			 * The item has multiple items, so we'll look up on
+			 * name too. If the name is not found, we choose the
+			 * simple entry.
+			 */
 			struct string_list_item *subitem;
 			subitem = lookup_prefix(&me->namemap, *name, *namelen);
 			if (subitem)
@@ -340,8 +351,9 @@ int map_user(struct string_list *map,
 				*name = mi->name;
 				*namelen = strlen(*name);
 		}
-		debug_mm("map_user:  to '%.*s' <%.*s>\n", (int)*namelen, debug_str(*name),
-				 (int)*emaillen, debug_str(*email));
+		debug_mm("map_user:  to '%.*s' <%.*s>\n",
+			 (int)*namelen, debug_str(*name),
+			 (int)*emaillen, debug_str(*email));
 		return 1;
 	}
 	debug_mm("map_user:  --\n");
-- 
1.8.3.2.804.g0da7a53

Re: [PATCH v2 9/9] mailmap: style fixes

[Junio C Hamano]

Thanks, the whole series looks sensible.  Will replace and queue.