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

[Brandon Williams <bmwill@google.com>]

On 06/14, Stefan Beller wrote:
> Hi Brandon,
> On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams <bmwill@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 <bmwill@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