[PATCH 0/4] Microfixes to mailmap

[PATCH 0/4] Microfixes to mailmap

[Junio C Hamano]

There were a few bugs in mailmap parsing and matching code.

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

Stefan Beller (1):
  Add a testcase for checking case insensitivity of mailmap

 mailmap.c          | 37 +++++++++++++++++++------------------
 t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 18 deletions(-)

-- 
1.8.3.2-941-gda9c3c8

[PATCH 1/4] mailmap: do not lose single-letter names

[Junio C Hamano]

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>
---
 mailmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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';
-- 
1.8.3.2-941-gda9c3c8

[PATCH] mailmap: Testing the single letter name case.

[Stefan Beller]

This is a regression test for a66e77eab70a08938fdc2227b7ada0f0465c6991

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 t/t4203-mailmap.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 842b754..9ec87a2 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -409,4 +409,45 @@ test_expect_success 'Blame output (complex mapping)' '
 	test_cmp expect actual.fuzz
 '
 
+cat >expect <<\EOF
+A <ShortName@company.xy> (2):
+      eighth
+      nineth
+
+A U Thor <author@example.com> (1):
+      initial
+
+CTO <cto@company.xx> (1):
+      seventh
+
+Other Author <other@author.xx> (2):
+      third
+      fourth
+
+Santa Claus <santa.claus@northpole.xx> (2):
+      fifth
+      sixth
+
+Some Dude <some@dude.xx> (1):
+      second
+
+EOF
+
+# Regression test
+# Using a single letter name to check for off-by-one errors in parse_name_and_email
+test_expect_success 'check mapping for short names' '
+	echo one >two &&
+	git add two &&
+	git commit --author "A <shortname@company.xx>" -m "eighth" &&
+
+	echo two >> two &&
+	git add two &&
+	git commit --author "A <ShortName@company.xy>" -m "nineth" &&
+
+	echo "A <ShortName@company.xy> <shortname@company.xx>" >> .mailmap &&
+	git shortlog HEAD -e >actual
+
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.3.2.776.gfcf213d

Re: [PATCH] mailmap: Testing the single letter name case.

[Junio C Hamano]

Stefan Beller <stefanbeller@googlemail.com> writes:

> This is a regression test for a66e77eab70a08938fdc2227b7ada0f0465c6991

Sorry, I do not quite get this.

If you apply the patch on top of the said commit before that commit, the
new test does not pass.

But if you apply the patch on top of the said commit, the new test
still does not pass.

So I am having hard time guessing what you meant by "regression
test".  It is not "a66e77 broke something that ought to work, and
this shows the breakage".  It is not "a66e77 fixed something and
this shows the previous breakage that got fixed."

It may be because the test is depending on ShortName not to be
downcased incorrectly, which was to be fixed by a later commit.  But
after applying this on top of jc/mailmap-case-insensitivity topic,
the test does not pass (and reverting a66e77 does not seem to affect
the result, either).

Re: [PATCH] mailmap: Testing the single letter name case.

[Stefan Beller]

On 07/13/2013 07:38 PM, Junio C Hamano wrote:
> Stefan Beller <stefanbeller@googlemail.com> writes:
> 
>> This is a regression test for a66e77eab70a08938fdc2227b7ada0f0465c6991
> 
> Sorry, I do not quite get this.
> 
> If you apply the patch on top of the said commit before that commit, the
> new test does not pass.
> 
> But if you apply the patch on top of the said commit, the new test
> still does not pass.
> 
> So I am having hard time guessing what you meant by "regression
> test".  It is not "a66e77 broke something that ought to work, and
> this shows the breakage".  It is not "a66e77 fixed something and
> this shows the previous breakage that got fixed."
> 
> It may be because the test is depending on ShortName not to be
> downcased incorrectly, which was to be fixed by a later commit.  But
> after applying this on top of jc/mailmap-case-insensitivity topic,
> the test does not pass (and reverting a66e77 does not seem to affect
> the result, either).
> 

I am sorry for the bad wording, sorry for wasting your time.

This patch was meant to replace 134d1ac9be2ce97c60a7e9187c32980681811cb5
(current test, latest commit on mailmap-case-insensitivity)

Indeed the patch tests for both bugs unintentionally.
So both the one letter name is being used and the email
case sensitivity is checked.
So maybe I should redo it again and make 2 explicit test cases
for each of the fixes?

Stefan

Re: [PATCH] mailmap: Testing the single letter name case.

[Junio C Hamano]

Stefan Beller <stefanbeller@googlemail.com> writes:

> Indeed the patch tests for both bugs unintentionally.

I was puzzled because I do not think that is what is happening with
the posted patch.

