Re: [PATCH v2 20/25] upload-pack: support define shallow boundary by excluding revisions

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 5, 2016, 05:03 UTC
Message-ID: <CAPig+cQ2thjGWxar3BSJBU_O1WBsD0FHjv8i_sSMQKh+wO7bjQ@mail.gmail.com>
In-Reply-To: <1454576641-29615-21-git-send-email-pclouds@gmail.com>

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 5 quoted lines

> This should allow the user to say "create a shallow clone of this branch
> after version <some-tag>".
>
> deepen-not cannot be used with deepen the same way deepen-since cannot
> be used with deepen.

As written, this is a bit of a brain twister and required several re-reads to digest. Perhaps it might be clearer if rephrased like this:

    Like deepen-since, deepen-not cannot be used with deepen.

or:

    Like deepen-since, deepen-not is incompatible with deepen.

Show 16 quoted lines

> But deepen-not can be mixed with deepen-since. The
> result is exactly how you do the command "git rev-list --since=... --not
> ref".
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
> @@ -188,6 +188,15 @@ specific time, instead of depth. Internally it's equivalent of doing
> +deepen-not
> +----------
> +
> +This capability adds "deepen-not" command to fetch-pack/upload-pack
> +protocol so the client can request shallow clones that are cut at a
> +specific revision, instead of depth. Internally it's equivalent of
> +doing "rev-list --not <rev>" on the server side. "deepen-not"
> +cannot be used with "deepen", but can be used with "deepen-since".

This description, on the other hand, is easy to grasp.

Show 7 quoted lines

> diff --git a/upload-pack.c b/upload-pack.c
> @@ -678,6 +679,16 @@ static void receive_needs(void)
> +               if (skip_prefix(line, "deepen-not ", &arg)) {
> +                       char *ref = NULL;
> +                       unsigned char sha1[20];
> +                       if (expand_ref(arg, strlen(arg), sha1, &ref) != 1)
> +                               die("Ambiguous deepen-not: %s", line);

With just a few exceptions, die() invocations in upload-pack.c prefix the message with "git upload-pack:" and start the actual diagnostic with lowercase, so perhaps:

    die("git upload-pack: ambiguous deepen-not: %s", line);

Show 15 quoted lines

> +                       string_list_append(&deepen_not, ref);
> +                       free(ref);
> +                       deepen_rev_list = 1;
> +                       continue;
> +               }
> @@ -746,6 +757,13 @@ static void receive_needs(void)
>                         struct object *o = want_obj.objects[i].item;
>                         argv_array_push(&av, oid_to_hex(&o->oid));
>                 }
> +               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);
> +                       }

The documentation for rev-list --not says it "Reverses the meaning ... up to the next --not", so would it make sense to add a final:

    argv_array_push(&av, "--not");

here to future-proof against some change pushing more arguments onto 'av' following this code?

> +               }
>                 deepen_by_rev_list(av.argc, av.argv, &shallows);
>                 argv_array_clear(&av);
>         }