[PATCH 0/2] t: add blame -e tests for mailmap

I sent both the fix and the tests. Another fix was applied, but we are still
missing the tests.

These are good before, and after the fix.

Felipe Contreras (2):
  t: mailmap: add 'git blame -e' tests
  t: mailmap: add simple name translation test

 t/t4203-mailmap.sh |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

-- 
1.7.9.1.g97f7d

Signed-off-by: Felipe Contreras <redacted>
---
 t/t4203-mailmap.sh |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 1f182f6..db12265 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh

@@ -255,4 +255,22 @@ test_expect_success 'Blame output (complex mapping)' '
 	test_cmp expect actual.fuzz
 '
 
+# git blame -e
+cat >expect <<\EOF
+^OBJI (<author@example.com>       DATE 1) one
+OBJID (<some@dude.xx>             DATE 2) two
+OBJID (<other@author.xx>          DATE 3) three
+OBJID (<other@author.xx>          DATE 4) four
+OBJID (<santa.claus@northpole.xx> DATE 5) five
+OBJID (<santa.claus@northpole.xx> DATE 6) six
+OBJID (<cto@company.xx>           DATE 7) seven
+EOF
+test_expect_success 'Blame output (complex mapping)' '
+	git blame -e one >actual &&
+	cp actual /tmp &&
+	cp internal_mailmap/.mailmap /tmp &&
+	fuzz_blame actual >actual.fuzz &&
+	test_cmp expect actual.fuzz
+'
+
 test_done

-- 
1.7.9.1.g97f7d

Signed-off-by: Felipe Contreras <redacted>
---
 t/t4203-mailmap.sh |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index db12265..fc3855a 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh

@@ -157,6 +157,9 @@ A U Thor <author@example.com> (1):
 CTO <cto@company.xx> (1):
       seventh
 
+Mr. Right <right@company.xx> (1):
+      eight
+
 Other Author <other@author.xx> (2):
       third
       fourth

@@ -196,6 +199,11 @@ test_expect_success 'Shortlog output (complex mapping)' '
 	test_tick &&
 	git commit --author "CTO <cto@coompany.xx>" -m seventh &&
 
+	echo eight >>one &&
+	git add one &&
+	test_tick &&
+	git commit --author "Wrong <right@company.xx>" -m eight &&
+
 	mkdir -p internal_mailmap &&
 	echo "Committed <committer@example.com>" > internal_mailmap/.mailmap &&
 	echo "<cto@company.xx>                       <cto@coompany.xx>" >> internal_mailmap/.mailmap &&

@@ -204,6 +212,7 @@ test_expect_success 'Shortlog output (complex mapping)' '
 	echo "Other Author <other@author.xx>         <nick2@company.xx>" >> internal_mailmap/.mailmap &&
 	echo "Santa Claus <santa.claus@northpole.xx> <me@company.xx>" >> internal_mailmap/.mailmap &&
 	echo "Santa Claus <santa.claus@northpole.xx> <me@company.xx>" >> internal_mailmap/.mailmap &&
+	echo "Mr. Right <right@company.xx>" >> internal_mailmap/.mailmap &&
 
 	git shortlog -e HEAD >actual &&
 	test_cmp expect actual

@@ -212,6 +221,9 @@ test_expect_success 'Shortlog output (complex mapping)' '
 
 # git log with --pretty format which uses the name and email mailmap placemarkers
 cat >expect <<\EOF
+Author Wrong <right@company.xx> maps to Mr. Right <right@company.xx>
+Committer C O Mitter <committer@example.com> maps to Committed <committer@example.com>
+
 Author CTO <cto@coompany.xx> maps to CTO <cto@company.xx>
 Committer C O Mitter <committer@example.com> maps to Committed <committer@example.com>
 

@@ -248,6 +260,7 @@ OBJID (Other Author DATE 4) four
 OBJID (Santa Claus  DATE 5) five
 OBJID (Santa Claus  DATE 6) six
 OBJID (CTO          DATE 7) seven
+OBJID (Mr. Right    DATE 8) eight
 EOF
 test_expect_success 'Blame output (complex mapping)' '
 	git blame one >actual &&

@@ -264,6 +277,7 @@ OBJID (<other@author.xx>          DATE 4) four
 OBJID (<santa.claus@northpole.xx> DATE 5) five
 OBJID (<santa.claus@northpole.xx> DATE 6) six
 OBJID (<cto@company.xx>           DATE 7) seven
+OBJID (<right@company.xx>         DATE 8) eight
 EOF
 test_expect_success 'Blame output (complex mapping)' '
 	git blame -e one >actual &&

-- 
1.7.9.1.g97f7d

Felipe Contreras [off-list ref] writes:

Signed-off-by: Felipe Contreras <redacted>
---

It was clear that we didn't have any test for "blame -e" hence it was no
brainer to judge that the patch 1/2 is good without any description.

But I am scratching my head, deciphering what this patch adds.

It appears to me that the existing tests that map author@example.com from
the original "A U Thor" to "Repo Guy" and inspect names and mails in
various output already cover this "Wrong with [off-list ref] can be
corrected to Mr. Right" case this patch adds.

