{"thread":{"id":"29626","subject":"[PATCH 0/2] t: add blame -e tests for mailmap","startedAt":"2012-02-14T16:11:32Z","lastAt":"2012-02-14T23:14:57Z","messageCount":29,"participants":["Felipe Contreras","Junio C Hamano","Jonathan Nieder","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"184659","messageId":"1329235894-20581-1-git-send-email-felipe.contreras@gmail.com","threadId":"29626","inReplyTo":null,"subject":"[PATCH 0/2] t: add blame -e tests for mailmap","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T16:11:32Z","receivedAt":"2012-02-14T16:11:32Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"I sent both the fix and the tests. Another fix was applied, but we are still\nmissing the tests.\n\nThese are good before, and after the fix.\n\nFelipe Contreras (2):\n t: mailmap: add 'git blame -e' tests\n t: mailmap: add simple name translation test\n\n t/t4203-mailmap.sh | 32 ++++++++++++++++++++++++++++++++\n 1 files changed, 32 insertions(+), 0 deletions(-)\n\n-- \n1.7.9.1.g97f7d\n"},{"id":"184660","messageId":"1329235894-20581-2-git-send-email-felipe.contreras@gmail.com","threadId":"29626","inReplyTo":"1329235894-20581-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/2] t: mailmap: add 'git blame -e' tests","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T16:11:33Z","receivedAt":"2012-02-14T16:11:33Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n t/t4203-mailmap.sh | 18 ++++++++++++++++++\n 1 files changed, 18 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex 1f182f6..db12265 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -255,4 +255,22 @@ test_expect_success 'Blame output (complex mapping)' '\n \ttest_cmp expect actual.fuzz\n '\n \n+# git blame -e\n+cat >expect <<\\EOF\n+^OBJI ( DATE 1) one\n+OBJID ( DATE 2) two\n+OBJID ( DATE 3) three\n+OBJID ( DATE 4) four\n+OBJID ( DATE 5) five\n+OBJID ( DATE 6) six\n+OBJID ( DATE 7) seven\n+EOF\n+test_expect_success 'Blame output (complex mapping)' '\n+\tgit blame -e one >actual &&\n+\tcp actual /tmp &&\n+\tcp internal_mailmap/.mailmap /tmp &&\n+\tfuzz_blame actual >actual.fuzz &&\n+\ttest_cmp expect actual.fuzz\n+'\n+\n test_done\n-- \n1.7.9.1.g97f7d\n"},{"id":"184661","messageId":"1329235894-20581-3-git-send-email-felipe.contreras@gmail.com","threadId":"29626","inReplyTo":"1329235894-20581-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/2] t: mailmap: add simple name translation test","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T16:11:34Z","receivedAt":"2012-02-14T16:11:34Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n t/t4203-mailmap.sh | 14 ++++++++++++++\n 1 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex db12265..fc3855a 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -157,6 +157,9 @@ A U Thor (1):\n CTO (1):\n seventh\n \n+Mr. Right (1):\n+ eight\n+\n Other Author (2):\n third\n fourth\n@@ -196,6 +199,11 @@ test_expect_success 'Shortlog output (complex mapping)' '\n \ttest_tick &&\n \tgit commit --author \"CTO \" -m seventh &&\n \n+\techo eight >>one &&\n+\tgit add one &&\n+\ttest_tick &&\n+\tgit commit --author \"Wrong \" -m eight &&\n+\n \tmkdir -p internal_mailmap &&\n \techo \"Committed \" > internal_mailmap/.mailmap &&\n \techo \" \" >> internal_mailmap/.mailmap &&\n@@ -204,6 +212,7 @@ test_expect_success 'Shortlog output (complex mapping)' '\n \techo \"Other Author \" >> internal_mailmap/.mailmap &&\n \techo \"Santa Claus \" >> internal_mailmap/.mailmap &&\n \techo \"Santa Claus \" >> internal_mailmap/.mailmap &&\n+\techo \"Mr. Right \" >> internal_mailmap/.mailmap &&\n \n \tgit shortlog -e HEAD >actual &&\n \ttest_cmp expect actual\n@@ -212,6 +221,9 @@ test_expect_success 'Shortlog output (complex mapping)' '\n \n # git log with --pretty format which uses the name and email mailmap placemarkers\n cat >expect <<\\EOF\n+Author Wrong maps to Mr. Right \n+Committer C O Mitter maps to Committed \n+\n Author CTO maps to CTO \n Committer C O Mitter maps to Committed \n \n@@ -248,6 +260,7 @@ OBJID (Other Author DATE 4) four\n OBJID (Santa Claus DATE 5) five\n OBJID (Santa Claus DATE 6) six\n OBJID (CTO DATE 7) seven\n+OBJID (Mr. Right DATE 8) eight\n EOF\n test_expect_success 'Blame output (complex mapping)' '\n \tgit blame one >actual &&\n@@ -264,6 +277,7 @@ OBJID ( DATE 4) four\n OBJID ( DATE 5) five\n OBJID ( DATE 6) six\n OBJID ( DATE 7) seven\n+OBJID ( DATE 8) eight\n EOF\n test_expect_success 'Blame output (complex mapping)' '\n \tgit blame -e one >actual &&\n-- \n1.7.9.1.g97f7d\n"},{"id":"184680","messageId":"7v8vk56us3.fsf@alter.siamese.dyndns.org","threadId":"29626","inReplyTo":"1329235894-20581-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 2/2] t: mailmap: add simple name translation test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T20:10:20Z","receivedAt":"2012-02-14T20:10:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Signed-off-by: Felipe Contreras \n> ---\n\nIt was clear that we didn't have any test for \"blame -e\" hence it was no\nbrainer to judge that the patch 1/2 is good without any description.\n\nBut I am scratching my head, deciphering what this patch adds.\n\nIt appears to me that the existing tests that map author@example.com from\nthe original \"A U Thor\" to \"Repo Guy\" and inspect names and mails in\nvarious output already cover this \"Wrong with can be\ncorrected to Mr. Right\" case this patch adds.\n\nWhat am I missing? Instead of explaining it to me, can it be explained in\nthe log message?\n\nThanks.\n\n> t/t4203-mailmap.sh | 14 ++++++++++++++\n> 1 files changed, 14 insertions(+), 0 deletions(-)\n>\n> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\n> index db12265..fc3855a 100755\n> --- a/t/t4203-mailmap.sh\n> +++ b/t/t4203-mailmap.sh\n> @@ -157,6 +157,9 @@ A U Thor (1):\n> CTO (1):\n> seventh\n> \n> +Mr. Right (1):\n> + eight\n> +\n> Other Author (2):\n> third\n> fourth\n> @@ -196,6 +199,11 @@ test_expect_success 'Shortlog output (complex mapping)' '\n> \ttest_tick &&\n> \tgit commit --author \"CTO \" -m seventh &&\n> \n> +\techo eight >>one &&\n> +\tgit add one &&\n> +\ttest_tick &&\n> +\tgit commit --author \"Wrong \" -m eight &&\n> +\n> \tmkdir -p internal_mailmap &&\n> \techo \"Committed \" > internal_mailmap/.mailmap &&\n> \techo \" \" >> internal_mailmap/.mailmap &&\n> @@ -204,6 +212,7 @@ test_expect_success 'Shortlog output (complex mapping)' '\n> \techo \"Other Author \" >> internal_mailmap/.mailmap &&\n> \techo \"Santa Claus \" >> internal_mailmap/.mailmap &&\n> \techo \"Santa Claus \" >> internal_mailmap/.mailmap &&\n> +\techo \"Mr. Right \" >> internal_mailmap/.mailmap &&\n> \n> \tgit shortlog -e HEAD >actual &&\n> \ttest_cmp expect actual\n> @@ -212,6 +221,9 @@ test_expect_success 'Shortlog output (complex mapping)' '\n> \n> # git log with --pretty format which uses the name and email mailmap placemarkers\n> cat >expect <<\\EOF\n> +Author Wrong maps to Mr. Right \n> +Committer C O Mitter maps to Committed \n> +\n> Author CTO maps to CTO \n> Committer C O Mitter maps to Committed \n> \n> @@ -248,6 +260,7 @@ OBJID (Other Author DATE 4) four\n> OBJID (Santa Claus DATE 5) five\n> OBJID (Santa Claus DATE 6) six\n> OBJID (CTO DATE 7) seven\n> +OBJID (Mr. Right DATE 8) eight\n> EOF\n> test_expect_success 'Blame output (complex mapping)' '\n> \tgit blame one >actual &&\n> @@ -264,6 +277,7 @@ OBJID ( DATE 4) four\n> OBJID ( DATE 5) five\n> OBJID ( DATE 6) six\n> OBJID ( DATE 7) seven\n> +OBJID ( DATE 8) eight\n> EOF\n> test_expect_success 'Blame output (complex mapping)' '\n> \tgit blame -e one >actual &&\n"},{"id":"184684","messageId":"CAMP44s2uZA_xYh8Rb8xNJM0Yf3LmNHgzzrLWd9C1wW64T2yVQA@mail.gmail.com","threadId":"29626","inReplyTo":"7v8vk56us3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] t: mailmap: add simple name translation test","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T20:28:14Z","receivedAt":"2012-02-14T20:28:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Feb 14, 2012 at 10:10 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>\n> It was clear that we didn't have any test for \"blame -e\" hence it was no\n> brainer to judge that the patch 1/2 is good without any description.\n>\n> But I am scratching my head, deciphering what this patch adds.\n>\n> It appears to me that the existing tests that map author@example.com from\n> the original \"A U Thor\" to \"Repo Guy\" and inspect names and mails in\n> various output already cover this \"Wrong with can be\n> corrected to Mr. Right\" case this patch adds.\n\nYes, but in the first tests they don't check for 'git blame', and much\nless 'git blame -e', and the second tests only check complex mappings.\n\n> What am I missing?  Instead of explaining it to me, can it be explained in\n> the log message?\n\nIf an explanations along the lines of the above make sense, I can resend.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"184688","messageId":"20120214203431.GB13210@burratino","threadId":"29626","inReplyTo":"1329235894-20581-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-14T20:34:31Z","receivedAt":"2012-02-14T20:34:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nFelipe Contreras wrote:\n\n> I sent both the fix and the tests. Another fix was applied, but we are still\n> missing the tests.\n>\n> These are good before, and after the fix.\n\nTo summarize the previous discussion[1]: some people had comments, and\nyou seem to have found value in exactly none of them. OK. CC-ing\nPeff, since he at least probably has looked over this code before.\n\nBy the way, the address you are using for Marius is out of date.\n\n> Felipe Contreras (2):\n> t: mailmap: add 'git blame -e' tests\n\nSo that people don't destroy this test in later refactorings, I would\nlike to collect statements that we want the test to ensure remain\ntrue.\n\nApparently the fix in f026358e (\"mailmap: always return a plain mail\naddress from map_user()\", 2012-02-05) was for the case of the name\nchanging and email address not changing due to mailmap mapping. Most\ncallers use a separate buffer for the email address so there is room\nto modify the name in place, but \"git blame\" keeps angle brackets in\nthe same buffer for no obvious reason I can see. (Callers should\nbe able to add the brackets themselves instead of relying on\nci.author_mail to contain them, but that's a story for another day.)\n\nAnyway, the existing tests for the returned email missed this since\nit does not affect \"git shortlog -e\" but only \"git blame -e\".\nTherefore this patch reuses the test data for shortlog -e and lets us\nuse it for blame, too. It is easier to understand after the other\npatch, IMHO.\n\nThis is _not_ meant as a more general test for the \"git blame -e\"\nformat (which would belong somewhere near t8008) as far as I can tell.\nIt is just checking that mailmap doesn't screw up.\n\n> t: mailmap: add simple name translation test\n\nBefore, I thought this might be a straightforward test for the bug\nfixed by f026358e. That didn't justify the patch that touches\nseveral different test assertions.\n\nIn fact it seems to be intended to test the case addressed by f026358e\n(name changing, email not) in various mailmap callers: \"git shortlog -e\",\n\"git log --pretty\", \"git blame\".\n\nHere's a reroll.\n\nEnjoy,\nJonathan\n\nFelipe Contreras (2):\n test: mailmap can change author name without changing email\n test: check that \"git blame -e\" uses mailmap correctly\n\n t/t4203-mailmap.sh | 28 ++++++++++++++++++++++++++++\n 1 files changed, 28 insertions(+), 0 deletions(-)\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/189896\n"},{"id":"184689","messageId":"20120214203519.GC13210@burratino","threadId":"29626","inReplyTo":"20120214203431.GB13210@burratino","subject":"[PATCH 1/2] test: mailmap can change author name without changing email","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-14T20:35:19Z","receivedAt":"2012-02-14T20:35:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Felipe Contreras \n\nA mailmap entry of the format 'A U Thor ' has two\neffects: (1) 'A U Thor' becomes the canonical author name for commits\nwith author address 'email@example.com', and (2) 'email@example.com'\nbecomes the canonical author email for commits with author name 'A U\nThor'.\n\nWe already have tests for the effect (1) in the committer name, but\nnot in the author name, so the tests do not cover the shortlog and\nblame codepaths as they should. Fix that.\n\n[jn: simplified by reusing Committer mailmap entry, clarified\n description]\n\nSigned-off-by: Felipe Contreras \nSigned-off-by: Jonathan Nieder \n---\n t/t4203-mailmap.sh | 12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex 1f182f61..45526395 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -157,6 +157,9 @@ A U Thor (1):\n CTO (1):\n seventh\n \n+Committed (1):\n+ eighth\n+\n Other Author (2):\n third\n fourth\n@@ -196,6 +199,11 @@ test_expect_success 'Shortlog output (complex mapping)' '\n \ttest_tick &&\n \tgit commit --author \"CTO \" -m seventh &&\n \n+\techo eight >>one &&\n+\tgit add one &&\n+\ttest_tick &&\n+\tgit commit --author \"C O Mitter \" -m eighth &&\n+\n \tmkdir -p internal_mailmap &&\n \techo \"Committed \" > internal_mailmap/.mailmap &&\n \techo \" \" >> internal_mailmap/.mailmap &&\n@@ -212,6 +220,9 @@ test_expect_success 'Shortlog output (complex mapping)' '\n \n # git log with --pretty format which uses the name and email mailmap placemarkers\n cat >expect <<\\EOF\n+Author C O Mitter maps to Committed \n+Committer C O Mitter maps to Committed \n+\n Author CTO maps to CTO \n Committer C O Mitter maps to Committed \n \n@@ -248,6 +259,7 @@ OBJID (Other Author DATE 4) four\n OBJID (Santa Claus DATE 5) five\n OBJID (Santa Claus DATE 6) six\n OBJID (CTO DATE 7) seven\n+OBJID (Committed DATE 8) eight\n EOF\n test_expect_success 'Blame output (complex mapping)' '\n \tgit blame one >actual &&\n-- \n1.7.9\n"},{"id":"184690","messageId":"20120214203603.GD13210@burratino","threadId":"29626","inReplyTo":"20120214203431.GB13210@burratino","subject":"[PATCH 2/2] test: check that \"git blame -e\" uses mailmap correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-14T20:36:03Z","receivedAt":"2012-02-14T20:36:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Felipe Contreras \n\nUntil f026358e (\"mailmap: always return a plain mail address from\nmap_user()\", 2012-02-05), git blame -e would add a spurious '>' after\nthe unchanged email address with brackets it passed to the mailmap\nmachinery, resulting in lines with a doubled '>' like this:\n\n 620456e6 (> 2005-04-07 15:20:13 -0700 8) eight\n\nAdd a test to make sure it doesn't happen again. This reuses the test\ndata for the existing \"shortlog -e\" test so it also tests other kinds\nof mail mapping.\n\n[jn: fixed some cut+paste cruft, added a patch description]\n\nSigned-off-by: Felipe Contreras \nSigned-off-by: Jonathan Nieder \n---\n t/t4203-mailmap.sh | 16 ++++++++++++++++\n 1 files changed, 16 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex 45526395..ef900d84 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -267,4 +267,20 @@ test_expect_success 'Blame output (complex mapping)' '\n \ttest_cmp expect actual.fuzz\n '\n \n+cat >expect <<\\EOF\n+^OBJI ( DATE 1) one\n+OBJID ( DATE 2) two\n+OBJID ( DATE 3) three\n+OBJID ( DATE 4) four\n+OBJID ( DATE 5) five\n+OBJID ( DATE 6) six\n+OBJID ( DATE 7) seven\n+OBJID ( DATE 8) eight\n+EOF\n+test_expect_success 'Blame -e output' '\n+\tgit blame -e one >actual &&\n+\tfuzz_blame actual >actual.fuzz &&\n+\ttest_cmp expect actual.fuzz\n+'\n+\n test_done\n-- \n1.7.9\n"},{"id":"184692","messageId":"CAMP44s3YRHgMPX2Hzydm_TLB27OABWETjABMcwrHmDk-=pN2hw@mail.gmail.com","threadId":"29626","inReplyTo":"20120214203431.GB13210@burratino","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T21:06:37Z","receivedAt":"2012-02-14T21:06:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\n\nOn Tue, Feb 14, 2012 at 10:34 PM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>\n>> I sent both the fix and the tests. Another fix was applied, but we are still\n>> missing the tests.\n>>\n>> These are good before, and after the fix.\n>\n> To summarize the previous discussion[1]: some people had comments, and\n> you seem to have found value in exactly none of them.  OK.  CC-ing\n> Peff, since he at least probably has looked over this code before.\n\nJust because you have comments doesn't mean I *must* address them. We\nhave a difference of opinion, nothing wrong with that.\n\n> By the way, the address you are using for Marius is out of date.\n>\n>> Felipe Contreras (2):\n>>   t: mailmap: add 'git blame -e' tests\n>\n> So that people don't destroy this test in later refactorings, I would\n> like to collect statements that we want the test to ensure remain\n> true.\n>\n> Apparently the fix in f026358e (\"mailmap: always return a plain mail\n> address from map_user()\", 2012-02-05) was for the case of the name\n> changing and email address not changing due to mailmap mapping.  Most\n> callers use a separate buffer for the email address so there is room\n> to modify the name in place, but \"git blame\" keeps angle brackets in\n> the same buffer for no obvious reason I can see.  (Callers should\n> be able to add the brackets themselves instead of relying on\n> ci.author_mail to contain them, but that's a story for another day.)\n>\n> Anyway, the existing tests for the returned email missed this since\n> it does not affect \"git shortlog -e\" but only \"git blame -e\".\n> Therefore this patch reuses the test data for shortlog -e and lets us\n> use it for blame, too.  It is easier to understand after the other\n> patch, IMHO.\n\nThis is not related to f026358e. The test added by this patch would\npass before and after f026358e.\n\nThis is what I mean by reading too much into the patch. There's\nnothing to it; \"add 'git blame -e' tests\" just adds tests for 'git\nblame -e', it doesn't catch any issues related to f026358e; it just\nmakes sure there are no further regressions with the tests that are\nalready passing.\n\nAgain, some tests for 'git blame -e' > no tests for 'git blame -e'.\n\n> This is _not_ meant as a more general test for the \"git blame -e\"\n> format (which would belong somewhere near t8008) as far as I can tell.\n> It is just checking that mailmap doesn't screw up.\n\nThus the 't: mailmap: ' prefix.\n\n>>   t: mailmap: add simple name translation test\n>\n> Before, I thought this might be a straightforward test for the bug\n> fixed by f026358e.  That didn't justify the patch that touches\n> several different test assertions.\n>\n> In fact it seems to be intended to test the case addressed by f026358e\n> (name changing, email not) in various mailmap callers: \"git shortlog -e\",\n> \"git log --pretty\", \"git blame\".\n\nNo. As the summary says, it's intended to add a simple name\ntranslation test, which is missing from all the tests that spawn from\nthe repository generated in 'Shortlog output (complex mapping)' test.\nThis is the most minimal patch that can be generated if you add a\ncommit to this repository, and any further tests that are related to\nit would look the same.\n\nAs Junio pointed out what is missing from the explanation is that this\nsimple name translation test is targeted toward the 'git blame'\ncommands, because such translation is not tested for them currently.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"184693","messageId":"20120214211402.GC23291@sigill.intra.peff.net","threadId":"29626","inReplyTo":"20120214203431.GB13210@burratino","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-14T21:14:02Z","receivedAt":"2012-02-14T21:14:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2012 at 02:34:31PM -0600, Jonathan Nieder wrote:\n\n> Felipe Contreras wrote:\n> \n> > I sent both the fix and the tests. Another fix was applied, but we are still\n> > missing the tests.\n> >\n> > These are good before, and after the fix.\n> \n> To summarize the previous discussion[1]: some people had comments, and\n> you seem to have found value in exactly none of them. OK. CC-ing\n> Peff, since he at least probably has looked over this code before.\n\nThe short answer is that both patches look OK to me.\n\nHere's a longer digression that you may feel free to ignore.\n\nWhen I investigated this problem, I looked only at the code (and I think\nJunio's fix is the right one). However, having just looked at your two\npatches without having previously looked at those tests, I found them\nquite painless to review.\n\nHaving participated in the bug-hunt, I knew they were probably related\nto testing that bug, but it was very unclear to me from the original\nseries why there were two patches, and not one. Or why, when we have\nmailmap tests, they did not catch this bug. And that is _now_, with the\ncontext of having just participated in the discussion; six months from\nnow I would have even less hope of understanding the context.\n\nMy general review process is (in this order):\n\n 1. Figure out why a change is needed. This should be explained in the\n commit message. And no, just adding tests is not assumed to be\n needed. Why did the old tests not cover this case? The answer can\n be a simple \"nobody bothered to write them\", and that's OK. But\n some description of the current state can help reviewers understand\n the rationale (e.g., \"we tested with shortlog, but not mailmap, and\n certain code paths are only exposed through mailmap\").\n\n 2. Figure out what the change should be doing. This should also be in\n the commit message, though at a high level. In the case of tests,\n obviously we're adding new tests. But are we properly covering all\n of the new cases? Does the \"what\" match the \"why\" from (1)?\n\n 3. Look at the patch. Do the changes match what the commit message\n claimed in (2)?\n\n 4. Look at the patch for style, implementation, etc. Is the code\n quality good enough to be included?\n\nThe steps are dependent on each other, and I usually quit if I can't get\npast one step. Sometimes I will look at the patch to try to figure out\n(1) or (2), especially if the patch is trivial. But in my ideal world,\nevery patch lets me just walk through those steps.\n\nI won't claim that these steps create error-free reviews. Based on those\nsteps, your patches look good to me, and it seems from Felipe's response\nthat there might be some inaccuracies in your description. But it at\nleast gives us a starting point for discussion.\n\n-Peff\n"},{"id":"184694","messageId":"20120214211552.GA9651@burratino","threadId":"29626","inReplyTo":"CAMP44s3YRHgMPX2Hzydm_TLB27OABWETjABMcwrHmDk-=pN2hw@mail.gmail.com","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-14T21:15:52Z","receivedAt":"2012-02-14T21:15:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n> On Tue, Feb 14, 2012 at 10:34 PM, Jonathan Nieder wrote:\n\n>> To summarize the previous discussion[1]: some people had comments, and\n>> you seem to have found value in exactly none of them.  OK.  CC-ing\n>> Peff, since he at least probably has looked over this code before.\n>\n> Just because you have comments doesn't mean I *must* address them. We\n> have a difference of opinion, nothing wrong with that.\n\nI said \"OK\", didn't I?\n\n[...]\n>> In fact it seems to be intended to test the case addressed by f026358e\n>> (name changing, email not) in various mailmap callers: \"git shortlog -e\",\n>> \"git log --pretty\", \"git blame\".\n>\n> No. As the summary says, it's intended to add a simple name\n> translation test, which is missing from all the tests that spawn from\n> the repository generated in 'Shortlog output (complex mapping)' test.\n> This is the most minimal patch that can be generated if you add a\n> commit to this repository, and any further tests that are related to\n> it would look the same.\n>\n> As Junio pointed out what is missing from the explanation is that this\n> simple name translation test is targeted toward the 'git blame'\n> commands, because such translation is not tested for them currently.\n\nUm. So this has nothing to do with f026358e at all? Mentioning that\ncommit and that this test does not pass with an older codebase is not\nuseful to the humans that will look over this test later?\n\nAdding explanation and rearranging things so people encountering this\nlater have to spend _less_ time to understand it is something I\nconsider useful. It means people are less likely to randomly break\nthings. I don't actually understand where the difference of opinion\ncomes from here.\n"},{"id":"184696","messageId":"20120214212742.GB9651@burratino","threadId":"29626","inReplyTo":"20120214211402.GC23291@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-14T21:27:42Z","receivedAt":"2012-02-14T21:27:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> The short answer is that both patches look OK to me.\n\nThanks for looking them over and for a nice explanation of the process.\n"},{"id":"184697","messageId":"CAMP44s3di25SbMa1T1=0_s6f-rKnniwEcA+o5HWT7xedcghSeg@mail.gmail.com","threadId":"29626","inReplyTo":"20120214203519.GC13210@burratino","subject":"Re: [PATCH 1/2] test: mailmap can change author name without changing email","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T21:35:33Z","receivedAt":"2012-02-14T21:35:33Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Feb 14, 2012 at 10:35 PM, Jonathan Nieder wrote:\n> From: Felipe Contreras \n\n> test: mailmap can change author name without changing email\n\nThat doesn't say anything to me, which is weird if I supposedly wrote\nthis patch. What is the *purpose* of this?\n\nAt least 'add simple name translation test' is clear about the\npurpose, albeit not clear enough as Junio pointed out.\n\n> (2) 'email@example.com'\n> becomes the canonical author email for commits with author name 'A U\n> Thor'.\n\nThat's not true. I initially thought that was the case, and I think it\nmight be useful to have that, but it's not the case now, and your\npatch doesn't test this.\n\n> We already have tests for the effect (1) in the committer name, but\n> not in the author name, so the tests do not cover the shortlog and\n> blame codepaths as they should.  Fix that.\n\nIn order to test that you would need additional changes, something\nalong the lines of:\n\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -157,8 +157,9 @@ A U Thor (1):\n CTO (1):\n seventh\n\n-Committed (1):\n+Committed (2):\n eighth\n+ nine\n\n Other Author (2):\n third\n@@ -204,6 +205,11 @@ test_expect_success 'Shortlog output (complex mapping)' '\n test_tick &&\n git commit --author \"C O Mitter \" -m eighth &&\n\n+ echo nine >>one &&\n+ git add one &&\n+ test_tick &&\n+ git commit --author \"Committed \" -m nine &&\n+\n mkdir -p internal_mailmap &&\n echo \"Committed \" > internal_mailmap/.mailmap &&\n echo \"\n\" >> internal_mailmap/.mailmap &&\n@@ -220,6 +226,9 @@ test_expect_success 'Shortlog output (complex mapping)' '\n\n # git log with --pretty format which uses the name and email mailmap\nplacemarkers\n cat >expect <<\\EOF\n+Author Committed maps to Committed\n\n+Committer C O Mitter maps to Committed\n\n+\n Author C O Mitter maps to Committed\n\n Committer C O Mitter maps to Committed\n\n\n@@ -260,6 +269,7 @@ OBJID (Santa Claus DATE 5) five\n OBJID (Santa Claus DATE 6) six\n OBJID (CTO DATE 7) seven\n OBJID (Committed DATE 8) eight\n+OBJID (Committed DATE 9) nine\n EOF\n test_expect_success 'Blame output (complex mapping)' '\n git blame one >actual &&\n\nBut that of course fails.\n\n> Signed-off-by: Felipe Contreras \n\nI most definitely did not sign this off, and I didn't add any of these\nlines, nor wrote anything about this commit message.\n\nIt might be possible to simplify my patch \"t: mailmap: add simple name\ntranslation test\" using the already existing \"Committed\n\" mapping, but that most likely is going to\nremove only one line, and would make the code less clear about what\nthat translation is trying to test.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"184698","messageId":"CAMP44s2M8Ava6xyKX32h9+pbxG+zq1wH1RkdWwfNsZMzcVQEmw@mail.gmail.com","threadId":"29626","inReplyTo":"20120214203603.GD13210@burratino","subject":"Re: [PATCH 2/2] test: check that \"git blame -e\" uses mailmap correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T21:41:07Z","receivedAt":"2012-02-14T21:41:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Feb 14, 2012 at 10:36 PM, Jonathan Nieder wrote:\n> From: Felipe Contreras \n\n> test: check that \"git blame -e\" uses mailmap correctly\n\nI wonder what extra information is in that text that is not in my\noriginal \"t: mailmap: add 'git blame -e' tests\". I guess all tests\n'check' something, and the purpose is to make sure things work\n'correctly.\n\n> Until f026358e (\"mailmap: always return a plain mail address from\n> map_user()\", 2012-02-05), git blame -e would add a spurious '>' after\n> the unchanged email address with brackets it passed to the mailmap\n> machinery, resulting in lines with a doubled '>' like this:\n>\n>  620456e6 (>   2005-04-07 15:20:13 -0700 8) eight\n>\n> Add a test to make sure it doesn't happen again.  This reuses the test\n> data for the existing \"shortlog -e\" test so it\n\n> also tests other kinds of mail mapping.\n\n'Also' is a keyword that strongly denotes this patch is doing more\nthan one logical thing. My patch adds those checks *independently* of\nthe fix on f026358e, so it's truly logically independent.\n\n> Signed-off-by: Felipe Contreras \n\nI did not sign this.\n\n-- \nFelipe Contreras\n"},{"id":"184702","messageId":"20120214215023.GC9651@burratino","threadId":"29626","inReplyTo":"CAMP44s3di25SbMa1T1=0_s6f-rKnniwEcA+o5HWT7xedcghSeg@mail.gmail.com","subject":"Re: [PATCH 1/2] test: mailmap can change author name without changing email","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-14T21:50:23Z","receivedAt":"2012-02-14T21:50:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n> On Tue, Feb 14, 2012 at 10:35 PM, Jonathan Nieder wrote:\n\n>> (2) 'email@example.com'\n>> becomes the canonical author email for commits with author name 'A U\n>> Thor'.\n>\n> That's not true. I initially thought that was the case, and I think it\n> might be useful to have that, but it's not the case now, and your\n> patch doesn't test this.\n\nThanks for explaining. I had indeed confused myself into thinking 'A\nU Thor ' would act like 'A U Thor\n '.\n\nI should have said:\n\n-- 8< --\nA mailmap entry in the format 'A U Thor ' means\nthat 'A U Thor' should be the canonical author name for commits\nwith author address 'email@example.com', and the email address\nshould be left alone.\n\nWe already have tests for this format regarding the committer name,\nbut not in the author name, so the tests do not cover the shortlog and\nblame codepaths as they should. Fix that.\n-- >8 --\n\n[...]\n>> Signed-off-by: Felipe Contreras \n>\n> I most definitely did not sign this off, and I didn't add any of these\n> lines, nor wrote anything about this commit message.\n\nThat's why I described the changes I made, signed with my initials,\nand put my own sign-off below yours. Did I screw up somewhere?\n\nNote that I am making these changes because, at its heart, I think\nyour patch is good and useful. Otherwise I would have ignored it and\nworked on something else. If you prefer that I don't make\nimprovements like this, please indicate why that's a good idea;\notherwise I will probably continue to do it when I see good patches,\ndespite all the signals you are giving that I have done something\nawful by corrupting your perfect patch in this way.\n\nJonathan\n"},{"id":"184703","messageId":"CAMP44s0Dp9Av+ikFHa=QcqKFA5XL9ESBrzWLY0jkSCdH-NxhMw@mail.gmail.com","threadId":"29626","inReplyTo":"20120214211402.GC23291@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T21:52:11Z","receivedAt":"2012-02-14T21:52:11Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Feb 14, 2012 at 11:14 PM, Jeff King wrote:\n> My general review process is (in this order):\n>\n>  1. Figure out why a change is needed. This should be explained in the\n>     commit message. And no, just adding tests is not assumed to be\n>     needed.  Why did the old tests not cover this case?\n\nAs I already mentioned more than once; the first patch is not related\nto any fix.\n\n> The answer can\n>     be a simple \"nobody bothered to write them\", and that's OK.\n\n That can be derived from the word \"add\". You can't add something that\nis already there.\n\n> But\n>     some description of the current state can help reviewers understand\n>     the rationale (e.g., \"we tested with shortlog, but not mailmap, and\n>     certain code paths are only exposed through mailmap\").\n\nYou are assuming too much. That's not the purpose, that's why I didn't menti\n\n>  2. Figure out what the change should be doing.\n\nt: mailmap: add 'git blame -e' tests\n\nThat's what the change should be doing; nothing more, nothing less.\n\nI wonder why you have to assume the worst. When I see a commit message\nlike that, I assume that there were no previous tests for that (thus\nthe word 'add'), and that's all I need to know.\n\nIf you want to extrude meaning from where there isn't, well, go ahead,\nbut there's nothing else really.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"184705","messageId":"20120214215930.GD9651@burratino","threadId":"29626","inReplyTo":"CAMP44s2M8Ava6xyKX32h9+pbxG+zq1wH1RkdWwfNsZMzcVQEmw@mail.gmail.com","subject":"Re: [PATCH 2/2] test: check that \"git blame -e\" uses mailmap correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-14T21:59:30Z","receivedAt":"2012-02-14T21:59:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n> On Tue, Feb 14, 2012 at 10:36 PM, Jonathan Nieder wrote:\n\n>> Signed-off-by: Felipe Contreras \n>\n> I did not sign this.\n\nDo you mean you do not agree with the following?\n\n\tDeveloper's Certificate of Origin 1.1\n\n\tBy making a contribution to this project, I certify that:\n\n\t(a) The contribution was created in whole or in part by me and I\n\t have the right to submit it under the open source license\n\t indicated in the file; or\n\n\t(b) The contribution is based upon previous work that, to the best\n\t of my knowledge, is covered under an appropriate open source\n\t license and I have the right under that license to submit that\n\t work with modifications, whether created in whole or in part\n\t by me, under the same open source license (unless I am\n\t permitted to submit under a different license), as indicated\n\t in the file; or\n\n\t(c) The contribution was provided directly to me by some other\n\t person who certified (a), (b) or (c) and I have not modified\n\t it.\n\n\t(d) I understand and agree that this project and the contribution\n\t are public and that a record of the contribution (including all\n\t personal information I submit with it, including my sign-off) is\n\t maintained indefinitely and may be redistributed consistent with\n\t this project or the open source license(s) involved.\n\nAnyway, Junio, please feel free to remove Felipe's sign-off from these\npatches, or add an \"Against-the-protest-of:\" or whatever if that's\nwhat he wants.\n\nI've reached the point of diminishing returns. I'll be happy to help\nin any way I can, given a polite explanation of how I can help.\n"},{"id":"184708","messageId":"20120214220732.GB24802@sigill.intra.peff.net","threadId":"29626","inReplyTo":"CAMP44s0Dp9Av+ikFHa=QcqKFA5XL9ESBrzWLY0jkSCdH-NxhMw@mail.gmail.com","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-14T22:07:32Z","receivedAt":"2012-02-14T22:07:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2012 at 11:52:11PM +0200, Felipe Contreras wrote:\n\n> On Tue, Feb 14, 2012 at 11:14 PM, Jeff King wrote:\n> > My general review process is (in this order):\n> >\n> >  1. Figure out why a change is needed. This should be explained in the\n> >     commit message. And no, just adding tests is not assumed to be\n> >     needed.  Why did the old tests not cover this case?\n> \n> As I already mentioned more than once; the first patch is not related\n> to any fix.\n\nReally? I didn't see it mentioned in your commit message, which\nconsisted entirely of:\n\n t: mailmap: add 'git blame -e' tests\n\nYes, I know you mentioned it elsewhere in the thread. But review should\nhappen on what is actually in the posted patch. That is what reviewers\nare guaranteed to have read, and it is what people reading \"git log\" in\na year will see. If there was other discussion or context that led to\nthe patch, it's helpful in both cases to summarize it.\n\n> > The answer can\n> >     be a simple \"nobody bothered to write them\", and that's OK.\n> \n> That can be derived from the word \"add\". You can't add something that\n> is already there.\n\nAre there already git-blame tests, but not \"blame -e\" tests? If there\nare already \"blame\" tests, why do we additionally need \"blame -e\" tests?\n\nI can guess, or I can do my own digging in the history to find out, but\nthat makes review a lot more painful. You already did the digging and\ncame to understand the problem when you made your patch. Why not just\nshare it?\n\n> > But\n> >     some description of the current state can help reviewers understand\n> >     the rationale (e.g., \"we tested with shortlog, but not mailmap, and\n> >     certain code paths are only exposed through mailmap\").\n> \n> You are assuming too much. That's not the purpose, that's why I didn't menti\n\nSorry, that should have been s/mailmap/blame/ above. But if I am making\nwrong assumptions about the rationale, then isn't that a sign that the\ncommit message is insufficient?\n\n> >  2. Figure out what the change should be doing.\n> \n> t: mailmap: add 'git blame -e' tests\n> \n> That's what the change should be doing; nothing more, nothing less.\n\nYes, I think you did describe the \"what\", which in this case is very\nsimple. But as I mentioned before, it is not just knowing the \"what\" but\nevaluating that the \"what\" meets the \"why\" from step 1.\n\n> I wonder why you have to assume the worst. When I see a commit message\n> like that, I assume that there were no previous tests for that (thus\n> the word 'add'), and that's all I need to know.\n\nI don't necessarily assume the worst. If I were the maintainer, I might\neven have taken your patch as-is, as it's pretty simple. But I found a\ndescription like the one Jonathan included to be _much_ easier to\nreview. Whether yours was above a minimum threshold or not, I think it's\nworth striving to be better.\n\n-Peff\n"},{"id":"184709","messageId":"CAMP44s1mV2cE=R49qYSLd8eZPhCpRx0hRnnG_-K3iBxp_YQEpQ@mail.gmail.com","threadId":"29626","inReplyTo":"20120214211552.GA9651@burratino","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T22:09:23Z","receivedAt":"2012-02-14T22:09:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Feb 14, 2012 at 11:15 PM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>> On Tue, Feb 14, 2012 at 10:34 PM, Jonathan Nieder wrote:\n>\n>>> To summarize the previous discussion[1]: some people had comments, and\n>>> you seem to have found value in exactly none of them.  OK.  CC-ing\n>>> Peff, since he at least probably has looked over this code before.\n>>\n>> Just because you have comments doesn't mean I *must* address them. We\n>> have a difference of opinion, nothing wrong with that.\n>\n> I said \"OK\", didn't I?\n\nYou also said I didn't find value in any of the comments which is\nbeing passively aggressive. Comments always have value, that doesn't\nmeant they must all be turned into actionables.\n\n> [...]\n>>> In fact it seems to be intended to test the case addressed by f026358e\n>>> (name changing, email not) in various mailmap callers: \"git shortlog -e\",\n>>> \"git log --pretty\", \"git blame\".\n>>\n>> No. As the summary says, it's intended to add a simple name\n>> translation test, which is missing from all the tests that spawn from\n>> the repository generated in 'Shortlog output (complex mapping)' test.\n>> This is the most minimal patch that can be generated if you add a\n>> commit to this repository, and any further tests that are related to\n>> it would look the same.\n>>\n>> As Junio pointed out what is missing from the explanation is that this\n>> simple name translation test is targeted toward the 'git blame'\n>> commands, because such translation is not tested for them currently.\n>\n> Um.  So this has nothing to do with f026358e at all?  Mentioning that\n> commit and that this test does not pass with an older codebase is not\n> useful to the humans that will look over this test later?\n\nI didn't say that. I say the *purpose* of the patch is different.\n\n> Adding explanation and rearranging things so people encountering this\n> later have to spend _less_ time to understand it is something I\n> consider useful.  It means people are less likely to randomly break\n> things.  I don't actually understand where the difference of opinion\n> comes from here.\n\nThe difference of opinion is that I consider the patch already good\nenough (adding the comments from Junio). Yeah, now that f026358e is\ncommitted might be worth mentioning, and what's the relationship, but\nthat's an *extra*. Even if f026358e wasn't there, and nobody knew what\nwas the issue, and possible fixes, the patch would be good by itself.\n\nAgain, some tests > no tests, and don't let the perfect be the enemy\nof the good. It even looks like you prefer no changes at all (status\nquo) to my proposed changes as you have never said these are \"good\",\nbut always paint them as \"wrong\" or \"broken\" in fundamental ways, as\nif they are not worthy of being applied.\n\nIn any case, I will address the comments from Junio, and perhaps add a\nnote regarding f026358e.\n\nI also can't help but feel you are applying double standards, as this\nis the original patch that seems to have broken things:\n\nommit d20d654fe8923a502527547b17fe284d15d6aec9\nAuthor: Marius Storm-Olsen \nDate: Sun Feb 8 15:34:30 2009 +0100\n\n Change current mailmap usage to do matching on both name and email\nof author/committer.\n\n Signed-off-by: Marius Storm-Olsen \n Signed-off-by: Junio C Hamano \n\n Documentation/pretty-formats.txt | 2 +\n builtin-blame.c | 50 +++++++++++-------\n builtin-shortlog.c | 22 ++++++--\n pretty.c | 57 +++++++++++----------\n t/t4203-mailmap.sh | 106 ++++++++++++++++++++++++++++++++++++++\n 5 files changed, 186 insertions(+), 51 deletions(-)\n\nNotice the short commit message. It would have been _nice_ to have a\nbigger commit message, but it's better to have this commit than\nnothing at all.\n\nAnd see the relevant commit f026358:\n\nf026358 mailmap: always return a plain mail address from map_user()\n mailmap.c | 18 ++++++++++--------\n 1 files changed, 10 insertions(+), 8 deletions(-)\n\nNotice how there's no tests, which would have reason enough to reject\nthe patches from many contributors.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"184712","messageId":"7v8vk55a99.fsf@alter.siamese.dyndns.org","threadId":"29626","inReplyTo":"CAMP44s0Dp9Av+ikFHa=QcqKFA5XL9ESBrzWLY0jkSCdH-NxhMw@mail.gmail.com","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T22:18:58Z","receivedAt":"2012-02-14T22:18:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Tue, Feb 14, 2012 at 11:14 PM, Jeff King wrote:\n> ...\n>> The answer can\n>> be a simple \"nobody bothered to write them\", and that's OK.\n>\n> That can be derived from the word \"add\". You can't add something that\n> is already there.\n\nYou are correct to say that you cannot add something that is already\nthere, but that does not mean you explained why that new thing is a good\nthing to add. In other words, you can add a new thing that we did not\nhave, but it would not result in a good addition if that new thing is\nirrelevant. Relevance needs to be explained.\n\nI do think in this particular case, the new check *is* relevant, because\n\n Although we did have \"blame\" test to see how the name part is shown,\n we had no \"blame -e\" test to see how the email part is shown.\n\nI do not understand why you are resisting to explain how your addition\nadds value to the system with such a simple two-liner, and instead are\nendlessly arguing. Is it to make sure you are the one to utter the last\nword in the thread?\n\nAs I am sort of getting tired of seeing you making things more difficult\nfor yourself, I'll refrain from commenting on this topic at least for a\nfew days to wait until things cool down.\n"},{"id":"184714","messageId":"20120214222128.GA24544@burratino","threadId":"29626","inReplyTo":"CAMP44s1mV2cE=R49qYSLd8eZPhCpRx0hRnnG_-K3iBxp_YQEpQ@mail.gmail.com","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-14T22:21:28Z","receivedAt":"2012-02-14T22:21:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> The difference of opinion is that I consider the patch already good\n> enough (adding the comments from Junio).\n\nOk, I can understand where you're coming from there.\n\nNow I have offered some suggestions for improving some of your\npatches. Applying these suggestions would be effortless, since I sent\nthem in patch form. What is your response? You send a point-by-point\nrebuttal explaining how each detail of my suggestions is bad, bad,\nbad, and then resend the original with comparatively small changes.\n\nCan you see how this might indicate a stronger difference of opinion\nthan you have mentioned? And how a reviewer might make the mistake of\nthinking his comments were unwelcome after such an experience?\n"},{"id":"184715","messageId":"CAMP44s2v25w=14t8ZK82n-_01UCYWuUr2kAb3jEf2YfRiu2uyA@mail.gmail.com","threadId":"29626","inReplyTo":"20120214220732.GB24802@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T22:22:53Z","receivedAt":"2012-02-14T22:22:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Feb 15, 2012 at 12:07 AM, Jeff King wrote:\n> On Tue, Feb 14, 2012 at 11:52:11PM +0200, Felipe Contreras wrote:\n>\n>> On Tue, Feb 14, 2012 at 11:14 PM, Jeff King wrote:\n>> > My general review process is (in this order):\n>> >\n>> >  1. Figure out why a change is needed. This should be explained in the\n>> >     commit message. And no, just adding tests is not assumed to be\n>> >     needed.  Why did the old tests not cover this case?\n>>\n>> As I already mentioned more than once; the first patch is not related\n>> to any fix.\n>\n> Really? I didn't see it mentioned in your commit message, which\n> consisted entirely of:\n>\n>  t: mailmap: add 'git blame -e' tests\n>\n> Yes, I know you mentioned it elsewhere in the thread. But review should\n> happen on what is actually in the posted patch. That is what reviewers\n> are guaranteed to have read, and it is what people reading \"git log\" in\n> a year will see. If there was other discussion or context that led to\n> the patch, it's helpful in both cases to summarize it.\n\nSeriously? You want to add a note saying \"These tests don't tackle any\nknown issues\". Well, if it did, logically, it would have been\nmentioned.\n\n>> >     The answer can\n>> >     be a simple \"nobody bothered to write them\", and that's OK.\n>>\n>>  That can be derived from the word \"add\". You can't add something that\n>> is already there.\n>\n> Are there already git-blame tests, but not \"blame -e\" tests? If there\n> are already \"blame\" tests, why do we additionally need \"blame -e\" tests?\n\nYou are right, it's impossible to decipher that the output of 'blame\n-e' is different than 'blame'. Surely, somebody must have made a\nmistake when the patch was applied.\n\nOr we can apply common sense.\n\n> I can guess, or I can do my own digging in the history to find out, but\n> that makes review a lot more painful. You already did the digging and\n> came to understand the problem when you made your patch. Why not just\n> share it?\n\nBecause there's no need.\n\nWhy would anybody add tests that are already there? Why do you have to\nassume the worst?\n\n>> >     But\n>> >     some description of the current state can help reviewers understand\n>> >     the rationale (e.g., \"we tested with shortlog, but not mailmap, and\n>> >     certain code paths are only exposed through mailmap\").\n>>\n>> You are assuming too much. That's not the purpose, that's why I didn't menti\n>\n> Sorry, that should have been s/mailmap/blame/ above. But if I am making\n> wrong assumptions about the rationale, then isn't that a sign that the\n> commit message is insufficient?\n\nIf you mean the second patch, this was already pointed out by Junio,\nand I already said I would clarify that these are meant to target 'git\nblame' output.\n\n>> >  2. Figure out what the change should be doing.\n>>\n>> t: mailmap: add 'git blame -e' tests\n>>\n>> That's what the change should be doing; nothing more, nothing less.\n>\n> Yes, I think you did describe the \"what\", which in this case is very\n> simple. But as I mentioned before, it is not just knowing the \"what\" but\n> evaluating that the \"what\" meets the \"why\" from step 1.\n\nThe why can be derived. Unless you seriously think Junio would allow\npatches that say they \"add\" tests for something (which imply there\nisn't tests for that), where they don't.\n\n>> I wonder why you have to assume the worst. When I see a commit message\n>> like that, I assume that there were no previous tests for that (thus\n>> the word 'add'), and that's all I need to know.\n>\n> I don't necessarily assume the worst. If I were the maintainer, I might\n> even have taken your patch as-is, as it's pretty simple. But I found a\n> description like the one Jonathan included to be _much_ easier to\n> review. Whether yours was above a minimum threshold or not, I think it's\n> worth striving to be better.\n\nBetter != more words. English, like code, is good as simple as\npossible (but not simpler).\n\nIf you start from the premise that the patch is good; it has been\ns-o-b by Junio, and it has been applied, and released, what more do\nyou need from the summary \"t: mailmap: add 'git blame -e' tests\". Can\nyou derive the rest?\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"184718","messageId":"CAMP44s0cJroUM2aahRQz2dVPe57XPPOnsxWy+5DfmjXGArz4wA@mail.gmail.com","threadId":"29626","inReplyTo":"7v8vk55a99.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T22:34:21Z","receivedAt":"2012-02-14T22:34:21Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Feb 15, 2012 at 12:18 AM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> On Tue, Feb 14, 2012 at 11:14 PM, Jeff King wrote:\n>> ...\n>>>     The answer can\n>>>     be a simple \"nobody bothered to write them\", and that's OK.\n>>\n>>  That can be derived from the word \"add\". You can't add something that\n>> is already there.\n>\n> You are correct to say that you cannot add something that is already\n> there, but that does not mean you explained why that new thing is a good\n> thing to add.  In other words, you can add a new thing that we did not\n> have, but it would not result in a good addition if that new thing is\n> irrelevant. Relevance needs to be explained.\n>\n> I do think in this particular case, the new check *is* relevant, because\n>\n>    Although we did have \"blame\" test to see how the name part is shown,\n>    we had no \"blame -e\" test to see how the email part is shown.\n>\n> I do not understand why you are resisting to explain how your addition\n> adds value to the system with such a simple two-liner, and instead are\n> endlessly arguing.  Is it to make sure you are the one to utter the last\n> word in the thread?\n\nAnd I don't understand why people want the obvious to be explained.\nFor each one of the questions I've heard, the answer can be derived\nfrom the summary *directly*.\n\nYour new point is \"you can add a new thing that we did not have, but\nit would not result in a good addition if that new thing is\nirrelevant\", but you already know what is the new thing from the\nsummary \"'git blame -e' tests\".\n\nEverybody seems to assume that a simple commit message = bad. I disagree.\n\n> As I am sort of getting tired of seeing you making things more difficult\n> for yourself,\n\nThe same argument can be applied both ways; you are the one that is\nmaking things more difficult. And I already pointed out the double\nstandards.\n\n> I'll refrain from commenting on this topic at least for a\n> few days to wait until things cool down.\n\nDoes this mean that disagreement is discouraged? From the discussion\nit seems we are in clear disagreement, but from the rhetoric it seems\nto me that you all assume you are right by default, and that simple\ncommit messages are by bad definition.\n\nIf that's the case I am disappointed, but I would rather just drop the\ndiscussion rather than defend my position.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"184719","messageId":"20120214223515.GA25374@sigill.intra.peff.net","threadId":"29626","inReplyTo":"CAMP44s2v25w=14t8ZK82n-_01UCYWuUr2kAb3jEf2YfRiu2uyA@mail.gmail.com","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-14T22:35:15Z","receivedAt":"2012-02-14T22:35:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 15, 2012 at 12:22:53AM +0200, Felipe Contreras wrote:\n\n> Better != more words. English, like code, is good as simple as\n> possible (but not simpler).\n\nI'm not going to bother responding to your email point by point, as I\nhave very little else to say.\n\nI am telling you that as a reviewer of git patches, I found the more\nverbose message by Jonathan to be much easier to review than your\none-line description. You may disagree. I offer my experience merely as\na data point, and I will let others draw their own conclusions. If you\nwant to ignore me, that's your right, but I don't see any point in\narguing further about it.\n\n-Peff\n"},{"id":"184721","messageId":"CAMP44s1adDHvDVCBpPGUb5YpwyLkqB8LcqjAou4b2EzJL7kmYQ@mail.gmail.com","threadId":"29626","inReplyTo":"20120214222128.GA24544@burratino","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T22:36:44Z","receivedAt":"2012-02-14T22:36:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Feb 15, 2012 at 12:21 AM, Jonathan Nieder wrote:\n> Now I have offered some suggestions for improving some of your\n> patches.  Applying these suggestions would be effortless, since I sent\n> them in patch form.  What is your response?  You send a point-by-point\n> rebuttal explaining how each detail of my suggestions is bad, bad,\n> bad, and then resend the original with comparatively small changes.\n\nThat's a difference of opinion. And I didn't say the where 'bad'; I\njust didn't agree with them.\n\n> Can you see how this might indicate a stronger difference of opinion\n> than you have mentioned?  And how a reviewer might make the mistake of\n> thinking his comments were unwelcome after such an experience?\n\nYour comments are welcome. My comments back are not?\n\n-- \nFelipe Contreras\n"},{"id":"184723","messageId":"CAMP44s1HitxHkfre_1BPOpt2oVo_rwMnbcer2zxFuA6KJXnj_g@mail.gmail.com","threadId":"29626","inReplyTo":"20120214215023.GC9651@burratino","subject":"Re: [PATCH 1/2] test: mailmap can change author name without changing email","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T22:48:10Z","receivedAt":"2012-02-14T22:48:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Feb 14, 2012 at 11:50 PM, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n>> On Tue, Feb 14, 2012 at 10:35 PM, Jonathan Nieder wrote:\n>\n>>> (2) 'email@example.com'\n>>> becomes the canonical author email for commits with author name 'A U\n>>> Thor'.\n>>\n>> That's not true. I initially thought that was the case, and I think it\n>> might be useful to have that, but it's not the case now, and your\n>> patch doesn't test this.\n>\n> Thanks for explaining.  I had indeed confused myself into thinking 'A\n> U Thor ' would act like 'A U Thor\n> '.\n>\n> I should have said:\n>\n> -- 8< --\n> A mailmap entry in the format 'A U Thor ' means\n> that 'A U Thor' should be the canonical author name for commits\n> with author address 'email@example.com', and the email address\n> should be left alone.\n>\n> We already have tests for this format regarding the committer name,\n> but not in the author name, so the tests do not cover the shortlog and\n> blame codepaths as they should.  Fix that.\n> -- >8 --\n\nAt which point the summary doesn't seem to be correct \"test: mailmap\ncan change author name without changing email\". Plus, I fail to see\nwhat would be the usefulness of this test, as changing name is\nchanging the name, regardless if it's the author or the committer.\n\n> [...]\n>>> Signed-off-by: Felipe Contreras \n>>\n>> I most definitely did not sign this off, and I didn't add any of these\n>> lines, nor wrote anything about this commit message.\n>\n> That's why I described the changes I made, signed with my initials,\n> and put my own sign-off below yours.  Did I screw up somewhere?\n\nYou more or less rewrote the whole thing, I don't think you should put\nmy s-o-b in those cases.\n\nAnyway, I have seen people use this format:\n\nSigned-off-by: Felipe Contreras \n[jn: rewrite the patch]\nSigned-off-by: Jonathan Nieder \n\nEven better:\n\nBased on a patch by Felipe Contreras.\n\nSigned-off-by: Jonathan Nieder \n\n> Note that I am making these changes because, at its heart, I think\n> your patch is good and useful.  Otherwise I would have ignored it and\n> worked on something else.  If you prefer that I don't make\n> improvements like this, please indicate why that's a good idea;\n> otherwise I will probably continue to do it when I see good patches,\n> despite all the signals you are giving that I have done something\n> awful by corrupting your perfect patch in this way.\n\nI didn't hint I preferred that. You said you wanted to improve my\npatches, why can't I do the same?\n\n-- \nFelipe Contreras\n"},{"id":"184724","messageId":"7vzkcl3u9o.fsf@alter.siamese.dyndns.org","threadId":"29626","inReplyTo":"CAMP44s0cJroUM2aahRQz2dVPe57XPPOnsxWy+5DfmjXGArz4wA@mail.gmail.com","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-14T22:49:39Z","receivedAt":"2012-02-14T22:49:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Wed, Feb 15, 2012 at 12:18 AM, Junio C Hamano wrote:\n>\n> And I don't understand why people want the obvious to be explained.\n\nHas it ever occurred to you the reason why people ask questions to you is\nperhaps because something that is obvious to you who wrote the patch is\nnot obvious at all to others? Has it also occurred to you that the\nmajority of people who need to understand the patch during the review and\n6 months down the road in \"git log\" output are not *you*?\n\n> Your new point is \"you can add a new thing that we did not have, but\n> it would not result in a good addition if that new thing is\n> irrelevant\", but you already know what is the new thing from the\n> summary \"'git blame -e' tests\".\n\nIt is not a \"new point\". Jonathan, Peff and I all never said that it is\nunclear \"what\" your patch adds. The suggestions for improvement given in\nthis thread were all to explain \"why\" better.\n\n> Everybody seems to assume that a simple commit message = bad. I disagree.\n\nIf you find *everybody* seems to disagree with you, it would help to\nconsider a slight possibility that you *might* be wrong. And \"simple\" is\nnot necessarily \"sufficient and simple\".\n\n> ... And I already pointed out the double standards.\n\nSorry, but the absolute uniform standards do not exist, unless you are\nliving in a fantasy land. I expect better from list regulars as new\ncontributors will inevitably learn from their behaviour (we also learn\nfrom our past mistakes).\n"},{"id":"184726","messageId":"CAMP44s0TmuxfLD=-hs8jf_AfPiet+rVy7h_5x_dtp_Nwjf+i+A@mail.gmail.com","threadId":"29626","inReplyTo":"20120214215930.GD9651@burratino","subject":"Re: [PATCH 2/2] test: check that \"git blame -e\" uses mailmap correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T22:56:26Z","receivedAt":"2012-02-14T22:56:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Feb 14, 2012 at 11:59 PM, Jonathan Nieder wrote:\n> Do you mean you do not agree with the following?\n\n>        (a) The contribution was created in whole or in part by me and I\n>            have the right to submit it under the open source license\n>            indicated in the file; or\n\nAnybody can come up with that patch from scratch, all you have to do\nis copy the previous test, change 'git blame' to use 'git blame -e',\nand fix the expect file. And the subtle diff from my own patch changes\nthe whole meaning of it, so I wouldn't say it came from me.\n\nBut I don't care much, do whatever is more convenient for you, but if\npossible, I would remove my s-o-b from it as I have not agreed on it.\nAnd as I said; this is two patches in one.\n\n-- \nFelipe Contreras\n"},{"id":"184729","messageId":"CAMP44s2EQX604OQjpBDyHZ2+FMYC8=V=8VBv8wBuviRxpimNXA@mail.gmail.com","threadId":"29626","inReplyTo":"7vzkcl3u9o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 0/2] test: tests for the \"double > from mailmap\" bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-14T23:14:57Z","receivedAt":"2012-02-14T23:14:57Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Feb 15, 2012 at 12:49 AM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> On Wed, Feb 15, 2012 at 12:18 AM, Junio C Hamano wrote:\n>>\n>> And I don't understand why people want the obvious to be explained.\n>\n> Has it ever occurred to you the reason why people ask questions to you is\n> perhaps because something that is obvious to you who wrote the patch is\n> not obvious at all to others?  Has it also occurred to you that the\n> majority of people who need to understand the patch during the review and\n> 6 months down the road in \"git log\" output are not *you*?\n\nYes, that's why I am still listening. However I have not yet found a\nquestion that cannot be answered from the simple description, except\nthe one you brought and I agreed to tackle.\n\nI would like to think I have the capacity of empathy, so I would be\nable to see if something cannot be inferred from the commit message.\nOf course, I might be wrong, but so far the feedback has not been \"no,\nit's not obvious\", but rather \" yes, it's obvious... but...\".\n\n>> Your new point is \"you can add a new thing that we did not have, but\n>> it would not result in a good addition if that new thing is\n>> irrelevant\", but you already know what is the new thing from the\n>> summary \"'git blame -e' tests\".\n>\n> It is not a \"new point\".  Jonathan, Peff and I all never said that it is\n> unclear \"what\" your patch adds.  The suggestions for improvement given in\n> this thread were all to explain \"why\" better.\n\nI have heard both. And the \"why\" can be easily inferred, at least on\nthe first patch. The second one I yet have to fix, as I already\nreplied to you.\n\n>> Everybody seems to assume that a simple commit message = bad. I disagree.\n>\n> If you find *everybody* seems to disagree with you, it would help to\n> consider a slight possibility that you *might* be wrong.  And \"simple\" is\n> not necessarily \"sufficient and simple\".\n\nOf course, it's *possible*, but ad populum is not a valid argument.\n\nThere's many projects out there, and very few have as verbose commit\nmessages as git. I do not say they are doing it better, as many times\na lot of the verbose commit messages do help, but I don't think this\nis the case.\n\nLooking at the latest commits in the Linux kernel show good examples\nof simple commit messages. Albeit they might be a bit \"too simple\", my\npoint that they do have a different view on what is \"sufficient and\nsimple\":\n\nhttp://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=27c3afe6e1cf129faac90405121203962da08ff4\nhttp://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=0d86f65ed0b727daa06d3aa176314cd175323db6\nhttp://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=10f296cbfe3b93188c41463fd7a53808ebdbcbe3\n\nOf course, what is \"sufficient and simple\" depends on a case by case\nbasis, but I wonder, if there's any case in which a single line in the\ncommit message summary would suffice, wouldn't adding missing\ninconspicuous tests for something be it?\n\n>> ... And I already pointed out the double standards.\n>\n> Sorry, but the absolute uniform standards do not exist, unless you are\n> living in a fantasy land.  I expect better from list regulars as new\n> contributors will inevitably learn from their behaviour (we also learn\n> from our past mistakes).\n\nOf course, but I can't help but wonder... Why so much fuss for simple\ntests? And why you didn't bother to add tests for your new code as\nwell?\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}