[PATCH 0/3] blame: fix output with mailmap

[PATCH 0/3] blame: fix output with mailmap

[Felipe Contreras]

The fix, and the tests.

Felipe Contreras (3):
  blame: fix email output with mailmap
  t: mailmap: add 'git blame -e' tests
  t: mailmap: add simple name translation test

 builtin/blame.c    |    9 ++++++---
 t/t4203-mailmap.sh |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 3 deletions(-)

-- 
1.7.9.1.g97f7d

[PATCH 1/3] blame: fix email output with mailmap

[Felipe Contreras]

An extra '>' is added in some cases (<spearce@spearce.org>>), for
example:

 % git blame -e -L 947,+7 contrib/completion/git-completion.bash v1.7.9

The current code assumes map_user() *always* returns plain emails, but
that's not true; when there's no email substitution (only name),
map_user() would return 1, but don't touch the mail.

Also, add some tests.

This got broken by d20d654[1].

[1] Change current mailmap usage to do matching on both name and email
    of author/committer.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/blame.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 5a67c20..dd69e51 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1403,10 +1403,13 @@ static void get_ac_line(const char *inbuf, const char *what,
 	 * Now, convert both name and e-mail using mailmap
 	 */
 	if (map_user(&mailmap, mail+1, mail_len-1, person, tmp-person-1)) {
-		/* Add a trailing '>' to email, since map_user returns plain emails
-		   Note: It already has '<', since we replace from mail+1 */
+		/*
+		 * Add a trailing '>' to email, since map_user returns plain
+		 * emails when it finds a matching mail.
+		 * Note: It already has '<', since we replace from mail + 1
+		 */
 		mailpos = memchr(mail, '\0', mail_len);
-		if (mailpos && mailpos-mail < mail_len - 1) {
+		if (mailpos && mailpos-mail < mail_len - 1 && *(mailpos - 1) != '>') {
 			*mailpos = '>';
 			*(mailpos+1) = '\0';
 		}
-- 
1.7.9.1.g97f7d

[PATCH 2/3] t: mailmap: add 'git blame -e' tests

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t4203-mailmap.sh |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 1f182f6..b1bc521 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -255,4 +255,22 @@ test_expect_success 'Blame output (complex mapping)' '
 	test_cmp expect actual.fuzz
 '
 
+# git blame
+cat >expect <<\EOF
+^OBJI (<author@example.com>       DATE 1) one
+OBJID (<some@dude.xx>             DATE 2) two
+OBJID (<other@author.xx>          DATE 3) three
+OBJID (<other@author.xx>          DATE 4) four
+OBJID (<santa.claus@northpole.xx> DATE 5) five
+OBJID (<santa.claus@northpole.xx> DATE 6) six
+OBJID (<cto@company.xx>           DATE 7) seven
+EOF
+test_expect_success 'Blame output (complex mapping)' '
+	git blame -e one >actual &&
+	cp actual /tmp &&
+	cp internal_mailmap/.mailmap /tmp &&
+	fuzz_blame actual >actual.fuzz &&
+	test_cmp expect actual.fuzz
+'
+
 test_done
-- 
1.7.9.1.g97f7d

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

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t4203-mailmap.sh |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index b1bc521..589e39e 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -157,6 +157,9 @@ A U Thor <author@example.com> (1):
 CTO <cto@company.xx> (1):
       seventh
 
+Mr. Right <right@company.xx> (1):
+      eight
+
 Other Author <other@author.xx> (2):
       third
       fourth
@@ -196,6 +199,11 @@ test_expect_success 'Shortlog output (complex mapping)' '
 	test_tick &&
 	git commit --author "CTO <cto@coompany.xx>" -m seventh &&
 
+	echo eight >>one &&
+	git add one &&
+	test_tick &&
+	git commit --author "Wrong <right@company.xx>" -m eight &&
+
 	mkdir -p internal_mailmap &&
 	echo "Committed <committer@example.com>" > internal_mailmap/.mailmap &&
 	echo "<cto@company.xx>                       <cto@coompany.xx>" >> internal_mailmap/.mailmap &&
@@ -204,6 +212,7 @@ test_expect_success 'Shortlog output (complex mapping)' '
 	echo "Other Author <other@author.xx>         <nick2@company.xx>" >> internal_mailmap/.mailmap &&
 	echo "Santa Claus <santa.claus@northpole.xx> <me@company.xx>" >> internal_mailmap/.mailmap &&
 	echo "Santa Claus <santa.claus@northpole.xx> <me@company.xx>" >> internal_mailmap/.mailmap &&
+	echo "Mr. Right <right@company.xx>" >> internal_mailmap/.mailmap &&
 
 	git shortlog -e HEAD >actual &&
 	test_cmp expect actual
@@ -212,6 +221,9 @@ test_expect_success 'Shortlog output (complex mapping)' '
 
 # git log with --pretty format which uses the name and email mailmap placemarkers
 cat >expect <<\EOF
+Author Wrong <right@company.xx> maps to Mr. Right <right@company.xx>
+Committer C O Mitter <committer@example.com> maps to Committed <committer@example.com>
+
 Author CTO <cto@coompany.xx> maps to CTO <cto@company.xx>
 Committer C O Mitter <committer@example.com> maps to Committed <committer@example.com>
 
@@ -248,6 +260,7 @@ OBJID (Other Author DATE 4) four
 OBJID (Santa Claus  DATE 5) five
 OBJID (Santa Claus  DATE 6) six
 OBJID (CTO          DATE 7) seven
+OBJID (Mr. Right    DATE 8) eight
 EOF
 test_expect_success 'Blame output (complex mapping)' '
 	git blame one >actual &&
@@ -264,6 +277,7 @@ OBJID (<other@author.xx>          DATE 4) four
 OBJID (<santa.claus@northpole.xx> DATE 5) five
 OBJID (<santa.claus@northpole.xx> DATE 6) six
 OBJID (<cto@company.xx>           DATE 7) seven
+OBJID (<right@company.xx>         DATE 8) eight
 EOF
 test_expect_success 'Blame output (complex mapping)' '
 	git blame -e one >actual &&
-- 
1.7.9.1.g97f7d

Re: [PATCH 2/3] t: mailmap: add 'git blame -e' tests

[Jonathan Nieder]

Felipe Contreras wrote:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Thanks for writing tests.  I think there is room for a few lines of
explanation above.

[...]
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -255,4 +255,22 @@ test_expect_success 'Blame output (complex mapping)' '
>  	test_cmp expect actual.fuzz
>  '
>  
> +# git blame
> +cat >expect <<\EOF
> +^OBJI (<author@example.com>       DATE 1) one
> +OBJID (<some@dude.xx>             DATE 2) two
> +OBJID (<other@author.xx>          DATE 3) three
> +OBJID (<other@author.xx>          DATE 4) four
> +OBJID (<santa.claus@northpole.xx> DATE 5) five
> +OBJID (<santa.claus@northpole.xx> DATE 6) six
> +OBJID (<cto@company.xx>           DATE 7) seven
> +EOF
> +test_expect_success 'Blame output (complex mapping)' '

Since I didn't receive a copy of the cover letter or patch 1, I don't
know what this is intended to test _for_.  Good --- I can more easily
convey the reaction of future readers who do not necessarily know the
context in which the patch was written (and the commit message does
not seem to say).

Looking above, I see

 - a lone comment "git blame".  What is it trying to tell me?  I guess
   you copy/pasted it, but is there any purpose to it?

 - a test asserting the claim "Blame output (complex mapping)".  This
   title is identical to the test before.  I have no idea what this is
   about.

Puzzled,
Jonathan

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

[Jonathan Nieder]

Felipe Contreras wrote:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Similar comments to the last patch apply here.  This time the patch
is even more mysterious, since it seems to touch a number of test
assertions, even while I assume not all of them relate to whatever
this is supposed to check for.

Re: [PATCH 2/3] t: mailmap: add 'git blame -e' tests

[Felipe Contreras]

On Sat, Feb 4, 2012 at 10:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Since I didn't receive a copy of the cover letter or patch 1, I don't
> know what this is intended to test _for_.  Good --- I can more easily
> convey the reaction of future readers who do not necessarily know the
> context in which the patch was written (and the commit message does
> not seem to say).
>
> Looking above, I see
>
>  - a lone comment "git blame".  What is it trying to tell me?  I guess
>   you copy/pasted it, but is there any purpose to it?
>
>  - a test asserting the claim "Blame output (complex mapping)".  This
>   title is identical to the test before.  I have no idea what this is
>   about.

Look at the title:
add 'git blame -e' tests

s/blame/blame -e/

-- 
Felipe Contreras

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

[Felipe Contreras]

On Sat, Feb 4, 2012 at 10:12 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> Similar comments to the last patch apply here.  This time the patch
> is even more mysterious, since it seems to touch a number of test
> assertions, even while I assume not all of them relate to whatever
> this is supposed to check for.

Title: mailmap: add simple name translation test

-- 
Felipe Contreras

Re: [PATCH 2/3] t: mailmap: add 'git blame -e' tests

[Jonathan Nieder]

Felipe Contreras wrote:
> On Sat, Feb 4, 2012 at 10:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> Since I didn't receive a copy of the cover letter or patch 1, I don't
>> know what this is intended to test _for_.  Good --- I can more easily
>> convey the reaction of future readers who do not necessarily know the
>> context in which the patch was written (and the commit message does
>> not seem to say).
[...]
> Look at the title:
> add 'git blame -e' tests
>
> s/blame/blame -e/

And?  After copy/pasting this particular test with that substitution,
what do we get a test for?  What class of problem is it supposed to
catch?  I do not think a sentence or two is too much to ask for.

By the way, "I blindly copy/pasted" does not seem like a very sensible
excuse for writing meaningless code (such as the "# git blame" comment
line).  Before the code contained one riddle.  Afterwards it has two.

Hope that helps,
Jonathan

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

[Jonathan Nieder]

Felipe Contreras wrote:

> Title: mailmap: add simple name translation test

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

Re: [PATCH 2/3] t: mailmap: add 'git blame -e' tests

[Felipe Contreras]

On Sat, Feb 4, 2012 at 11:13 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Sat, Feb 4, 2012 at 10:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> Since I didn't receive a copy of the cover letter or patch 1, I don't
>>> know what this is intended to test _for_.  Good --- I can more easily
>>> convey the reaction of future readers who do not necessarily know the
>>> context in which the patch was written (and the commit message does
>>> not seem to say).
> [...]
>> Look at the title:
>> add 'git blame -e' tests
>>
>> s/blame/blame -e/
>
> And?  After copy/pasting this particular test with that substitution,
> what do we get a test for?

For 'git blame -e'.

> What class of problem is it supposed to catch?

Problems related to 'git blame -e'?

> By the way, "I blindly copy/pasted" does not seem like a very sensible
> excuse for writing meaningless code (such as the "# git blame" comment
> line).  Before the code contained one riddle.  Afterwards it has two.

Fine, the drop the patch then... Who needs to test 'git blame -e'
anyway, the current situation of having zero tests for it is perfectly
fine.

Or just apply it. Don't let the perfect be the enemy of the good.

Cheers.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Sat, Feb 4, 2012 at 11:15 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> Title: mailmap: add simple name translation test
>
> 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.

-- 
Felipe Contreras

Re: [PATCH 1/3] blame: fix email output with mailmap

[Jeff King]

On Sat, Feb 04, 2012 at 09:50:22PM +0200, Felipe Contreras wrote:

> diff --git a/builtin/blame.c b/builtin/blame.c
> index 5a67c20..dd69e51 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1403,10 +1403,13 @@ static void get_ac_line(const char *inbuf, const char *what,
>  	 * Now, convert both name and e-mail using mailmap
>  	 */
>  	if (map_user(&mailmap, mail+1, mail_len-1, person, tmp-person-1)) {
> -		/* Add a trailing '>' to email, since map_user returns plain emails
> -		   Note: It already has '<', since we replace from mail+1 */
> +		/*
> +		 * Add a trailing '>' to email, since map_user returns plain
> +		 * emails when it finds a matching mail.
> +		 * Note: It already has '<', since we replace from mail + 1
> +		 */
>  		mailpos = memchr(mail, '\0', mail_len);
> -		if (mailpos && mailpos-mail < mail_len - 1) {
> +		if (mailpos && mailpos-mail < mail_len - 1 && *(mailpos - 1) != '>') {
>  			*mailpos = '>';
>  			*(mailpos+1) = '\0';

I'm not sure if it's possible, but do you need to be checking that
"mailpos > mail" to avoid reading off the beginning of the buffer?
It would mean the email field is empty, which may or may not be
possible.

-Peff

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

[Jeff King]

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.

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

-Peff

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

[Jonathan Nieder]

Felipe Contreras wrote:

> You mean the commit message, you haven't made any comment about the code.

No, for this patch, more important than the absence of any explanation
in the commit message (which is also important) is the code change
that seems unnecessarily invasive.

You've already demonstrated that I do not have the right communication
style to explain such things to you and work towards a fix that
addresses both our concerns.  So I give up.  I'll just give my
feedback on patches that concern code I care about and an explanation
for the sake of others on the list that are better able to interact
with you.  I am willing to work with or answer questions from anyone
including you, though.

Sorry,
Jonathan

Re: [PATCH 1/3] blame: fix email output with mailmap

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Cc: git@vger.kernel.org,  Junio C Hamano <gitster@pobox.com>,  "Brian Gianforcaro" <b.gianfo@gmail.com>,  "Marius Storm-Olsen" <marius@trolltech.com>,  "Junio C Hamano" <junkio@cox.net>

I hope you noticed that Cc lists like the above really prove my point
that you cannot automate common sense.  Your cccmd script apparently
uses the line range in

> diff --git a/builtin/blame.c b/builtin/blame.c
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1403,10 +1403,13 @@

to determine who to Cc.  But in doing so, it dug up an old address for
Junio.  It also added Brian whose only fault in all of this was fixing
style of the 'if' you are patching.

And on top of that it had absolutely no way of knowing that Cc'ing Peff
would have been a good idea, seeing as you two were involved in an
earlier discussion about this precise bug.

(Granted, omitting *Peff* doesn't make that much of a difference, since
for all I know he reads every email that crosses this list.  But my
point still stands.)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 2/3] t: mailmap: add 'git blame -e' tests

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>>> Look at the title:
>>> add 'git blame -e' tests
>>>
>>> s/blame/blame -e/
>>
>> And?  After copy/pasting this particular test with that substitution,
>> what do we get a test for?
>
> For 'git blame -e'.
>
>> What class of problem is it supposed to catch?
>
> Problems related to 'git blame -e'?

You very well know that we know you better than that, so it is no use to
pretend to be dumb.  It does not do anything other than wasting bandwidth
and irritating readers.

You know that you are not addressing "git blame with the -e option shows
wrong line numbers on its output".  The symptom you are checking with is
"e-mail address output from 'blame -e' used to add an extra '>' at the end
when only name is mapped, and I fixed it with the previous patch."

Why is it so hard to either

 (1) give the more descriptive answer upfront when somebody who did not
     read the first patch of the 3-patch series pointed it out the comment
     is not descriptive enough the first time; or

 (2) give the more descriptive answer and then say "we could do that, but
     when somebody views this change in "git log" as a part of 3-patch
     series, it should be clear enough---so let's aim for brevity instead
     of adding that two-line description" to defend the line in the patch?

> Or just apply it. Don't let the perfect be the enemy of the good.

Perfect is the enemy of the good, but it is not an excuse to be sloppy.

I tend to think that a single line "# blame -e" is sufficient if this were
a part of just a single patch that has the fix and the test to guard the
fix against future breakage (i.e. "not sloppy"). I would even say that not
even "# blame" is necessary in such a case.

But if this is a standalone patch to add this test, it does not describe
what it wants to test very well.

In any case, I have doubts that the fix should go to blame and not to
map_user(), so I'll see what happens in the further discussions.

Thanks.

Re: [PATCH 1/3] blame: fix email output with mailmap

[Felipe Contreras]

On Sun, Feb 5, 2012 at 9:57 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Cc: git@vger.kernel.org,  Junio C Hamano <gitster@pobox.com>,  "Brian Gianforcaro" <b.gianfo@gmail.com>,  "Marius Storm-Olsen" <marius@trolltech.com>,  "Junio C Hamano" <junkio@cox.net>
>
> I hope you noticed that Cc lists like the above really prove my point
> that you cannot automate common sense.  Your cccmd script apparently
> uses the line range in

The fact that my script is less than perfect, doesn't mean you
*cannot* automate. Again, look at Linux's scripts/get_maintainer.pl.
It works * perfectly* fine.

>> diff --git a/builtin/blame.c b/builtin/blame.c
>> --- a/builtin/blame.c
>> +++ b/builtin/blame.c
>> @@ -1403,10 +1403,13 @@
>
> to determine who to Cc.  But in doing so, it dug up an old address for
> Junio.  It also added Brian whose only fault in all of this was fixing
> style of the 'if' you are patching.

Yes, it can be improved. But it's still better than nothing.

> And on top of that it had absolutely no way of knowing that Cc'ing Peff
> would have been a good idea, seeing as you two were involved in an
> earlier discussion about this precise bug.
>
> (Granted, omitting *Peff* doesn't make that much of a difference, since
> for all I know he reads every email that crosses this list.  But my
> point still stands.)

That's not the fault of the script; that's what --cc is for.

-- 
Felipe Contreras

Re: [PATCH 2/3] t: mailmap: add 'git blame -e' tests

[Felipe Contreras]

On Sun, Feb 5, 2012 at 10:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>>> Look at the title:
>>>> add 'git blame -e' tests
>>>>
>>>> s/blame/blame -e/
>>>
>>> And?  After copy/pasting this particular test with that substitution,
>>> what do we get a test for?
>>
>> For 'git blame -e'.
>>
>>> What class of problem is it supposed to catch?
>>
>> Problems related to 'git blame -e'?
>
> You very well know that we know you better than that, so it is no use to
> pretend to be dumb.  It does not do anything other than wasting bandwidth
> and irritating readers.

That description is *perfectly* fine. It's succinct, there's nothing
wrong with that.

There's *nothing* else to say. There's no tests for 'git blame -e',
the patch adds tests for 'git blame -e', that's all, thus the title
"add 'git blame -e' tests.

The patch is good by itself, it doesn't _need_ any other context. It
would detect regressions on 'git blame -e', obviously, which is good.

> You know that you are not addressing "git blame with the -e option shows
> wrong line numbers on its output".  The symptom you are checking with is
> "e-mail address output from 'blame -e' used to add an extra '>' at the end
> when only name is mapped, and I fixed it with the previous patch."

No, that's not true. This patch doesn't do that.

> Why is it so hard to either
>
>  (1) give the more descriptive answer upfront when somebody who did not
>     read the first patch of the 3-patch series pointed it out the comment
>     is not descriptive enough the first time; or

Because it's not needed. Adding tests for 'git blame -e' is good in itself.

Some tests = better than no tests.

>  (2) give the more descriptive answer and then say "we could do that, but
>     when somebody views this change in "git log" as a part of 3-patch
>     series, it should be clear enough---so let's aim for brevity instead
>     of adding that two-line description" to defend the line in the patch?

What is unclear of "add 'git blame -e' tests"? It adds tests, that's good.

>> Or just apply it. Don't let the perfect be the enemy of the good.
>
> Perfect is the enemy of the good, but it is not an excuse to be sloppy.
>
> I tend to think that a single line "# blame -e" is sufficient if this were
> a part of just a single patch that has the fix and the test to guard the
> fix against future breakage (i.e. "not sloppy"). I would even say that not
> even "# blame" is necessary in such a case.
>
> But if this is a standalone patch to add this test, it does not describe
> what it wants to test very well.

It wants to test the output of 'git blame -e', as it can be obviates
from the title. Is there something wrong with that?

> In any case, I have doubts that the fix should go to blame and not to
> map_user(), so I'll see what happens in the further discussions.

This patch is orthogonal to that; there are no tests for 'git blame -e'.

-- 
Felipe Contreras

Re: [PATCH 1/3] blame: fix email output with mailmap

[Jeff King]

On Sun, Feb 05, 2012 at 08:57:17PM +0100, Thomas Rast wrote:

> (Granted, omitting *Peff* doesn't make that much of a difference, since
> for all I know he reads every email that crosses this list.  But my
> point still stands.)

It's not true at all. I didn't read this message, for instance.

-Peff

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

[Felipe Contreras]

On Sun, Feb 5, 2012 at 8:17 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> You mean the commit message, you haven't made any comment about the code.
>
> No, for this patch, more important than the absence of any explanation
> in the commit message (which is also important) is the code change
> that seems unnecessarily invasive.

It only seems that way if you are not familiar with the code, and you
assume the worst.

Feel free propose an alternative change. You would find there's no
better way to simplify those changes.

Here's a tip: look at the previous change "CTO <cto@company.xx>", and
see how it's right next to each an every of the hunks introduced in
this patch.

> You've already demonstrated that I do not have the right communication
> style to explain such things to you and work towards a fix that
> addresses both our concerns.  So I give up.

You are assuming your concerns are valid without actually looking at the code.

> I'll just give my
> feedback on patches that concern code I care about and an explanation
> for the sake of others on the list that are better able to interact
> with you.  I am willing to work with or answer questions from anyone
> including you, though.

Your feedback is most certainly welcome, but you shouldn't assume you
are always right. There's nothing wrong with disagreeing, and in this
case I most definitely disagree, and there's nothing wrong with that.

-- 
Felipe Contreras

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

[Felipe Contreras]

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