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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Feb 6, 2012, 21:32 UTC
Message-ID: <CAMP44s356usz+Zxf1ooV=FEVxrFa2jRG0dWX5beLCvBTs-OMug@mail.gmail.com>
In-Reply-To: <20120204234237.GB1366@sigill.intra.peff.net>

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

Show 14 quoted lines

> 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