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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Feb 14, 2012, 22:09 UTC
Message-ID: <CAMP44s1mV2cE=R49qYSLd8eZPhCpRx0hRnnG_-K3iBxp_YQEpQ@mail.gmail.com>
In-Reply-To: <20120214211552.GA9651@burratino>

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

Show 11 quoted lines

> Felipe Contreras wrote:
>> On Tue, Feb 14, 2012 at 10:34 PM, Jonathan Nieder <jrnieder@gmail.com> 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.
>>
>> 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.

Show 19 quoted lines

> [...]
>>> 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.

Show 5 quoted lines

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