patch, 2 partst: 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(-)

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

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

Felipe Contreras <felipe.contreras@gmail.com> writes:

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 <right@company.xx> 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 <gitster@pobox.com> wrote:

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.

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

Cheers.

Hi again,

Felipe Contreras wrote:

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.

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.

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 <felipe.contreras@gmail.com>

A mailmap entry of the format 'A U Thor <email@example.com>' 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 <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4203-mailmap.sh |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

On Tue, Feb 14, 2012 at 10:35 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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.

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.

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

Felipe Contreras wrote:

Thanks for explaining. I had indeed confused myself into thinking 'A U Thor <email@example.com>' would act like 'A U Thor <email@example.com> <email@example.com>'.

I should have said:

-- 8< -- A mailmap entry in the format 'A U Thor <email@example.com>' 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 --

[...]

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:50 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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.

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 <felipe.contreras@gmail.com>
[jn: rewrite the patch]
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Even better:

Based on a patch by Felipe Contreras.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

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

From: Felipe Contreras <felipe.contreras@gmail.com>

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 (<committer@example.com>>   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 <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4203-mailmap.sh |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

On Tue, Feb 14, 2012 at 10:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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.

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

I did not sign this.

Felipe Contreras wrote:

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:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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.

Hi,

On Tue, Feb 14, 2012 at 10:34 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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

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

Thus the 't: mailmap: ' prefix.

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 wrote:

I said "OK", didn't I?

[...]

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.

On Tue, Feb 14, 2012 at 11:15 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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.

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

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 <marius@trolltech.com>
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 <marius@trolltech.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

 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 wrote:

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:21 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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

Your comments are welcome. My comments back are not?

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

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

Jeff King wrote:

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

On Tue, Feb 14, 2012 at 11:14 PM, Jeff King <peff@peff.net> wrote:

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

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

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

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.

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

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.

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?

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?

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 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 Wed, Feb 15, 2012 at 12:07 AM, Jeff King <peff@peff.net> wrote:

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.

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.

Because there's no need.

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

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.

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.

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.

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

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

Felipe Contreras <felipe.contreras@gmail.com> writes:

You are correct to say that you cannot add something that is already there, but that does not mean you explained why that new thing is a good thing to add. In other words, you can add a new thing that we did not have, but it would not result in a good addition if that new thing is irrelevant. Relevance needs to be explained.

I do think in this particular case, the new check *is* relevant, because

    Although we did have "blame" test to see how the name part is shown,
    we had no "blame -e" test to see how the email part is shown.

I do not understand why you are resisting to explain how your addition adds value to the system with such a simple two-liner, and instead are endlessly arguing. Is it to make sure you are the one to utter the last word in the thread?

As I am sort of getting tired of seeing you making things more difficult for yourself, I'll refrain from commenting on this topic at least for a few days to wait until things cool down.

On Wed, Feb 15, 2012 at 12:18 AM, Junio C Hamano <gitster@pobox.com> wrote:

And I don't understand why people want the obvious to be explained. For each one of the questions I've heard, the answer can be derived from the summary *directly*.

Your new point is "you can add a new thing that we did not have, but it would not result in a good addition if that new thing is irrelevant", but you already know what is the new thing from the summary "'git blame -e' tests".

Everybody seems to assume that a simple commit message = bad. I disagree.

The same argument can be applied both ways; you are the one that is making things more difficult. And I already pointed out the double standards.

Does this mean that disagreement is discouraged? From the discussion it seems we are in clear disagreement, but from the rhetoric it seems to me that you all assume you are right by default, and that simple commit messages are by bad definition.

If that's the case I am disappointed, but I would rather just drop the discussion rather than defend my position.

Cheers.

Felipe Contreras <felipe.contreras@gmail.com> writes:

Has it ever occurred to you the reason why people ask questions to you is perhaps because something that is obvious to you who wrote the patch is not obvious at all to others? Has it also occurred to you that the majority of people who need to understand the patch during the review and 6 months down the road in "git log" output are not *you*?

It is not a "new point". Jonathan, Peff and I all never said that it is unclear "what" your patch adds. The suggestions for improvement given in this thread were all to explain "why" better.

If you find *everybody* seems to disagree with you, it would help to consider a slight possibility that you *might* be wrong. And "simple" is not necessarily "sufficient and simple".

Sorry, but the absolute uniform standards do not exist, unless you are living in a fantasy land. I expect better from list regulars as new contributors will inevitably learn from their behaviour (we also learn from our past mistakes).

On Wed, Feb 15, 2012 at 12:49 AM, Junio C Hamano <gitster@pobox.com> wrote:

Yes, that's why I am still listening. However I have not yet found a question that cannot be answered from the simple description, except the one you brought and I agreed to tackle.

I would like to think I have the capacity of empathy, so I would be able to see if something cannot be inferred from the commit message. Of course, I might be wrong, but so far the feedback has not been "no, it's not obvious", but rather " yes, it's obvious... but...".

I have heard both. And the "why" can be easily inferred, at least on the first patch. The second one I yet have to fix, as I already replied to you.

Of course, it's *possible*, but ad populum is not a valid argument.

There's many projects out there, and very few have as verbose commit messages as git. I do not say they are doing it better, as many times a lot of the verbose commit messages do help, but I don't think this is the case.

Looking at the latest commits in the Linux kernel show good examples of simple commit messages. Albeit they might be a bit "too simple", my point that they do have a different view on what is "sufficient and simple":

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=27c3afe6e1cf129faac90405121203962da08ff4 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=0d86f65ed0b727daa06d3aa176314cd175323db6 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=10f296cbfe3b93188c41463fd7a53808ebdbcbe3

Of course, what is "sufficient and simple" depends on a case by case basis, but I wonder, if there's any case in which a single line in the commit message summary would suffice, wouldn't adding missing inconspicuous tests for something be it?

Of course, but I can't help but wonder... Why so much fuss for simple tests? And why you didn't bother to add tests for your new code as well?

Cheers.