Re: [PATCH v2 20/25] upload-pack: support define shallow boundary by excluding revisions
On Fri, Feb 5, 2016 at 12:05 PM, Eric Sunshine wrote:
> On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy
> wrote:
>> @@ -732,7 +743,7 @@ static void receive_needs(void)
>> if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
>> return;
>> if (depth > 0 && deepen_rev_list)
>> - die("--depth and --shallow-since cannot be used together");
>> + die("--depth and --shallow-since (or --shallow-exclude)
>> cannot be used together");
>
> Also, does this change belong in patch 21/25?
21/25 is about client side support, so no, it should belong here. But
I probably should not mention client's option names here. After all
upload-pack can be used with some other client that names things
differently. I think I'm going with something like "git upload-pack:
deepen and deepen-since (or deepen-not) cannot be used together".
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 20/25] upload-pack: support define shallow boundary by excluding revisions
Eric Sunshine writes:
>> @@ -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?
Yup.
We enumerate the objects that are reachable are by traversing from
"want", but "--max-age" and "--not these refs" are optionally used
to truncate the enumeration, and usually we write "options" before
"parameters", so from that point of view, if this new code comes
before adding want_obj.objects[] (i.e. positive endpoints), that
would even be more readable. And it would force this new code to
have a trailing "--not" ;-)
>> + }
>> deepen_by_rev_list(av.argc, av.argv, &shallows);
>> argv_array_clear(&av);
>> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 20/25] upload-pack: support define shallow boundary by excluding revisions
On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy wrote:
> @@ -732,7 +743,7 @@ static void receive_needs(void)
> if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
> return;
> if (depth > 0 && deepen_rev_list)
> - die("--depth and --shallow-since cannot be used together");
> + die("--depth and --shallow-since (or --shallow-exclude)
> cannot be used together");
Also, does this change belong in patch 21/25?
> if (depth > 0)
> deepen(depth, &shallows);
> else if (deepen_rev_list) {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 20/25] upload-pack: support define shallow boundary by excluding revisions
On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy wrote:
> This should allow the user to say "create a shallow clone of this branch
> after version ".
>
> 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.
> 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
> ---
> 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 " 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.
> 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);
> + 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);
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
