Re: [PATCH v3 2/2] receive-pack: gently write messages to proc-receive

From

Jiang Xin <worldhello.net@gmail.com>

Date

Nov 11, 2020, 11:03 UTC

Message-ID

<CANYiYbEd1bDtwsq44Xqsc1o3=a6XO7aX5b_ZQ9gh7-xOS5O5rg@mail.gmail.com>

In-Reply-To

<20201110215250.GB3263091@coredump.intra.peff.net>

Jeff King <peff@peff.net> 于2020年11月11日周三 上午5:52写道：

> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index bb9909c52e..697a4e8802 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -974,9 +974,10 @@ static int read_proc_receive_report(struct packet_reader *reader,
> >       struct command *cmd;
> >       struct command *hint = NULL;
> >       struct ref_push_report *report = NULL;
> > -     int new_report = 0;
> >       int code = 0;
> > +     int new_report = 0;
>
> This is just noise in the diff, I think. It does not matter either way.
>
> > @@ -984,8 +985,14 @@ static int read_proc_receive_report(struct packet_reader *reader,
> >               const char *refname;
> >               char *p;
> >
> > -             if (packet_reader_read(reader) != PACKET_READ_NORMAL)
> > +             if (packet_reader_read(reader) != PACKET_READ_NORMAL) {
> > +                     if (!response) {
> > +                             strbuf_addstr(errmsg, "no response from proc-receive hook");
> > +                             return -1;
> > +                     }
> >                       break;
> > +             }
> > +             response++;
>
> This extra check seems orthogonal to the rest of the commit. It does
> seem like it might be a reasonable thing to check, but I wondered:
>
>   - if the hook has nothing to report, wouldn't it just send a flush
>     packet? Does that break protocol or not?

-- snip --

@@ -977,15 +977,25 @@ static int read_proc_receive_report(struct
packet_reader *reader,
        for (;;) {
                struct object_id old_oid, new_oid;
                const char *head;
                const char *refname;
                char *p;
-
-               if (packet_reader_read(reader) != PACKET_READ_NORMAL)
+               enum packet_read_status status;
+
+               status = packet_reader_read(reader);
+               if (status != PACKET_READ_NORMAL) {
+                       /* Check whether proc-receive exited abnormally */
+                       if (status == PACKET_READ_EOF && !response) {
+                               strbuf_addstr(errmsg, "proc-receive
exited abnormally");
+                               return -1;
+                       }
                        break;
+               }
+               response++;
-- snap --

>
> > @@ -1100,7 +1107,7 @@ static int run_proc_receive_hook(struct command *commands,
> >       struct strbuf cap = STRBUF_INIT;
> >       struct strbuf errmsg = STRBUF_INIT;
> >       int hook_use_push_options = 0;
> > -     int version = 0;
> > +     int version = -1;
> > [...]
> > -     if (version != 1) {
> > +     if (version == -1) {
> > +             strbuf_addstr(&errmsg, "fail to negotiate version with proc-receive hook");
> > +             code = -1;
> > +             goto cleanup;
> > +     } else if (version != 1) {
> >               strbuf_addf(&errmsg, "proc-receive version '%d' is not supported",
> >                           version);
>
> Likewise this seems orthogonal to the main point of the patch. Though it
> seems like a good idea in general to check when the other side doesn't
> report a version (assuming it is a protocol breakage not to report the
> version, and we're not simply supposed to default).

Will add a new patch for default version for proc-receive.