On 02/27, Jonathan Nieder wrote:
I'll make the documentation changes you suggested.
> > + deepen <depth>
> > + Request that the fetch/clone should be shallow having a commit depth of
>
> nit: s/Request/Requests/, for consistency with the others?
>
> > + <depth> relative to the remote side.
>
> What does the value of <depth> 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 <depth> always a positive number?
>
> What happens if <depth> 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(&av, "rev-list");
> > + if (deepen_since)
> > + argv_array_pushf(&av, "--max-age=%"PRItime, deepen_since);
> > + if (deepen_not->nr) {
> > + argv_array_push(&av, "--not");
> > + for (i = 0; i < deepen_not->nr; i++) {
> > + struct string_list_item *s = deepen_not->items + i;
> > + argv_array_push(&av, 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(&av, "--not");
> > - for (i = 0; i < deepen_not.nr; i++) {
> > - struct string_list_item *s = deepen_not.items + i;
> > - argv_array_push(&av, s->string);
>
> Huh. Looks like some of the above comments are better addressed to an
> earlier patch.
If someone wants to fix this after the fact they can, I just moved this
logic, I didn't add it.
>
> [...]
> > @@ -1071,6 +1085,13 @@ struct upload_pack_data {
> > struct object_array wants;
> > struct oid_array haves;
> >
> > + struct object_array shallows;
> > + struct string_list deepen_not;
> > + int depth;
> > + timestamp_t deepen_since;
> > + int deepen_rev_list;
> > + int deepen_relative;
>
> Nice.
>
> Comments describing deepen_Rev_list and deepen_relative would be nice.
>
> Are those boolean? Can they be unsigned:1 to make that
> self-explanatory?
They are boolean but are passed via reference at some points so they
can't be bit flags.
>
> [...]
> > @@ -1080,12 +1101,14 @@ struct upload_pack_data {
> > unsigned done : 1;
> > };
> >
> > -#define UPLOAD_PACK_DATA_INIT { OBJECT_ARRAY_INIT, OID_ARRAY_INIT, 0, 0, 0, 0, 0, 0 }
> > +#define UPLOAD_PACK_DATA_INIT { OBJECT_ARRAY_INIT, OID_ARRAY_INIT, OBJECT_ARRAY_INIT, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }
>
> Long line, "make style" should be able to fix it.
>
I'll fix this.
--
Brandon Williams
