Re: [PATCH v10 2/8] receive-pack: add new proc-receive hook

>
> Jiang Xin <worldhello.net@gmail.com> writes:
> >     # Receive result from the hook.
> >     # OK, run this command successfully.
> >     H: PKT-LINE(<old-oid> <new-oid> <ref> ok)
> >     # NO, I reject it.
> >     H: PKT-LINE(<old-oid> <new-oid> <ref> ng <reason>)
> >     # Fall through, let 'receive-pack' to execute it.
> >     H: PKT-LINE(<old-oid> <new-oid> <ref> ft)
> >     # OK, but has an alternate reference.  The alternate reference name
> >     # and other status are given in key=value pairs after the null
> >     # character.
> >     H: PKT-LINE(<old-oid> <new-oid> <ref> ok\0ref=refs/pull/123/head
> >                 forced-update)
>
> The semantics of this one is fuzzy.  We made an update to a ref that
> is different from the one that was requested (presumably that is what
> is reported after "\0ref="), OK, but did we update the ref in the
> request, too, or did we leave the ref in the request intact?  Or, do
> we say "no\0ref=..." if we don't update the requested ref and
> instead update a different one?  Let's hold the answer at this point
> and keep thinking...

> >     H: ... ...
> >     H: flush-pkt
> >
> > After receiving a command, the hook will execute the command, and may
> > create/update different reference.  For example, a command for a pseudo
> > reference "refs/for/master/topic" may create/update different reference
> > such as "refs/pull/123/head".  The alternate reference name and other
> > status are given in key-value pairs as extended status of the report
> > line.
> >
> > The list of commands returned from "proc-receive" will replace the
> > relevant commands that are sent from user to "receive-pack", and
> > "receive-pack" will continue to run the "execute_commands" function and
> > other routines.  Finally, the result of the execution of these commands
> > will be reported to end user.
>
> It is not clear if there needs to be any correspondence between the
> sequence of input commands and the sequence of output commands.  I
> am guessing that there is not any and the hook is allowed to do
> anything as it wants to.  For example:
>
>  - it is OK to execute them all and report them, but in a totally
>    random order.
>
>  - it is OK to ignore all input and report no update (not even
>    "ng").
>
>  - it is OK to be asked to update one ref, but update and report
>    updates to two refs that are not related to the requested ref.
>
> Is my understanding correct?
>
> Or does the design tie the set of input and output ref updates
> closely together, i.e. one input corresponds to one output and they
> are in order, so all the hook is allowed to do is
>
>  - to execute and update as it is told, and report "ok",
>
>  - to reject and report "ng", or
>
>  - to update *another* ref without updating the requested one, with
>    "ok\0ref=..." mechanism.
>
> I am not sure which one is more sensible, though.
>
> If we choose to use the "anything goes" approach, I do not think
> there is no need for the "ok\nref=..." mechanism---we can just give
> two output records, one "ok" for the original request, and a new
> "ok" that reports the "we updated this one instead".

> > +             } else if (strcmp("ok", status)) {
> > +                     strbuf_addf(errmsg, "proc-receive has bad status '%s' for '%s'\n",
> > +                                 status, reader->line);
> > +                     code = -1;
> > +                     /* Skip marking it as RUN_PROC_RECEIVE_RETURNED */
>
> But then shouldn't we be complaining if msg is not NULL here,
> instead of silently ignoring?  Also we didn't see what happened to
> the promised "ok\0ref=..." stuff here, or the passthru "ft".
> Puzzled...

> > +                     continue;
> > +             }
> > +             oidcpy(&hint->old_oid, &old_oid);
> > +             oidcpy(&hint->new_oid, &new_oid);
> > +             hint->run_proc_receive |= RUN_PROC_RECEIVE_RETURNED;
>
> Or, is this last one the catch all for "ft" (in other words, the
> hook does not have to say "ft", but as long as it says two non-SP
> letters that are not "ok" nor "ng", it is taken as "ft")?

>
> > +     }
> > +
> > +     for (cmd = commands; cmd; cmd = cmd->next)
> > +             if (cmd->run_proc_receive && !cmd->error_string &&
> > +                 !(cmd->run_proc_receive & RUN_PROC_RECEIVE_RETURNED)) {
> > +                 cmd->error_string = "no report from proc-receive";
> > +                 code = -1;
> > +             }
> > +
> > +     return code;
> > +}
>
> OK, so this sort-of answers my earlier question.  But not quite...
>
> The output records and the input requests are tied by "<ref>" each
> input record wanted to udpate.  The order does not matter, but not
> having a corresponding report in the hook's output is like the hook
> reporting a "ng" failure.  It also means that the hook can update
> two refs in response to one request, but it is awkward.
>

> > +static int run_proc_receive_hook(struct command *commands,
> > +                              const struct string_list *push_options)
> > +{
> > +     struct child_process proc = CHILD_PROCESS_INIT;
> > +     struct async muxer;
> > +     struct command *cmd;
> > +     const char *argv[2];
> > +     struct packet_reader reader;
> > +     struct strbuf cap = STRBUF_INIT;
> > +     struct strbuf errmsg = STRBUF_INIT;
> > +     int pr_use_push_options = 0;
> > +     int version = 0;
> > +     int code;
> > +
> > +     argv[0] = find_hook("proc-receive");
> > +     if (!argv[0]) {
> > +             rp_error("cannot find hook 'proc-receive'");
> > +             return -1;
> > +     }
> > +     argv[1] = NULL;
> > +
> > +     proc.argv = argv;
> > +     proc.in = -1;
> > +     proc.out = -1;
> > +     proc.trace2_hook_name = "proc-receive";
> > +
>
> Isn't this a brand new protocol between receive-pack and a hook?  I
> am puzzled why we want a choice between using and not using
> sideband.  It's not like you are maintaining compatibility with
> ancient version of Git that talked with proc-receive hook but
> without sideband, because it did not know how to multiplex sideband
> communication.  Shouldn't we just always assume that the sideband is
> available?

> Or are we just letting the hook directly answer the push client and
> that is why this thing needs to know if we are using sideband
> between us and the client?  I kind of expected that you'd keep the
> two communication channels on both sides isolated so that the side
> that talks with the hook does not need to know how we are talking
> with the client.

> > +     if (use_sideband) {
> > +             memset(&muxer, 0, sizeof(muxer));
> > +             muxer.proc = copy_to_sideband;
> > +             muxer.in = -1;
> > +             code = start_async(&muxer);
> > +             if (code)
> > +                     return code;
> > +             proc.err = muxer.in;
> > +     } else {
> > +             proc.err = 0;
> > +     }
> > +
> > +     code = start_command(&proc);
> > +     if (code) {
> > +             if (use_sideband)
> > +                     finish_async(&muxer);
> > +             return code;
> > +     }
> > +
> > +     sigchain_push(SIGPIPE, SIG_IGN);
> > +
> > +     /* Version negotiaton */
> > +     packet_reader_init(&reader, proc.out, NULL, 0,
> > +                        PACKET_READ_CHOMP_NEWLINE |
> > +                        PACKET_READ_DIE_ON_ERR_PACKET);
> > ...
> > @@ -1497,6 +1731,20 @@ static void execute_commands(struct command *commands,
> >
> >       reject_updates_to_hidden(commands);
> >
> > +     /* Try to find commands that have special prefix in their reference names,
>
> Style?