If I drop the tip one from jc/mailmap-case-insensitivity and apply
this patch instead, the test passes (which is good).

	git checkout 1ab5d42
        git am <sb.mbox
        make && cd t && sh t4203-mailmap.sh

Of course, if I revert 543f991 (i.e. do not downcase fix), the test
in this patch fails as expected.

	git checkout 1ab5d42
        git am <sb.mbox
	git show 543f991 | git apply -R
        make && cd t && sh t4203-mailmap.sh ;# should fail and does

But if I only revert a66e77e (i.e. off-by-one fix) while still
keeping the downcase fix, the test in this patch should fail---but
it doesn't seem to.

	git checkout 1ab5d42
        git am <sb.mbox
	git show a66e77e | git apply -R -3
        make && cd t && sh t4203-mailmap.sh ;# should fail but doesn't

The off-by-one fix seems to be correct from code inspection, but the
new test does not seem to demonstrate a case where the code before
the fix misbehaves.

Re: [PATCH] mailmap: Testing the single letter name case.

[Eric Sunshine]

On Sat, Jul 13, 2013 at 4:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <stefanbeller@googlemail.com> writes:
>
>> Indeed the patch tests for both bugs unintentionally.
>
> I was puzzled because I do not think that is what is happening with
> the posted patch.
>
> The off-by-one fix seems to be correct from code inspection, but the
> new test does not seem to demonstrate a case where the code before
> the fix misbehaves.

I've written tests which check for the two bugs which Junio's patches
fix. I'll be sending them along with a re-roll of Junio's series along
with a few other fixes to mailmap's debug mode.

Re: [PATCH] mailmap: Testing the single letter name case.

[Junio C Hamano]

Thanks.

Re: [PATCH] mailmap: Testing the single letter name case.

[Eric Sunshine]

On Sat, Jul 13, 2013 at 3:20 AM, Stefan Beller
<stefanbeller@googlemail.com> wrote:
> This is a regression test for a66e77eab70a08938fdc2227b7ada0f0465c6991
>
> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
> ---
>  t/t4203-mailmap.sh | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index 842b754..9ec87a2 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -409,4 +409,45 @@ test_expect_success 'Blame output (complex mapping)' '
>         test_cmp expect actual.fuzz
>  '
>
> +# Regression test
> +# Using a single letter name to check for off-by-one errors in parse_name_and_email
> +test_expect_success 'check mapping for short names' '
> +       echo one >two &&
> +       git add two &&
> +       git commit --author "A <shortname@company.xx>" -m "eighth" &&
> +
> +       echo two >> two &&

s/>> />>/

> +       git add two &&
> +       git commit --author "A <ShortName@company.xy>" -m "nineth" &&
> +
> +       echo "A <ShortName@company.xy> <shortname@company.xx>" >> .mailmap &&

s/>> />>/

> +       git shortlog HEAD -e >actual

Broken &&-chain: s/actual/actual &&/

> +
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 1.8.3.2.776.gfcf213d
>

[PATCH 2/4] mailmap: do not downcase mailmap entries

[Junio C Hamano]

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>
---
 mailmap.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 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);
-- 
1.8.3.2-941-gda9c3c8

[PATCH 3/4] mailmap: style fixes

[Junio C Hamano]

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

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 mailmap.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index a7e92db..5a3347d 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -37,7 +37,8 @@ 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: removing entries for <%s>, with %d sub-entries\n",
+		 s, me->namemap.nr);
 	debug_mm("mailmap: - simple: '%s' <%s>\n", me->name, me->email);
 	free(me->name);
 	free(me->email);
@@ -73,7 +74,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);
@@ -85,7 +87,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)
@@ -315,8 +318,10 @@ int map_user(struct string_list *map,
 	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)
-- 
1.8.3.2-941-gda9c3c8

[PATCH 4/4] add a testcase for checking case insensitivity of mailmap

[Junio C Hamano]

From: Stefan Beller <stefanbeller@googlemail.com>

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 842b754..07152e9 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -409,4 +409,37 @@ test_expect_success 'Blame output (complex mapping)' '
 	test_cmp expect actual.fuzz
 '
 
