threads / patch / 34425

patcht4203: fix checks for email address remapping

Subject: [PATCH] t4203: fix checks for email address remapping

## tl;dr

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

replies: 2people: 2as markdown or json

Eric Sunshine· Jul 13, 2013, 00:35 UTC · lore

Two tests in t4203-mailmap.sh set up the mapping <bugs@company.xx> => <bugs@company.xy> in an apparent attempt to check that email address remapping works as expected (in addition to name remapping which is also tested). To test the remapping, git-shortlog is invoked but the invocation lacks the -e option instructing it to show email addresses, hence the tests do not actually prove that address remapping succeeded. Fix this by instructing git-shortlog to output email addresses as well.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

The very last git-shortlog "complex" test in the script does use -e and checks that email address remapping actually works, so it's not clear that this patch is needed. The <bugs@company.xx> => <bugs@company.xy> remapping done by the two tests touched by this patch, however, is misleading to the reader since it seems to imply that these two tests want to check address remapping as well. Perhaps a better change would be to remove the address remapping from these two tests.

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

Show changes to t/t4203-mailmap.sh +6 −6

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 842b754..3cf64de 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -102,10 +102,10 @@ test_expect_success 'mailmap.file non-existent' '
 '
 
 cat >expect <<\EOF
-Internal Guy (1):
+Internal Guy <bugs@company.xy> (1):
       second
 
-Repo Guy (1):
+Repo Guy <author@example.com> (1):
       initial
 
 EOF
@@ -114,15 +114,15 @@ test_expect_success 'name entry after email entry' '
 	mkdir -p internal_mailmap &&
 	echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
 	echo "Internal Guy <bugs@company.xx>" >>internal_mailmap/.mailmap &&
-	git shortlog HEAD >actual &&
+	git shortlog -e HEAD >actual &&
 	test_cmp expect actual
 '
 
 cat >expect <<\EOF
-Internal Guy (1):
+Internal Guy <bugs@company.xy> (1):
       second
 
-Repo Guy (1):
+Repo Guy <author@example.com> (1):
       initial
 
 EOF
@@ -131,7 +131,7 @@ test_expect_success 'name entry after email entry, case-insensitive' '
 	mkdir -p internal_mailmap &&
 	echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
 	echo "Internal Guy <BUGS@Company.xx>" >>internal_mailmap/.mailmap &&
-	git shortlog HEAD >actual &&
+	git shortlog -e HEAD >actual &&
 	test_cmp expect actual
 '

-- 
1.8.3.2.804.g0da7a53

Stefan Beller· Jul 13, 2013, 06:29 UTC · re: Eric Sunshine · lore

Re: [PATCH] t4203: fix checks for email address remapping

On 07/13/2013 02:35 AM, Eric Sunshine wrote:

Show 63 quoted lines

> Two tests in t4203-mailmap.sh set up the mapping <bugs@company.xx> =>
> <bugs@company.xy> in an apparent attempt to check that email address
> remapping works as expected (in addition to name remapping which is also
> tested).  To test the remapping, git-shortlog is invoked but the
> invocation lacks the -e option instructing it to show email addresses,
> hence the tests do not actually prove that address remapping succeeded.
> Fix this by instructing git-shortlog to output email addresses as well.
> 
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
> 
> The very last git-shortlog "complex" test in the script does use -e and
> checks that email address remapping actually works, so it's not clear
> that this patch is needed. The <bugs@company.xx> => <bugs@company.xy>
> remapping done by the two tests touched by this patch, however, is
> misleading to the reader since it seems to imply that these two tests
> want to check address remapping as well. Perhaps a better change would
> be to remove the address remapping from these two tests.
> 
> 
>  t/t4203-mailmap.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index 842b754..3cf64de 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -102,10 +102,10 @@ test_expect_success 'mailmap.file non-existent' '
>  '
>  
>  cat >expect <<\EOF
> -Internal Guy (1):
> +Internal Guy <bugs@company.xy> (1):
>        second
>  
> -Repo Guy (1):
> +Repo Guy <author@example.com> (1):
>        initial
>  
>  EOF
> @@ -114,15 +114,15 @@ test_expect_success 'name entry after email entry' '
>  	mkdir -p internal_mailmap &&
>  	echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
>  	echo "Internal Guy <bugs@company.xx>" >>internal_mailmap/.mailmap &&
> -	git shortlog HEAD >actual &&
> +	git shortlog -e HEAD >actual &&
>  	test_cmp expect actual
>  '
>  
>  cat >expect <<\EOF
> -Internal Guy (1):
> +Internal Guy <bugs@company.xy> (1):
>        second
>  
> -Repo Guy (1):
> +Repo Guy <author@example.com> (1):
>        initial
>  
>  EOF
> @@ -131,7 +131,7 @@ test_expect_success 'name entry after email entry, case-insensitive' '
>  	mkdir -p internal_mailmap &&
>  	echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
>  	echo "Internal Guy <BUGS@Company.xx>" >>internal_mailmap/.mailmap &&+

