Re: [PATCH v2] t5411: consistent result for proc-receive broken test

[Jeff King <peff@peff.net>]

On Mon, Nov 09, 2020 at 03:22:32PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I.e., I think the bug is in receive-pack's run_proc_receive_hook().
> 
> I thought that this was merely to work it around and give the
> particular test a reliable failure code, though.  Either way,
> receive-pack will get a failure when the hook fails (either the
> failure message from the hook, or unexpected EOF), so no matter what
> this patch does around this area, it would not affect interactions
> with real-life hooks, I would think.

Yes. Either way receive-pack will fail and won't update any refs, which
is good. The question is whether or not it gives a useful message back
to the client or not. So the tests are exposing an interesting
user-visible behavior, albeit one which we wouldn't expect to come up
much in practice.

> > I think the patch really ought to be in receive-pack, converting
> > packet_write_fmt() and packet_flush() into their "gently" forms.
> 
> That would be a real fix, not "work around flakey tests".  Yes, the
> hook driver roughly mimics run_and_feed_hook() and I suspect it
> started from copy-and-paste from that function, so we might need to
> review that original copy, too.

Good idea. I think it is fine, though. It does all of its writing
through write_in_full(), and breaks out of the write loop on error.
Whereas run_proc_receive_hook() switched to using the packet_*
functions, which by default die on write errors. So I think that's the
root of the problem.

I found it a little curious that run_and_feed_hook() did not complain on
write error, but I think it is explicitly allowing a hook to "exit 0"
immediately without even reading the input. As long as the hook exits
non-zero, we'd still consider that an error (and because we handled the
write error gracefully, we'd do so reliably).  It would be weird if we
saw some _other_ error (EIO or something) while writing, though. I
suspect that's all but impossible for a pipe write like this, but it
wouldn't be wrong to tighten it up, I suppose.

And back to run_proc_receive_hook(), I think it _should_ complain about
a write error, rather than allowing the hook to quietly say "exit 0".
The protocol for the proc-receive hook is more complicated, and we are
looking for an exchange of pkt-lines, and not just "we dumped some data
and it gave us an exit code". So I think it failing to read should
probably be considered a break of that pkt-line protocol.

-Peff