Re: [PATCH v3 22/35] upload-pack: support shallow requests

[Brandon Williams]

On 02/27, Jonathan Nieder wrote:

I'll make the documentation changes you suggested.

> > +deepen 
> > +   Request that the fetch/clone should be shallow having a commit depth of
> 
> nit: s/Request/Requests/, for consistency with the others?
> 
> > +relative to the remote side.
> 
> What does the value of  mean? E.g. does a depth of 1 mean to
> fetch only the commits named in "have", 2 to fetch those commits plus
> their parents, etc, or am I off by one?

Honestly I have no clue, what does the current protocol do?  There isn't
any documentation about it and this just reuses the logic from that.

> 
> Is  always a positive number?
> 
> What happens if  starts with a 0?  Is that a client error?
> 

> >  output = *section
> > -section = (acknowledgments | packfile)
> > +section = (acknowledgments | shallow-info | packfile)
> >   (flush-pkt | delim-pkt)
> 
> It looks like sections can go in an arbitrary order.  Are there
> tests to make sure the server can cope with reordering?  (I ask
> not because I mistrust the server but because I have some vague
> hope that other server implementations might be inspired by our
> tests.)

I'll fix this so that they don't come in arbitrary order

> 
> [...]
> > @@ -215,6 +245,11 @@ header.
> >  nak = PKT-LINE("NAK" LF)
> >  ack = PKT-LINE("ACK" SP obj-id LF)
> >  
> > +shallow-info = PKT-LINE("shallow-info" LF)
> > +  *PKT-LINE((shallow | unshallow) LF)
> > +shallow = "shallow" SP obj-id
> > +unshallow = "unshallow" SP obj-id
> 
> Likewise: it looks like shallows and unshallows can be intermixed; can
> this be either (a) tightened or (b) covered by tests to make sure a
> later refactoring doesn't accidentally tighten it?

This reuses the existing logic from v0 so its due to that spec.

> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -710,7 +710,6 @@ static void deepen(int depth, int deepen_relative,
> > }
> >  
> > send_unshallow(shallows);
> > -   packet_flush(1);
> 
> What does this part do?
> >  }
> >  
> >  static void deepen_by_rev_list(int ac, const char **av,
> > @@ -722,7 +721,52 @@ static void deepen_by_rev_list(int ac, const char **av,
> > send_shallow(result);
> > free_commit_list(result);
> > send_unshallow(shallows);
> > -   packet_flush(1);
> 
> Same question.

Pulling out the flush packet so that the logic can be reused for v2, the
flush is added back in for the v0 case but not for the v2 case.

> 
> > +}
> > +
> > +static int send_shallow_list(int depth, int deepen_rev_list,
> > +timestamp_t deepen_since,
> > +struct string_list *deepen_not,
> > +struct object_array *shallows)
> 
> What does the return value from this function represent?  It doesn't
> appear to be the usual "0 means success, -1 means failure" so a
> comment would help.

I'll add a comment.

> 
> > +{
> > +   int ret = 0;
> > +
> > +   if (depth > 0 && deepen_rev_list)
> > +   die("git upload-pack: deepen and deepen-since (or deepen-not) 
> > cannot be used together");
> 
> nit: long line (can/should "make style" find these?)
> 
> The error message is pretty long, longer than a typical 80-column
> terminal, so probably best to find a way to make the message shorter.
> E.g.
> 
>   die("upload-pack: deepen cannot be combined with other deepen-* 
> options");
> 
> That still would be >80 columns with the indent, so the usual style
> would be to break it into multiple strings and use C preprocessor
> concatenation (yuck):
> 
>   die("upload-pack: "
>   "deepen cannot be combined with other deepen-* options");
> 

> [...]
> > +   if (depth > 0) {
> > +   deepen(depth, deepen_relative, shallows);
> > +   ret = 1;
> > +   } else if (deepen_rev_list) {
> > +   struct argv_array av = ARGV_ARRAY_INIT;
> > +   int i;
> > +
> > +   argv_array_push(, "rev-list");
> > +   if (deepen_since)
> > +   argv_array_pushf(, "--max-age=%"PRItime, 
> > deepen_since);
> > +   if (deepen_not->nr) {
> > +   argv_array_push(, "--not");
> > +   for (i = 0; i < deepen_not->nr; i++) {
> > +   struct string_list_item *s = deepen_not->items 
> > + i;
> > +   argv_array_push(, s->string);
> 
> This accepts arbitrary rev-list arguments, which feels dangerous
> (could end up doing an expensive operation or reading arbitrary files
> or finding a way to execute arbitrary code).
> 
> [...]
> > -   if (deepen_not.nr) {
> > -   argv_array_push(, "--not");
> > -   for (i = 0; i < deepen_not.nr; i++) {
> > -   struct string_list_item *s = deepen_not.items + 
> > i;
> > -   argv_array_push(, s->string);
> 
> Huh.  Looks like some of the above comments are better addressed to an
> earlier patch.

Re: [PATCH v3 22/35] upload-pack: support shallow requests

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

> Add the 'shallow' feature to the protocol version 2 command 'fetch'
> which indicates that the server supports shallow clients and deepen
> requets.
>
> Signed-off-by: Brandon Williams 
> ---
>  Documentation/technical/protocol-v2.txt |  67 +++-
>  serve.c |   2 +-
>  t/t5701-git-serve.sh|   2 +-
>  upload-pack.c   | 138 
> +++-
>  upload-pack.h   |   3 +
>  5 files changed, 173 insertions(+), 39 deletions(-)

Yay!  We've been running with this for a while at Google (for file://
fetches, at least) and it's been working well.

[...]
> --- a/Documentation/technical/protocol-v2.txt
> +++ b/Documentation/technical/protocol-v2.txt
> @@ -201,12 +201,42 @@ packet-lines:
>   to its base by position in pack rather than by an oid.  That is,
>   they can read OBJ_OFS_DELTA (ake type 6) in a packfile.
>  
> +shallow 
> + A client must notify the server of all objects for which it only
> + has shallow copies of (meaning that it doesn't have the parents

Grammar nit: "for which it only has shallow copies of" should be e.g.
"for which it only has shallow copies" or "that it only has shallow
copies of" or "that it only has shallow copies for".

I think s/objects/commits/ would also help clarify.

> + of a commit) by supplying a 'shallow ' line for each such
> + object so that the serve is aware of the limitations of the

s/serve/server/

> + client's history.

Is it worth mentioning that this is about negotiation?  E.g. "so that
the server is aware that the client may not have all objects reachable
from such commits".

> +
> +deepen 
> + Request that the fetch/clone should be shallow having a commit depth of

nit: s/Request/Requests/, for consistency with the others?

> +  relative to the remote side.

What does the value of  mean? E.g. does a depth of 1 mean to
fetch only the commits named in "have", 2 to fetch those commits plus
their parents, etc, or am I off by one?

Is  always a positive number?

What happens if  starts with a 0?  Is that a client error?

> +
> +deepen-relative
> + Requests that the semantics of the "deepen" command be changed
> + to indicate that the depth requested is relative to the clients
> + current shallow boundary, instead of relative to the remote
> + refs.

s/clients/client's/

s/remote refs/requested commits/ or "wants" or something.

> +
> +deepen-since 
> + Requests that the shallow clone/fetch should be cut at a
> + specific time, instead of depth.  Internally it's equivalent of
> + doing "rev-list --max-age=". Cannot be used with
> + "deepen".

Nits:
  s/rev-list/git rev-list/
  s/equivalent of/equivalent to/ or 'the equivalent of'.

Since the git-rev-list(1) manpage doesn't tell me: what is the format
of ?  And is the requested time interval inclusive of
exclusive?

> +
> +deepen-not 
> + Requests that the shallow clone/fetch should be cut at a
> + specific revision specified by '', instead of a depth.
> + Internally it's equivalent of doing "rev-list --not ".
> + Cannot be used with "deepen", but can be used with
> + "deepen-since".

Interesting.

nit: s/rev-list/git rev-list/

What is the format of ?  E.g. can it be an arbitrary revision
specifier or is it an oid?

[...]
>  output = *section
> -section = (acknowledgments | packfile)
> +section = (acknowledgments | shallow-info | packfile)
> (flush-pkt | delim-pkt)

It looks like sections can go in an arbitrary order.  Are there
tests to make sure the server can cope with reordering?  (I ask
not because I mistrust the server but because I have some vague
hope that other server implementations might be inspired by our
tests.)

[...]
> @@ -215,6 +245,11 @@ header.
>  nak = PKT-LINE("NAK" LF)
>  ack = PKT-LINE("ACK" SP obj-id LF)
>  
> +shallow-info = PKT-LINE("shallow-info" LF)
> +*PKT-LINE((shallow | unshallow) LF)
> +shallow = "shallow" SP obj-id
> +unshallow = "unshallow" SP obj-id

Likewise: it looks like shallows and unshallows can be intermixed; can
this be either (a) tightened or (b) covered by tests to make sure a
later refactoring doesn't accidentally tighten it?

[...]
> @@ -247,6 +282,36 @@ header.
> determined the objects it plans to send to the client and no
> further negotiation is needed.
>  
> +
> +shallow-info section
> + If the client has requested a shallow fetch/clone, a shallow
> + client requests a fetch or the server is shallow then the
> + server's response may include a shallow-info section.

I'm having trouble parsing this sentence.

>  The
> + shallow-info section will be include if (due to one of the above

nit: s/include/included/

> + conditions) the 

Re: [PATCH v3 22/35] upload-pack: support shallow requests

[Brandon Williams]

On 02/07, Stefan Beller wrote:
> On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  wrote:
> > Add the 'shallow' feature to the protocol version 2 command 'fetch'
> > which indicates that the server supports shallow clients and deepen
> > requets.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  Documentation/technical/protocol-v2.txt |  67 +++-
> >  serve.c |   2 +-
> >  t/t5701-git-serve.sh|   2 +-
> >  upload-pack.c   | 138 
> > +++-
> >  upload-pack.h   |   3 +
> >  5 files changed, 173 insertions(+), 39 deletions(-)
> >
> > diff --git a/Documentation/technical/protocol-v2.txt 
> > b/Documentation/technical/protocol-v2.txt
> > index 4d5096dae..fedeb6b77 100644
> > --- a/Documentation/technical/protocol-v2.txt
> > +++ b/Documentation/technical/protocol-v2.txt
> > @@ -201,12 +201,42 @@ packet-lines:
> > to its base by position in pack rather than by an oid.  That is,
> > they can read OBJ_OFS_DELTA (ake type 6) in a packfile.
> >
> > +shallow 
> > +   A client must notify the server of all objects for which it only
> 
> s/all objects/all commits/ for preciseness
> 
> > +   has shallow copies of (meaning that it doesn't have the parents
> > +   of a commit) by supplying a 'shallow ' line for each such
> > +   object so that the serve is aware of the limitations of the
> > +   client's history.
> > +
> > +deepen 
> > +   Request that the fetch/clone should be shallow having a commit 
> > depth of
> > +relative to the remote side.
> 
> What does depth mean? number of commits, or number of edges?
> Are there any special numbers (-1, 0, 1, max int) ?
> 
> From reading ahead: "Cannot be used with deepen-since, but
> can be combined with deepen-relative" ?

It just uses the current logic, which has no documentation on any of
that so...I'm not really sure?

> 
> 
> > +
> > +deepen-relative
> > +   Requests that the semantics of the "deepen" command be changed
> > +   to indicate that the depth requested is relative to the clients
> > +   current shallow boundary, instead of relative to the remote
> > +   refs.
> > +
> > +deepen-since 
> > +   Requests that the shallow clone/fetch should be cut at a
> > +   specific time, instead of depth.  Internally it's equivalent of
> > +   doing "rev-list --max-age=". Cannot be used with
> > +   "deepen".
> > +
> > +deepen-not 
> > +   Requests that the shallow clone/fetch should be cut at a
> > +   specific revision specified by '', instead of a depth.
> > +   Internally it's equivalent of doing "rev-list --not ".
> > +   Cannot be used with "deepen", but can be used with
> > +   "deepen-since".
> 
> What happens if those are given in combination?

Should act as an AND, it uses the old logic and there isn't very much
documentation on that...

-- 
Brandon Williams

Re: [PATCH v3 22/35] upload-pack: support shallow requests

[Duy Nguyen]

On Thu, Feb 8, 2018 at 2:00 AM, Stefan Beller  wrote:
>> +
>> +deepen-relative
>> +   Requests that the semantics of the "deepen" command be changed
>> +   to indicate that the depth requested is relative to the clients
>> +   current shallow boundary, instead of relative to the remote
>> +   refs.
>> +
>> +deepen-since 
>> +   Requests that the shallow clone/fetch should be cut at a
>> +   specific time, instead of depth.  Internally it's equivalent of
>> +   doing "rev-list --max-age=". Cannot be used with
>> +   "deepen".
>> +
>> +deepen-not 
>> +   Requests that the shallow clone/fetch should be cut at a
>> +   specific revision specified by '', instead of a depth.
>> +   Internally it's equivalent of doing "rev-list --not ".
>> +   Cannot be used with "deepen", but can be used with
>> +   "deepen-since".
>
> What happens if those are given in combination?

It should be described in the old protocol document or I did a bad job
documenting it. Some of these can be combined (I think it's AND logic
from rev-list point of view), with the exception of --depth which does
not use rev-list underneath and cannot be combined with the others.
-- 
Duy

Re: [PATCH v3 22/35] upload-pack: support shallow requests

[Stefan Beller]

On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  wrote:
> Add the 'shallow' feature to the protocol version 2 command 'fetch'
> which indicates that the server supports shallow clients and deepen
> requets.
>
> Signed-off-by: Brandon Williams 
> ---
>  Documentation/technical/protocol-v2.txt |  67 +++-
>  serve.c |   2 +-
>  t/t5701-git-serve.sh|   2 +-
>  upload-pack.c   | 138 
> +++-
>  upload-pack.h   |   3 +
>  5 files changed, 173 insertions(+), 39 deletions(-)
>
> diff --git a/Documentation/technical/protocol-v2.txt 
> b/Documentation/technical/protocol-v2.txt
> index 4d5096dae..fedeb6b77 100644
> --- a/Documentation/technical/protocol-v2.txt
> +++ b/Documentation/technical/protocol-v2.txt
> @@ -201,12 +201,42 @@ packet-lines:
> to its base by position in pack rather than by an oid.  That is,
> they can read OBJ_OFS_DELTA (ake type 6) in a packfile.
>
> +shallow 
> +   A client must notify the server of all objects for which it only

s/all objects/all commits/ for preciseness

> +   has shallow copies of (meaning that it doesn't have the parents
> +   of a commit) by supplying a 'shallow ' line for each such
> +   object so that the serve is aware of the limitations of the
> +   client's history.
> +
> +deepen 
> +   Request that the fetch/clone should be shallow having a commit depth 
> of
> +relative to the remote side.

What does depth mean? number of commits, or number of edges?
Are there any special numbers (-1, 0, 1, max int) ?

>From reading ahead: "Cannot be used with deepen-since, but
can be combined with deepen-relative" ?


> +
> +deepen-relative
> +   Requests that the semantics of the "deepen" command be changed
> +   to indicate that the depth requested is relative to the clients
> +   current shallow boundary, instead of relative to the remote
> +   refs.
> +
> +deepen-since 
> +   Requests that the shallow clone/fetch should be cut at a
> +   specific time, instead of depth.  Internally it's equivalent of
> +   doing "rev-list --max-age=". Cannot be used with
> +   "deepen".
> +
> +deepen-not 
> +   Requests that the shallow clone/fetch should be cut at a
> +   specific revision specified by '', instead of a depth.
> +   Internally it's equivalent of doing "rev-list --not ".
> +   Cannot be used with "deepen", but can be used with
> +   "deepen-since".

What happens if those are given in combination?