Jeff King <peff@xxxxxxxx> 于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?
It's OK to send a flush packet without any payload, so will check
status PACKET_READ_EOF for broken proc-receive.
-- 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.
