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.
> It
> cleverly ignores SIGPIPE exactly to avoid dying during the write phase,
> but then it proceeds to call packet_write_fmt(), etc, that will die on
> any error (going to extra effort to emulate sigpipe, no less!). So we
> die and take our sideband muxer with us.
>
> So instead of this hunk:
>
>> > @@ -79,9 +82,15 @@ static void proc_receive_read_commands(struct packet_reader *reader,
>> > *p++ != ' ' ||
>> > parse_oid_hex(p, &new_oid, &p) ||
>> > *p++ != ' ' ||
>> > - die_readline)
>> > + die_readline) {
>> > + char *bad_line = xstrdup(reader->line);
>> > + while (packet_reader_read(reader) != PACKET_READ_EOF)
>> > + ; /* do nothing */
>> > + if (die_readline)
>> > + die("die with the --die-readline option");
>> > die("protocol error: expected 'old new ref', got '%s'",
>> > - reader->line);
>> > + bad_line);
>> > + }
>>
>> This part is different from the previous one in that it slurps all
>> the input before dying evein in die_readline case.
>
> 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.
> And when we see a write error, close our pipe to the hook, set errmsg to
> "hook failed to run" or similar, and then jump to the "cleanup" label,
> where we'll wait on our sideband muxer to finish (which in turn will
> wait pump any remaining data out of the hook's stderr).
>
> Optionally we can pump the hook stdout to see if it gave us a better
> message, but I think if write() failed, then all bets are off. The hook
> broke protocol; a well-behaved hook that wanted to pass along a specific
> per-ref message to the user would actually read all the input and then
> report on each ref).
Yup, sounds good.
