From: Felipe Contreras Date: Sun, 05 Feb 2012 21:07:10 GMT Subject: Re: [PATCH 2/3] t: mailmap: add 'git blame -e' tests Message-ID: In-Reply-To: <7vvcnlggno.fsf@alter.siamese.dyndns.org> On Sun, Feb 5, 2012 at 10:38 PM, Junio C Hamano wrote: > Felipe Contreras 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