So here it is capitalized email address (BUGS@), but at the expect file
it's still lower cased. I think this is a bug.
Junio was trying to fix it in 543f99173c2d2f648d8f846e24875150f7de03d3
(origin/jc/mailmap-case-insensitivity)
So I think we need another yet test case there:
commited:
    Internal Guy <BUGS@Company.xx>
    Internal Guy <bugs@company.xy>

Having just one entry in the mailmap
    Internal Guy <BUGS@Company.xx> <bugs@Company.xy>

should still work with the "shortlog -e"

Show 6 quoted lines

> -	git shortlog HEAD >actual &&
> +	git shortlog -e HEAD >actual &&
>  	test_cmp expect actual
>  '
>  
>

Eric Sunshine· Jul 15, 2013, 07:00 UTC · re: Stefan Beller · lore

Re: [PATCH] t4203: fix checks for email address remapping

On Sat, Jul 13, 2013 at 2:29 AM, Stefan Beller <stefanbeller@googlemail.com> wrote:

Show 39 quoted lines

> On 07/13/2013 02:35 AM, Eric Sunshine wrote:
>> Two tests in t4203-mailmap.sh set up the mapping <bugs@company.xx> =>
>> <bugs@company.xy> in an apparent attempt to check that email address
>> remapping works as expected (in addition to name remapping which is also
>> tested).  To test the remapping, git-shortlog is invoked but the
>> invocation lacks the -e option instructing it to show email addresses,
>> hence the tests do not actually prove that address remapping succeeded.
>> Fix this by instructing git-shortlog to output email addresses as well.
>>
>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>> ---
>>
>> The very last git-shortlog "complex" test in the script does use -e and
>> checks that email address remapping actually works, so it's not clear
>> that this patch is needed. The <bugs@company.xx> => <bugs@company.xy>
>> remapping done by the two tests touched by this patch, however, is
>> misleading to the reader since it seems to imply that these two tests
>> want to check address remapping as well. Perhaps a better change would
>> be to remove the address remapping from these two tests.
>>
>>  cat >expect <<\EOF
>> -Internal Guy (1):
>> +Internal Guy <bugs@company.xy> (1):
>>        second
>>
>> -Repo Guy (1):
>> +Repo Guy <author@example.com> (1):
>>        initial
>>
>>  EOF
>> @@ -131,7 +131,7 @@ test_expect_success 'name entry after email entry, case-insensitive' '
>>       mkdir -p internal_mailmap &&
>>       echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap &&
>>       echo "Internal Guy <BUGS@Company.xx>" >>internal_mailmap/.mailmap &&+
>
> So here it is capitalized email address (BUGS@), but at the expect file
> it's still lower cased. I think this is a bug.
> Junio was trying to fix it in 543f99173c2d2f648d8f846e24875150f7de03d3
> (origin/jc/mailmap-case-insensitivity)

With the re-roll of Junio's series which I just sent out [1], this case-preserving problem is tested (and fixed), and email address remapping is tested in general, so I think this submission should be retired.

(It might still be worthwhile, as mentioned in the patch commentary, to drop the confusing lines altogether from those two tests, but that's an issue for another day.)

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

Show 14 quoted lines

> So I think we need another yet test case there:
> commited:
>     Internal Guy <BUGS@Company.xx>
>     Internal Guy <bugs@company.xy>
>
> Having just one entry in the mailmap
>     Internal Guy <BUGS@Company.xx> <bugs@Company.xy>
>
> should still work with the "shortlog -e"
>
>> -     git shortlog HEAD >actual &&
>> +     git shortlog -e HEAD >actual &&
>>       test_cmp expect actual
>>  '