On Mon, Nov 09, 2020 at 03:22:32PM -0800, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> 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
