threads / patch / 34437

v2, 9 partsmailmap fixes

Subject: [PATCH v2 0/9] mailmap fixes

## tl;dr

12 messages between Jul 15, 2013 and Jul 15, 2013. Diffs are folded; open one to read it.

replies: 11people: 2as markdown or json

Eric Sunshine· Jul 15, 2013, 06:54 UTC · lore

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
Eric Sunshine· Jul 15, 2013, 06:54 UTC · re: Eric Sunshine · lore

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

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(+)
Show changes to t/t4203-mailmap.sh +9 −0
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
Eric Sunshine· Jul 15, 2013, 06:54 UTC · re: Eric Sunshine · lore

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

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(-)
Show changes to 2 files +2 −2

mailmap.c, t/t4203-mailmap.sh

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
Eric Sunshine· Jul 15, 2013, 06:54 UTC · re: Eric Sunshine · lore

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

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(+)
Show changes to t/t4203-mailmap.sh +9 −0
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
Eric Sunshine· Jul 15, 2013, 06:54 UTC · re: Eric Sunshine · lore

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

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(-)
Show changes to 2 files +9 −13

mailmap.c, t/t4203-mailmap.sh

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
Eric Sunshine· Jul 15, 2013, 06:54 UTC · re: Eric Sunshine · lore

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

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(-)
Show changes to mailmap.c +1 −1
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
Junio C Hamano· Jul 15, 2013, 15:18 UTC · re: Eric Sunshine · lore

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

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-<.

Show 17 quoted lines
> ---
>  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) {
Eric Sunshine· Jul 15, 2013, 06:54 UTC · re: Eric Sunshine · lore

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

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(-)
Show changes to mailmap.c +1 −1
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
Eric Sunshine· Jul 15, 2013, 06:54 UTC · re: Eric Sunshine · lore

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

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(-)
Show changes to mailmap.c +3 −3
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
Eric Sunshine· Jul 15, 2013, 06:54 UTC · re: Eric Sunshine · lore

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

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(-)
Show changes to mailmap.c +9 −6
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
Eric Sunshine· Jul 15, 2013, 06:54 UTC · re: Eric Sunshine · lore

[PATCH v2 9/9] mailmap: style fixes

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(-)
Show changes to mailmap.c +27 −15
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

← back to recent threads