+test_expect_success 'cleanup after mailmap.blob tests' '
+	rm -f .mailmap
+'
+
+cat >expect <<\EOF
+     2	A <A@example.org>
+     2	Other Author <other@author.xx>
+     2	Santa Claus <santa.claus@northpole.xx>
+     1	A U Thor <author@example.com>
+     1	CTO <cto@company.xx>
+     1	Some Dude <some@dude.xx>
+EOF
+
+test_expect_success 'Test case sensitivity of Names' '
+	# do a commit:
+	echo "asdf" > test1
+	git add test1
+	git commit -a --author="A <A@example.org>" -m "add test1"
+
+	# commit with same name, but different email
+	# (different capitalization does the trick already,
+	# but here I am going to use a different mail)
+	echo "asdf" > test2
+	git add test2
+	git commit -a --author="A <changed_email@example.org>" -m "add test2"
+
+	# Adding the line to the mailmap should make life easy, so we know
+	# it is the same person
+	echo "A <A@example.org> <changed_email@example.org>" > .mailmap
+
+	git shortlog -sne HEAD >actual && test_cmp expect actual
+'
+
 test_done
-- 
1.8.3.2-941-gda9c3c8

Re: [PATCH 4/4] add a testcase for checking case insensitivity of mailmap

[Eric Sunshine]

On Fri, Jul 12, 2013 at 5:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> From: Stefan Beller <stefanbeller@googlemail.com>
>
> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index 842b754..07152e9 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -409,4 +409,37 @@ test_expect_success 'Blame output (complex mapping)' '
>         test_cmp expect actual.fuzz
>  '
>
> +test_expect_success 'cleanup after mailmap.blob tests' '
> +       rm -f .mailmap
> +'

This "test" was copied from earlier in the file, but the description
was not updated; it has nothing to do with mailmap.blob tests for
which cleanup has already taken place.

It's also misleading since no .mailmap file exists at this point.
Instead, configuration key mailmap.file is set to
internal_mailmap/.mailmap, so if you need to clean up anything, it
would be that.

(You're also recreating .mailmap from scratch directly in your test
via "echo foo >.mailmap", so this "test" doesn't really add anything.)

> +cat >expect <<\EOF
> +     2 A <A@example.org>
> +     2 Other Author <other@author.xx>
> +     2 Santa Claus <santa.claus@northpole.xx>
> +     1 A U Thor <author@example.com>
> +     1 CTO <cto@company.xx>
> +     1 Some Dude <some@dude.xx>
> +EOF
> +
> +test_expect_success 'Test case sensitivity of Names' '

s/Names/names/

Also, most of the test descriptions in this script start with
lowercase, so s/Test/test/ would be appropriate.

> +       # do a commit:
> +       echo "asdf" > test1

Git practice normally avoids whitespace after the redirection
operator. This particular test script, has a mix of ">foo" and ">
foo", but we may want to avoid adding more of the latter form.

> +       git add test1
> +       git commit -a --author="A <A@example.org>" -m "add test1"
> +
> +       # commit with same name, but different email
> +       # (different capitalization does the trick already,
> +       # but here I am going to use a different mail)

Without context of the mailing list discussion, the reader does not
know what "trick" is or precisely how this may have failed in the
past. It's also not clear why the test is being performed with a
different email address entirely rather than one which differs only by
case (which is what the test description talks about).

> +       echo "asdf" > test2

Whitespace after redirection.

> +       git add test2
> +       git commit -a --author="A <changed_email@example.org>" -m "add test2"
> +
> +       # Adding the line to the mailmap should make life easy, so we know
> +       # it is the same person

The comment can probably be dropped, as the new .mailmap entry is
self-explanatory.

> +       echo "A <A@example.org> <changed_email@example.org>" > .mailmap

Whitespace after redirection.

> +
> +       git shortlog -sne HEAD >actual && test_cmp expect actual

For consistency with more other tests, perhaps break the line apart:

  git shortlog -sne HEAD >actual &&
  test_cmp expect actual

> +'
> +
>  test_done
> --
> 1.8.3.2-941-gda9c3c8

Re: [PATCH 4/4] add a testcase for checking case insensitivity of mailmap

[Eric Sunshine]

On Fri, Jul 12, 2013 at 5:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> From: Stefan Beller <stefanbeller@googlemail.com>
>
> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index 842b754..07152e9 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> +test_expect_success 'Test case sensitivity of Names' '
> +       # do a commit:
> +       echo "asdf" > test1
> +       git add test1
> +       git commit -a --author="A <A@example.org>" -m "add test1"
> +
> +       # commit with same name, but different email
> +       # (different capitalization does the trick already,
> +       # but here I am going to use a different mail)
> +       echo "asdf" > test2
> +       git add test2
> +       git commit -a --author="A <changed_email@example.org>" -m "add test2"
> +
> +       # Adding the line to the mailmap should make life easy, so we know
> +       # it is the same person
> +       echo "A <A@example.org> <changed_email@example.org>" > .mailmap
> +
> +       git shortlog -sne HEAD >actual && test_cmp expect actual
> +'

I forgot to mention that the &&-chain is broken (missing) in the
entire test (except for the last line).