Re: [PATCH v2 2/8] upload-pack: implement ref-in-want

On 06/14, Stefan Beller wrote:

Hi Brandon,
On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams [off-list ref] wrote:

quoted

negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement.

... and the repository is not replicated evenly to all servers, yet.

I'll update the commit msg to also include this.

quoted

In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2.  This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref <ref>" parameter.  At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.

This paragraph makes it sound as if it can be combined technically,
i.e.

client:
    want 01234...
    want-ref master

.. usual back and forth + pack..

server:

  wanted-ref: master 2345..

What happens if the client "wants" a sha1 that is advertised,
but happens to be the same as a wanted-ref?

This would be fine, same as sending a want line with the same sha1 lots
of times.  Though there would still be a wanted-ref section from the
server for the wanted-ref.

quoted

Signed-off-by: Brandon Williams <redacted>
---
 Documentation/config.txt                |   7 ++
 Documentation/technical/protocol-v2.txt |  29 ++++-
 t/t5703-upload-pack-ref-in-want.sh      | 153 ++++++++++++++++++++++++
 upload-pack.c                           |  64 ++++++++++
 4 files changed, 252 insertions(+), 1 deletion(-)
 create mode 100755 t/t5703-upload-pack-ref-in-want.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a..fb1dd7428 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it is seen in the
 repository-level config (this is a safety measure against fetching from
 untrusted repositories).

+uploadpack.allowRefInWant::
+       If this option is set, `upload-pack` will support the `ref-in-want`
+       feature of the protocol version 2 `fetch` command.  This feature
+       is intended for the benefit of load-balanced servers which may
+       not have the same view of what OIDs their refs point to due to
+       replication delay.

Instead of saying who benefits, can we also say what the feature is about?
Didn't someone mention on the first round of this series, that technically
ref-in-want also provides smaller net work load as refs usually are shorter
than oids (specifically as oids will grow in the hash transisition plan later)?
Is that worth mentioning?

Well I basically just took this from what a previous reviewer thought it
should say.  I think what you have listed here isn't really a big
benefit of using ref-in-want, its the issue with load-balanced servers
that this is trying to solve.

When using this feature is a ref advertisement still needed?

Maybe in the future no, but as of right now the code is structured to
still request a ref advertisement.

quoted

+
 url.<base>.insteadOf::
        Any URL that starts with this value will be rewritten to
        start, instead, with <base>. In cases where some site serves a

diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 49bda76d2..6020632b4 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt

@@ -299,12 +299,22 @@ included in the client's request:
        for use with partial clone and partial fetch operations. See
        `rev-list` for possible "filter-spec" values.

+If the 'ref-in-want' feature is advertised, the following argument can
+be included in the client's request as well as the potential addition of
+the 'wanted-refs' section in the server's response as explained below.
+
+    want-ref <ref>
+       Indicates to the server that the client wants to retrieve a
+       particular ref, where <ref> is the full name of a ref on the
+       server.  A server should ignore any "want-ref <ref>" lines where
+       <ref> doesn't exist on the server.

Are patterns allowed?, e.g. I might want refs/tags/* at all times.

Nope, "Where <ref> is the full name of a ref".  We can maybe allow this
at a later point in time.

quoted

@@ -319,6 +329,10 @@ header.
     shallow = "shallow" SP obj-id
     unshallow = "unshallow" SP obj-id

+    wanted-refs = PKT-LINE("wanted-refs" LF)
+                 *PKT-LINE(wanted-ref LF)
+    wanted-ref = obj-id SP refname
+
     packfile = PKT-LINE("packfile" LF)
               *PKT-LINE(%x01-03 *%x00-ff)

@@ -379,6 +393,19 @@ header.
        * This section is only included if a packfile section is also
          included in the response.

+    wanted-refs section
+       * This section is only included if the client has requested a
+         ref using a 'want-ref' line and if a packfile section is also
+         included in the response.

Is it possible to fetch non-fast-forwarded refs this way? Or specifcially
refs that were reset to an older point in history such that no pack file
is needed to transfer; would we transfer an empty pack and then
the wanted-refs section for that use case?

Yeah there are cases where an empty packfile would be sent like you've
described.

quoted

+
+# c(o/foo) d(o/bar)
+#        \ /
+#         b   e(baz)  f(master)
+#          \__  |  __/
+#             \ | /
+#               a

time is up in this diagram, most diagrams I looked at in tests
are sideways. Should be fine either way.

quoted

+test_expect_success 'invalid want-ref line' '
+       test-pkt-line pack >in <<-EOF &&
+       command=fetch
+       0001
+       no-progress
+       want-ref refs/heads/non-existent
+       done
+       0000
+       EOF
+
+       test_must_fail git serve --stateless-rpc 2>out <in &&
+       grep "unknown ref" out

The docs disagree with the test?
     A server should ignore any "want-ref <ref>" lines where
    <ref> doesn't exist on the server.

I forgot to remove this when i updated the docs.  I'll remove this test
as it fails now :(

quoted

+
+test_expect_success 'mix want and want-ref' '

cool!

-- 
Brandon Williams