patch, 3 partsblame: fix output with mailmap

The fix, and the tests.

Felipe Contreras (3):
  blame: fix email output with mailmap
  t: mailmap: add 'git blame -e' tests
  t: mailmap: add simple name translation test

 builtin/blame.c    |    9 ++++++---
 t/t4203-mailmap.sh |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 3 deletions(-)

An extra '>' is added in some cases (<spearce@spearce.org>>), for example:

 % git blame -e -L 947,+7 contrib/completion/git-completion.bash v1.7.9

The current code assumes map_user() *always* returns plain emails, but that's not true; when there's no email substitution (only name), map_user() would return 1, but don't touch the mail.

Also, add some tests.

This got broken by d20d654[1].

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/blame.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

On Sat, Feb 04, 2012 at 09:50:22PM +0200, Felipe Contreras wrote:

I'm not sure if it's possible, but do you need to be checking that "mailpos > mail" to avoid reading off the beginning of the buffer? It would mean the email field is empty, which may or may not be possible.

-Peff

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

I hope you noticed that Cc lists like the above really prove my point that you cannot automate common sense. Your cccmd script apparently uses the line range in

to determine who to Cc. But in doing so, it dug up an old address for Junio. It also added Brian whose only fault in all of this was fixing style of the 'if' you are patching.

And on top of that it had absolutely no way of knowing that Cc'ing Peff would have been a good idea, seeing as you two were involved in an earlier discussion about this precise bug.

