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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Feb 14, 2012, 21:06 UTC
Message-ID: <CAMP44s3YRHgMPX2Hzydm_TLB27OABWETjABMcwrHmDk-=pN2hw@mail.gmail.com>
In-Reply-To: <20120214203431.GB13210@burratino>

Hi,

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

Show 10 quoted lines

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

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

Show 23 quoted lines

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

Show 9 quoted lines

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