On 06/14, Stefan Beller wrote:
> Hi Brandon,
> On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams <bmw...@google.com> wrote:
> > 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.
>
> > 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.
>
> > Signed-off-by: Brandon Williams <bmw...@google.com>
> > ---
> > 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.
>
> > +
> > 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.
>
> > @@ -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.
>
>
> > +
> > +# 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.
I forgot to remove this when i updated the docs. I'll remove this test
as it fails now :(
>
>
> > +
> > +test_expect_success 'mix want and want-ref' '
>
> cool!
--
Brandon Williams
