On Sun, Feb 5, 2012 at 10:38 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Felipe Contreras <felipe.contreras@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html