Re: [PATCH 1/2] transport-helper: report errors properly

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Apr 11, 2013, 16:49 UTC
Message-ID: <CAMP44s2-4i_tSzz8Y88_YnK5d1AjNoTqOa7eXZ0W5Vzk9Uosng@mail.gmail.com>
In-Reply-To: <20130411161845.GA665@sigill.intra.peff.net>

On Thu, Apr 11, 2013 at 11:18 AM, Jeff King <peff@peff.net> wrote:

Show 26 quoted lines

> 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.

Show 5 quoted lines

> 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.

Show 14 quoted lines

>> >         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?

Show 8 quoted lines

>   # 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