threads / bug / 34418

Bug in .mailmap handling?

Subject: Bug in .mailmap handling?

## tl;dr

8 messages between Jul 12, 2013 and Jul 12, 2013.

replies: 7people: 2as markdown or json

Stefan Beller· Jul 12, 2013, 16:07 UTC · lore
Hello,

you may have noticed I am currently trying to bring the mailmap file of git itself up to date. I noticed some behavior, which I did not expect. Have a look yourself:

---
	# prepare test environment:
	mkdir testmailmap
	cd testmailmap/
	git init
	# 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"
	# how do we know it's the same person?
	git shortlog
	A (2):
		  add test1
		  add test2
	# reports as expected:
	git shortlog -sne
		  1  A <A@example.org>
		  1  A <changed_email@example.org>
		  
	# Adding the line to the mailmap should make life easy, so we know
	# it's the same person
	echo "A <A@example.org> <changed_email@example.org>" > .mailmap
	# Come on, I just wanted to have it reported as one person!
	git shortlog -sne
		 1  A <A@example.org>
		 1  A <a@example.org>
		 
	# So let's try another line in the mailmap file, (small 'a')
	echo "A <a@example.org> <changed_email@example.org>" > .mailmap
	# We're not there yet?
	git shortlog -sne
		 1  A <A@example.org>
		 1  A <a@example.org>
	# Now let's write it rather explicit: 
	# (essentially just write 2 lines into the mailmap file)
	cat << EOF > .mailmap
	A <a@example.org> <changed_email@example.org>
	A <a@example.org> <A@example.org>
	EOF
		 
	# works as expected now
	git shortlog -sne
		 2  A <a@example.org>
	# works as expected now as well
	git shortlog      
	A (2):
		  add test1
		  add test2
Junio C Hamano· Jul 12, 2013, 17:35 UTC · re: Stefan Beller · lore

Re: Bug in .mailmap handling?

Stefan Beller <stefanbeller@googlemail.com> writes:
Show 29 quoted lines
> Hello,
>
> you may have noticed I am currently trying to bring the 
> mailmap file of git itself up to date. I noticed
> some behavior, which I did not expect. Have a look yourself:
>
> ---
> 	# prepare test environment:
> 	mkdir testmailmap
> 	cd testmailmap/
> 	git init
>
> 	# 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"
>
> 	# how do we know it's the same person?
> 	git shortlog
> 	A (2):
> 		  add test1
> 		  add test2
You don't, and it is a long known behaviour.
> 	# reports as expected:
> 	git shortlog -sne
> 		  1  A <A@example.org>
> 		  1  A <changed_email@example.org>
Yes.
Show 8 quoted lines
> 	# Adding the line to the mailmap should make life easy, so we know
> 	# it's the same person
> 	echo "A <A@example.org> <changed_email@example.org>" > .mailmap
>
> 	# Come on, I just wanted to have it reported as one person!
> 	git shortlog -sne
> 		 1  A <A@example.org>
> 		 1  A <a@example.org>

Err, where does the lowercase a@ come from in the above? Are we missing some steps before we get here?

> 	# So let's try another line in the mailmap file, (small 'a')
> 	echo "A <a@example.org> <changed_email@example.org>" > .mailmap

This is ">", not ">>", I presume? Otherwise changed_email is mapped to two destination, no?

> 	# We're not there yet?
> 	git shortlog -sne
> 		 1  A <A@example.org>
> 		 1  A <a@example.org>

Expected, as long as some hidden set-up you did not describe that caused me to say "Err, where does the lowercase a@ come from" is there, i.e. one of the two commits is done by <a@example.org>.

Show 10 quoted lines
> 	# Now let's write it rather explicit: 
> 	# (essentially just write 2 lines into the mailmap file)
> 	cat << EOF > .mailmap
> 	A <a@example.org> <changed_email@example.org>
> 	A <a@example.org> <A@example.org>
> 	EOF
> 		 
> 	# works as expected now
> 	git shortlog -sne
> 		 2  A <a@example.org>
Makes sense.
Show 5 quoted lines
> 	# works as expected now as well
> 	git shortlog      
> 	A (2):
> 		  add test1
> 		  add test2
Stefan Beller· Jul 12, 2013, 17:47 UTC · re: Junio C Hamano · lore

Re: Bug in .mailmap handling?

