Re: [PATCH 3/3] t: mailmap: add simple name translation test

[Felipe Contreras <felipe.contreras@gmail.com>]

On Sun, Feb 5, 2012 at 1:42 AM, Jeff King <peff@peff.net> wrote:
> On Sun, Feb 05, 2012 at 12:19:53AM +0200, 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. :/
>>
>> 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.
>
> Then say that in the commit message.

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

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

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.

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

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