Re: [PATCH v2 1/5] receive-pack: add new proc-receive hook

From: Jiang Xin <worldhello.net@gmail.com>
Date: Mar 10, 2020, 06:03 UTC
Message-ID: <CANYiYbHvqLX_OozgAXJ8MbuLipqUdj4CpbExe0oiCcyUyb=Osw@mail.gmail.com>
In-Reply-To: <xmqq7dzt1mhi.fsf@gitster-ct.c.googlers.com>

Junio C Hamano <gitster@pobox.com> 于2020年3月10日周二 上午1:12写道：

Show 35 quoted lines

> Why do you even need "must-exist" in the first place?  In other
> parts of Git, when a hook does not exist, processing by that missing
> hook is simply not done.  Why does it have to cause an error?
>
> If there is a reasonable answer to the above question, then the
> concept that hooks can be marked as "must exist -- otherwise it is
> an error" would be independently useful and the support for that
> concept should be split out as a separate step before this one, I
> would think.
>
> Also, I am not sure why you have to have one single "special" ref
> hierarchy to be handled by the hook.  Why not feed everything and
> let the hook decide?
>
> I would have expected a design more along the lines of "if the hook
> exists, then feed all the refs to the hook, let the hook do whatever
> it wants and report what it did back to the receive-pack process, so
> that the client side can learn what got accepted and what got
> rejected (and possibly other things that may require protocol
> extension later)".  One of the possible things the hook can report
> may be "I did not do anything for this ref, please do whatever you
> normally do".  That way, you do not need to introduce "there are two
> classes of refs, normal and special"---that becomes an unused and
> unnecessary concept.  For whichever ref the hook considers "normal",
> it can use the mechanism to say "I want you to do the normal thing".
> And when it sees refs/for/ prefix (or whatever is considered
> "special" at the hook's discretion), it can do whatever it wants,
> and report "OK, I ate it" or "No, I reject it" (which are only the
> two things that can be returned back to the client within the
> current protocol, IIUC).  We may later want to extend the mechanism
> to allow richer response by the hook at the same time to extend the
> on-the-wire protocol and then it may become possible for the hook to
> take a push to refs/for/master, update refs/heads/master to the
> commit (or to some other commit), and report "OK, I accepted it and
> also updated refs/heads/master to this other commit".

In patch 3/5, I add a new config variable "receive.procReceiveRefs" for developers to choose whether all of the commands or only a part of them should pipe to the "proc-receive" hook. For example, a developer may set the config variable on the server like this:

    git config --system --add receive.procReceiveRefs refs/

Then all commands will pipe to the "proc-receive" hook.

Think there may be thousands of commands in the initial stage for setting up a new repository, and all of the commands are "normal" (creating branches or tags). If the "proc-receive" hook has to handle all of the "normal" commands by piping these commands to a "git update-ref --stdin" process, or pass these commands back to "receive-pack" by a new protocol, this will greatly impact the performance. I think it's better to let the developer to choose by using a config variable like "receive.procReceiveRefs".

Patch 4/5 (receive-pack: read env from proc-receive output) is unnecessary, and extend the on-the-wire protocol is a good idea. The protocol may using pkt-line format, and negotiation between receive-pack (S) and the hook (H) like this:

    S: PKT-LINE(version=1\0side-band-64k ...)  # I'm not sure it is
necessary for the capabilities for all controllable server side
softwares.
    S: flush-pkt
    H: PKT-LINE(version=1\0side-band-64k ...)
    H: flush-pkt
    S: PKT-LINE(old-oid new-oid ref)
    S: ... ...
    S: flush-pkt
    S: PKT-LINE(push-option)
    S: ... ...
    S: flush-pkt
    H: PKT-LINE( sideband< progress... > )
    H: ... ...
    H: PKT-LINE(old-oid new-oid ref OK) # OK, I ate it
    H: PKT-LINE(old-oid new-oid ref error-msg...) # NO, I reject it
    H: PKT-LINE(old-oid new-oid ref)  # Back to receive-pack to handle it
    H: ... ...
    H: flush-pkt

After piping the "special" commands to the "proc-receive" hook, receive-pack should discard the "special" commands, and add the commands back from the output of the "proc-receive" hook. Then "receive-pack" will continue to execute the internal "execute_commands" function for the unexecuted commands.

In order to have performance similar to the vanilla receive-pack, it is necessary to keep the config variable and the concept of "normal" and "special" references. If the config variable "receive.procReceiveRefs" is defined on the server, but the "proc-receive" hook does not exist, it's better give an error instead of fall back to the internal "execute_commands" function or doing nothing.

-- Jiang Xin