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

From: Jeff King <peff@peff.net>
Date: Nov 10, 2020, 00:03 UTC
Message-ID: <20201110000322.GA687107@coredump.intra.peff.net>
In-Reply-To: <xmqqk0uu9kuf.fsf@gitster.c.googlers.com>

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

Show 10 quoted lines

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

Show 7 quoted lines

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