What am I missing?  Instead of explaining it to me, can it be explained in
the log message?

Thanks.

On Tue, Feb 14, 2012 at 10:10 PM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

Signed-off-by: Felipe Contreras <redacted>
---

It was clear that we didn't have any test for "blame -e" hence it was no
brainer to judge that the patch 1/2 is good without any description.

But I am scratching my head, deciphering what this patch adds.

It appears to me that the existing tests that map author@example.com from
the original "A U Thor" to "Repo Guy" and inspect names and mails in
various output already cover this "Wrong with [off-list ref] can be
corrected to Mr. Right" case this patch adds.

Yes, but in the first tests they don't check for 'git blame', and much
less 'git blame -e', and the second tests only check complex mappings.

What am I missing?  Instead of explaining it to me, can it be explained in
the log message?

If an explanations along the lines of the above make sense, I can resend.

Cheers.

-- 
Felipe Contreras

Hi again,

Felipe Contreras wrote:

I sent both the fix and the tests. Another fix was applied, but we are still
missing the tests.

These are good before, and after the fix.

To summarize the previous discussion[1]: some people had comments, and
you seem to have found value in exactly none of them.  OK.  CC-ing
Peff, since he at least probably has looked over this code before.

By the way, the address you are using for Marius is out of date.

Felipe Contreras (2):
  t: mailmap: add 'git blame -e' tests

So that people don't destroy this test in later refactorings, I would
like to collect statements that we want the test to ensure remain
true.

