Re: [PATCH v2 2/8] upload-pack: implement ref-in-want
From: Stefan Beller <hidden>
Date: 2018-06-14 18:40:55
Hi Brandon, On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams [off-list ref] wrote:
Currently, while performing packfile negotiation, clients are only allowed to specify their desired objects using object ids. This causes a vulnerability to failure when an object turns non-existent during
I stopped reading when stumbling upon 'vulnerability to failure' a I found it hard to read. A quick search turns out this is insider slang of civil engineers. :)
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.
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?
quoted hunk ↗ jump to hunk
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.shdiff --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? When using this feature is a ref advertisement still needed?
quoted hunk ↗ jump to hunk
+ url.<base>.insteadOf:: Any URL that starts with this value will be rewritten to start, instead, with <base>. In cases where some site serves adiff --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.
quoted hunk ↗ jump to hunk
@@ -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?
+ +# 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.
+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.
+ +test_expect_success 'mix want and want-ref' '
cool!