On Tue, Apr 28, 2020 at 01:00:00AM +0800, Jiang Xin wrote:
> For over a week, I have not received more comments on reroll v13 on
> this topic ("jx/proc-receive-hook"). Therefore, I invite Peff and
> Couder to review because I think it would be very interesting to add a
> centralized workflow to Github and Gitlab.
>
> * [PATCH v13 3/8]:
> https://public-inbox.org/git/20200418160334.15631-4-worldhello.net@xxxxxxxxx/
>
> This patch introduces a new hook "proc-receive" on the server side. It
> won't break anything except GitHub's "spokes" architecture (I guess).
> Because in Alibaba, we have such issue when we implement our own
> "spokes" architecture. In order to make this special push operation
> (on a special ref such as "refs/for/master/topic") idempotently on
> multiple replicas, we extended the protocol of "receive-pack" and let
> "spokes" to send a request with a flag, which specifies one replica as
> master replica to execute the "proc-receive" hook.
We don't do any magic ref-rewriting like this, so we wouldn't turn such
a hook on. Presumably without such a hook, the protocol would remain the
same (i.e., if there was no rewrite, then the report-status response
wouldn't have the extra field).
And I'm not sure if you're proposing to also use it as the trigger point
to coordinate the ref update across replicas. That wouldn't be helpful
for our architecture. All Git client connections to GitHub terminate at
a proxy layer that handles load balancing, authentication, and routing.
For pushes, that's the layer that multiplexes the packfile to each
replica, and it drives the ref update through an out-of-band procedure.
So there is no master replica at all; they're all talking equally to the
proxy layer which coordinates things (and the same is true for non-push
updates; they're coordinated by whichever entity is trying to perform
the update).
We do run receive-pack on each replica backend. We have a hacky patch
for a config option that tells receive-pack to just skip the actual
ref-transaction, leaving it up to the proxy layer to do. I've been
pushing for us to actually abandon receive-pack entirely, since most of
its heavy lifting can be done by sub-programs (for-each-ref for the
advertisement, index-pack to receive the pack, and update-ref to update
refs). But it's a non-trivial change, and the benefits are only
moderate, so it hasn't quite been worth the effort yet.
So I don't really have much of an opinion on it either way.
Reading the commit message for patch 3, I do have one small comment. I
see this:
The reporting function from "receive-pack" to "send-pack" is extended
using a backward compatible way by adding key-value pairs after an null
character, like:
# OK, run this command successfully with optional extended-status.
ok <reference>\0ref=refs/pull/123/head old-oid=...
# NO, I reject it.
ng <reference> <error message>
but we should probably avoid trickery like stuffing extra data after a
NUL byte. We could quite easily extend the protocol with a new
capability here. The server advertises "report-status-v2" or something,
and the client responds with the appropriate capability to indicate that
it understands. And then the new format can be something more
extensible, like:
pktline(ref=refs/heads/master)
pktline(status=ok)
pktline(old-oid=...)
pktline(rewrite=refs/pull/123/head)
delim-pkt(0001)
pktline(ref=refs/heads/other)
pktline(status=error)
pktline(message=<error message>
flush-pkt(0000)
which would give us room for more keys later, but without worrying about
parsing issues after the NUL, running out of room in a pktline if you
have too many keys, etc.
I don't have any real thoughts on the hook interface itself. It seems
like the ok/no/ft/alt responses would allow just about any hook you'd
want. It does seems to also use NUL trickery in the early part. If we're
designing from scratch, I think we should strive for a cleaner protocol.
-Peff