(Granted, omitting *Peff* doesn't make that much of a difference, since for all I know he reads every email that crosses this list. But my point still stands.)

On Sun, Feb 5, 2012 at 9:57 PM, Thomas Rast <trast@inf.ethz.ch> wrote:

The fact that my script is less than perfect, doesn't mean you *cannot* automate. Again, look at Linux's scripts/get_maintainer.pl. It works * perfectly* fine.

Yes, it can be improved. But it's still better than nothing.

That's not the fault of the script; that's what --cc is for.

On Sun, Feb 05, 2012 at 08:57:17PM +0100, Thomas Rast wrote:

It's not true at all. I didn't read this message, for instance.

-Peff

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

Felipe Contreras wrote:

Thanks for writing tests. I think there is room for a few lines of explanation above.

[...]

Since I didn't receive a copy of the cover letter or patch 1, I don't know what this is intended to test _for_. Good --- I can more easily convey the reaction of future readers who do not necessarily know the context in which the patch was written (and the commit message does not seem to say).

Looking above, I see

 - a lone comment "git blame".  What is it trying to tell me?  I guess
   you copy/pasted it, but is there any purpose to it?

 - a test asserting the claim "Blame output (complex mapping)".  This
   title is identical to the test before.  I have no idea what this is
   about.

Puzzled, Jonathan

On Sat, Feb 4, 2012 at 10:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Look at the title: add 'git blame -e' tests

s/blame/blame -e/

Felipe Contreras wrote:

[...]

And? After copy/pasting this particular test with that substitution, what do we get a test for? What class of problem is it supposed to catch? I do not think a sentence or two is too much to ask for.

By the way, "I blindly copy/pasted" does not seem like a very sensible excuse for writing meaningless code (such as the "# git blame" comment line). Before the code contained one riddle. Afterwards it has two.

Hope that helps, Jonathan

On Sat, Feb 4, 2012 at 11:13 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

For 'git blame -e'.

Problems related to 'git blame -e'?

Fine, the drop the patch then... Who needs to test 'git blame -e' anyway, the current situation of having zero tests for it is perfectly fine.

Or just apply it. Don't let the perfect be the enemy of the good.

Cheers.

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

You very well know that we know you better than that, so it is no use to pretend to be dumb. It does not do anything other than wasting bandwidth and irritating readers.

You know that you are not addressing "git blame with the -e option shows wrong line numbers on its output". The symptom you are checking with is "e-mail address output from 'blame -e' used to add an extra '>' at the end when only name is mapped, and I fixed it with the previous patch."

Why is it so hard to either

 (1) give the more descriptive answer upfront when somebody who did not
     read the first patch of the 3-patch series pointed it out the comment
     is not descriptive enough the first time; or

 (2) give the more descriptive answer and then say "we could do that, but
     when somebody views this change in "git log" as a part of 3-patch
     series, it should be clear enough---so let's aim for brevity instead
     of adding that two-line description" to defend the line in the patch?

Perfect is the enemy of the good, but it is not an excuse to be sloppy.

I tend to think that a single line "# blame -e" is sufficient if this were a part of just a single patch that has the fix and the test to guard the fix against future breakage (i.e. "not sloppy"). I would even say that not even "# blame" is necessary in such a case.

But if this is a standalone patch to add this test, it does not describe what it wants to test very well.

In any case, I have doubts that the fix should go to blame and not to map_user(), so I'll see what happens in the further discussions.

Thanks.

On Sun, Feb 5, 2012 at 10:38 PM, Junio C Hamano <gitster@pobox.com> wrote:

That description is *perfectly* fine. It's succinct, there's nothing wrong with that.

There's *nothing* else to say. There's no tests for 'git blame -e', the patch adds tests for 'git blame -e', that's all, thus the title "add 'git blame -e' tests.

The patch is good by itself, it doesn't _need_ any other context. It would detect regressions on 'git blame -e', obviously, which is good.

No, that's not true. This patch doesn't do that.

Because it's not needed. Adding tests for 'git blame -e' is good in itself.

Some tests = better than no tests.

What is unclear of "add 'git blame -e' tests"? It adds tests, that's good.

It wants to test the output of 'git blame -e', as it can be obviates from the title. Is there something wrong with that?

This patch is orthogonal to that; there are no tests for 'git blame -e'.

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

Felipe Contreras wrote:

Similar comments to the last patch apply here. This time the patch is even more mysterious, since it seems to touch a number of test assertions, even while I assume not all of them relate to whatever this is supposed to check for.

On Sat, Feb 4, 2012 at 10:12 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Title: mailmap: add simple name translation test

Felipe Contreras wrote:

Thanks. I guess you think I'm stupid. I have no idea how I can correct that assumption and help you to actually work with me to make the code better. :/

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

You mean the commit message, you haven't made any comment about the code.

If you want to know why I had to modify those test assertions, you really need to look at the code. In essence; all of them use the same repo, and obviously adding a new commit message changes the output of the commands.

On Sun, Feb 05, 2012 at 12:19:53AM +0200, Felipe Contreras wrote:

Then say that in the commit message.

Looking at this series, I wonder if the tests should simply be squashed into the bugfix patch, which might make what is going on more obvious. Keep in mind that as reviewers now, we read the whole series. But in a year, as "git log" users, we may see the commits in isolation.

-Peff

On Sun, Feb 5, 2012 at 1:42 AM, Jeff King <peff@peff.net> wrote:

I believe that's overkill. If somebody needs an explanation, it's because they are not familiar with the code being modified, and introducing people to some code in each and every patch that modifies it definitely seems like overkill to me. There is not even such introduction in the code itself for this test, and in most of git's code, presumably because we are following the principle of having self-documented code.

If you look at the code, it would become obvious why so many hunks are introduced, in fact, if you look closely at the patch you can see it as well: look for strings related to CTO <cto@company.xx>.

Because it's a logically independent change?

There's _nothing_ that prevents this patch from being applied to master *right now*. Of course, it would conflict, because it depends on the 'git blame -e' tests, but if you solve that conflict by just removing that hunk, it would apply and run just fine, and it would detect regressions orthogonal from my other proposed patches.

Sure, and this patch by itself is good; it's adding a missing test (even if you ignore the 'git blame -e' part).

Cheers.

Felipe Contreras wrote:

No, for this patch, more important than the absence of any explanation in the commit message (which is also important) is the code change that seems unnecessarily invasive.

You've already demonstrated that I do not have the right communication style to explain such things to you and work towards a fix that addresses both our concerns. So I give up. I'll just give my feedback on patches that concern code I care about and an explanation for the sake of others on the list that are better able to interact with you. I am willing to work with or answer questions from anyone including you, though.

Sorry, Jonathan

On Sun, Feb 5, 2012 at 8:17 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

It only seems that way if you are not familiar with the code, and you assume the worst.

Feel free propose an alternative change. You would find there's no better way to simplify those changes.

Here's a tip: look at the previous change "CTO <cto@company.xx>", and see how it's right next to each an every of the hunks introduced in this patch.

You are assuming your concerns are valid without actually looking at the code.

Your feedback is most certainly welcome, but you shouldn't assume you are always right. There's nothing wrong with disagreeing, and in this case I most definitely disagree, and there's nothing wrong with that.