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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Apr 11, 2013 at 11:18 AM, Jeff King <peff@xxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]