On Thu, Apr 11, 2013 at 11:18 AM, Jeff King <p...@peff.net> wrote: > On Thu, Apr 11, 2013 at 08:22:26AM -0500, Felipe Contreras wrote: > >> > We >> > currently do so robustly when the helper uses the "done" >> > feature (and that is what we test). We cannot do so >> > reliably when the helper does not use the "done" feature, >> > but it is not even worth testing; the right solution is for >> > the helper to start using "done". >> >> This doesn't help anyone, and it's not even accurate. I think it might >> be possible enforce remote-helpers to implement the "done" feature, >> and we might want to do that later. But of course, discussing what bad >> things remote-helpers could do, and how we should test and babysit >> them is not relevant here. >> >> If it was important to explain the subtleties and reasoning behind >> this change, it should be a separate patch. > > I am OK with adding the test for import as a separate patch. What I am > not OK with (and this goes for the rest of the commit message, too) is > failing to explain any back-story at all for why the change is done in > the way it is. > > _You_ may understand it _right now_, but that is not the primary > audience of the message. The primary audience is somebody else a year > from now who is wondering why this patch was done the way it was.
Who would be this person? Somebody who wonders why this test is using "feature done"? I doubt such a person would exist, as using this feature is standard, as can be seen below this chunk. *If* the test was *not* using this "feature done", *then* sure, an explanation would be needed. But why is this test doing something expected is not a question anybody would benefit from asking. > When > they are trying to find out why git does not detect errors in a helper, > and they notice that our test for failure only check the "done" case, > isn't it more helpful to say "we considered the other case, but it was > not worth fixing" rather than leaving them to guess? If you are worried about such hypothetical people, they would be better served by a comment in the source code of the test, or even better, the c file, or even better, to document that remote helpers should use this feature. But wait: --- Just like 'push', a batch sequence of one or more 'import' is terminated with a blank line. For each batch of 'import', the remote helper should produce a fast-import stream terminated by a 'done' command. --- So it's already explained, if somebody fails to follow this documentation, it's dubious a commit message that introduces a test would help. Surely, the writer of this bad remote helper would _never_ look there. > I may be more verbose than necessary in some of my commit messages, but > I would much rather err on the side of explaining too much than too > little. I wouldn't. The only thing an overload of information achieves is that the reader would simply skip or skim it. >> > export) >> > + if test -n "$GIT_REMOTE_TESTGIT_FAILURE" >> > + then >> > + # consume input so fast-export doesn't get SIGPIPE; >> >> I think this is explanation enough. >> >> > + # git would also notice that case, but we want >> > + # to make sure we are exercising the later >> > + # error checks >> >> I don't understand what is being said here. What is "that case"? > > The case that fast-export gets SIGPIPE. If we are trying to avoid SIGPIPE wouldn't that imply that git notices the SIGPIPE? > # consume input so fast-export doesn't get SIGPIPE; > # we do not technically need to do so in order for > # git to notice the failure to export, as it will > # detect problems either with fast-export or with > # the helper failing to report ref status. But since > # we are trying to demonstrate that the latter > # check works, we must avoid the SIGPIPE, which would > # trigger the former. # consume input so fast-export doesn't get SIGPIPE; we want to test the remote-helper's code after fast-export. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
