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
But why is this test doing something expected is not a question
anybody would benefit from asking.
> 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'
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_
> 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
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
> # 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.
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