Bug in .mailmap handling?

PATCH New Stefan Beller 2 participants 8 messages Jun 15, 2016

Share & tools

Stefan Beller

author

2016-06-15 22:58 UTC #0 lore raw

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 <[email protected]>" -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 <[email protected]>" -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 <[email protected]>
		  1  A <[email protected]>

# Adding the line to the mailmap should make life easy, so we know # it's the same person
echo "A <[email protected]> <[email protected]>" > .mailmap

# Come on, I just wanted to have it reported as one person! git shortlog -sne
1 A <[email protected]>
1 A <[email protected]>

# So let's try another line in the mailmap file, (small 'a') echo "A <[email protected]> <[email protected]>" > .mailmap

# We're not there yet?
git shortlog -sne
1 A <[email protected]>
1 A <[email protected]>

# Now let's write it rather explicit:
# (essentially just write 2 lines into the mailmap file) cat << EOF > .mailmap
A <[email protected]> <[email protected]> A <[email protected]> <[email protected]>
EOF

# works as expected now
git shortlog -sne
2 A <[email protected]>

# works as expected now as well
git shortlog
A (2):
add test1
add test2

Junio C Hamano

2016-06-15 22:58 UTC #1 lore raw

Quoted 3 lines · Stefan Beller

# Adding the line to the mailmap should make life easy, so we know # it's the same person
echo "A <[email protected]> <[email protected]>" > .mailmap

While I was looking at this, I noticed this piece of code:

mailmap.c +1 -1

index 2a7b366..418081e 100644
@@ -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 <[email protected]>...", 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

<[email protected]> <[email protected]>

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

Quoted 3 lines

git shortlog -sne
1 A <[email protected]>
1 A <[email protected]>

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

[email protected] is mapped to [email protected] because of this downcasing, while "A <[email protected]>" 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

author

2016-06-15 22:58 UTC #2 lore raw

Quoted 40 lines · Junio C Hamano

Quoted 3 lines · Stefan Beller
# Adding the line to the mailmap should make life easy, so we know # it's the same person
echo "A <[email protected]> <[email protected]>" > .mailmap
While I was looking at this, I noticed this piece of code:
mailmap.c +1 -1
index 2a7b366..418081e 100644
@@ -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 <[email protected]>...", 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
<[email protected]> <[email protected]>
without the name. I do not think this bug affected anything you observed, though.
Quoted 3 lines
git shortlog -sne
1 A <[email protected]>
1 A <[email protected]>

Quoted 8 lines

This is coming from mailmap.c::add_mapping() that downcases the e-mail address.
[email protected] is mapped to [email protected] because of this downcasing, while "A <[email protected]>" 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 <[email protected]>
1 Knut Franke <[email protected]>

which seems to downcase the whole first email.

Junio C Hamano

2016-06-15 22:58 UTC #3 lore raw

In reply to Junio C Hamano

Quoted 6 lines · Junio C Hamano

Quoted 3 lines · Stefan Beller
git shortlog -sne
1 A <[email protected]>
1 A <[email protected]>

Quoted 7 lines

This is coming from mailmap.c::add_mapping() that downcases the e-mail address.
[email protected] is mapped to [email protected] because of this downcasing, while "A <[email protected]>" 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 <[email protected]> in the input is mangled using the entry for <[email protected]> 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

2016-06-15 22:58 UTC #4 lore raw

Quoted 19 lines · Junio C Hamano

Quoted 6 lines · Junio C Hamano
Quoted 3 lines · Stefan Beller
git shortlog -sne
1 A <[email protected]>
1 A <[email protected]>
Quoted 7 lines
This is coming from mailmap.c::add_mapping() that downcases the e-mail address.
[email protected] is mapped to [email protected] because of this downcasing, while "A <[email protected]>" 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.

Quoted 8 lines

It is perfectly fine to do the comparison case insensitively, to allow <[email protected]> in the input is mangled using the entry for <[email protected]> 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(-)

mailmap.c +0 -3

index 418081e..c64a53d 100644
@@ -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

author

2016-06-15 22:58 UTC #5 lore raw

