Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug

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