The commands are exactly as given in the first mail. (I run it multiple times, and this exact behavior comes up)

I think there is some problem with the capitalisation of the first (if not all) characters. (Hence the small 'a')

So either check with these example commands yourself, or take my latest patch for the mailmap to reproduce on real names, please.

On 07/12/2013 07:35 PM, Junio C Hamano wrote:
Show 86 quoted lines
> Stefan Beller <stefanbeller@googlemail.com> writes:
> 
>> Hello,
>>
>> you may have noticed I am currently trying to bring the 
>> mailmap file of git itself up to date. I noticed
>> some behavior, which I did not expect. Have a look yourself:
>>
>> ---
>> 	# prepare test environment:
>> 	mkdir testmailmap
>> 	cd testmailmap/
>> 	git init
>>
>> 	# 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"
>>
>> 	# how do we know it's the same person?
>> 	git shortlog
>> 	A (2):
>> 		  add test1
>> 		  add test2
> 
> You don't, and it is a long known behaviour.
> 
>> 	# reports as expected:
>> 	git shortlog -sne
>> 		  1  A <A@example.org>
>> 		  1  A <changed_email@example.org>
> 
> Yes.
> 
>> 	# Adding the line to the mailmap should make life easy, so we know
>> 	# it's the same person
>> 	echo "A <A@example.org> <changed_email@example.org>" > .mailmap
>>
>> 	# Come on, I just wanted to have it reported as one person!
>> 	git shortlog -sne
>> 		 1  A <A@example.org>
>> 		 1  A <a@example.org>
> 
> Err, where does the lowercase a@ come from in the above?  Are we
> missing some steps before we get here?
> 
>> 	# So let's try another line in the mailmap file, (small 'a')
>> 	echo "A <a@example.org> <changed_email@example.org>" > .mailmap
> 
> This is ">", not ">>", I presume?  Otherwise changed_email is mapped
> to two destination, no?
> 
>> 	# We're not there yet?
>> 	git shortlog -sne
>> 		 1  A <A@example.org>
>> 		 1  A <a@example.org>
> 
> Expected, as long as some hidden set-up you did not describe that
> caused me to say "Err, where does the lowercase a@ come from" is
> there, i.e. one of the two commits is done by <a@example.org>.
> 
>> 	# Now let's write it rather explicit: 
>> 	# (essentially just write 2 lines into the mailmap file)
>> 	cat << EOF > .mailmap
>> 	A <a@example.org> <changed_email@example.org>
>> 	A <a@example.org> <A@example.org>
>> 	EOF
>> 		 
>> 	# works as expected now
>> 	git shortlog -sne
>> 		 2  A <a@example.org>
> 
> Makes sense.
> 
>> 	# works as expected now as well
>> 	git shortlog      
>> 	A (2):
>> 		  add test1
>> 		  add test2
Junio C Hamano· Jul 12, 2013, 20:28 UTC · re: Stefan Beller · lore

Re: Bug in .mailmap handling?

Stefan Beller <stefanbeller@googlemail.com> writes:
> 	# Adding the line to the mailmap should make life easy, so we know
> 	# it's the same person
> 	echo "A <A@example.org> <changed_email@example.org>" > .mailmap
While I was looking at this, I noticed this piece of code:
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';

The function is given a buffer "A <A@example.org>...", 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 ends
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> <changed_email@example.org>

without the name.  I do not think this bug affected anything you
observed, though.

> 	git shortlog -sne
> 		 1  A <A@example.org>
> 		 1  A <a@example.org>

This is coming from mailmap.c::add_mapping() that downcases the
e-mail address.

changed_email@example.org is mapped to a@example.org because of this
downcasing, while "A <A@example.org>" does not have any entry for it
in the .mailmap file, so it is given back as-is.  Hence we see two
distinct entries.
Stefan Beller· Jul 12, 2013, 20:35 UTC · re: Junio C Hamano · lore

Re: Bug in .mailmap handling?

