Hi Brandon,
On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams <bmwill@xxxxxxxxxx> 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?
> Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx>
> ---
> 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?
When using this feature is a ref advertisement still needed?
> +
> 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.
> @@ -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!
