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

On Thu, Apr 11, 2013 at 11:18 AM, Jeff King [off-list ref] wrote:

On Thu, Apr 11, 2013 at 08:22:26AM -0500, Felipe Contreras wrote:

quoted

quoted

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.

quoted

quoted

        export)
+               if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+               then
+                       # consume input so fast-export doesn't get SIGPIPE;

I think this is explanation enough.

quoted

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