On 07/12/2013 10:28 PM, Junio C Hamano wrote:
Show 48 quoted lines
> Stefan Beller <stefanbeller@googlemail.com> writes:
> 
>> 	# Adding the line to the mailmap should make life easy, so we know
>> 	# it's the same person
>> 	echo "A <A@example.org> <changed_email@example.org>" > .mailmap
> 
> While I was looking at this, I noticed this piece of code:
> 
> 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';
> 
> The function is given a buffer "A <A@example.org>...", 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 ends
> 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> <changed_email@example.org>
> 
> without the name.  I do not think this bug affected anything you
> observed, though.
> 
>> 	git shortlog -sne
>> 		 1  A <A@example.org>
>> 		 1  A <a@example.org>
> 
> This is coming from mailmap.c::add_mapping() that downcases the
> e-mail address.
> 
> changed_email@example.org is mapped to a@example.org because of this
> downcasing, while "A <A@example.org>" does not have any entry for it
> in the .mailmap file, so it is given back as-is.  Hence we see two
> distinct entries.
> 
So do I understand it right, we're having 2 bugs in here?
One being triggered by the short name, only one character.
So if you want to debug the other bug with a longer name,
you can either use a made up name, or run
    git shortlog -sne |grep Knut
in the git repository having the mailmap file already updated.
The way the mailmap file is written, I'd assume only one line
to be found, as of now 2 lines come up
     2	Knut Franke <knut.franke@gmx.de>
     1	Knut Franke <Knut.Franke@gmx.de>
which seems to downcase the whole first email.
Junio C Hamano· Jul 12, 2013, 20:38 UTC · re: Junio C Hamano · lore

Re: Bug in .mailmap handling?

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> Stefan Beller <stefanbeller@googlemail.com> writes:
>
>> 	git shortlog -sne
>> 		 1  A <A@example.org>
>> 		 1  A <a@example.org>
>
> This is coming from mailmap.c::add_mapping() that downcases the
> e-mail address.
>
> changed_email@example.org is mapped to a@example.org because of this
> downcasing, while "A <A@example.org>" does not have any entry for it
> in the .mailmap file, so it is given back as-is.  Hence we see two
> distinct entries.

I think it is wrong for the parser to lose information by downcasing.

It is perfectly fine to do the comparison case insensitively, to allow <changed_Email@example.org> in the input is mangled using the entry for <changed_email@example.org> in the .mailmap file; it is not fine to downcase the parsed result, especially the side that is used as the "rewritten result" (i.e. the tokens earlier on the line), as that would mean mangling the output.

Let me see if I can quickly whip up a fix.
Junio C Hamano· Jul 12, 2013, 20:50 UTC · re: Junio C Hamano · lore

Re: Bug in .mailmap handling?

Junio C Hamano <gitster@pobox.com> writes:
Show 27 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <stefanbeller@googlemail.com> writes:
>>
>>> 	git shortlog -sne
>>> 		 1  A <A@example.org>
>>> 		 1  A <a@example.org>
>>
>> This is coming from mailmap.c::add_mapping() that downcases the
>> e-mail address.
>>
>> changed_email@example.org is mapped to a@example.org because of this
>> downcasing, while "A <A@example.org>" does not have any entry for it
>> in the .mailmap file, so it is given back as-is.  Hence we see two
>> distinct entries.
>
> I think it is wrong for the parser to lose information by
> downcasing.
>
> It is perfectly fine to do the comparison case insensitively, to
> allow <changed_Email@example.org> in the input is mangled using the
> entry for <changed_email@example.org> in the .mailmap file; it is
> not fine to downcase the parsed result, especially the side that is
> used as the "rewritten result" (i.e. the tokens earlier on the line),
> as that would mean mangling the output.
>
> Let me see if I can quickly whip up a fix.
It might be just the matter of doing this.

I suspect that we could drop the downcasing of old-email, too, but the new-email is supposed to be the rewritten result that appears on the output, and downcasing it is very wrong.

I also suspect that this was an old workaround for the original string-list that did not know how to match items case insensitively, which we should have removed when we added case sensitivity support to the string-list at around 577f63e7 (Merge branch 'ap/log-mailmap', 2013-01-20).

 mailmap.c | 3 ---
 1 file changed, 3 deletions(-)
diff --git a/mailmap.c b/mailmap.c
index 418081e..c64a53d 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -56,9 +56,6 @@ static void add_mapping(struct string_list *map,
 	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;
Stefan Beller· Jul 12, 2013, 21:13 UTC · re: Junio C Hamano · lore

[PATCH] Add a testcase for checking case insensitivity of mail map

Your patch seems to do it. I added a test case which doesn't fail, if your patch is added.

Signed-off-by: Stefan Beller <stefanbeller@googlemail.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.776.gfcf213d

← back to recent threads