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

[Jiang Xin <worldhello.net@gmail.com>]

Jeff King <peff@peff.net> 于2020年11月10日周二 上午7:12写道：
> > > @@ -52,8 +53,10 @@ static void proc_receive_verison(struct packet_reader *reader) {
> > >             }
> > >     }
> > >
> > > -   if (server_version != 1 || die_version)
> > > +   if (server_version != 1)
> > >             die("bad protocol version: %d", server_version);
> > > +   if (die_version)
> > > +           die("die with the --die-version option");
> >
> > If any of these trigger, wouldn't we end up dying without consuming
> > what receive-pack said?
>
> Yeah, I think they would have the same race that the commit message
> describes (proc-receive hook writes to stderr and dies, receive-pack
> gets an error writing to now-closed hook pipe and never relays the
> stderr).
>
> But it seems like fixing this in the hook is the wrong place. The hook
> has failed and has nothing else to say. Adding a pump-the-stdin-to-eof
> loop to every die() is a lot of effort. Not to mention that the hook
> could fail for reasons outside its usual flow control (e.g., segfault,
> oom, etc, and receive-pack should be able to handle that gracefully,
> even if the hook doesn't appear to behave.
>
> I.e., I think the bug is in receive-pack's run_proc_receive_hook(). 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.

Thanks Peff for pointing the root cause. Will use the "gently" forms
of packet_write_fmt() and packet_flush() in patch v3.

--
Jiang Xin