Re: [PATCH 4/4] Doc: push with --base

[Jonathan Nieder <jrnieder@gmail.com>]

(+cc: Elijah Newren and Derrick Stolee because of [1])
Hi,

Jonathan Tan wrote:

> Includes protocol documentation and a design document.

Thanks for writing this.

> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  Documentation/technical/pack-protocol.txt  | 10 ++--
>  Documentation/technical/push-with-base.txt | 61 ++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/technical/push-with-base.txt
> 
> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
> index e13a2c064d..0485616701 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -59,9 +59,9 @@ Parameters", and are supported by the Git, SSH, and HTTP protocols.
>  Each Extra Parameter takes the form of `<key>=<value>` or `<key>`.
>  
>  Servers that receive any such Extra Parameters MUST ignore all
> -unrecognized keys. Currently, the only Extra Parameter recognized is
> -"version" with a value of '1' or '2'.  See protocol-v2.txt for more
> -information on protocol version 2.
> +unrecognized keys. Currently, the only Extra Parameters recognized are
> +"version" with a value of '1' or '2' and, for push, "base" with an OID.  See
> +protocol-v2.txt for more information on protocol version 2.

It's nice to see an example of extra parameters being useful in
protocol v0 as well. :)

>  
>  Git Transport
>  -------------
> @@ -506,6 +506,10 @@ real difference is that the capability listing is different - the only
>  possible values are 'report-status', 'report-status-v2', 'delete-refs',
>  'ofs-delta', 'atomic' and 'push-options'.
>  
> +If a "base=<oid>" Extra Parameter was sent by the client, and the
> +server recognizes that object, the server MAY send "<oid> .have" in
> +lieu of all the reference obj-ids and names.
> +

Can this only appear once, or is it permitted to pass multiple oids
this way?

I'm also curious about what Junio asked about elsewhere in the thread:
for cases that benefit from more complex negotiation than the client
proposing a particular oid, what comes next in this roadmap?  I like
that this is an optional feature so we could clean up later by
removing support for it; do we expect to?  If we do expect to, is
there anything we could do to minimize the impact of the feature later
(e.g. using a less short-and-sweet key name than `base`, maybe)?

>  Reference Update Request and Packfile Transfer
>  ----------------------------------------------
>  
> diff --git a/Documentation/technical/push-with-base.txt b/Documentation/technical/push-with-base.txt
> new file mode 100644
> index 0000000000..d56aa7f900
> --- /dev/null
> +++ b/Documentation/technical/push-with-base.txt
> @@ -0,0 +1,61 @@
> +Push with base design notes
> +===========================
> +
> +This feature allows clients, when pushing, to indicate that a
> +certain object is an ancestor of all pushed commits and that they
> +believe that the server knows of this object. This in turn allows
> +servers to send an abbreviated ref advertisement containing only that
> +object.
> +
> +Besides bandwidth savings, this also ensures that the ref
> +advertisement contains information relevant to the client. For
> +example, at least one project (Gerrit [1]) have included workarounds
> +to send ancestors of refs that move often, even though the ref
> +advertisement is only meant to contain refs.
> +
> +[1] https://gerrit.googlesource.com/gerrit/+/refs/heads/master/java/com/google/gerrit/server/git/receive/HackPushNegotiateHook.java
> +
> +
> +Design overview
> +---------------
> +
> +The "base" being sent is sent as an Extra Parameter, supported in the
> +git://, ssh://, and http(s):// protocols. By sending it as an Extra
> +Parameter, the server is aware of this parameter before it generates
> +the ref advertisement, thus making it able to tailor the ref
> +advertisement accordingly. Sending it as an Extra Parameter also makes
> +this protocol backwards-compatible, as servers will ignore any Extra
> +Parameters they do not understand. (The push will then proceed as if
> +neither party had this feature.)
> +
> +The remote helper protocol has been extended to support the
> +"push-base" capability and an option of the same name. When a remote
> +helper advertises this capability, it thus indicates that it supports
> +this option. Git then will send "option push-base" if the user
> +specifies it when invoking "git push".
> +
> +The remote-curl remote helper bundled with Git has been updated to
> +support this capability and option.

Since this is an optional remote helper capability, it could be
removed later.  Good (but the capability and option names would still
need to be reserved).

> +
> +
> +Future work
> +-----------
> +
> +In the future, we might want a way to automatically determine the base
> +instead of always having the user specify it. However, this does not
> +make obsolete any of the current work - once the base is automatically
> +determined, we still need this protocol to communicate it to the
> +server, and allowing the user to specify the base manually is still
> +useful.

Makes sense.  I think this isn't "might", but "would"; most users
that do not know why their push is slow wouldn't know to use this
feature.

> +
> +
> +Alternatives
> +------------
> +
> +- Making a more substantial protocol change like "fetch" protocol v2.
> +  This would eliminate the need for some of the remote helper updates;
> +  as part of the protocol change, the protocol could be made to
> +  support "stateless-connect" and thus no remote helper updates (like
> +  "push-base") would be needed. For "fetch", the protocol change has
> +  enabled features like wanted-refs and packfile-uris, but I do not
> +  have any similar ideas in mind for "push".

I think you're saying that we don't need a "push" v2 because v0
already has what a user would want.

Git protocol v2 for fetch brought two major changes:

- it changed the response for the initial request, allowing
  abbreviating the ref advertisement at last

- it defined a structure for requests and responses, simplifying the
  addition of later protocol improvements.  In particular, because the
  initial response is a capability advertisement, it allows changing
  the ref advertisement format more in the future.

Both of those changes would be valuable for push.  The ref
advertisements are large, and matching the structure of commands used
by fetchv2 would make debugging easier.

There are some specific applications I'm interested in after that
(e.g., pushing symrefs), but the fundamental extensibility improvement
is larger than any particular application I could think of.

That said, I'm not against experimenting with extra parameters before
we go there, as a way of getting more information about what a
workable negotiation for push looks like.  The push ref advertisement
serves two roles: it advertises commits, to serve as negotiation, and
it tells the client the current values of refs they may want to
update, so they can send an appropriate compare-and-swap command for
a force-push.  If we have a workable replacement negotiation system,
then the other role can be replaced with something simpler.  For
example:

- listing ref updates before the pack, to allow the server to bail
  out early if they would be rejected, and

- allowing an explicit force update of a ref instead of requiring the
  client to compare-and-swap if they don't care about the old value
  (but taking care to still make the client "fetch first" when
  appropriate!)

Thanks,
Jonathan

[1] https://lore.kernel.org/git/CABPp-BEswHLhymQ1_07g3qqu=7kFR3eQyAHR0qMgSvi6THy=zQ@mail.gmail.com/