PATCH 1/1 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 <[email protected]>

---
 t/t4203-mailmap.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

t/t4203-mailmap.sh +33 -0

index 842b754..07152e9 100755
@@ -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 <[email protected]>
+     2	Other Author <[email protected]>
+     2	Santa Claus <[email protected]>
+     1	A U Thor <[email protected]>
+     1	CTO <[email protected]>
+     1	Some Dude <[email protected]>
+EOF
+
+test_expect_success 'Test case sensitivity of Names' '
+	# do a commit:
+	echo "asdf" > test1
+	git add test1
+	git commit -a --author="A <[email protected]>" -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 <[email protected]>" -m "add test2"
+
+	# Adding the line to the mailmap should make life easy, so we know
+	# it is the same person
+	echo "A <[email protected]> <[email protected]>" > .mailmap
+
+	git shortlog -sne HEAD >actual && test_cmp expect actual
+'
+
 test_done

Signature · 1 line

1.8.3.2.776.gfcf213d

Junio C Hamano

2016-06-15 22:58 UTC #6 lore raw

In reply to Stefan Beller

Quoted 24 lines · Stefan Beller

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 <[email protected]>" -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 <[email protected]>" -m "add test2"

Quoted 5 lines

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

Quoted 4 lines

	# reports as expected:
	git shortlog -sne
		  1  A <[email protected]>
		  1  A <[email protected]>

Yes.

Quoted 8 lines

# Adding the line to the mailmap should make life easy, so we know # it's the same person
echo "A <[email protected]> <[email protected]>" > .mailmap
# Come on, I just wanted to have it reported as one person! git shortlog -sne
1 A <[email protected]>
1 A <[email protected]>

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

Quoted 2 lines

# So let's try another line in the mailmap file, (small 'a') echo "A <[email protected]> <[email protected]>" > .mailmap

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

Quoted 4 lines

# We're not there yet?
git shortlog -sne
1 A <[email protected]>
1 A <[email protected]>

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 <[email protected]>.

Quoted 10 lines

# Now let's write it rather explicit:
# (essentially just write 2 lines into the mailmap file) cat << EOF > .mailmap
A <[email protected]> <[email protected]> A <[email protected]> <[email protected]>
EOF
# works as expected now
git shortlog -sne
2 A <[email protected]>

Makes sense.

Quoted 5 lines

	# works as expected now as well
	git shortlog      
	A (2):
		  add test1
		  add test2

Stefan Beller

author

2016-06-15 22:58 UTC #7 lore raw

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.

Quoted 86 lines · Junio C Hamano

Quoted 24 lines · Stefan Beller
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 <[email protected]>" -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 <[email protected]>" -m "add test2"
Quoted 5 lines
	# 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.
Quoted 4 lines
	# reports as expected:
	git shortlog -sne
		  1  A <[email protected]>
		  1  A <[email protected]>
Yes.
Quoted 8 lines
# Adding the line to the mailmap should make life easy, so we know # it's the same person
echo "A <[email protected]> <[email protected]>" > .mailmap
# Come on, I just wanted to have it reported as one person! git shortlog -sne
1 A <[email protected]>
1 A <[email protected]>
Err, where does the lowercase a@ come from in the above? Are we missing some steps before we get here?
Quoted 2 lines
# So let's try another line in the mailmap file, (small 'a') echo "A <[email protected]> <[email protected]>" > .mailmap
This is ">", not ">>", I presume? Otherwise changed_email is mapped to two destination, no?
Quoted 4 lines
# We're not there yet?
git shortlog -sne
1 A <[email protected]>
1 A <[email protected]>
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 <[email protected]>.
Quoted 10 lines
# Now let's write it rather explicit:
# (essentially just write 2 lines into the mailmap file) cat << EOF > .mailmap
A <[email protected]> <[email protected]> A <[email protected]> <[email protected]>
EOF
# works as expected now
git shortlog -sne
2 A <[email protected]>
Makes sense.
Quoted 5 lines
	# works as expected now as well
	git shortlog      
	A (2):
		  add test1
		  add test2