Apparently the fix in f026358e ("mailmap: always return a plain mail
address from map_user()", 2012-02-05) was for the case of the name
changing and email address not changing due to mailmap mapping.  Most
callers use a separate buffer for the email address so there is room
to modify the name in place, but "git blame" keeps angle brackets in
the same buffer for no obvious reason I can see.  (Callers should
be able to add the brackets themselves instead of relying on
ci.author_mail to contain them, but that's a story for another day.)

Anyway, the existing tests for the returned email missed this since
it does not affect "git shortlog -e" but only "git blame -e".
Therefore this patch reuses the test data for shortlog -e and lets us
use it for blame, too.  It is easier to understand after the other
patch, IMHO.

This is _not_ meant as a more general test for the "git blame -e"
format (which would belong somewhere near t8008) as far as I can tell.
It is just checking that mailmap doesn't screw up.

  t: mailmap: add simple name translation test

Before, I thought this might be a straightforward test for the bug
fixed by f026358e.  That didn't justify the patch that touches
several different test assertions.

In fact it seems to be intended to test the case addressed by f026358e
(name changing, email not) in various mailmap callers: "git shortlog -e",
"git log --pretty", "git blame".

Here's a reroll.

Enjoy,
Jonathan

Felipe Contreras (2):
  test: mailmap can change author name without changing email
  test: check that "git blame -e" uses mailmap correctly

 t/t4203-mailmap.sh |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

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

From: Felipe Contreras <redacted>

A mailmap entry of the format 'A U Thor [off-list ref]' has two
effects: (1) 'A U Thor' becomes the canonical author name for commits
with author address 'email@example.com', and (2) 'email@example.com'
becomes the canonical author email for commits with author name 'A U
Thor'.

We already have tests for the effect (1) in the committer name, but
not in the author name, so the tests do not cover the shortlog and
blame codepaths as they should.  Fix that.

[jn: simplified by reusing Committer mailmap entry, clarified
 description]

Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
 t/t4203-mailmap.sh |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 1f182f61..45526395 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh

@@ -157,6 +157,9 @@ A U Thor <author@example.com> (1):
 CTO <cto@company.xx> (1):
       seventh
 
+Committed <committer@example.com> (1):
+      eighth
+
 Other Author <other@author.xx> (2):
       third
       fourth

@@ -196,6 +199,11 @@ test_expect_success 'Shortlog output (complex mapping)' '
 	test_tick &&
 	git commit --author "CTO <cto@coompany.xx>" -m seventh &&
 
+	echo eight >>one &&
+	git add one &&
+	test_tick &&
+	git commit --author "C O Mitter <committer@example.com>" -m eighth &&
+
 	mkdir -p internal_mailmap &&
 	echo "Committed <committer@example.com>" > internal_mailmap/.mailmap &&
 	echo "<cto@company.xx>                       <cto@coompany.xx>" >> internal_mailmap/.mailmap &&

@@ -212,6 +220,9 @@ test_expect_success 'Shortlog output (complex mapping)' '
 
 # git log with --pretty format which uses the name and email mailmap placemarkers
 cat >expect <<\EOF
+Author C O Mitter <committer@example.com> maps to Committed <committer@example.com>
+Committer C O Mitter <committer@example.com> maps to Committed <committer@example.com>
+
 Author CTO <cto@coompany.xx> maps to CTO <cto@company.xx>
 Committer C O Mitter <committer@example.com> maps to Committed <committer@example.com>
 

@@ -248,6 +259,7 @@ OBJID (Other Author DATE 4) four
 OBJID (Santa Claus  DATE 5) five
 OBJID (Santa Claus  DATE 6) six
 OBJID (CTO          DATE 7) seven
+OBJID (Committed    DATE 8) eight
 EOF
 test_expect_success 'Blame output (complex mapping)' '
 	git blame one >actual &&

-- 
1.7.9

From: Felipe Contreras <redacted>

Until f026358e ("mailmap: always return a plain mail address from
map_user()", 2012-02-05), git blame -e would add a spurious '>' after
the unchanged email address with brackets it passed to the mailmap
machinery, resulting in lines with a doubled '>' like this:

 620456e6 ([off-list ref]>   2005-04-07 15:20:13 -0700 8) eight

Add a test to make sure it doesn't happen again.  This reuses the test
data for the existing "shortlog -e" test so it also tests other kinds
of mail mapping.

[jn: fixed some cut+paste cruft, added a patch description]

Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
 t/t4203-mailmap.sh |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 45526395..ef900d84 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh

@@ -267,4 +267,20 @@ test_expect_success 'Blame output (complex mapping)' '
 	test_cmp expect actual.fuzz
 '
 
+cat >expect <<\EOF
+^OBJI (<author@example.com>       DATE 1) one
+OBJID (<some@dude.xx>             DATE 2) two
+OBJID (<other@author.xx>          DATE 3) three
+OBJID (<other@author.xx>          DATE 4) four
+OBJID (<santa.claus@northpole.xx> DATE 5) five
+OBJID (<santa.claus@northpole.xx> DATE 6) six
+OBJID (<cto@company.xx>           DATE 7) seven
+OBJID (<committer@example.com>    DATE 8) eight
+EOF
+test_expect_success 'Blame -e output' '
+	git blame -e one >actual &&
+	fuzz_blame actual >actual.fuzz &&
+	test_cmp expect actual.fuzz
+'
+
 test_done

-- 
1.7.9

Hi,


On Tue, Feb 14, 2012 at 10:34 PM, Jonathan Nieder [off-list ref] wrote:

Felipe Contreras wrote:

quoted

I sent both the fix and the tests. Another fix was applied, but we are still
missing the tests.

These are good before, and after the fix.

To summarize the previous discussion[1]: some people had comments, and
you seem to have found value in exactly none of them.  OK.  CC-ing
Peff, since he at least probably has looked over this code before.

Just because you have comments doesn't mean I *must* address them. We
have a difference of opinion, nothing wrong with that.

By the way, the address you are using for Marius is out of date.

quoted

Felipe Contreras (2):
  t: mailmap: add 'git blame -e' tests

So that people don't destroy this test in later refactorings, I would
like to collect statements that we want the test to ensure remain
true.

Apparently the fix in f026358e ("mailmap: always return a plain mail
address from map_user()", 2012-02-05) was for the case of the name
changing and email address not changing due to mailmap mapping.  Most
callers use a separate buffer for the email address so there is room
to modify the name in place, but "git blame" keeps angle brackets in
the same buffer for no obvious reason I can see.  (Callers should
be able to add the brackets themselves instead of relying on
ci.author_mail to contain them, but that's a story for another day.)

Anyway, the existing tests for the returned email missed this since
it does not affect "git shortlog -e" but only "git blame -e".
Therefore this patch reuses the test data for shortlog -e and lets us
use it for blame, too.  It is easier to understand after the other
patch, IMHO.

This is not related to f026358e. The test added by this patch would
pass before and after f026358e.

This is what I mean by reading too much into the patch. There's
nothing to it; "add 'git blame -e' tests" just adds tests for 'git
blame -e', it doesn't catch any issues related to f026358e; it just
makes sure there are no further regressions with the tests that are
already passing.

Again, some tests for 'git blame -e' > no tests for 'git blame -e'.

This is _not_ meant as a more general test for the "git blame -e"
format (which would belong somewhere near t8008) as far as I can tell.
It is just checking that mailmap doesn't screw up.

Thus the 't: mailmap: ' prefix.

quoted

  t: mailmap: add simple name translation test

Before, I thought this might be a straightforward test for the bug
fixed by f026358e.  That didn't justify the patch that touches
several different test assertions.

In fact it seems to be intended to test the case addressed by f026358e
(name changing, email not) in various mailmap callers: "git shortlog -e",
"git log --pretty", "git blame".

No. As the summary says, it's intended to add a simple name
translation test, which is missing from all the tests that spawn from
the repository generated in 'Shortlog output (complex mapping)' test.
This is the most minimal patch that can be generated if you add a
commit to this repository, and any further tests that are related to
it would look the same.

As Junio pointed out what is missing from the explanation is that this
simple name translation test is targeted toward the 'git blame'
commands, because such translation is not tested for them currently.

Cheers.

-- 
Felipe Contreras

On Tue, Feb 14, 2012 at 02:34:31PM -0600, Jonathan Nieder wrote:

Felipe Contreras wrote:

quoted

I sent both the fix and the tests. Another fix was applied, but we are still
missing the tests.

These are good before, and after the fix.

To summarize the previous discussion[1]: some people had comments, and
you seem to have found value in exactly none of them.  OK.  CC-ing
Peff, since he at least probably has looked over this code before.

The short answer is that both patches look OK to me.

Here's a longer digression that you may feel free to ignore.

When I investigated this problem, I looked only at the code (and I think
Junio's fix is the right one). However, having just looked at your two
patches without having previously looked at those tests, I found them
quite painless to review.

Having participated in the bug-hunt, I knew they were probably related
to testing that bug, but it was very unclear to me from the original
series why there were two patches, and not one. Or why, when we have
mailmap tests, they did not catch this bug. And that is _now_, with the
context of having just participated in the discussion; six months from
now I would have even less hope of understanding the context.

My general review process is (in this order):

  1. Figure out why a change is needed. This should be explained in the
     commit message. And no, just adding tests is not assumed to be
     needed.  Why did the old tests not cover this case? The answer can
     be a simple "nobody bothered to write them", and that's OK. But
     some description of the current state can help reviewers understand
     the rationale (e.g., "we tested with shortlog, but not mailmap, and
     certain code paths are only exposed through mailmap").

  2. Figure out what the change should be doing. This should also be in
     the commit message, though at a high level. In the case of tests,
     obviously we're adding new tests. But are we properly covering all
     of the new cases? Does the "what" match the "why" from (1)?

  3. Look at the patch. Do the changes match what the commit message
     claimed in (2)?

  4. Look at the patch for style, implementation, etc. Is the code
     quality good enough to be included?

The steps are dependent on each other, and I usually quit if I can't get
past one step. Sometimes I will look at the patch to try to figure out
(1) or (2), especially if the patch is trivial. But in my ideal world,
every patch lets me just walk through those steps.

I won't claim that these steps create error-free reviews. Based on those
steps, your patches look good to me, and it seems from Felipe's response
that there might be some inaccuracies in your description. But it at
least gives us a starting point for discussion.

-Peff

Felipe Contreras wrote:

On Tue, Feb 14, 2012 at 10:34 PM, Jonathan Nieder [off-list ref] wrote:

quoted

To summarize the previous discussion[1]: some people had comments, and
you seem to have found value in exactly none of them.  OK.  CC-ing
Peff, since he at least probably has looked over this code before.

Just because you have comments doesn't mean I *must* address them. We
have a difference of opinion, nothing wrong with that.

I said "OK", didn't I?

[...]

quoted

In fact it seems to be intended to test the case addressed by f026358e
(name changing, email not) in various mailmap callers: "git shortlog -e",
"git log --pretty", "git blame".

No. As the summary says, it's intended to add a simple name
translation test, which is missing from all the tests that spawn from
the repository generated in 'Shortlog output (complex mapping)' test.
This is the most minimal patch that can be generated if you add a
commit to this repository, and any further tests that are related to
it would look the same.

As Junio pointed out what is missing from the explanation is that this
simple name translation test is targeted toward the 'git blame'
commands, because such translation is not tested for them currently.

Um.  So this has nothing to do with f026358e at all?  Mentioning that
commit and that this test does not pass with an older codebase is not
useful to the humans that will look over this test later?

Adding explanation and rearranging things so people encountering this
later have to spend _less_ time to understand it is something I
consider useful.  It means people are less likely to randomly break
things.  I don't actually understand where the difference of opinion
comes from here.

Jeff King wrote:

The short answer is that both patches look OK to me.

Thanks for looking them over and for a nice explanation of the process.

On Tue, Feb 14, 2012 at 10:35 PM, Jonathan Nieder [off-list ref] wrote:

From: Felipe Contreras <redacted>

test: mailmap can change author name without changing email

That doesn't say anything to me, which is weird if I supposedly wrote
this patch. What is the *purpose* of this?

At least 'add simple name translation test' is clear about the
purpose, albeit not clear enough as Junio pointed out.

(2) 'email@example.com'
becomes the canonical author email for commits with author name 'A U
Thor'.

That's not true. I initially thought that was the case, and I think it
might be useful to have that, but it's not the case now, and your
patch doesn't test this.

We already have tests for the effect (1) in the committer name, but
not in the author name, so the tests do not cover the shortlog and
blame codepaths as they should.  Fix that.

In order to test that you would need additional changes, something
along the lines of:

--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh

@@ -157,8 +157,9 @@ A U Thor <author@example.com> (1):
 CTO <cto@company.xx> (1):
       seventh

-Committed <committer@example.com> (1):
+Committed <committer@example.com> (2):
       eighth
+      nine

 Other Author <other@author.xx> (2):
       third

@@ -204,6 +205,11 @@ test_expect_success 'Shortlog output (complex mapping)' '
        test_tick &&
        git commit --author "C O Mitter <committer@example.com>" -m eighth &&

+       echo nine >>one &&
+       git add one &&
+       test_tick &&
+       git commit --author "Committed <bad@example.com>" -m nine &&
+
        mkdir -p internal_mailmap &&
        echo "Committed <committer@example.com>" > internal_mailmap/.mailmap &&
        echo "<cto@company.xx>

[off-list ref]" >> internal_mailmap/.mailmap &&

@@ -220,6 +226,9 @@ test_expect_success 'Shortlog output (complex mapping)' '

 # git log with --pretty format which uses the name and email mailmap

placemarkers
 cat >expect <<\EOF
+Author Committed [off-list ref] maps to Committed
[off-list ref]
+Committer C O Mitter [off-list ref] maps to Committed
[off-list ref]
+
 Author C O Mitter [off-list ref] maps to Committed
[off-list ref]
 Committer C O Mitter [off-list ref] maps to Committed
[off-list ref]

@@ -260,6 +269,7 @@ OBJID (Santa Claus  DATE 5) five
 OBJID (Santa Claus  DATE 6) six
 OBJID (CTO          DATE 7) seven
 OBJID (Committed    DATE 8) eight
+OBJID (Committed    DATE 9) nine
 EOF
 test_expect_success 'Blame output (complex mapping)' '
        git blame one >actual &&

But that of course fails.

Signed-off-by: Felipe Contreras <redacted>

I most definitely did not sign this off, and I didn't add any of these
lines, nor wrote anything about this commit message.

It might be possible to simplify my patch "t: mailmap: add simple name
translation test" using the already existing "Committed
[off-list ref]" mapping, but that most likely is going to
remove only one line, and would make the code less clear about what
that translation is trying to test.

Cheers.

-- 
Felipe Contreras

On Tue, Feb 14, 2012 at 10:36 PM, Jonathan Nieder [off-list ref] wrote:

From: Felipe Contreras <redacted>

test: check that "git blame -e" uses mailmap correctly

I wonder what extra information is in that text that is not in my
original "t: mailmap: add 'git blame -e' tests". I guess all tests
'check' something, and the purpose is to make sure things work
'correctly.

Until f026358e ("mailmap: always return a plain mail address from
map_user()", 2012-02-05), git blame -e would add a spurious '>' after
the unchanged email address with brackets it passed to the mailmap
machinery, resulting in lines with a doubled '>' like this:

 620456e6 ([off-list ref]>   2005-04-07 15:20:13 -0700 8) eight

Add a test to make sure it doesn't happen again.  This reuses the test
data for the existing "shortlog -e" test so it

also tests other kinds of mail mapping.

'Also' is a keyword that strongly denotes this patch is doing more
than one logical thing. My patch adds those checks *independently* of
the fix on f026358e, so it's truly logically independent.

Signed-off-by: Felipe Contreras <redacted>

I did not sign this.

-- 
Felipe Contreras

Felipe Contreras wrote:

On Tue, Feb 14, 2012 at 10:35 PM, Jonathan Nieder [off-list ref] wrote:

quoted

(2) 'email@example.com'
becomes the canonical author email for commits with author name 'A U
Thor'.

That's not true. I initially thought that was the case, and I think it
might be useful to have that, but it's not the case now, and your
patch doesn't test this.

Thanks for explaining.  I had indeed confused myself into thinking 'A
U Thor [off-list ref]' would act like 'A U Thor
[off-list ref] [off-list ref]'.

I should have said:

-- 8< --
A mailmap entry in the format 'A U Thor [off-list ref]' means
that 'A U Thor' should be the canonical author name for commits
with author address 'email@example.com', and the email address
should be left alone.

We already have tests for this format regarding the committer name,
but not in the author name, so the tests do not cover the shortlog and
blame codepaths as they should.  Fix that.
-- >8 --

[...]

quoted

Signed-off-by: Felipe Contreras <redacted>

I most definitely did not sign this off, and I didn't add any of these
lines, nor wrote anything about this commit message.

That's why I described the changes I made, signed with my initials,
and put my own sign-off below yours.  Did I screw up somewhere?

Note that I am making these changes because, at its heart, I think
your patch is good and useful.  Otherwise I would have ignored it and
worked on something else.  If you prefer that I don't make
improvements like this, please indicate why that's a good idea;
otherwise I will probably continue to do it when I see good patches,
despite all the signals you are giving that I have done something
awful by corrupting your perfect patch in this way.

Jonathan

On Tue, Feb 14, 2012 at 11:14 PM, Jeff King [off-list ref] wrote:

My general review process is (in this order):

 1. Figure out why a change is needed. This should be explained in the
    commit message. And no, just adding tests is not assumed to be
    needed.  Why did the old tests not cover this case?

As I already mentioned more than once; the first patch is not related
to any fix.

    The answer can
    be a simple "nobody bothered to write them", and that's OK.

 That can be derived from the word "add". You can't add something that
is already there.

    But
    some description of the current state can help reviewers understand
    the rationale (e.g., "we tested with shortlog, but not mailmap, and
    certain code paths are only exposed through mailmap").

You are assuming too much. That's not the purpose, that's why I didn't menti

 2. Figure out what the change should be doing.

t: mailmap: add 'git blame -e' tests

That's what the change should be doing; nothing more, nothing less.

I wonder why you have to assume the worst. When I see a commit message
like that, I assume that there were no previous tests for that (thus
the word 'add'), and that's all I need to know.

If you want to extrude meaning from where there isn't, well, go ahead,
but there's nothing else really.

Cheers.

-- 
Felipe Contreras

Felipe Contreras wrote:

On Tue, Feb 14, 2012 at 10:36 PM, Jonathan Nieder [off-list ref] wrote:

quoted

Signed-off-by: Felipe Contreras <redacted>

I did not sign this.

Do you mean you do not agree with the following?

	Developer's Certificate of Origin 1.1

	By making a contribution to this project, I certify that:

	(a) The contribution was created in whole or in part by me and I
	    have the right to submit it under the open source license
	    indicated in the file; or

	(b) The contribution is based upon previous work that, to the best
	    of my knowledge, is covered under an appropriate open source
	    license and I have the right under that license to submit that
	    work with modifications, whether created in whole or in part
	    by me, under the same open source license (unless I am
	    permitted to submit under a different license), as indicated
	    in the file; or

	(c) The contribution was provided directly to me by some other
	    person who certified (a), (b) or (c) and I have not modified
	    it.

	(d) I understand and agree that this project and the contribution
	    are public and that a record of the contribution (including all
	    personal information I submit with it, including my sign-off) is
	    maintained indefinitely and may be redistributed consistent with
	    this project or the open source license(s) involved.

Anyway, Junio, please feel free to remove Felipe's sign-off from these
patches, or add an "Against-the-protest-of:" or whatever if that's
what he wants.

I've reached the point of diminishing returns.  I'll be happy to help
in any way I can, given a polite explanation of how I can help.

On Tue, Feb 14, 2012 at 11:52:11PM +0200, Felipe Contreras wrote:

On Tue, Feb 14, 2012 at 11:14 PM, Jeff King [off-list ref] wrote:

quoted

My general review process is (in this order):

 1. Figure out why a change is needed. This should be explained in the
    commit message. And no, just adding tests is not assumed to be
    needed.  Why did the old tests not cover this case?

As I already mentioned more than once; the first patch is not related
to any fix.

Really? I didn't see it mentioned in your commit message, which
consisted entirely of:

  t: mailmap: add 'git blame -e' tests

Yes, I know you mentioned it elsewhere in the thread. But review should
happen on what is actually in the posted patch. That is what reviewers
are guaranteed to have read, and it is what people reading "git log" in
a year will see. If there was other discussion or context that led to
the patch, it's helpful in both cases to summarize it.

quoted

    The answer can
    be a simple "nobody bothered to write them", and that's OK.

 That can be derived from the word "add". You can't add something that
is already there.

Are there already git-blame tests, but not "blame -e" tests? If there
are already "blame" tests, why do we additionally need "blame -e" tests?

I can guess, or I can do my own digging in the history to find out, but
that makes review a lot more painful. You already did the digging and
came to understand the problem when you made your patch. Why not just
share it?

quoted

    But
    some description of the current state can help reviewers understand
    the rationale (e.g., "we tested with shortlog, but not mailmap, and
    certain code paths are only exposed through mailmap").

You are assuming too much. That's not the purpose, that's why I didn't menti

Sorry, that should have been s/mailmap/blame/ above. But if I am making
wrong assumptions about the rationale, then isn't that a sign that the
commit message is insufficient?

quoted

 2. Figure out what the change should be doing.

t: mailmap: add 'git blame -e' tests

That's what the change should be doing; nothing more, nothing less.

Yes, I think you did describe the "what", which in this case is very
simple. But as I mentioned before, it is not just knowing the "what" but
evaluating that the "what" meets the "why" from step 1.

I wonder why you have to assume the worst. When I see a commit message
like that, I assume that there were no previous tests for that (thus
the word 'add'), and that's all I need to know.

I don't necessarily assume the worst. If I were the maintainer, I might
even have taken your patch as-is, as it's pretty simple. But I found a
description like the one Jonathan included to be _much_ easier to
review. Whether yours was above a minimum threshold or not, I think it's
worth striving to be better.

-Peff

On Tue, Feb 14, 2012 at 11:15 PM, Jonathan Nieder [off-list ref] wrote:

Felipe Contreras wrote:

quoted

On Tue, Feb 14, 2012 at 10:34 PM, Jonathan Nieder [off-list ref] wrote:

quoted

quoted

To summarize the previous discussion[1]: some people had comments, and
you seem to have found value in exactly none of them.  OK.  CC-ing
Peff, since he at least probably has looked over this code before.

Just because you have comments doesn't mean I *must* address them. We
have a difference of opinion, nothing wrong with that.

I said "OK", didn't I?

You also said I didn't find value in any of the comments which is
being passively aggressive. Comments always have value, that doesn't
meant they must all be turned into actionables.

[...]

quoted

quoted

In fact it seems to be intended to test the case addressed by f026358e
(name changing, email not) in various mailmap callers: "git shortlog -e",
"git log --pretty", "git blame".

No. As the summary says, it's intended to add a simple name
translation test, which is missing from all the tests that spawn from
the repository generated in 'Shortlog output (complex mapping)' test.
This is the most minimal patch that can be generated if you add a
commit to this repository, and any further tests that are related to
it would look the same.

As Junio pointed out what is missing from the explanation is that this
simple name translation test is targeted toward the 'git blame'
commands, because such translation is not tested for them currently.

Um.  So this has nothing to do with f026358e at all?  Mentioning that
commit and that this test does not pass with an older codebase is not
useful to the humans that will look over this test later?

I didn't say that. I say the *purpose* of the patch is different.

Adding explanation and rearranging things so people encountering this
later have to spend _less_ time to understand it is something I
consider useful.  It means people are less likely to randomly break
things.  I don't actually understand where the difference of opinion
comes from here.

The difference of opinion is that I consider the patch already good
enough (adding the comments from Junio). Yeah, now that f026358e is
committed might be worth mentioning, and what's the relationship, but
that's an *extra*. Even if f026358e wasn't there, and nobody knew what
was the issue, and possible fixes, the patch would be good by itself.

Again, some tests > no tests, and don't let the perfect be the enemy
of the good. It even looks like you prefer no changes at all (status
quo) to my proposed changes as you have never said these are "good",
but always paint them as "wrong" or "broken" in fundamental ways, as
if they are not worthy of being applied.

In any case, I will address the comments from Junio, and perhaps add a
note regarding f026358e.

I also can't help but feel you are applying double standards, as this
is the original patch that seems to have broken things:

ommit d20d654fe8923a502527547b17fe284d15d6aec9
Author: Marius Storm-Olsen [off-list ref]
Date:   Sun Feb 8 15:34:30 2009 +0100

    Change current mailmap usage to do matching on both name and email
of author/committer.

    Signed-off-by: Marius Storm-Olsen [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]

 Documentation/pretty-formats.txt |    2 +
 builtin-blame.c                  |   50 +++++++++++-------
 builtin-shortlog.c               |   22 ++++++--
 pretty.c                         |   57 +++++++++++----------
 t/t4203-mailmap.sh               |  106 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 186 insertions(+), 51 deletions(-)

Notice the short commit message. It would have been _nice_ to have a
bigger commit message, but it's better to have this commit than
nothing at all.

And see the relevant commit f026358:

f026358 mailmap: always return a plain mail address from map_user()
 mailmap.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

Notice how there's no tests, which would have reason enough to reject
the patches from many contributors.

Cheers.

-- 
Felipe Contreras

Felipe Contreras wrote:

The difference of opinion is that I consider the patch already good
enough (adding the comments from Junio).

Ok, I can understand where you're coming from there.

Now I have offered some suggestions for improving some of your
patches.  Applying these suggestions would be effortless, since I sent
them in patch form.  What is your response?  You send a point-by-point
rebuttal explaining how each detail of my suggestions is bad, bad,
bad, and then resend the original with comparatively small changes.

Can you see how this might indicate a stronger difference of opinion
than you have mentioned?  And how a reviewer might make the mistake of
thinking his comments were unwelcome after such an experience?

On Wed, Feb 15, 2012 at 12:07 AM, Jeff King [off-list ref] wrote:

On Tue, Feb 14, 2012 at 11:52:11PM +0200, Felipe Contreras wrote:

quoted

On Tue, Feb 14, 2012 at 11:14 PM, Jeff King [off-list ref] wrote:

quoted

My general review process is (in this order):

 1. Figure out why a change is needed. This should be explained in the
    commit message. And no, just adding tests is not assumed to be
    needed.  Why did the old tests not cover this case?

As I already mentioned more than once; the first patch is not related
to any fix.

Really? I didn't see it mentioned in your commit message, which
consisted entirely of:

 t: mailmap: add 'git blame -e' tests

Yes, I know you mentioned it elsewhere in the thread. But review should
happen on what is actually in the posted patch. That is what reviewers
are guaranteed to have read, and it is what people reading "git log" in
a year will see. If there was other discussion or context that led to
the patch, it's helpful in both cases to summarize it.

Seriously? You want to add a note saying "These tests don't tackle any
known issues". Well, if it did, logically, it would have been
mentioned.

quoted

quoted

    The answer can
    be a simple "nobody bothered to write them", and that's OK.

 That can be derived from the word "add". You can't add something that
is already there.

Are there already git-blame tests, but not "blame -e" tests? If there
are already "blame" tests, why do we additionally need "blame -e" tests?

You are right, it's impossible to decipher that the output of 'blame
-e' is different than 'blame'. Surely, somebody must have made a
mistake when the patch was applied.

Or we can apply common sense.

I can guess, or I can do my own digging in the history to find out, but
that makes review a lot more painful. You already did the digging and
came to understand the problem when you made your patch. Why not just
share it?

Because there's no need.

Why would anybody add tests that are already there? Why do you have to
assume the worst?

quoted

quoted

    But
    some description of the current state can help reviewers understand
    the rationale (e.g., "we tested with shortlog, but not mailmap, and
    certain code paths are only exposed through mailmap").

You are assuming too much. That's not the purpose, that's why I didn't menti

Sorry, that should have been s/mailmap/blame/ above. But if I am making
wrong assumptions about the rationale, then isn't that a sign that the
commit message is insufficient?

If you mean the second patch, this was already pointed out by Junio,
and I already said I would clarify that these are meant to target 'git
blame' output.

quoted

quoted

 2. Figure out what the change should be doing.

t: mailmap: add 'git blame -e' tests

That's what the change should be doing; nothing more, nothing less.

Yes, I think you did describe the "what", which in this case is very
simple. But as I mentioned before, it is not just knowing the "what" but
evaluating that the "what" meets the "why" from step 1.

The why can be derived. Unless you seriously think Junio would allow
patches that say they "add" tests for something (which imply there
isn't tests for that), where they don't.

quoted

I wonder why you have to assume the worst. When I see a commit message
like that, I assume that there were no previous tests for that (thus
the word 'add'), and that's all I need to know.

I don't necessarily assume the worst. If I were the maintainer, I might
even have taken your patch as-is, as it's pretty simple. But I found a
description like the one Jonathan included to be _much_ easier to
review. Whether yours was above a minimum threshold or not, I think it's
worth striving to be better.

Better != more words. English, like code, is good as simple as
possible (but not simpler).

If you start from the premise that the patch is good; it has been
s-o-b by Junio, and it has been applied, and released, what more do
you need from the summary "t: mailmap: add 'git blame -e' tests". Can
you derive the rest?

Cheers.

-- 
Felipe Contreras

On Wed, Feb 15, 2012 at 12:22:53AM +0200, Felipe Contreras wrote:

Better != more words. English, like code, is good as simple as
possible (but not simpler).

I'm not going to bother responding to your email point by point, as I
have very little else to say.

I am telling you that as a reviewer of git patches, I found the more
verbose message by Jonathan to be much easier to review than your
one-line description. You may disagree. I offer my experience merely as
a data point, and I will let others draw their own conclusions. If you
want to ignore me, that's your right, but I don't see any point in
arguing further about it.

-Peff

On Wed, Feb 15, 2012 at 12:21 AM, Jonathan Nieder [off-list ref] wrote:

Now I have offered some suggestions for improving some of your
patches.  Applying these suggestions would be effortless, since I sent
them in patch form.  What is your response?  You send a point-by-point
rebuttal explaining how each detail of my suggestions is bad, bad,
bad, and then resend the original with comparatively small changes.

That's a difference of opinion. And I didn't say the where 'bad'; I
just didn't agree with them.

Can you see how this might indicate a stronger difference of opinion
than you have mentioned?  And how a reviewer might make the mistake of
thinking his comments were unwelcome after such an experience?

Your comments are welcome. My comments back are not?

-- 
Felipe Contreras

On Tue, Feb 14, 2012 at 11:50 PM, Jonathan Nieder [off-list ref] wrote:

Felipe Contreras wrote:

quoted

On Tue, Feb 14, 2012 at 10:35 PM, Jonathan Nieder [off-list ref] wrote:

quoted

quoted

(2) 'email@example.com'
becomes the canonical author email for commits with author name 'A U
Thor'.

That's not true. I initially thought that was the case, and I think it
might be useful to have that, but it's not the case now, and your
patch doesn't test this.

Thanks for explaining.  I had indeed confused myself into thinking 'A
U Thor [off-list ref]' would act like 'A U Thor
[off-list ref] [off-list ref]'.

I should have said:

-- 8< --
A mailmap entry in the format 'A U Thor [off-list ref]' means
that 'A U Thor' should be the canonical author name for commits
with author address 'email@example.com', and the email address
should be left alone.

We already have tests for this format regarding the committer name,
but not in the author name, so the tests do not cover the shortlog and
blame codepaths as they should.  Fix that.
-- >8 --

At which point the summary doesn't seem to be correct "test: mailmap
can change author name without changing email". Plus, I fail to see
what would be the usefulness of this test, as changing name is
changing the name, regardless if it's the author or the committer.

[...]

quoted

quoted

Signed-off-by: Felipe Contreras <redacted>

I most definitely did not sign this off, and I didn't add any of these
lines, nor wrote anything about this commit message.

That's why I described the changes I made, signed with my initials,
and put my own sign-off below yours.  Did I screw up somewhere?

You more or less rewrote the whole thing, I don't think you should put
my s-o-b in those cases.

Anyway, I have seen people use this format:

Signed-off-by: Felipe Contreras <redacted>
[jn: rewrite the patch]
Signed-off-by: Jonathan Nieder <redacted>

Even better:

Based on a patch by Felipe Contreras.

Signed-off-by: Jonathan Nieder <redacted>

Note that I am making these changes because, at its heart, I think
your patch is good and useful.  Otherwise I would have ignored it and
worked on something else.  If you prefer that I don't make
improvements like this, please indicate why that's a good idea;
otherwise I will probably continue to do it when I see good patches,
despite all the signals you are giving that I have done something
awful by corrupting your perfect patch in this way.

I didn't hint I preferred that. You said you wanted to improve my
patches, why can't I do the same?

-- 
Felipe Contreras

On Tue, Feb 14, 2012 at 11:59 PM, Jonathan Nieder [off-list ref] wrote:

Do you mean you do not agree with the following?

       (a) The contribution was created in whole or in part by me and I
           have the right to submit it under the open source license
           indicated in the file; or

Anybody can come up with that patch from scratch, all you have to do
is copy the previous test, change 'git blame' to use 'git blame -e',
and fix the expect file. And the subtle diff from my own patch changes
the whole meaning of it, so I wouldn't say it came from me.

But I don't care much, do whatever is more convenient for you, but if
possible, I would remove my s-o-b from it as I have not agreed on it.
And as I said; this is two patches in one.

-- 
Felipe Contreras