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

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

Junio C Hamano <gitster@pobox.com> 于2020年4月13日周一 上午5:30写道：
>
> 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...

Before making a decision, we must consider some limitations or backward
compatibility issues.  See the limitations from following code snippet
of "send-pack.c" and "transport.c":

* "receive-pack" should not send status other than "ok" and "ng" to "send-pack".
  We must keep this for backward compatibility.

    # File: send-pack.c
    #
    152 static int receive_status(struct packet_reader *reader, struct ref *refs)
    153 {
    159     while (1) {
    160         const char *refname;
    161         char *msg;
    162         if (packet_reader_read(reader) != PACKET_READ_NORMAL)
    163             break;
    164         if (!starts_with(reader->line, "ok ") && !starts_with(reader->line, "ng ")) {
    165             error("invalid ref status from remote: %s", reader->line);
    166             ret = -1;
    167             break;
    168         }

* Do not report unknown reference to "send-pack". Each returned reference
  must be included in `remote_refs`.

    175         /* first try searching at our hint, falling back to all refs */
    176         if (hint)
    177             hint = find_ref_by_name(hint, refname);
    178         if (!hint)
    179             hint = find_ref_by_name(refs, refname);
    180         if (!hint) {
    181             warning("remote reported status on unknown ref: %s",
    182                     refname);

* Do not report unexpected reference to "send-pack".

    185         if (hint->status != REF_STATUS_EXPECTING_REPORT) {
    186             warning("remote reported status on unexpected ref: %s",
    187                     refname);
    188             continue;
    189         }

* Client will complain if a expecting command not is not received from
  the report.

    # File: transport.c
    #
    531 static int print_one_push_status(struct ref *ref, const char *dest, int count,
    532                                  int porcelain, int summary_width)
    540     switch(ref->status) {
    584     case REF_STATUS_EXPECTING_REPORT:
    585         print_ref_status('!', "[remote failure]", ref,
    586                          ref->deletion ? NULL : ref->peer_ref,
    587                          "remote failed to report status",
    588                          porcelain, summary_width);


So we should not invent new status between "receive-pack" and "send-pack".
It is reasonable to to extend the report function from 'receive-pack'
to 'send-pack' (not the report from 'proc-receive' to 'receive-pack')
by adding a null character and multiple key-value pairs as extended
status after the "ok" and "ng" message.

For example, the following git-push command:

    git push origin HEAD:refs/for/master/topic

The refspec points to a pseudo reference "refs/for/master/topic", which
does not exist on the server, so the old-oid of the command to
"receive-pack" must be a zero-oid. The command for 'proc-receive' is:

    <zero-oid> <new-oid> refs/for/master/topic

If "proc-receive" find this `git-push` command is associated with an already
exist pull request (such as "refs/pull/123/head"), it will update this
reference instead of creating a new one.

For this case, we can extend the report line (from "receive-pack" to
"send-pack”) like this:

    ok refs/for/master/topic\0ref=refs/pull/123/head old-oid=<hash> [forced-push]

This extended report line is backward compatible. Old version of git
client will show message like this:

    To upstream.git
     * [new branch]          HEAD -> refs/for/master/topic

New version of git has the knowledge of how to handle the extended-status
after the null character, will print the following message:

    To upstream.git
     + 1234567...7654321     HEAD -> refs/pull/124/head (forced update)


Actually, both 'ok' and 'ng' status have the knowledge of '<msg>', even
though the '<msg>' is ignored by the 'ok' status. After the extension,
both of them can take key-value pairs as extended-status, like:

    ok <reference> [msg]\0key=value ...
    ng <reference> [msg]\0key=value ...

We can use '<msg>' and the extended status to fit our needs in the future.

As for the design of status report from 'proc-receive' to 'receive-pack',
we should follow the same rules:

1. Never report status for non-exists commands.

2. Do not forget to report some of the commands.

3. After receive report for commands from 'proc-receive', we should have
   enough information to generate report to 'send-pack' from these commands.

For the above example, when user push to 'refs/for/master/topic',
'proc-receive' may report to 'receive-pack' like this:

    PKT-LINE(<old-oid> <new-oid> <ref> ok\0ref=refs/pull/123/head forced-update)

'send-pack' will save the key-value pairs into the cooresponding `ref->remote_status`.
If the report line has a different '<old-oid>' or '<new-oid>' for the '<ref>',
'send-pack' will substitude the oids of the ref, and append new key-value
pairs (old-oid=<hash> and new-oid=<hash>) to `ref->remote_status`.

> >     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".

I saw you have noticed the code for replacing the commands of 'receive-pack'
by comparing ref name.

> > +             } 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...

I implement "ft" and "\0key=value..." features in patch 6/8, and it
depends another refactoring commit.  I can squash it to this commit
in next reroll, and rearrange the refactoring commmit and test cases.


> > +                     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")?

"ft" is implemented in patch 6/8.  This code is used to help to find
missing reported references.

>
> > +     }
> > +
> > +     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.
>

Right sort order of the output commnands has better performace.
The hook can only update commands marked with 'run_proc_receive'.

"update two refs", do you mean duplicate report line from 'proc-receive'? 
If 'proc-receive' really want to update several references for one
command, it must report all of the updates in one report line by adding
message, or adding additional key-value pairs.

> > +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?

The sideband message stream (stderr of 'proc-receive') will be sent
directly to the client.  If the client is a very old version of git,
is is still safe?

> 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.

In test cases for 'proc-receive', I output some debug info on stderr
of 'proc-receive' hook.  If the client (very old version of git)
does not support sideband, it's ok.

> > +     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?

Will fix.
>
> > +      * and mark them to run an external "proc-receive" hook later.
> > +      */