On Tue, Feb 14, 2012 at 11:15 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Felipe Contreras wrote:
>> On Tue, Feb 14, 2012 at 10:34 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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.
> [...]
>>> 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 <marius@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html