[PATCH v2 00/25] More flexibility in making shallow clones

[PATCH v2 00/25] More flexibility in making shallow clones

[Nguyễn Thái Ngọc Duy]

This series brings three new options to shallow clone/fetch, to let
you specify cut point by time, or by excluding some refs, or to let
you extend shallow boundary by <N> commits.

The series is now complete. Changes since v1 [1]

 - smart http support
 - option names --not and --since are changed to --shallow-exclude
   and --shallow-since
 - fix the last patch per Eric's comments (the tests were totally
   broken but I didn't realize)

The meat starts since 14/25. Before that is just cleanups and stuff.
Happy (shallowly) cloning!

[1] http://thread.gmane.org/gmane.comp.version-control.git/283110

Nguyễn Thái Ngọc Duy (25):
  remote-curl.c: convert fetch_git() to use argv_array
  transport-helper.c: refactor set_helper_option()
  transport-helper.c: do not send null option to remote helper
  upload-pack: move shallow deepen code out of receive_needs()
  upload-pack: move "shallow" sending code out of deepen()
  upload-pack: remove unused variable "backup"
  upload-pack: move "unshallow" sending code out of deepen()
  upload-pack: use skip_prefix() instead of starts_with() when possible
  upload-pack: tighten number parsing at "deepen" lines
  upload-pack: move rev-list code out of check_non_tip()
  fetch-pack: use skip_prefix() instead of starts_with() when possible
  fetch-pack: use a common function for verbose printing
  fetch-pack: use a separate flag for fetch in deepening mode
  shallow.c: implement a generic shallow boundary finder based on rev-list
  upload-pack: add deepen-since to cut shallow repos based on time
  fetch: define shallow boundary with --shallow-since
  clone: define shallow clone boundary based on time with --shallow-since
  t5500, t5539: tests for shallow depth since a specific date
  refs: add expand_ref()
  upload-pack: support define shallow boundary by excluding revisions
  fetch: define shallow boundary with --shallow-exclude
  clone: define shallow clone boundary with --shallow-exclude
  t5500, t5539: tests for shallow depth excluding a ref
  upload-pack: make check_reachable_object() return unreachable list if asked
  fetch, upload-pack: --deepen=N extends shallow boundary by N commits

 Documentation/fetch-options.txt                   |  14 +
 Documentation/git-clone.txt                       |   8 +
 Documentation/git-fetch-pack.txt                  |  14 +
 Documentation/gitremote-helpers.txt               |  11 +
 Documentation/technical/pack-protocol.txt         |   4 +-
 Documentation/technical/protocol-capabilities.txt |  25 ++
 builtin/clone.c                                   |  32 ++-
 builtin/fetch-pack.c                              |  31 ++-
 builtin/fetch.c                                   |  41 ++-
 commit.h                                          |   2 +
 fetch-pack.c                                      | 128 +++++----
 fetch-pack.h                                      |   4 +
 object.h                                          |   2 +-
 refs.c                                            |   8 +-
 refs.h                                            |   1 +
 remote-curl.c                                     |  75 ++++--
 shallow.c                                         |  85 ++++++
 t/t5500-fetch-pack.sh                             |  67 +++++
 t/t5539-fetch-http-shallow.sh                     |  72 ++++++
 transport-helper.c                                |  71 +++--
 transport.c                                       |  11 +
 transport.h                                       |  14 +
 upload-pack.c                                     | 301 ++++++++++++++++------
 23 files changed, 832 insertions(+), 189 deletions(-)

-- 
2.7.0.377.g4cd97dd

[PATCH v2 01/25] remote-curl.c: convert fetch_git() to use argv_array

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 remote-curl.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index c704857..3ac5b6c 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -726,37 +726,31 @@ static int fetch_git(struct discovery *heads,
 	struct rpc_state rpc;
 	struct strbuf preamble = STRBUF_INIT;
 	char *depth_arg = NULL;
-	int argc = 0, i, err;
-	const char *argv[17];
+	int i, err;
+	struct argv_array args = ARGV_ARRAY_INIT;
 
-	argv[argc++] = "fetch-pack";
-	argv[argc++] = "--stateless-rpc";
-	argv[argc++] = "--stdin";
-	argv[argc++] = "--lock-pack";
+	argv_array_pushl(&args, "fetch-pack", "--stateless-rpc",
+			 "--stdin", "--lock-pack",
+			 NULL);
 	if (options.followtags)
-		argv[argc++] = "--include-tag";
+		argv_array_push(&args, "--include-tag");
 	if (options.thin)
-		argv[argc++] = "--thin";
+		argv_array_push(&args, "--thin");
 	if (options.verbosity >= 3) {
-		argv[argc++] = "-v";
-		argv[argc++] = "-v";
+		argv_array_push(&args, "-v");
+		argv_array_push(&args, "-v");
 	}
 	if (options.check_self_contained_and_connected)
-		argv[argc++] = "--check-self-contained-and-connected";
+		argv_array_push(&args, "--check-self-contained-and-connected");
 	if (options.cloning)
-		argv[argc++] = "--cloning";
+		argv_array_push(&args, "--cloning");
 	if (options.update_shallow)
-		argv[argc++] = "--update-shallow";
+		argv_array_push(&args, "--update-shallow");
 	if (!options.progress)
-		argv[argc++] = "--no-progress";
-	if (options.depth) {
-		struct strbuf buf = STRBUF_INIT;
-		strbuf_addf(&buf, "--depth=%lu", options.depth);
-		depth_arg = strbuf_detach(&buf, NULL);
-		argv[argc++] = depth_arg;
-	}
-	argv[argc++] = url.buf;
-	argv[argc++] = NULL;
+		argv_array_push(&args, "--no-progress");
+	if (options.depth)
+		argv_array_pushf(&args, "--depth=%lu", options.depth);
+	argv_array_push(&args, url.buf);
 
 	for (i = 0; i < nr_heads; i++) {
 		struct ref *ref = to_fetch[i];
@@ -769,7 +763,7 @@ static int fetch_git(struct discovery *heads,
 
 	memset(&rpc, 0, sizeof(rpc));
 	rpc.service_name = "git-upload-pack",
-	rpc.argv = argv;
+	rpc.argv = args.argv;
 	rpc.stdin_preamble = &preamble;
 	rpc.gzip_request = 1;
 
@@ -779,6 +773,7 @@ static int fetch_git(struct discovery *heads,
 	strbuf_release(&rpc.result);
 	strbuf_release(&preamble);
 	free(depth_arg);
+	argv_array_clear(&args);
 	return err;
 }
 
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 01/25] remote-curl.c: convert fetch_git() to use argv_array

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Yay, it's about time ;-)

> diff --git a/remote-curl.c b/remote-curl.c
> index c704857..3ac5b6c 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -726,37 +726,31 @@ static int fetch_git(struct discovery *heads,
>  	struct rpc_state rpc;
>  	struct strbuf preamble = STRBUF_INIT;
>  	char *depth_arg = NULL;
> -	int argc = 0, i, err;
> -	const char *argv[17];
> +	int i, err;
> +	struct argv_array args = ARGV_ARRAY_INIT;
>  
> -	argv[argc++] = "fetch-pack";
> -	argv[argc++] = "--stateless-rpc";
> -	argv[argc++] = "--stdin";
> -	argv[argc++] = "--lock-pack";
> +	argv_array_pushl(&args, "fetch-pack", "--stateless-rpc",
> +			 "--stdin", "--lock-pack",
> +			 NULL);

A lone NULL after a reasonably short line looks somewhat irritating,
but if you are planning to add more on that short line in the later
patch, it is OK.  Let's keep reading.

The remainder of this patch looked obviously correct.

Thanks.

[PATCH v2 02/25] transport-helper.c: refactor set_helper_option()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 transport-helper.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index a6bff8b..35023da 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -260,6 +260,28 @@ static const char *boolean_options[] = {
 	TRANS_OPT_FOLLOWTAGS,
 	};
 
+static int strbuf_set_helper_option(struct helper_data *data,
+				    struct strbuf *buf)
+{
+	int ret;
+
+	sendline(data, buf);
+	if (recvline(data, buf))
+		exit(128);
+
+	if (!strcmp(buf->buf, "ok"))
+		ret = 0;
+	else if (starts_with(buf->buf, "error")) {
+		ret = -1;
+	} else if (!strcmp(buf->buf, "unsupported"))
+		ret = 1;
+	else {
+		warning("%s unexpectedly said: '%s'", data->name, buf->buf);
+		ret = 1;
+	}
+	return ret;
+}
+
 static int set_helper_option(struct transport *transport,
 			  const char *name, const char *value)
 {
@@ -291,20 +313,7 @@ static int set_helper_option(struct transport *transport,
 		quote_c_style(value, &buf, NULL, 0);
 	strbuf_addch(&buf, '\n');
 
-	sendline(data, &buf);
-	if (recvline(data, &buf))
-		exit(128);
-
-	if (!strcmp(buf.buf, "ok"))
-		ret = 0;
-	else if (starts_with(buf.buf, "error")) {
-		ret = -1;
-	} else if (!strcmp(buf.buf, "unsupported"))
-		ret = 1;
-	else {
-		warning("%s unexpectedly said: '%s'", data->name, buf.buf);
-		ret = 1;
-	}
+	ret = strbuf_set_helper_option(data, &buf);
 	strbuf_release(&buf);
 	return ret;
 }
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 02/25] transport-helper.c: refactor set_helper_option()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Just like other patches, this is thin on the rationale.  You are
planning to invent another function that prepares the requrest in a
strbuf and allow it to call this helper to perform a single round
trip with the other side?  If that is the case, the split is very
sensible, but you need to say something about "why".

>  transport-helper.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index a6bff8b..35023da 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -260,6 +260,28 @@ static const char *boolean_options[] = {
>  	TRANS_OPT_FOLLOWTAGS,
>  	};
>  
> +static int strbuf_set_helper_option(struct helper_data *data,
> +				    struct strbuf *buf)
> +{
> +	int ret;
> +
> +	sendline(data, buf);
> +	if (recvline(data, buf))
> +		exit(128);
> +
> +	if (!strcmp(buf->buf, "ok"))
> +		ret = 0;
> +	else if (starts_with(buf->buf, "error")) {
> +		ret = -1;
> +	} else if (!strcmp(buf->buf, "unsupported"))
> +		ret = 1;
> +	else {
> +		warning("%s unexpectedly said: '%s'", data->name, buf->buf);
> +		ret = 1;
> +	}
> +	return ret;
> +}
> +
>  static int set_helper_option(struct transport *transport,
>  			  const char *name, const char *value)
>  {
> @@ -291,20 +313,7 @@ static int set_helper_option(struct transport *transport,
>  		quote_c_style(value, &buf, NULL, 0);
>  	strbuf_addch(&buf, '\n');
>  
> -	sendline(data, &buf);
> -	if (recvline(data, &buf))
> -		exit(128);
> -
> -	if (!strcmp(buf.buf, "ok"))
> -		ret = 0;
> -	else if (starts_with(buf.buf, "error")) {
> -		ret = -1;
> -	} else if (!strcmp(buf.buf, "unsupported"))
> -		ret = 1;
> -	else {
> -		warning("%s unexpectedly said: '%s'", data->name, buf.buf);
> -		ret = 1;
> -	}
> +	ret = strbuf_set_helper_option(data, &buf);
>  	strbuf_release(&buf);
>  	return ret;
>  }

[PATCH v2 03/25] transport-helper.c: do not send null option to remote helper

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 transport-helper.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/transport-helper.c b/transport-helper.c
index 35023da..2e78c4d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -309,8 +309,12 @@ static int set_helper_option(struct transport *transport,
 	strbuf_addf(&buf, "option %s ", name);
 	if (is_bool)
 		strbuf_addstr(&buf, value ? "true" : "false");
-	else
+	else if (value)
 		quote_c_style(value, &buf, NULL, 0);
+	else {
+		strbuf_release(&buf);
+		return 0;
+	}
 	strbuf_addch(&buf, '\n');
 
 	ret = strbuf_set_helper_option(data, &buf);
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 03/25] transport-helper.c: do not send null option to remote helper

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

This is even more strange.  Are the current callers broken and some
sends value==NULL for an option that is not is_bool, resulting in
a call to quote_c_style() with NULL?  I somehow find it hard to
believe as that would lead to an immediate segfault.

Assuming that no current caller passes NULL to value when is_bool is
not in effect, there needs an explanation why future new callers may
need to do so.  An alternative for a valueless option could be to
send "option name\n" instead of the usual "option name value\n", but
without such an explanation, readers cannot tell why not sending
anything about "name", which is what this patch chooses to implement,
is a better idea.

>  transport-helper.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 35023da..2e78c4d 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -309,8 +309,12 @@ static int set_helper_option(struct transport *transport,
>  	strbuf_addf(&buf, "option %s ", name);
>  	if (is_bool)
>  		strbuf_addstr(&buf, value ? "true" : "false");
> -	else
> +	else if (value)
>  		quote_c_style(value, &buf, NULL, 0);
> +	else {
> +		strbuf_release(&buf);
> +		return 0;
> +	}
>  	strbuf_addch(&buf, '\n');
>  
>  	ret = strbuf_set_helper_option(data, &buf);

Re: [PATCH v2 03/25] transport-helper.c: do not send null option to remote helper

[Duy Nguyen]

On Fri, Feb 5, 2016 at 6:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>
> This is even more strange.  Are the current callers broken and some
> sends value==NULL for an option that is not is_bool, resulting in
> a call to quote_c_style() with NULL?  I somehow find it hard to
> believe as that would lead to an immediate segfault.
>
> Assuming that no current caller passes NULL to value when is_bool is
> not in effect, there needs an explanation why future new callers may
> need to do so.  An alternative for a valueless option could be to
> send "option name\n" instead of the usual "option name value\n", but
> without such an explanation, readers cannot tell why not sending
> anything about "name", which is what this patch chooses to implement,
> is a better idea.

The source is backfill_tags() which, in future, resets some transport
options back to defaults. The current set_option() in there only deals
with booleans or number (depth). But in future it resets deepen-since,
which is a string.

I think the main reason is, we do not have a way to reset (or unset) a
transport option. Should I keep this commit and explain about this, or
have a new transport API to reset option?
-- 
Duy

Re: [PATCH v2 03/25] transport-helper.c: do not send null option to remote helper

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, Feb 5, 2016 at 6:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>
>> This is even more strange.  Are the current callers broken and some
>> sends value==NULL for an option that is not is_bool, resulting in
>> a call to quote_c_style() with NULL?  I somehow find it hard to
>> believe as that would lead to an immediate segfault.
>>
>> Assuming that no current caller passes NULL to value when is_bool is
>> not in effect, there needs an explanation why future new callers may
>> need to do so.  An alternative for a valueless option could be to
>> send "option name\n" instead of the usual "option name value\n", but
>> without such an explanation, readers cannot tell why not sending
>> anything about "name", which is what this patch chooses to implement,
>> is a better idea.
>
> The source is backfill_tags() which, in future, resets some transport
> options back to defaults. The current set_option() in there only deals
> with booleans or number (depth). But in future it resets deepen-since,
> which is a string.
>
> I think the main reason is, we do not have a way to reset (or unset) a
> transport option. Should I keep this commit and explain about this, or
> have a new transport API to reset option?

Addition of new API is OK, but if this commit remains in the
history, it itself has to explain why it is necessary.

[PATCH v2 04/25] upload-pack: move shallow deepen code out of receive_needs()

[Nguyễn Thái Ngọc Duy]

This is a prep step for further refactoring. Besides reindentation and
s/shallows\./shallows->/g, no other changes are expected.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 upload-pack.c | 99 +++++++++++++++++++++++++++++++----------------------------
 1 file changed, 52 insertions(+), 47 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index b3f6653..97ed620 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -538,6 +538,55 @@ error:
 	}
 }
 
+static void deepen(int depth, const struct object_array *shallows)
+{
+	struct commit_list *result = NULL, *backup = NULL;
+	int i;
+	if (depth == INFINITE_DEPTH && !is_repository_shallow())
+		for (i = 0; i < shallows->nr; i++) {
+			struct object *object = shallows->objects[i].item;
+			object->flags |= NOT_SHALLOW;
+		}
+	else
+		backup = result =
+			get_shallow_commits(&want_obj, depth,
+					    SHALLOW, NOT_SHALLOW);
+	while (result) {
+		struct object *object = &result->item->object;
+		if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
+			packet_write(1, "shallow %s",
+				     oid_to_hex(&object->oid));
+			register_shallow(object->oid.hash);
+			shallow_nr++;
+		}
+		result = result->next;
+	}
+	free_commit_list(backup);
+	for (i = 0; i < shallows->nr; i++) {
+		struct object *object = shallows->objects[i].item;
+		if (object->flags & NOT_SHALLOW) {
+			struct commit_list *parents;
+			packet_write(1, "unshallow %s",
+				     oid_to_hex(&object->oid));
+			object->flags &= ~CLIENT_SHALLOW;
+			/* make sure the real parents are parsed */
+			unregister_shallow(object->oid.hash);
+			object->parsed = 0;
+			parse_commit_or_die((struct commit *)object);
+			parents = ((struct commit *)object)->parents;
+			while (parents) {
+				add_object_array(&parents->item->object,
+						 NULL, &want_obj);
+				parents = parents->next;
+			}
+			add_object_array(object, NULL, &extra_edge_obj);
+		}
+		/* make sure commit traversal conforms to client */
+		register_shallow(object->oid.hash);
+	}
+	packet_flush(1);
+}
+
 static void receive_needs(void)
 {
 	struct object_array shallows = OBJECT_ARRAY_INIT;
@@ -630,53 +679,9 @@ static void receive_needs(void)
 
 	if (depth == 0 && shallows.nr == 0)
 		return;
-	if (depth > 0) {
-		struct commit_list *result = NULL, *backup = NULL;
-		int i;
-		if (depth == INFINITE_DEPTH && !is_repository_shallow())
-			for (i = 0; i < shallows.nr; i++) {
-				struct object *object = shallows.objects[i].item;
-				object->flags |= NOT_SHALLOW;
-			}
-		else
-			backup = result =
-				get_shallow_commits(&want_obj, depth,
-						    SHALLOW, NOT_SHALLOW);
-		while (result) {
-			struct object *object = &result->item->object;
-			if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
-				packet_write(1, "shallow %s",
-						oid_to_hex(&object->oid));
-				register_shallow(object->oid.hash);
-				shallow_nr++;
-			}
-			result = result->next;
-		}
-		free_commit_list(backup);
-		for (i = 0; i < shallows.nr; i++) {
-			struct object *object = shallows.objects[i].item;
-			if (object->flags & NOT_SHALLOW) {
-				struct commit_list *parents;
-				packet_write(1, "unshallow %s",
-					oid_to_hex(&object->oid));
-				object->flags &= ~CLIENT_SHALLOW;
-				/* make sure the real parents are parsed */
-				unregister_shallow(object->oid.hash);
-				object->parsed = 0;
-				parse_commit_or_die((struct commit *)object);
-				parents = ((struct commit *)object)->parents;
-				while (parents) {
-					add_object_array(&parents->item->object,
-							NULL, &want_obj);
-					parents = parents->next;
-				}
-				add_object_array(object, NULL, &extra_edge_obj);
-			}
-			/* make sure commit traversal conforms to client */
-			register_shallow(object->oid.hash);
-		}
-		packet_flush(1);
-	} else
+	if (depth > 0)
+		deepen(depth, &shallows);
+	else
 		if (shallows.nr > 0) {
 			int i;
 			for (i = 0; i < shallows.nr; i++)
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 04/25] upload-pack: move shallow deepen code out of receive_needs()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This is a prep step for further refactoring. Besides reindentation and
> s/shallows\./shallows->/g, no other changes are expected.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Good.

>  upload-pack.c | 99 +++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 52 insertions(+), 47 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index b3f6653..97ed620 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -538,6 +538,55 @@ error:
>  	}
>  }
>  
> +static void deepen(int depth, const struct object_array *shallows)
> +{
> +	struct commit_list *result = NULL, *backup = NULL;
> +	int i;
> +	if (depth == INFINITE_DEPTH && !is_repository_shallow())
> +		for (i = 0; i < shallows->nr; i++) {
> +			struct object *object = shallows->objects[i].item;
> +			object->flags |= NOT_SHALLOW;
> +		}
> +	else
> +		backup = result =
> +			get_shallow_commits(&want_obj, depth,
> +					    SHALLOW, NOT_SHALLOW);
> +	while (result) {
> +		struct object *object = &result->item->object;
> +		if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
> +			packet_write(1, "shallow %s",
> +				     oid_to_hex(&object->oid));
> +			register_shallow(object->oid.hash);
> +			shallow_nr++;
> +		}
> +		result = result->next;
> +	}
> +	free_commit_list(backup);
> +	for (i = 0; i < shallows->nr; i++) {
> +		struct object *object = shallows->objects[i].item;
> +		if (object->flags & NOT_SHALLOW) {
> +			struct commit_list *parents;
> +			packet_write(1, "unshallow %s",
> +				     oid_to_hex(&object->oid));
> +			object->flags &= ~CLIENT_SHALLOW;
> +			/* make sure the real parents are parsed */
> +			unregister_shallow(object->oid.hash);
> +			object->parsed = 0;
> +			parse_commit_or_die((struct commit *)object);
> +			parents = ((struct commit *)object)->parents;
> +			while (parents) {
> +				add_object_array(&parents->item->object,
> +						 NULL, &want_obj);
> +				parents = parents->next;
> +			}
> +			add_object_array(object, NULL, &extra_edge_obj);
> +		}
> +		/* make sure commit traversal conforms to client */
> +		register_shallow(object->oid.hash);
> +	}
> +	packet_flush(1);
> +}
> +
>  static void receive_needs(void)
>  {
>  	struct object_array shallows = OBJECT_ARRAY_INIT;
> @@ -630,53 +679,9 @@ static void receive_needs(void)
>  
>  	if (depth == 0 && shallows.nr == 0)
>  		return;
> -	if (depth > 0) {
> -		struct commit_list *result = NULL, *backup = NULL;
> -		int i;
> -		if (depth == INFINITE_DEPTH && !is_repository_shallow())
> -			for (i = 0; i < shallows.nr; i++) {
> -				struct object *object = shallows.objects[i].item;
> -				object->flags |= NOT_SHALLOW;
> -			}
> -		else
> -			backup = result =
> -				get_shallow_commits(&want_obj, depth,
> -						    SHALLOW, NOT_SHALLOW);
> -		while (result) {
> -			struct object *object = &result->item->object;
> -			if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
> -				packet_write(1, "shallow %s",
> -						oid_to_hex(&object->oid));
> -				register_shallow(object->oid.hash);
> -				shallow_nr++;
> -			}
> -			result = result->next;
> -		}
> -		free_commit_list(backup);
> -		for (i = 0; i < shallows.nr; i++) {
> -			struct object *object = shallows.objects[i].item;
> -			if (object->flags & NOT_SHALLOW) {
> -				struct commit_list *parents;
> -				packet_write(1, "unshallow %s",
> -					oid_to_hex(&object->oid));
> -				object->flags &= ~CLIENT_SHALLOW;
> -				/* make sure the real parents are parsed */
> -				unregister_shallow(object->oid.hash);
> -				object->parsed = 0;
> -				parse_commit_or_die((struct commit *)object);
> -				parents = ((struct commit *)object)->parents;
> -				while (parents) {
> -					add_object_array(&parents->item->object,
> -							NULL, &want_obj);
> -					parents = parents->next;
> -				}
> -				add_object_array(object, NULL, &extra_edge_obj);
> -			}
> -			/* make sure commit traversal conforms to client */
> -			register_shallow(object->oid.hash);
> -		}
> -		packet_flush(1);
> -	} else
> +	if (depth > 0)
> +		deepen(depth, &shallows);
> +	else
>  		if (shallows.nr > 0) {
>  			int i;
>  			for (i = 0; i < shallows.nr; i++)

[PATCH v2 05/25] upload-pack: move "shallow" sending code out of deepen()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 upload-pack.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 97ed620..0eb9a0b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -538,6 +538,20 @@ error:
 	}
 }
 
+static void send_shallow(struct commit_list *result)
+{
+	while (result) {
+		struct object *object = &result->item->object;
+		if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
+			packet_write(1, "shallow %s",
+				     oid_to_hex(&object->oid));
+			register_shallow(object->oid.hash);
+			shallow_nr++;
+		}
+		result = result->next;
+	}
+}
+
 static void deepen(int depth, const struct object_array *shallows)
 {
 	struct commit_list *result = NULL, *backup = NULL;
@@ -551,16 +565,7 @@ static void deepen(int depth, const struct object_array *shallows)
 		backup = result =
 			get_shallow_commits(&want_obj, depth,
 					    SHALLOW, NOT_SHALLOW);
-	while (result) {
-		struct object *object = &result->item->object;
-		if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
-			packet_write(1, "shallow %s",
-				     oid_to_hex(&object->oid));
-			register_shallow(object->oid.hash);
-			shallow_nr++;
-		}
-		result = result->next;
-	}
+	send_shallow(result);
 	free_commit_list(backup);
 	for (i = 0; i < shallows->nr; i++) {
 		struct object *object = shallows->objects[i].item;
-- 
2.7.0.377.g4cd97dd

[PATCH v2 06/25] upload-pack: remove unused variable "backup"

[Nguyễn Thái Ngọc Duy]

After the last patch, "result" and "backup" are the same. "result" used
to move, but the movement is now contained in send_shallow(). Delete
this redundant variable.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 upload-pack.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 0eb9a0b..ee5d20b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -554,7 +554,7 @@ static void send_shallow(struct commit_list *result)
 
 static void deepen(int depth, const struct object_array *shallows)
 {
-	struct commit_list *result = NULL, *backup = NULL;
+	struct commit_list *result = NULL;
 	int i;
 	if (depth == INFINITE_DEPTH && !is_repository_shallow())
 		for (i = 0; i < shallows->nr; i++) {
@@ -562,11 +562,10 @@ static void deepen(int depth, const struct object_array *shallows)
 			object->flags |= NOT_SHALLOW;
 		}
 	else
-		backup = result =
-			get_shallow_commits(&want_obj, depth,
-					    SHALLOW, NOT_SHALLOW);
+		result = get_shallow_commits(&want_obj, depth,
+					     SHALLOW, NOT_SHALLOW);
 	send_shallow(result);
-	free_commit_list(backup);
+	free_commit_list(result);
 	for (i = 0; i < shallows->nr; i++) {
 		struct object *object = shallows->objects[i].item;
 		if (object->flags & NOT_SHALLOW) {
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 06/25] upload-pack: remove unused variable "backup"

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> After the last patch, "result" and "backup" are the same. "result" used
> to move, but the movement is now contained in send_shallow(). Delete
> this redundant variable.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

OK.  05 and 06 squashed together would also have been readable, but
either way is fine; it is not worth merging them into one.

>  upload-pack.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 0eb9a0b..ee5d20b 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -554,7 +554,7 @@ static void send_shallow(struct commit_list *result)
>  
>  static void deepen(int depth, const struct object_array *shallows)
>  {
> -	struct commit_list *result = NULL, *backup = NULL;
> +	struct commit_list *result = NULL;
>  	int i;
>  	if (depth == INFINITE_DEPTH && !is_repository_shallow())
>  		for (i = 0; i < shallows->nr; i++) {
> @@ -562,11 +562,10 @@ static void deepen(int depth, const struct object_array *shallows)
>  			object->flags |= NOT_SHALLOW;
>  		}
>  	else
> -		backup = result =
> -			get_shallow_commits(&want_obj, depth,
> -					    SHALLOW, NOT_SHALLOW);
> +		result = get_shallow_commits(&want_obj, depth,
> +					     SHALLOW, NOT_SHALLOW);
>  	send_shallow(result);
> -	free_commit_list(backup);
> +	free_commit_list(result);
>  	for (i = 0; i < shallows->nr; i++) {
>  		struct object *object = shallows->objects[i].item;
>  		if (object->flags & NOT_SHALLOW) {

[PATCH v2 07/25] upload-pack: move "unshallow" sending code out of deepen()

[Nguyễn Thái Ngọc Duy]

Also add some more comments in this code because it takes too long to
understand what it does (to me, who should be familiar enough to
understand this code well!)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 upload-pack.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index ee5d20b..bfb7985 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -552,20 +552,10 @@ static void send_shallow(struct commit_list *result)
 	}
 }
 
-static void deepen(int depth, const struct object_array *shallows)
+static void send_unshallow(const struct object_array *shallows)
 {
-	struct commit_list *result = NULL;
 	int i;
-	if (depth == INFINITE_DEPTH && !is_repository_shallow())
-		for (i = 0; i < shallows->nr; i++) {
-			struct object *object = shallows->objects[i].item;
-			object->flags |= NOT_SHALLOW;
-		}
-	else
-		result = get_shallow_commits(&want_obj, depth,
-					     SHALLOW, NOT_SHALLOW);
-	send_shallow(result);
-	free_commit_list(result);
+
 	for (i = 0; i < shallows->nr; i++) {
 		struct object *object = shallows->objects[i].item;
 		if (object->flags & NOT_SHALLOW) {
@@ -573,7 +563,13 @@ static void deepen(int depth, const struct object_array *shallows)
 			packet_write(1, "unshallow %s",
 				     oid_to_hex(&object->oid));
 			object->flags &= ~CLIENT_SHALLOW;
-			/* make sure the real parents are parsed */
+			/*
+			 * We want to _register_ "object" as shallow, but we
+			 * also need to traverse object's parents to deepen a
+			 * shallow clone. Unregister it for now so we can
+			 * parse and add the parents to the want list, then
+			 * re-register it.
+			 */
 			unregister_shallow(object->oid.hash);
 			object->parsed = 0;
 			parse_commit_or_die((struct commit *)object);
@@ -588,6 +584,23 @@ static void deepen(int depth, const struct object_array *shallows)
 		/* make sure commit traversal conforms to client */
 		register_shallow(object->oid.hash);
 	}
+}
+
+static void deepen(int depth, const struct object_array *shallows)
+{
+	struct commit_list *result = NULL;
+	int i;
+	if (depth == INFINITE_DEPTH && !is_repository_shallow())
+		for (i = 0; i < shallows->nr; i++) {
+			struct object *object = shallows->objects[i].item;
+			object->flags |= NOT_SHALLOW;
+		}
+	else
+		result = get_shallow_commits(&want_obj, depth,
+					     SHALLOW, NOT_SHALLOW);
+	send_shallow(result);
+	free_commit_list(result);
+	send_unshallow(shallows);
 	packet_flush(1);
 }
 
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 07/25] upload-pack: move "unshallow" sending code out of deepen()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Also add some more comments in this code because it takes too long to
> understand what it does (to me, who should be familiar enough to
> understand this code well!)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

The diff is harder to read than necessary (not your fault), but
splitting this into a separate helper makes sense to me.

Thanks.

>  upload-pack.c | 39 ++++++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index ee5d20b..bfb7985 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -552,20 +552,10 @@ static void send_shallow(struct commit_list *result)
>  	}
>  }
>  
> -static void deepen(int depth, const struct object_array *shallows)
> +static void send_unshallow(const struct object_array *shallows)
>  {
> -	struct commit_list *result = NULL;
>  	int i;
> -	if (depth == INFINITE_DEPTH && !is_repository_shallow())
> -		for (i = 0; i < shallows->nr; i++) {
> -			struct object *object = shallows->objects[i].item;
> -			object->flags |= NOT_SHALLOW;
> -		}
> -	else
> -		result = get_shallow_commits(&want_obj, depth,
> -					     SHALLOW, NOT_SHALLOW);
> -	send_shallow(result);
> -	free_commit_list(result);
> +
>  	for (i = 0; i < shallows->nr; i++) {
>  		struct object *object = shallows->objects[i].item;
>  		if (object->flags & NOT_SHALLOW) {
> @@ -573,7 +563,13 @@ static void deepen(int depth, const struct object_array *shallows)
>  			packet_write(1, "unshallow %s",
>  				     oid_to_hex(&object->oid));
>  			object->flags &= ~CLIENT_SHALLOW;
> -			/* make sure the real parents are parsed */
> +			/*
> +			 * We want to _register_ "object" as shallow, but we
> +			 * also need to traverse object's parents to deepen a
> +			 * shallow clone. Unregister it for now so we can
> +			 * parse and add the parents to the want list, then
> +			 * re-register it.
> +			 */
>  			unregister_shallow(object->oid.hash);
>  			object->parsed = 0;
>  			parse_commit_or_die((struct commit *)object);
> @@ -588,6 +584,23 @@ static void deepen(int depth, const struct object_array *shallows)
>  		/* make sure commit traversal conforms to client */
>  		register_shallow(object->oid.hash);
>  	}
> +}
> +
> +static void deepen(int depth, const struct object_array *shallows)
> +{
> +	struct commit_list *result = NULL;
> +	int i;
> +	if (depth == INFINITE_DEPTH && !is_repository_shallow())
> +		for (i = 0; i < shallows->nr; i++) {
> +			struct object *object = shallows->objects[i].item;
> +			object->flags |= NOT_SHALLOW;
> +		}
> +	else
> +		result = get_shallow_commits(&want_obj, depth,
> +					     SHALLOW, NOT_SHALLOW);
> +	send_shallow(result);
> +	free_commit_list(result);
> +	send_unshallow(shallows);
>  	packet_flush(1);
>  }

[PATCH v2 08/25] upload-pack: use skip_prefix() instead of starts_with() when possible

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 upload-pack.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index bfb7985..257ad48 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -276,7 +276,7 @@ static void create_pack_file(void)
 	die("git upload-pack: %s", abort_msg);
 }
 
-static int got_sha1(char *hex, unsigned char *sha1)
+static int got_sha1(const char *hex, unsigned char *sha1)
 {
 	struct object *o;
 	int we_knew_they_have = 0;
@@ -382,6 +382,8 @@ static int get_common_commits(void)
 
 	for (;;) {
 		char *line = packet_read_line(0, NULL);
+		const char *arg;
+
 		reset_timeout();
 
 		if (!line) {
@@ -403,8 +405,8 @@ static int get_common_commits(void)
 			got_other = 0;
 			continue;
 		}
-		if (starts_with(line, "have ")) {
-			switch (got_sha1(line+5, sha1)) {
+		if (skip_prefix(line, "have ", &arg)) {
+			switch (got_sha1(arg, sha1)) {
 			case -1: /* they have what we do not */
 				got_other = 1;
 				if (multi_ack && ok_to_give_up()) {
@@ -616,14 +618,16 @@ static void receive_needs(void)
 		const char *features;
 		unsigned char sha1_buf[20];
 		char *line = packet_read_line(0, NULL);
+		const char *arg;
+
 		reset_timeout();
 		if (!line)
 			break;
 
-		if (starts_with(line, "shallow ")) {
+		if (skip_prefix(line, "shallow ", &arg)) {
 			unsigned char sha1[20];
 			struct object *object;
-			if (get_sha1_hex(line + 8, sha1))
+			if (get_sha1_hex(arg, sha1))
 				die("invalid shallow line: %s", line);
 			object = parse_object(sha1);
 			if (!object)
@@ -636,19 +640,19 @@ static void receive_needs(void)
 			}
 			continue;
 		}
-		if (starts_with(line, "deepen ")) {
+		if (skip_prefix(line, "deepen ", &arg)) {
 			char *end;
-			depth = strtol(line + 7, &end, 0);
-			if (end == line + 7 || depth <= 0)
+			depth = strtol(arg, &end, 0);
+			if (end == arg || depth <= 0)
 				die("Invalid deepen: %s", line);
 			continue;
 		}
-		if (!starts_with(line, "want ") ||
-		    get_sha1_hex(line+5, sha1_buf))
+		if (!skip_prefix(line, "want ", &arg) ||
+		    get_sha1_hex(arg, sha1_buf))
 			die("git upload-pack: protocol error, "
 			    "expected to get sha, not '%s'", line);
 
-		features = line + 45;
+		features = arg + 40;
 
 		if (parse_feature_request(features, "multi_ack_detailed"))
 			multi_ack = 2;
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 08/25] upload-pack: use skip_prefix() instead of starts_with() when possible

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

The result is certainly easier to read.  "when possible" is not
quite a right way to look at it, though.  I think the patch does it
when it makes sense (i.e. when the result becomes easier to read),
which is much better ;-)

I initially thought that the name "arg" was too bland, but we can
think of these as pointing at an argument on each line that begins
with a command (e.g. "have", "shallow", etc.), and when viewed that
way, calling the second token on the line an "arg" makes sense.



>  upload-pack.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index bfb7985..257ad48 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -276,7 +276,7 @@ static void create_pack_file(void)
>  	die("git upload-pack: %s", abort_msg);
>  }
>  
> -static int got_sha1(char *hex, unsigned char *sha1)
> +static int got_sha1(const char *hex, unsigned char *sha1)
>  {
>  	struct object *o;
>  	int we_knew_they_have = 0;
> @@ -382,6 +382,8 @@ static int get_common_commits(void)
>  
>  	for (;;) {
>  		char *line = packet_read_line(0, NULL);
> +		const char *arg;
> +
>  		reset_timeout();
>  
>  		if (!line) {
> @@ -403,8 +405,8 @@ static int get_common_commits(void)
>  			got_other = 0;
>  			continue;
>  		}
> -		if (starts_with(line, "have ")) {
> -			switch (got_sha1(line+5, sha1)) {
> +		if (skip_prefix(line, "have ", &arg)) {
> +			switch (got_sha1(arg, sha1)) {
>  			case -1: /* they have what we do not */
>  				got_other = 1;
>  				if (multi_ack && ok_to_give_up()) {
> @@ -616,14 +618,16 @@ static void receive_needs(void)
>  		const char *features;
>  		unsigned char sha1_buf[20];
>  		char *line = packet_read_line(0, NULL);
> +		const char *arg;
> +
>  		reset_timeout();
>  		if (!line)
>  			break;
>  
> -		if (starts_with(line, "shallow ")) {
> +		if (skip_prefix(line, "shallow ", &arg)) {
>  			unsigned char sha1[20];
>  			struct object *object;
> -			if (get_sha1_hex(line + 8, sha1))
> +			if (get_sha1_hex(arg, sha1))
>  				die("invalid shallow line: %s", line);
>  			object = parse_object(sha1);
>  			if (!object)
> @@ -636,19 +640,19 @@ static void receive_needs(void)
>  			}
>  			continue;
>  		}
> -		if (starts_with(line, "deepen ")) {
> +		if (skip_prefix(line, "deepen ", &arg)) {
>  			char *end;
> -			depth = strtol(line + 7, &end, 0);
> -			if (end == line + 7 || depth <= 0)
> +			depth = strtol(arg, &end, 0);
> +			if (end == arg || depth <= 0)
>  				die("Invalid deepen: %s", line);
>  			continue;
>  		}
> -		if (!starts_with(line, "want ") ||
> -		    get_sha1_hex(line+5, sha1_buf))
> +		if (!skip_prefix(line, "want ", &arg) ||
> +		    get_sha1_hex(arg, sha1_buf))
>  			die("git upload-pack: protocol error, "
>  			    "expected to get sha, not '%s'", line);
>  
> -		features = line + 45;
> +		features = arg + 40;
>  
>  		if (parse_feature_request(features, "multi_ack_detailed"))
>  			multi_ack = 2;

[PATCH v2 09/25] upload-pack: tighten number parsing at "deepen" lines

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 upload-pack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 257ad48..9f14933 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -641,9 +641,9 @@ static void receive_needs(void)
 			continue;
 		}
 		if (skip_prefix(line, "deepen ", &arg)) {
-			char *end;
+			char *end = NULL;
 			depth = strtol(arg, &end, 0);
-			if (end == arg || depth <= 0)
+			if (!end || *end || depth <= 0)
 				die("Invalid deepen: %s", line);
 			continue;
 		}
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 09/25] upload-pack: tighten number parsing at "deepen" lines

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Hmm, so "deepen 10-by-the-way-let-me-tell-you-something-else" was an
acceptable input that some (third-party) version of "git fetch"
could have used, but now we are rejecting it.

That "let me tell you something else" part wouldn't have reached
outside this code, so it is inconceivable that anybody would relied
on that looseness as a "feature", so the only practical risk would
be if somebody wrote a protocol driver, mumbling "on the Internet,
the end of line is CRLF, just like SMTP does", that sends a "deepen
10<CR><LF>".  We used not to notice, but now we reject such a
reimplementation of Git.

>  upload-pack.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 257ad48..9f14933 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -641,9 +641,9 @@ static void receive_needs(void)
>  			continue;
>  		}
>  		if (skip_prefix(line, "deepen ", &arg)) {
> -			char *end;
> +			char *end = NULL;
>  			depth = strtol(arg, &end, 0);
> -			if (end == arg || depth <= 0)
> +			if (!end || *end || depth <= 0)
>  				die("Invalid deepen: %s", line);
>  			continue;
>  		}

Re: [PATCH v2 09/25] upload-pack: tighten number parsing at "deepen" lines

[Duy Nguyen]

On Fri, Feb 5, 2016 at 6:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>
> Hmm, so "deepen 10-by-the-way-let-me-tell-you-something-else" was an
> acceptable input that some (third-party) version of "git fetch"
> could have used, but now we are rejecting it.
>
> That "let me tell you something else" part wouldn't have reached
> outside this code, so it is inconceivable that anybody would relied
> on that looseness as a "feature", so the only practical risk would
> be if somebody wrote a protocol driver, mumbling "on the Internet,
> the end of line is CRLF, just like SMTP does", that sends a "deepen
> 10<CR><LF>".  We used not to notice, but now we reject such a
> reimplementation of Git.

On the other hand, if a broken client sends "deepen 10f" instead of
"deepen 271", we should reject and let the client be fixed instead of
sending a fetch of 10 commits deep back. "10<CR>" is not that bad, but
fixing it is still a good idea.
-- 
Duy

[PATCH v2 10/25] upload-pack: move rev-list code out of check_non_tip()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 upload-pack.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 9f14933..c8dafe8 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -451,7 +451,7 @@ static int is_our_ref(struct object *o)
 	return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
 
-static void check_non_tip(void)
+static int check_unreachable(struct object_array *src)
 {
 	static const char *argv[] = {
 		"rev-list", "--stdin", NULL,
@@ -461,14 +461,6 @@ static void check_non_tip(void)
 	char namebuf[42]; /* ^ + SHA-1 + LF */
 	int i;
 
-	/*
-	 * In the normal in-process case without
-	 * uploadpack.allowReachableSHA1InWant,
-	 * non-tip requests can never happen.
-	 */
-	if (!stateless_rpc && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))
-		goto error;
-
 	cmd.argv = argv;
 	cmd.git_cmd = 1;
 	cmd.no_stderr = 1;
@@ -476,7 +468,7 @@ static void check_non_tip(void)
 	cmd.out = -1;
 
 	if (start_command(&cmd))
-		goto error;
+		return 0;
 
 	/*
 	 * If rev-list --stdin encounters an unknown commit, it
@@ -495,16 +487,16 @@ static void check_non_tip(void)
 			continue;
 		memcpy(namebuf + 1, oid_to_hex(&o->oid), GIT_SHA1_HEXSZ);
 		if (write_in_full(cmd.in, namebuf, 42) < 0)
-			goto error;
+			return 0;
 	}
 	namebuf[40] = '\n';
-	for (i = 0; i < want_obj.nr; i++) {
-		o = want_obj.objects[i].item;
+	for (i = 0; i < src->nr; i++) {
+		o = src->objects[i].item;
 		if (is_our_ref(o))
 			continue;
 		memcpy(namebuf, oid_to_hex(&o->oid), GIT_SHA1_HEXSZ);
 		if (write_in_full(cmd.in, namebuf, 41) < 0)
-			goto error;
+			return 0;
 	}
 	close(cmd.in);
 
@@ -516,7 +508,7 @@ static void check_non_tip(void)
 	 */
 	i = read_in_full(cmd.out, namebuf, 1);
 	if (i)
-		goto error;
+		return 0;
 	close(cmd.out);
 
 	/*
@@ -525,15 +517,29 @@ static void check_non_tip(void)
 	 * even when it showed no commit.
 	 */
 	if (finish_command(&cmd))
-		goto error;
+		return 0;
 
 	/* All the non-tip ones are ancestors of what we advertised */
-	return;
+	return 1;
+}
+
+static void check_non_tip(void)
+{
+	int i;
+
+	/*
+	 * In the normal in-process case without
+	 * uploadpack.allowReachableSHA1InWant,
+	 * non-tip requests can never happen.
+	 */
+	if (!stateless_rpc && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))
+		;		/* error */
+	else if (check_unreachable(&want_obj))
+		return;
 
-error:
 	/* Pick one of them (we know there at least is one) */
 	for (i = 0; i < want_obj.nr; i++) {
-		o = want_obj.objects[i].item;
+		struct object *o = want_obj.objects[i].item;
 		if (!is_our_ref(o))
 			die("git upload-pack: not our ref %s",
 			    oid_to_hex(&o->oid));
-- 
2.7.0.377.g4cd97dd

[PATCH v2 11/25] fetch-pack: use skip_prefix() instead of starts_with() when possible

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/fetch-pack.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 9b2a514..0482077 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -58,13 +58,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 
 	for (i = 1; i < argc && *argv[i] == '-'; i++) {
 		const char *arg = argv[i];
+		const char *value;
 
-		if (starts_with(arg, "--upload-pack=")) {
-			args.uploadpack = arg + 14;
+		if (skip_prefix(arg, "--upload-pack=", &value)) {
+			args.uploadpack = value;
 			continue;
 		}
-		if (starts_with(arg, "--exec=")) {
-			args.uploadpack = arg + 7;
+		if (skip_prefix(arg, "--exec=", &value)) {
+			args.uploadpack = value;
 			continue;
 		}
 		if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
@@ -100,8 +101,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			args.verbose = 1;
 			continue;
 		}
-		if (starts_with(arg, "--depth=")) {
-			args.depth = strtol(arg + 8, NULL, 0);
+		if (skip_prefix(arg, "--depth=", &value)) {
+			args.depth = strtol(value, NULL, 0);
 			continue;
 		}
 		if (!strcmp("--no-progress", arg)) {
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 11/25] fetch-pack: use skip_prefix() instead of starts_with() when possible

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

OK, with the same comment on "when possible".  I would have reused
the same "arg" without inventing a separate variable "value" if I
were doing this conversion, but either way would be OK.

>  builtin/fetch-pack.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 9b2a514..0482077 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -58,13 +58,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  
>  	for (i = 1; i < argc && *argv[i] == '-'; i++) {
>  		const char *arg = argv[i];
> +		const char *value;
>  
> -		if (starts_with(arg, "--upload-pack=")) {
> -			args.uploadpack = arg + 14;
> +		if (skip_prefix(arg, "--upload-pack=", &value)) {
> +			args.uploadpack = value;
>  			continue;
>  		}
> -		if (starts_with(arg, "--exec=")) {
> -			args.uploadpack = arg + 7;
> +		if (skip_prefix(arg, "--exec=", &value)) {
> +			args.uploadpack = value;
>  			continue;
>  		}
>  		if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
> @@ -100,8 +101,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  			args.verbose = 1;
>  			continue;
>  		}
> -		if (starts_with(arg, "--depth=")) {
> -			args.depth = strtol(arg + 8, NULL, 0);
> +		if (skip_prefix(arg, "--depth=", &value)) {
> +			args.depth = strtol(value, NULL, 0);
>  			continue;
>  		}
>  		if (!strcmp("--no-progress", arg)) {

[PATCH v2 12/25] fetch-pack: use a common function for verbose printing

[Nguyễn Thái Ngọc Duy]

This reduces the number of "if (verbose)" which makes it a bit easier
to read imo. It also makes it easier to redirect all these printouts,
to a file for example.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 fetch-pack.c | 88 +++++++++++++++++++++++++++++-------------------------------
 1 file changed, 42 insertions(+), 46 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 01e34b6..16917f9 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -50,6 +50,21 @@ static int non_common_revs, multi_ack, use_sideband;
 #define ALLOW_REACHABLE_SHA1	02
 static unsigned int allow_unadvertised_object_request;
 
+__attribute__((format (printf, 2, 3)))
+static inline void print_verbose(const struct fetch_pack_args *args,
+				 const char *fmt, ...)
+{
+	va_list params;
+
+	if (!args->verbose)
+		return;
+
+	va_start(params, fmt);
+	vfprintf(stderr, fmt, params);
+	va_end(params);
+	fputc('\n', stderr);
+}
+
 static void rev_list_push(struct commit *commit, int mark)
 {
 	if (!(commit->object.flags & mark)) {
@@ -375,8 +390,7 @@ static int find_common(struct fetch_pack_args *args,
 	retval = -1;
 	while ((sha1 = get_rev())) {
 		packet_buf_write(&req_buf, "have %s\n", sha1_to_hex(sha1));
-		if (args->verbose)
-			fprintf(stderr, "have %s\n", sha1_to_hex(sha1));
+		print_verbose(args, "have %s", sha1_to_hex(sha1));
 		in_vain++;
 		if (flush_at <= ++count) {
 			int ack;
@@ -397,9 +411,9 @@ static int find_common(struct fetch_pack_args *args,
 			consume_shallow_list(args, fd[0]);
 			do {
 				ack = get_ack(fd[0], result_sha1);
-				if (args->verbose && ack)
-					fprintf(stderr, "got ack %d %s\n", ack,
-							sha1_to_hex(result_sha1));
+				if (ack)
+					print_verbose(args, "got ack %d %s", ack,
+						      sha1_to_hex(result_sha1));
 				switch (ack) {
 				case ACK:
 					flushes = 0;
@@ -438,8 +452,7 @@ static int find_common(struct fetch_pack_args *args,
 			} while (ack);
 			flushes--;
 			if (got_continue && MAX_IN_VAIN < in_vain) {
-				if (args->verbose)
-					fprintf(stderr, "giving up\n");
+				print_verbose(args, "giving up");
 				break; /* give up */
 			}
 		}
@@ -449,8 +462,7 @@ done:
 		packet_buf_write(&req_buf, "done\n");
 		send_request(args, fd[1], &req_buf);
 	}
-	if (args->verbose)
-		fprintf(stderr, "done\n");
+	print_verbose(args, "done");
 	if (retval != 0) {
 		multi_ack = 0;
 		flushes++;
@@ -462,9 +474,8 @@ done:
 	while (flushes || multi_ack) {
 		int ack = get_ack(fd[0], result_sha1);
 		if (ack) {
-			if (args->verbose)
-				fprintf(stderr, "got ack (%d) %s\n", ack,
-					sha1_to_hex(result_sha1));
+			print_verbose(args, "got ack (%d) %s", ack,
+				      sha1_to_hex(result_sha1));
 			if (ack == ACK)
 				return 0;
 			multi_ack = 1;
@@ -509,9 +520,8 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,
 					 unsigned long cutoff)
 {
 	while (complete && cutoff <= complete->item->date) {
-		if (args->verbose)
-			fprintf(stderr, "Marking %s as complete\n",
-				oid_to_hex(&complete->item->object.oid));
+		print_verbose(args, "Marking %s as complete",
+			      oid_to_hex(&complete->item->object.oid));
 		pop_most_recent_commit(&complete, COMPLETE);
 	}
 }
@@ -652,18 +662,12 @@ static int everything_local(struct fetch_pack_args *args,
 		o = lookup_object(remote);
 		if (!o || !(o->flags & COMPLETE)) {
 			retval = 0;
-			if (!args->verbose)
-				continue;
-			fprintf(stderr,
-				"want %s (%s)\n", sha1_to_hex(remote),
-				ref->name);
+			print_verbose(args, "want %s (%s)", sha1_to_hex(remote),
+				      ref->name);
 			continue;
 		}
-		if (!args->verbose)
-			continue;
-		fprintf(stderr,
-			"already have %s (%s)\n", sha1_to_hex(remote),
-			ref->name);
+		print_verbose(args, "already have %s (%s)", sha1_to_hex(remote),
+			      ref->name);
 	}
 	return retval;
 }
@@ -810,39 +814,32 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 	if ((args->depth > 0 || is_repository_shallow()) && !server_supports("shallow"))
 		die("Server does not support shallow clients");
 	if (server_supports("multi_ack_detailed")) {
-		if (args->verbose)
-			fprintf(stderr, "Server supports multi_ack_detailed\n");
+		print_verbose(args, "Server supports multi_ack_detailed");
 		multi_ack = 2;
 		if (server_supports("no-done")) {
-			if (args->verbose)
-				fprintf(stderr, "Server supports no-done\n");
+			print_verbose(args, "Server supports no-done");
 			if (args->stateless_rpc)
 				no_done = 1;
 		}
 	}
 	else if (server_supports("multi_ack")) {
-		if (args->verbose)
-			fprintf(stderr, "Server supports multi_ack\n");
+		print_verbose(args, "Server supports multi_ack");
 		multi_ack = 1;
 	}
 	if (server_supports("side-band-64k")) {
-		if (args->verbose)
-			fprintf(stderr, "Server supports side-band-64k\n");
+		print_verbose(args, "Server supports side-band-64k");
 		use_sideband = 2;
 	}
 	else if (server_supports("side-band")) {
-		if (args->verbose)
-			fprintf(stderr, "Server supports side-band\n");
+		print_verbose(args, "Server supports side-band");
 		use_sideband = 1;
 	}
 	if (server_supports("allow-tip-sha1-in-want")) {
-		if (args->verbose)
-			fprintf(stderr, "Server supports allow-tip-sha1-in-want\n");
+		print_verbose(args, "Server supports allow-tip-sha1-in-want");
 		allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
 	}
 	if (server_supports("allow-reachable-sha1-in-want")) {
-		if (args->verbose)
-			fprintf(stderr, "Server supports allow-reachable-sha1-in-want\n");
+		print_verbose(args, "Server supports allow-reachable-sha1-in-want\n");
 		allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
 	}
 	if (!server_supports("thin-pack"))
@@ -851,17 +848,16 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		args->no_progress = 0;
 	if (!server_supports("include-tag"))
 		args->include_tag = 0;
-	if (server_supports("ofs-delta")) {
-		if (args->verbose)
-			fprintf(stderr, "Server supports ofs-delta\n");
-	} else
+	if (server_supports("ofs-delta"))
+		print_verbose(args, "Server supports ofs-delta");
+	else
 		prefer_ofs_delta = 0;
 
 	if ((agent_feature = server_feature_value("agent", &agent_len))) {
 		agent_supported = 1;
-		if (args->verbose && agent_len)
-			fprintf(stderr, "Server version is %.*s\n",
-				agent_len, agent_feature);
+		if (agent_len)
+			print_verbose(args, "Server version is %.*s",
+				      agent_len, agent_feature);
 	}
 
 	if (everything_local(args, &ref, sought, nr_sought)) {
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 12/25] fetch-pack: use a common function for verbose printing

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This reduces the number of "if (verbose)" which makes it a bit easier
> to read imo. It also makes it easier to redirect all these printouts,
> to a file for example.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

>  fetch-pack.c | 88 +++++++++++++++++++++++++++++-------------------------------
>  1 file changed, 42 insertions(+), 46 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 01e34b6..16917f9 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -50,6 +50,21 @@ static int non_common_revs, multi_ack, use_sideband;
>  #define ALLOW_REACHABLE_SHA1	02
>  static unsigned int allow_unadvertised_object_request;
>  
> +__attribute__((format (printf, 2, 3)))
> +static inline void print_verbose(const struct fetch_pack_args *args,
> +				 const char *fmt, ...)
> +{
> +	va_list params;
> +
> +	if (!args->verbose)
> +		return;
> +
> +	va_start(params, fmt);
> +	vfprintf(stderr, fmt, params);
> +	va_end(params);
> +	fputc('\n', stderr);
> +}

This does "print if told to be verbose".  Sounds more like 'tracef',
'reportf', 'debugf', etc.

I am debating myself if it is a good idea to omit the final "\n" on
the calling side and add one unconditionally here, but the benefit
of brevity on the many callsites outweigh the reduced flexibility, I
guess.

Thanks.

Re: [PATCH v2 12/25] fetch-pack: use a common function for verbose printing

[Eric Sunshine]

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> This reduces the number of "if (verbose)" which makes it a bit easier
> to read imo. It also makes it easier to redirect all these printouts,
> to a file for example.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/fetch-pack.c b/fetch-pack.c
> @@ -810,39 +814,32 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>         else if (server_supports("side-band")) {
> -               if (args->verbose)
> -                       fprintf(stderr, "Server supports side-band\n");
> +               print_verbose(args, "Server supports side-band");
>                 use_sideband = 1;
>         }
>         if (server_supports("allow-tip-sha1-in-want")) {
> -               if (args->verbose)
> -                       fprintf(stderr, "Server supports allow-tip-sha1-in-want\n");
> +               print_verbose(args, "Server supports allow-tip-sha1-in-want");
>                 allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
>         }
>         if (server_supports("allow-reachable-sha1-in-want")) {
> -               if (args->verbose)
> -                       fprintf(stderr, "Server supports allow-reachable-sha1-in-want\n");
> +               print_verbose(args, "Server supports allow-reachable-sha1-in-want\n");

I think you want to drop the newline here as you did with all the
other call-sites since print_verbose() adds its own.

>                 allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
>         }
>         if (!server_supports("thin-pack"))

[PATCH v2 13/25] fetch-pack: use a separate flag for fetch in deepening mode

[Nguyễn Thái Ngọc Duy]

The shallow repo could be deepened or shortened when then user gives
--depth. But in future that won't be the only way to deepen/shorten a
repo. Stop relying on args->depth in this mode. Future deepening
methods can simply set this flag on instead of updating all these if
expressions.

The new name "deepen" was chosen after the command to define shallow
boundary in pack protocol. New commands also follow this tradition.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 fetch-pack.c | 14 ++++++++------
 fetch-pack.h |  1 +
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 16917f9..e947514 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -197,7 +197,7 @@ enum ack_type {
 
 static void consume_shallow_list(struct fetch_pack_args *args, int fd)
 {
-	if (args->stateless_rpc && args->depth > 0) {
+	if (args->stateless_rpc && args->deepen) {
 		/* If we sent a depth we will get back "duplicate"
 		 * shallow and unshallow commands every time there
 		 * is a block of have lines exchanged.
@@ -348,7 +348,7 @@ static int find_common(struct fetch_pack_args *args,
 	packet_buf_flush(&req_buf);
 	state_len = req_buf.len;
 
-	if (args->depth > 0) {
+	if (args->deepen) {
 		char *line;
 		const char *arg;
 		unsigned char sha1[20];
@@ -557,7 +557,7 @@ static void filter_refs(struct fetch_pack_args *args,
 		}
 
 		if (!keep && args->fetch_all &&
-		    (!args->depth || !starts_with(ref->name, "refs/tags/")))
+		    (!args->deepen || !starts_with(ref->name, "refs/tags/")))
 			keep = 1;
 
 		if (keep) {
@@ -627,7 +627,7 @@ static int everything_local(struct fetch_pack_args *args,
 		}
 	}
 
-	if (!args->depth) {
+	if (!args->deepen) {
 		for_each_ref(mark_complete_oid, NULL);
 		for_each_alternate_ref(mark_alternate_complete, NULL);
 		commit_list_sort_by_date(&complete);
@@ -813,6 +813,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 
 	if ((args->depth > 0 || is_repository_shallow()) && !server_supports("shallow"))
 		die("Server does not support shallow clients");
+	if (args->depth > 0)
+		args->deepen = 1;
 	if (server_supports("multi_ack_detailed")) {
 		print_verbose(args, "Server supports multi_ack_detailed");
 		multi_ack = 2;
@@ -873,7 +875,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 
 	if (args->stateless_rpc)
 		packet_flush(fd[1]);
-	if (args->depth > 0)
+	if (args->deepen)
 		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
 					NULL);
 	else if (si->nr_ours || si->nr_theirs)
@@ -940,7 +942,7 @@ static void update_shallow(struct fetch_pack_args *args,
 	int *status;
 	int i;
 
-	if (args->depth > 0 && alternate_shallow_file) {
+	if (args->deepen && alternate_shallow_file) {
 		if (*alternate_shallow_file == '\0') { /* --unshallow */
 			unlink_or_warn(git_path_shallow());
 			rollback_lock_file(&shallow_lock);
diff --git a/fetch-pack.h b/fetch-pack.h
index bb7fd76..4d0adb0 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -25,6 +25,7 @@ struct fetch_pack_args {
 	unsigned self_contained_and_connected:1;
 	unsigned cloning:1;
 	unsigned update_shallow:1;
+	unsigned deepen:1;
 };
 
 /*
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 13/25] fetch-pack: use a separate flag for fetch in deepening mode

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> The shallow repo could be deepened or shortened when then user gives
> --depth. But in future that won't be the only way to deepen/shorten a
> repo. Stop relying on args->depth in this mode. Future deepening
> methods can simply set this flag on instead of updating all these if
> expressions.
>
> The new name "deepen" was chosen after the command to define shallow
> boundary in pack protocol. New commands also follow this tradition.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

OK.  Up to here things look more-or-less sensible overall.

Thanks.

>  fetch-pack.c | 14 ++++++++------
>  fetch-pack.h |  1 +
>  2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 16917f9..e947514 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -197,7 +197,7 @@ enum ack_type {
>  
>  static void consume_shallow_list(struct fetch_pack_args *args, int fd)
>  {
> -	if (args->stateless_rpc && args->depth > 0) {
> +	if (args->stateless_rpc && args->deepen) {
>  		/* If we sent a depth we will get back "duplicate"
>  		 * shallow and unshallow commands every time there
>  		 * is a block of have lines exchanged.
> @@ -348,7 +348,7 @@ static int find_common(struct fetch_pack_args *args,
>  	packet_buf_flush(&req_buf);
>  	state_len = req_buf.len;
>  
> -	if (args->depth > 0) {
> +	if (args->deepen) {
>  		char *line;
>  		const char *arg;
>  		unsigned char sha1[20];
> @@ -557,7 +557,7 @@ static void filter_refs(struct fetch_pack_args *args,
>  		}
>  
>  		if (!keep && args->fetch_all &&
> -		    (!args->depth || !starts_with(ref->name, "refs/tags/")))
> +		    (!args->deepen || !starts_with(ref->name, "refs/tags/")))
>  			keep = 1;
>  
>  		if (keep) {
> @@ -627,7 +627,7 @@ static int everything_local(struct fetch_pack_args *args,
>  		}
>  	}
>  
> -	if (!args->depth) {
> +	if (!args->deepen) {
>  		for_each_ref(mark_complete_oid, NULL);
>  		for_each_alternate_ref(mark_alternate_complete, NULL);
>  		commit_list_sort_by_date(&complete);
> @@ -813,6 +813,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  
>  	if ((args->depth > 0 || is_repository_shallow()) && !server_supports("shallow"))
>  		die("Server does not support shallow clients");
> +	if (args->depth > 0)
> +		args->deepen = 1;
>  	if (server_supports("multi_ack_detailed")) {
>  		print_verbose(args, "Server supports multi_ack_detailed");
>  		multi_ack = 2;
> @@ -873,7 +875,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  
>  	if (args->stateless_rpc)
>  		packet_flush(fd[1]);
> -	if (args->depth > 0)
> +	if (args->deepen)
>  		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
>  					NULL);
>  	else if (si->nr_ours || si->nr_theirs)
> @@ -940,7 +942,7 @@ static void update_shallow(struct fetch_pack_args *args,
>  	int *status;
>  	int i;
>  
> -	if (args->depth > 0 && alternate_shallow_file) {
> +	if (args->deepen && alternate_shallow_file) {
>  		if (*alternate_shallow_file == '\0') { /* --unshallow */
>  			unlink_or_warn(git_path_shallow());
>  			rollback_lock_file(&shallow_lock);
> diff --git a/fetch-pack.h b/fetch-pack.h
> index bb7fd76..4d0adb0 100644
> --- a/fetch-pack.h
> +++ b/fetch-pack.h
> @@ -25,6 +25,7 @@ struct fetch_pack_args {
>  	unsigned self_contained_and_connected:1;
>  	unsigned cloning:1;
>  	unsigned update_shallow:1;
> +	unsigned deepen:1;
>  };
>  
>  /*

Re: [PATCH v2 13/25] fetch-pack: use a separate flag for fetch in deepening mode

[Eric Sunshine]

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> The shallow repo could be deepened or shortened when then user gives

s/then/the/

> --depth. But in future that won't be the only way to deepen/shorten a
> repo. Stop relying on args->depth in this mode. Future deepening
> methods can simply set this flag on instead of updating all these if
> expressions.
>
> The new name "deepen" was chosen after the command to define shallow
> boundary in pack protocol. New commands also follow this tradition.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

[PATCH v2 14/25] shallow.c: implement a generic shallow boundary finder based on rev-list

[Nguyễn Thái Ngọc Duy]

Instead of a custom commit walker like get_shallow_commits(), this new
function uses rev-list to mark NOT_SHALLOW to all reachable commits,
except borders. The definition of reachable is to be defined by the
protocol later. This makes it more flexible to define shallow boundary.

Note: if a commit has one NOT_SHALLOW parent and one SHALLOW parent,
then it's considered the boundary. Which means in the client side, this
commit has _no_ parents. This could lead to surprising cuts if we're not
careful.

Another option is to include more commits and only mark commits whose
all parents are SHALLOW as boundary.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 commit.h  |  2 ++
 shallow.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/commit.h b/commit.h
index 5d58be0..b717be1 100644
--- a/commit.h
+++ b/commit.h
@@ -258,6 +258,8 @@ extern int for_each_commit_graft(each_commit_graft_fn, void *);
 extern int is_repository_shallow(void);
 extern struct commit_list *get_shallow_commits(struct object_array *heads,
 		int depth, int shallow_flag, int not_shallow_flag);
+extern struct commit_list *get_shallow_commits_by_rev_list(
+		int ac, const char **av, int shallow_flag, int not_shallow_flag);
 extern void set_alternate_shallow_file(const char *path, int override);
 extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 				 const struct sha1_array *extra);
diff --git a/shallow.c b/shallow.c
index 60f1505..f5d5c1d 100644
--- a/shallow.c
+++ b/shallow.c
@@ -10,6 +10,8 @@
 #include "diff.h"
 #include "revision.h"
 #include "commit-slab.h"
+#include "revision.h"
+#include "list-objects.h"
 
 static int is_shallow = -1;
 static struct stat_validity shallow_stat;
@@ -137,6 +139,89 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 	return result;
 }
 
+static void show_commit(struct commit *commit, void *data)
+{
+	commit->object.flags |= *(int *)data;
+}
+
+/*
+ * Given rev-list arguments, run rev-list. All reachable commits
+ * except border ones are marked with not_shallow_flag. Border commits
+ * are marked with shallow_flag. The list of border/shallow commits
+ * are also returned.
+ */
+struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,
+						    int shallow_flag,
+						    int not_shallow_flag)
+{
+	struct commit_list *result = NULL, *p;
+	struct rev_info revs;
+	unsigned int i, nr;
+
+	/*
+	 * SHALLOW (excluded) and NOT_SHALLOW (included) should not be
+	 * set at this point. But better be safe than sorry.
+	 */
+	nr = get_max_object_index();
+	for (i = 0; i < nr; i++) {
+		struct object *o = get_indexed_object(i);
+		if (!o || o->type != OBJ_COMMIT)
+			continue;
+		o->flags &= ~(shallow_flag | not_shallow_flag);
+	}
+
+	is_repository_shallow(); /* make sure shallows are read */
+
+	init_revisions(&revs, NULL);
+	save_commit_buffer = 0;
+	setup_revisions(ac, av, &revs, NULL);
+
+	/* Mark all reachable commits as NOT_SHALLOW */
+	if (prepare_revision_walk(&revs))
+		die("revision walk setup failed");
+	traverse_commit_list(&revs, show_commit, NULL, &not_shallow_flag);
+
+	/*
+	 * mark border commits SHALLOW + NOT_SHALLOW.
+	 * We cannot clear NOT_SHALLOW right now. Imagine border
+	 * commit A is processed first, then commit B, whose parent is
+	 * A, later. If NOT_SHALLOW on A is cleared at step 1, B
+	 * itself is considered border at step 2, which is incorrect.
+	 */
+	nr = get_max_object_index();
+	for (i = 0; i < nr; i++) {
+		struct object *o = get_indexed_object(i);
+		struct commit *c = (struct commit *)o;
+
+		if (!o || o->type != OBJ_COMMIT ||
+		    !(o->flags & not_shallow_flag))
+			continue;
+
+		if (parse_commit(c))
+			die("unable to parse commit %s",
+			    oid_to_hex(&c->object.oid));
+
+		for (p = c->parents; p; p = p->next)
+			if (!(p->item->object.flags & not_shallow_flag)) {
+				o->flags |= shallow_flag;
+				commit_list_insert(c, &result);
+				break;
+			}
+	}
+
+	/*
+	 * Now we can clean up NOT_SHALLOW on border commits. Having
+	 * both flags set can confuse the caller.
+	 */
+	for (p = result; p; p = p->next) {
+		struct object *ro = &p->item->object;
+		if ((ro->flags & not_shallow_flag) &&
+		    (ro->flags & shallow_flag))
+			ro->flags &= ~not_shallow_flag;
+	}
+	return result;
+}
+
 static void check_shallow_file_for_update(void)
 {
 	if (is_shallow == -1)
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 14/25] shallow.c: implement a generic shallow boundary finder based on rev-list

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Instead of a custom commit walker like get_shallow_commits(), this new
> function uses rev-list to mark NOT_SHALLOW to all reachable commits,
> except borders. The definition of reachable is to be defined by the
> protocol later. This makes it more flexible to define shallow boundary.
>
> Note: if a commit has one NOT_SHALLOW parent and one SHALLOW parent,
> then it's considered the boundary. Which means in the client side, this
> commit has _no_ parents. This could lead to surprising cuts if we're not
> careful.
>
> Another option is to include more commits and only mark commits whose
> all parents are SHALLOW as boundary.

The second and third are greek to me at this point ;-) but hopefully
they will become clear as we read on.

> +/*
> + * Given rev-list arguments, run rev-list. All reachable commits
> + * except border ones are marked with not_shallow_flag. Border commits
> + * are marked with shallow_flag. The list of border/shallow commits
> + * are also returned.
> + */
> +struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,
> +						    int shallow_flag,
> +						    int not_shallow_flag)
> +{
> +	struct commit_list *result = NULL, *p;
> +	struct rev_info revs;
> +	unsigned int i, nr;
> +
> +	/*
> +	 * SHALLOW (excluded) and NOT_SHALLOW (included) should not be
> +	 * set at this point. But better be safe than sorry.
> +	 */
> +	nr = get_max_object_index();
> +	for (i = 0; i < nr; i++) {
> +		struct object *o = get_indexed_object(i);
> +		if (!o || o->type != OBJ_COMMIT)
> +			continue;
> +		o->flags &= ~(shallow_flag | not_shallow_flag);
> +	}

This is slightly different from clear_object_flags(), but I cannot
tell if it is intended, or if you forgot that the function exists.

> +	is_repository_shallow(); /* make sure shallows are read */
> +
> +	init_revisions(&revs, NULL);
> +	save_commit_buffer = 0;
> +	setup_revisions(ac, av, &revs, NULL);
> +
> +	/* Mark all reachable commits as NOT_SHALLOW */
> +	if (prepare_revision_walk(&revs))
> +		die("revision walk setup failed");
> +	traverse_commit_list(&revs, show_commit, NULL, &not_shallow_flag);
> +
> +	/*
> +	 * mark border commits SHALLOW + NOT_SHALLOW.
> +	 * We cannot clear NOT_SHALLOW right now. Imagine border
> +	 * commit A is processed first, then commit B, whose parent is
> +	 * A, later. If NOT_SHALLOW on A is cleared at step 1, B
> +	 * itself is considered border at step 2, which is incorrect.
> +	 */
> +	nr = get_max_object_index();
> +	for (i = 0; i < nr; i++) {

I'd really like not to see a loop over 0..get_max_object_index().
Are there many codepaths that peek into the in-core entire object
store already?  Would it work equally well to keep track of the
commits discovered in show_commit() to use as the set of commits
you need to visit in this second pass?

> +		struct object *o = get_indexed_object(i);
> +		struct commit *c = (struct commit *)o;
> +
> +		if (!o || o->type != OBJ_COMMIT ||
> +		    !(o->flags & not_shallow_flag))
> +			continue;
> +
> +		if (parse_commit(c))
> +			die("unable to parse commit %s",
> +			    oid_to_hex(&c->object.oid));
> +
> +		for (p = c->parents; p; p = p->next)
> +			if (!(p->item->object.flags & not_shallow_flag)) {
> +				o->flags |= shallow_flag;
> +				commit_list_insert(c, &result);
> +				break;
> +			}
> +	}
> +
> +	/*
> +	 * Now we can clean up NOT_SHALLOW on border commits. Having
> +	 * both flags set can confuse the caller.
> +	 */
> +	for (p = result; p; p = p->next) {
> +		struct object *ro = &p->item->object;

Why "ro" only in this third pass, unlike the other two passes that
said "o" which is in a sense more descriptive?

> +		if ((ro->flags & not_shallow_flag) &&
> +		    (ro->flags & shallow_flag))

If you introduce a "both_flags = shallow_flag | not_shallow_flag"
at the very beginning, this will become

	if (o->flags & both_flags)
        	o->flags &= ~not_shallow_flag;

which would probably be easier to read.  You can pass the same to
clear_object_flags() at the first pass.

> +			ro->flags &= ~not_shallow_flag;
> +	}
> +	return result;
> +}

Other than that, this step looks quite straight-forward to me.

Thanks.

> +
>  static void check_shallow_file_for_update(void)
>  {
>  	if (is_shallow == -1)

Re: [PATCH v2 14/25] shallow.c: implement a generic shallow boundary finder based on rev-list

[Duy Nguyen]

On Tue, Feb 9, 2016 at 4:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> +     is_repository_shallow(); /* make sure shallows are read */
>> +
>> +     init_revisions(&revs, NULL);
>> +     save_commit_buffer = 0;
>> +     setup_revisions(ac, av, &revs, NULL);
>> +
>> +     /* Mark all reachable commits as NOT_SHALLOW */
>> +     if (prepare_revision_walk(&revs))
>> +             die("revision walk setup failed");
>> +     traverse_commit_list(&revs, show_commit, NULL, &not_shallow_flag);
>> +
>> +     /*
>> +      * mark border commits SHALLOW + NOT_SHALLOW.
>> +      * We cannot clear NOT_SHALLOW right now. Imagine border
>> +      * commit A is processed first, then commit B, whose parent is
>> +      * A, later. If NOT_SHALLOW on A is cleared at step 1, B
>> +      * itself is considered border at step 2, which is incorrect.
>> +      */
>> +     nr = get_max_object_index();
>> +     for (i = 0; i < nr; i++) {
>
> I'd really like not to see a loop over 0..get_max_object_index().
> Are there many codepaths that peek into the in-core entire object
> store already?

You started it with check_non_tip(). At least that's how I know about
this loop. But I think that's the only code path, not counting this.

> Would it work equally well to keep track of the
> commits discovered in show_commit() to use as the set of commits
> you need to visit in this second pass?

We can't do this in show_commit. In this loop, we check
not_shallow_flag of parent commits. If one parent commit is not
show_commit'd yet, the flag is not set and we may incorrectly think
this is a border commit. The only way to avoid going through the
entire in-core object database is keeping a new commit_list and go
through it here. Which way is preferred?

>> +             struct object *o = get_indexed_object(i);
>> +             struct commit *c = (struct commit *)o;
>> +
>> +             if (!o || o->type != OBJ_COMMIT ||
>> +                 !(o->flags & not_shallow_flag))
>> +                     continue;
>> +
>> +             if (parse_commit(c))
>> +                     die("unable to parse commit %s",
>> +                         oid_to_hex(&c->object.oid));
>> +
>> +             for (p = c->parents; p; p = p->next)
>> +                     if (!(p->item->object.flags & not_shallow_flag)) {
>> +                             o->flags |= shallow_flag;
>> +                             commit_list_insert(c, &result);
>> +                             break;
>> +                     }
>> +     }
-- 
Duy

Re: [PATCH v2 14/25] shallow.c: implement a generic shallow boundary finder based on rev-list

[Duy Nguyen]

On Mon, Feb 08, 2016 at 01:09:24PM -0800, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> 
> > Instead of a custom commit walker like get_shallow_commits(), this new
> > function uses rev-list to mark NOT_SHALLOW to all reachable commits,
> > except borders. The definition of reachable is to be defined by the
> > protocol later. This makes it more flexible to define shallow boundary.
> >
> > Note: if a commit has one NOT_SHALLOW parent and one SHALLOW parent,
> > then it's considered the boundary. Which means in the client side, this
> > commit has _no_ parents. This could lead to surprising cuts if we're not
> > careful.
> >
> > Another option is to include more commits and only mark commits whose
> > all parents are SHALLOW as boundary.
> 
> The second and third are greek to me at this point ;-) but hopefully
> they will become clear as we read on.

Yeah. Everything looks clearer with illustration. This should be a
better. The question is should we do something about it now, or leave
it as is.

I'm tempted to go with "the first way" in future (so add some comments
about this in is_repository_shallow, instead of leaving it as commit
message in this patch).

-- 8< --
The way we find find border is paint all reachable commits NOT_SHALLOW.
Any of them that "touches" commits without NOT_SHALLOW flag are
considered shallow (e.g. zero parents via grafting mechanism). Shallow
commits and their true parents are all marked SHALLOW. Then NOT_SHALLOW
is removed from shallow commits at the end.

There is interesting observation, though somewhat off topic for this
patch. In the following graph, "x" is unreachable commits. "b" is the
parent of "a".

           x -- a -- o
	       /    /
         x -- b -- o

And as a result, "a" and "b" are both considered shallow commits. After
grafting occurs at the client side, what we see is

                a -- o
	            /
              b -- o

Notice that because of grafting, "a" has zero parents, so "b" is no
longer a parent of "a".

This is unfortunate and may be solved in two ways. The first is change
the way shallow grafting works and keeps "b -- a" connection if "b"
exists and is a shallow commit.

The second way is produce this graph (at client side) instead

           x -- a -- o
	       /    /
              b -- o

Which means we mark "x" as a shallow commit instead of "a".
-- 8< --
--
Duy

[PATCH v2 15/25] upload-pack: add deepen-since to cut shallow repos based on time

[Nguyễn Thái Ngọc Duy]

This should allow the user to say "create a shallow clone containing the
work from last year" (once the client side is fixed up, of course).

In theory deepen-since and deepen (aka --depth) can be used together to
draw the shallow boundary (whether it's intersection or union is up to
discussion, but if rev-list is used, it's likely intersection). However,
because deepen goes with a custom commit walker, we can't mix the two
yet.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/technical/pack-protocol.txt         |  3 +-
 Documentation/technical/protocol-capabilities.txt |  9 +++++
 upload-pack.c                                     | 45 ++++++++++++++++++++++-
 3 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index c6977bb..9251df1 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -219,7 +219,8 @@ out of what the server said it could do with the first 'want' line.
 
   shallow-line      =  PKT-LINE("shallow" SP obj-id)
 
-  depth-request     =  PKT-LINE("deepen" SP depth)
+  depth-request     =  PKT-LINE("deepen" SP depth) /
+		       PKT-LINE("deepen-since" SP timestamp)
 
   first-want        =  PKT-LINE("want" SP obj-id SP capability-list)
   additional-want   =  PKT-LINE("want" SP obj-id)
diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index eaab6b4..f08cc4e 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -179,6 +179,15 @@ This capability adds "deepen", "shallow" and "unshallow" commands to
 the  fetch-pack/upload-pack protocol so clients can request shallow
 clones.
 
+deepen-since
+------------
+
+This capability adds "deepen-since" command to fetch-pack/upload-pack
+protocol so the client can request shallow clones that are cut at a
+specific time, instead of depth. Internally it's equivalent of doing
+"rev-list --max-age=<timestamp>" on the server side. "deepen-since"
+cannot be used with "deepen".
+
 no-progress
 -----------
 
diff --git a/upload-pack.c b/upload-pack.c
index c8dafe8..794736c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -14,6 +14,7 @@
 #include "sigchain.h"
 #include "version.h"
 #include "string-list.h"
+#include "argv-array.h"
 
 static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<n>] <dir>";
 
@@ -612,11 +613,25 @@ static void deepen(int depth, const struct object_array *shallows)
 	packet_flush(1);
 }
 
+static void deepen_by_rev_list(int ac, const char **av,
+			       struct object_array *shallows)
+{
+	struct commit_list *result;
+
+	result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
+	send_shallow(result);
+	free_commit_list(result);
+	send_unshallow(shallows);
+	packet_flush(1);
+}
+
 static void receive_needs(void)
 {
 	struct object_array shallows = OBJECT_ARRAY_INIT;
 	int depth = 0;
 	int has_non_tip = 0;
+	unsigned long deepen_since = 0;
+	int deepen_rev_list = 0;
 
 	shallow_nr = 0;
 	for (;;) {
@@ -653,6 +668,16 @@ static void receive_needs(void)
 				die("Invalid deepen: %s", line);
 			continue;
 		}
+		if (skip_prefix(line, "deepen-since ", &arg)) {
+			char *end = NULL;
+			deepen_since = strtoul(arg, &end, 0);
+			if (!end || *end || !deepen_since ||
+			    /* revisions.c's max_age -1 is special */
+			    deepen_since == -1)
+				die("Invalid deepen-since: %s", line);
+			deepen_rev_list = 1;
+			continue;
+		}
 		if (!skip_prefix(line, "want ", &arg) ||
 		    get_sha1_hex(arg, sha1_buf))
 			die("git upload-pack: protocol error, "
@@ -704,10 +729,26 @@ static void receive_needs(void)
 	if (!use_sideband && daemon_mode)
 		no_progress = 1;
 
-	if (depth == 0 && shallows.nr == 0)
+	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");
 	if (depth > 0)
 		deepen(depth, &shallows);
+	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=%lu", deepen_since);
+		for (i = 0; i < want_obj.nr; i++) {
+			struct object *o = want_obj.objects[i].item;
+			argv_array_push(&av, oid_to_hex(&o->oid));
+		}
+		deepen_by_rev_list(av.argc, av.argv, &shallows);
+		argv_array_clear(&av);
+	}
 	else
 		if (shallows.nr > 0) {
 			int i;
@@ -756,7 +797,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
 		    int flag, void *cb_data)
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
-		" side-band-64k ofs-delta shallow no-progress"
+		" side-band-64k ofs-delta shallow deepen-since no-progress"
 		" include-tag multi_ack_detailed";
 	const char *refname_nons = strip_namespace(refname);
 	struct object_id peeled;
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 15/25] upload-pack: add deepen-since to cut shallow repos based on time

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This should allow the user to say "create a shallow clone containing the
> work from last year" (once the client side is fixed up, of course).
>
> In theory deepen-since and deepen (aka --depth) can be used together to
> draw the shallow boundary (whether it's intersection or union is up to
> discussion, but if rev-list is used, it's likely intersection). However,
> because deepen goes with a custom commit walker, we can't mix the two
> yet.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Looks quite straight-forward.

Thanks.

>  Documentation/technical/pack-protocol.txt         |  3 +-
>  Documentation/technical/protocol-capabilities.txt |  9 +++++
>  upload-pack.c                                     | 45 ++++++++++++++++++++++-
>  3 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
> index c6977bb..9251df1 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -219,7 +219,8 @@ out of what the server said it could do with the first 'want' line.
>  
>    shallow-line      =  PKT-LINE("shallow" SP obj-id)
>  
> -  depth-request     =  PKT-LINE("deepen" SP depth)
> +  depth-request     =  PKT-LINE("deepen" SP depth) /
> +		       PKT-LINE("deepen-since" SP timestamp)
>  
>    first-want        =  PKT-LINE("want" SP obj-id SP capability-list)
>    additional-want   =  PKT-LINE("want" SP obj-id)
> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
> index eaab6b4..f08cc4e 100644
> --- a/Documentation/technical/protocol-capabilities.txt
> +++ b/Documentation/technical/protocol-capabilities.txt
> @@ -179,6 +179,15 @@ This capability adds "deepen", "shallow" and "unshallow" commands to
>  the  fetch-pack/upload-pack protocol so clients can request shallow
>  clones.
>  
> +deepen-since
> +------------
> +
> +This capability adds "deepen-since" command to fetch-pack/upload-pack
> +protocol so the client can request shallow clones that are cut at a
> +specific time, instead of depth. Internally it's equivalent of doing
> +"rev-list --max-age=<timestamp>" on the server side. "deepen-since"
> +cannot be used with "deepen".
> +
>  no-progress
>  -----------
>  
> diff --git a/upload-pack.c b/upload-pack.c
> index c8dafe8..794736c 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -14,6 +14,7 @@
>  #include "sigchain.h"
>  #include "version.h"
>  #include "string-list.h"
> +#include "argv-array.h"
>  
>  static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<n>] <dir>";
>  
> @@ -612,11 +613,25 @@ static void deepen(int depth, const struct object_array *shallows)
>  	packet_flush(1);
>  }
>  
> +static void deepen_by_rev_list(int ac, const char **av,
> +			       struct object_array *shallows)
> +{
> +	struct commit_list *result;
> +
> +	result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
> +	send_shallow(result);
> +	free_commit_list(result);
> +	send_unshallow(shallows);
> +	packet_flush(1);
> +}
> +
>  static void receive_needs(void)
>  {
>  	struct object_array shallows = OBJECT_ARRAY_INIT;
>  	int depth = 0;
>  	int has_non_tip = 0;
> +	unsigned long deepen_since = 0;
> +	int deepen_rev_list = 0;
>  
>  	shallow_nr = 0;
>  	for (;;) {
> @@ -653,6 +668,16 @@ static void receive_needs(void)
>  				die("Invalid deepen: %s", line);
>  			continue;
>  		}
> +		if (skip_prefix(line, "deepen-since ", &arg)) {
> +			char *end = NULL;
> +			deepen_since = strtoul(arg, &end, 0);
> +			if (!end || *end || !deepen_since ||
> +			    /* revisions.c's max_age -1 is special */
> +			    deepen_since == -1)
> +				die("Invalid deepen-since: %s", line);
> +			deepen_rev_list = 1;
> +			continue;
> +		}
>  		if (!skip_prefix(line, "want ", &arg) ||
>  		    get_sha1_hex(arg, sha1_buf))
>  			die("git upload-pack: protocol error, "
> @@ -704,10 +729,26 @@ static void receive_needs(void)
>  	if (!use_sideband && daemon_mode)
>  		no_progress = 1;
>  
> -	if (depth == 0 && shallows.nr == 0)
> +	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");
>  	if (depth > 0)
>  		deepen(depth, &shallows);
> +	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=%lu", deepen_since);
> +		for (i = 0; i < want_obj.nr; i++) {
> +			struct object *o = want_obj.objects[i].item;
> +			argv_array_push(&av, oid_to_hex(&o->oid));
> +		}
> +		deepen_by_rev_list(av.argc, av.argv, &shallows);
> +		argv_array_clear(&av);
> +	}
>  	else
>  		if (shallows.nr > 0) {
>  			int i;
> @@ -756,7 +797,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
>  		    int flag, void *cb_data)
>  {
>  	static const char *capabilities = "multi_ack thin-pack side-band"
> -		" side-band-64k ofs-delta shallow no-progress"
> +		" side-band-64k ofs-delta shallow deepen-since no-progress"
>  		" include-tag multi_ack_detailed";
>  	const char *refname_nons = strip_namespace(refname);
>  	struct object_id peeled;

[PATCH v2 16/25] fetch: define shallow boundary with --shallow-since

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/fetch-options.txt     |  4 ++++
 Documentation/git-fetch-pack.txt    |  4 ++++
 Documentation/gitremote-helpers.txt |  3 +++
 builtin/fetch-pack.c                |  4 ++++
 builtin/fetch.c                     | 12 ++++++++++--
 fetch-pack.c                        | 12 +++++++++++-
 fetch-pack.h                        |  1 +
 remote-curl.c                       | 11 +++++++++--
 transport.c                         |  4 ++++
 transport.h                         |  4 ++++
 10 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 952dfdf..8738d3d 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -14,6 +14,10 @@
 	linkgit:git-clone[1]), deepen or shorten the history to the specified
 	number of commits. Tags for the deepened commits are not fetched.
 
+--shallow-since=<date>::
+	Deepen or shorten the history of a shallow repository to
+	include all reachable commits after <date>.
+
 --unshallow::
 	If the source repository is complete, convert a shallow
 	repository to a complete one, removing all the limitations
diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 8680f45..99e6257 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -87,6 +87,10 @@ be in a separate packet, and the list must end with a flush packet.
 	'git-upload-pack' treats the special depth 2147483647 as
 	infinite even if there is an ancestor-chain that long.
 
+--shallow-since=<date>::
+	Deepen or shorten the history of a shallow'repository to
+	include all reachable commits after <date>.
+
 --no-progress::
 	Do not show the progress.
 
diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index 78e0b27..9971d9a 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -415,6 +415,9 @@ set by Git if the remote helper has the 'option' capability.
 'option depth' <depth>::
 	Deepens the history of a shallow repository.
 
+'option deepen-since <timestamp>::
+	Deepens the history of a shallow repository based on time.
+
 'option followtags' {'true'|'false'}::
 	If enabled the helper should automatically fetch annotated
 	tag objects if the object the tag points at was transferred
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 0482077..192c1ae 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -105,6 +105,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			args.depth = strtol(value, NULL, 0);
 			continue;
 		}
+		if (skip_prefix(arg, "--shallow-since=", &value)) {
+			args.deepen_since = xstrdup(value);
+			continue;
+		}
 		if (!strcmp("--no-progress", arg)) {
 			args.no_progress = 1;
 			continue;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8e74213..7d4d082 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -36,9 +36,10 @@ static int prune = -1; /* unspecified */
 
 static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
-static int tags = TAGS_DEFAULT, unshallow, update_shallow;
+static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
 static int max_children = 1;
 static const char *depth;
+static const char *deepen_since;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
 static struct transport *gtransport;
@@ -115,6 +116,8 @@ static struct option builtin_fetch_options[] = {
 	OPT_BOOL(0, "progress", &progress, N_("force progress reporting")),
 	OPT_STRING(0, "depth", &depth, N_("depth"),
 		   N_("deepen history of shallow clone")),
+	OPT_STRING(0, "shallow-since", &deepen_since, N_("time"),
+		   N_("deepen history of shallow repository based on time")),
 	{ OPTION_SET_INT, 0, "unshallow", &unshallow, NULL,
 		   N_("convert to a complete repository"),
 		   PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },
@@ -754,7 +757,7 @@ static int quickfetch(struct ref *ref_map)
 	 * really need to perform.  Claiming failure now will ensure
 	 * we perform the network exchange to deepen our history.
 	 */
-	if (depth)
+	if (deepen)
 		return -1;
 	return check_everything_connected(iterate_ref_map, 1, &rm);
 }
@@ -870,6 +873,8 @@ static struct transport *prepare_transport(struct remote *remote)
 		set_option(transport, TRANS_OPT_KEEP, "yes");
 	if (depth)
 		set_option(transport, TRANS_OPT_DEPTH, depth);
+	if (deepen_since)
+		set_option(transport, TRANS_OPT_DEEPEN_SINCE, deepen_since);
 	if (update_shallow)
 		set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
 	return transport;
@@ -884,6 +889,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)
 
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
+	transport_set_option(transport, TRANS_OPT_DEEPEN_SINCE, NULL);
 	fetch_refs(transport, ref_map);
 
 	if (gsecondary) {
@@ -1167,6 +1173,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	/* no need to be strict, transport_set_option() will validate it again */
 	if (depth && atoi(depth) < 1)
 		die(_("depth %s is not a positive number"), depth);
+	if (depth || deepen_since)
+		deepen = 1;
 
 	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
 		if (recurse_submodules_default) {
diff --git a/fetch-pack.c b/fetch-pack.c
index e947514..db23998 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -21,6 +21,7 @@ static int fetch_unpack_limit = -1;
 static int unpack_limit = 100;
 static int prefer_ofs_delta = 1;
 static int no_done;
+static int deepen_since_ok;
 static int fetch_fsck_objects = -1;
 static int transfer_fsck_objects = -1;
 static int agent_supported;
@@ -326,6 +327,7 @@ static int find_common(struct fetch_pack_args *args,
 			if (args->no_progress)   strbuf_addstr(&c, " no-progress");
 			if (args->include_tag)   strbuf_addstr(&c, " include-tag");
 			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
+			if (deepen_since_ok)    strbuf_addstr(&c, " deepen-since");
 			if (agent_supported)    strbuf_addf(&c, " agent=%s",
 							    git_user_agent_sanitized());
 			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
@@ -345,6 +347,10 @@ static int find_common(struct fetch_pack_args *args,
 		write_shallow_commits(&req_buf, 1, NULL);
 	if (args->depth > 0)
 		packet_buf_write(&req_buf, "deepen %d", args->depth);
+	if (args->deepen_since) {
+		unsigned long max_age = approxidate(args->deepen_since);
+		packet_buf_write(&req_buf, "deepen-since %lu", max_age);
+	}
 	packet_buf_flush(&req_buf);
 	state_len = req_buf.len;
 
@@ -813,7 +819,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 
 	if ((args->depth > 0 || is_repository_shallow()) && !server_supports("shallow"))
 		die("Server does not support shallow clients");
-	if (args->depth > 0)
+	if (args->depth > 0 || args->deepen_since)
 		args->deepen = 1;
 	if (server_supports("multi_ack_detailed")) {
 		print_verbose(args, "Server supports multi_ack_detailed");
@@ -861,6 +867,10 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 			print_verbose(args, "Server version is %.*s",
 				      agent_len, agent_feature);
 	}
+	if (server_supports("deepen-since"))
+		deepen_since_ok = 1;
+	else if (args->deepen_since)
+		die("Server does not support --shallow-since");
 
 	if (everything_local(args, &ref, sought, nr_sought)) {
 		packet_flush(fd[1]);
diff --git a/fetch-pack.h b/fetch-pack.h
index 4d0adb0..f7eadb2 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -10,6 +10,7 @@ struct fetch_pack_args {
 	const char *uploadpack;
 	int unpacklimit;
 	int depth;
+	const char *deepen_since;
 	unsigned quiet:1;
 	unsigned keep_pack:1;
 	unsigned lock_pack:1;
diff --git a/remote-curl.c b/remote-curl.c
index 3ac5b6c..ce30961 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -20,6 +20,7 @@ static struct strbuf url = STRBUF_INIT;
 struct options {
 	int verbosity;
 	unsigned long depth;
+	char *deepen_since;
 	unsigned progress : 1,
 		check_self_contained_and_connected : 1,
 		cloning : 1,
@@ -60,6 +61,10 @@ static int set_option(const char *name, const char *value)
 		options.depth = v;
 		return 0;
 	}
+	else if (!strcmp(name, "deepen-since")) {
+		options.deepen_since = xstrdup(value);
+		return 0;
+	}
 	else if (!strcmp(name, "followtags")) {
 		if (!strcmp(value, "true"))
 			options.followtags = 1;
@@ -699,8 +704,8 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)
 	char **targets = xmalloc(nr_heads * sizeof(char*));
 	int ret, i;
 
-	if (options.depth)
-		die("dumb http transport does not support --depth");
+	if (options.depth || options.deepen_since)
+		die("dumb http transport does not support shallow capabilities");
 	for (i = 0; i < nr_heads; i++)
 		targets[i] = xstrdup(oid_to_hex(&to_fetch[i]->old_oid));
 
@@ -750,6 +755,8 @@ static int fetch_git(struct discovery *heads,
 		argv_array_push(&args, "--no-progress");
 	if (options.depth)
 		argv_array_pushf(&args, "--depth=%lu", options.depth);
+	if (options.deepen_since)
+		argv_array_pushf(&args, "--shallow-since=%s", options.deepen_since);
 	argv_array_push(&args, url.buf);
 
 	for (i = 0; i < nr_heads; i++) {
diff --git a/transport.c b/transport.c
index 67f3666..4902036 100644
--- a/transport.c
+++ b/transport.c
@@ -477,6 +477,9 @@ static int set_git_option(struct git_transport_options *opts,
 				die("transport: invalid depth option '%s'", value);
 		}
 		return 0;
+	} else if (!strcmp(name, TRANS_OPT_DEEPEN_SINCE)) {
+		opts->deepen_since = value;
+		return 0;
 	}
 	return 1;
 }
@@ -530,6 +533,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.quiet = (transport->verbose < 0);
 	args.no_progress = !transport->progress;
 	args.depth = data->options.depth;
+	args.deepen_since = data->options.deepen_since;
 	args.check_self_contained_and_connected =
 		data->options.check_self_contained_and_connected;
 	args.cloning = transport->cloning;
diff --git a/transport.h b/transport.h
index 8ebaaf2..9c10a44 100644
--- a/transport.h
+++ b/transport.h
@@ -13,6 +13,7 @@ struct git_transport_options {
 	unsigned self_contained_and_connected : 1;
 	unsigned update_shallow : 1;
 	int depth;
+	const char *deepen_since;
 	const char *uploadpack;
 	const char *receivepack;
 	struct push_cas_option *cas;
@@ -171,6 +172,9 @@ int transport_restrict_protocols(void);
 /* Limit the depth of the fetch if not null */
 #define TRANS_OPT_DEPTH "depth"
 
+/* Limit the depth of the fetch based on time if not null */
+#define TRANS_OPT_DEEPEN_SINCE "deepen-since"
+
 /* Aggressively fetch annotated tags if possible */
 #define TRANS_OPT_FOLLOWTAGS "followtags"
 
-- 
2.7.0.377.g4cd97dd

[PATCH v2 17/25] clone: define shallow clone boundary based on time with --shallow-since

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-clone.txt |  3 +++
 builtin/clone.c             | 16 +++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 789b668..1b6b639 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -194,6 +194,9 @@ objects from the source repository into a pack in the cloned repository.
 	`--no-single-branch` is given to fetch the histories near the
 	tips of all branches.
 
+--shallow-since=<date>::
+	Create a shallow clone with a history after the specified time.
+
 --[no-]single-branch::
 	Clone only the history leading to the tip of a single branch,
 	either specified by the `--branch` option or the primary
diff --git a/builtin/clone.c b/builtin/clone.c
index bcba080..dc2ef4f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -40,7 +40,8 @@ static const char * const builtin_clone_usage[] = {
 
 static int option_no_checkout, option_bare, option_mirror, option_single_branch = -1;
 static int option_local = -1, option_no_hardlinks, option_shared, option_recursive;
-static char *option_template, *option_depth;
+static int deepen;
+static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
 static char *option_branch = NULL;
 static const char *real_git_dir;
@@ -86,6 +87,8 @@ static struct option builtin_clone_options[] = {
 		   N_("path to git-upload-pack on the remote")),
 	OPT_STRING(0, "depth", &option_depth, N_("depth"),
 		    N_("create a shallow clone of that depth")),
+	OPT_STRING(0, "shallow-since", &option_since, N_("time"),
+		    N_("create a shallow clone since a specific time")),
 	OPT_BOOL(0, "single-branch", &option_single_branch,
 		    N_("clone only one branch, HEAD or --branch")),
 	OPT_STRING(0, "separate-git-dir", &real_git_dir, N_("gitdir"),
@@ -849,8 +852,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		usage_msg_opt(_("You must specify a repository to clone."),
 			builtin_clone_usage, builtin_clone_options);
 
+	if (option_depth || option_since)
+		deepen = 1;
 	if (option_single_branch == -1)
-		option_single_branch = option_depth ? 1 : 0;
+		option_single_branch = deepen ? 1 : 0;
 
 	if (option_mirror)
 		option_bare = 1;
@@ -976,6 +981,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (is_local) {
 		if (option_depth)
 			warning(_("--depth is ignored in local clones; use file:// instead."));
+		if (option_since)
+			warning(_("--shallow-since is ignored in local clones; use file:// instead."));
 		if (!access(mkpath("%s/shallow", path), F_OK)) {
 			if (option_local > 0)
 				warning(_("source repository is shallow, ignoring --local"));
@@ -994,6 +1001,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_depth)
 		transport_set_option(transport, TRANS_OPT_DEPTH,
 				     option_depth);
+	if (option_since)
+		transport_set_option(transport, TRANS_OPT_DEEPEN_SINCE,
+				     option_since);
 	if (option_single_branch)
 		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
 
@@ -1001,7 +1011,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK,
 				     option_upload_pack);
 
-	if (transport->smart_options && !option_depth)
+	if (transport->smart_options && !deepen)
 		transport->smart_options->check_self_contained_and_connected = 1;
 
 	refs = transport_get_remote_refs(transport);
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 17/25] clone: define shallow clone boundary based on time with --shallow-since

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

It is kind of surprising that 16 & 17 can be so simple and does not
have to update the way the cut-off points at the client side are
computed or recorded.  We must have done something right when we
designed the initial "--depth" support ;-).

On the other hand, that probably means we have the same "we clone
once, wait for a while and then do a shallow fetch with too short a
history span--the objects in the original clone all go too stale
that they become invisible in the resulting history" property (I
would not call that "an issue") as before.  It is just the way the
shallow boundary is specified got more user friendly (from a number
of parent-child hops to timespan).

Let's keep reading...

>  Documentation/git-clone.txt |  3 +++
>  builtin/clone.c             | 16 +++++++++++++---
>  2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index 789b668..1b6b639 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -194,6 +194,9 @@ objects from the source repository into a pack in the cloned repository.
>  	`--no-single-branch` is given to fetch the histories near the
>  	tips of all branches.
>  
> +--shallow-since=<date>::
> +	Create a shallow clone with a history after the specified time.
> +
>  --[no-]single-branch::
>  	Clone only the history leading to the tip of a single branch,
>  	either specified by the `--branch` option or the primary
> diff --git a/builtin/clone.c b/builtin/clone.c
> index bcba080..dc2ef4f 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -40,7 +40,8 @@ static const char * const builtin_clone_usage[] = {
>  
>  static int option_no_checkout, option_bare, option_mirror, option_single_branch = -1;
>  static int option_local = -1, option_no_hardlinks, option_shared, option_recursive;
> -static char *option_template, *option_depth;
> +static int deepen;
> +static char *option_template, *option_depth, *option_since;
>  static char *option_origin = NULL;
>  static char *option_branch = NULL;
>  static const char *real_git_dir;
> @@ -86,6 +87,8 @@ static struct option builtin_clone_options[] = {
>  		   N_("path to git-upload-pack on the remote")),
>  	OPT_STRING(0, "depth", &option_depth, N_("depth"),
>  		    N_("create a shallow clone of that depth")),
> +	OPT_STRING(0, "shallow-since", &option_since, N_("time"),
> +		    N_("create a shallow clone since a specific time")),
>  	OPT_BOOL(0, "single-branch", &option_single_branch,
>  		    N_("clone only one branch, HEAD or --branch")),
>  	OPT_STRING(0, "separate-git-dir", &real_git_dir, N_("gitdir"),
> @@ -849,8 +852,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		usage_msg_opt(_("You must specify a repository to clone."),
>  			builtin_clone_usage, builtin_clone_options);
>  
> +	if (option_depth || option_since)
> +		deepen = 1;
>  	if (option_single_branch == -1)
> -		option_single_branch = option_depth ? 1 : 0;
> +		option_single_branch = deepen ? 1 : 0;
>  
>  	if (option_mirror)
>  		option_bare = 1;
> @@ -976,6 +981,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	if (is_local) {
>  		if (option_depth)
>  			warning(_("--depth is ignored in local clones; use file:// instead."));
> +		if (option_since)
> +			warning(_("--shallow-since is ignored in local clones; use file:// instead."));
>  		if (!access(mkpath("%s/shallow", path), F_OK)) {
>  			if (option_local > 0)
>  				warning(_("source repository is shallow, ignoring --local"));
> @@ -994,6 +1001,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	if (option_depth)
>  		transport_set_option(transport, TRANS_OPT_DEPTH,
>  				     option_depth);
> +	if (option_since)
> +		transport_set_option(transport, TRANS_OPT_DEEPEN_SINCE,
> +				     option_since);
>  	if (option_single_branch)
>  		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
>  
> @@ -1001,7 +1011,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		transport_set_option(transport, TRANS_OPT_UPLOADPACK,
>  				     option_upload_pack);
>  
> -	if (transport->smart_options && !option_depth)
> +	if (transport->smart_options && !deepen)
>  		transport->smart_options->check_self_contained_and_connected = 1;
>  
>  	refs = transport_get_remote_refs(transport);

[PATCH v2 18/25] t5500, t5539: tests for shallow depth since a specific date

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t5500-fetch-pack.sh         | 22 ++++++++++++++++++++++
 t/t5539-fetch-http-shallow.sh | 24 ++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index e5f83bf..453c571 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -637,4 +637,26 @@ test_expect_success MINGW 'fetch-pack --diag-url c:repo' '
 	check_prot_path c:repo file c:repo
 '
 
+test_expect_success 'clone shallow since ...' '
+	test_create_repo shallow-since &&
+	(
+	cd shallow-since &&
+	GIT_COMMITTER_DATE="100000000 +0700" git commit --allow-empty -m one &&
+	GIT_COMMITTER_DATE="200000000 +0700" git commit --allow-empty -m two &&
+	GIT_COMMITTER_DATE="300000000 +0700" git commit --allow-empty -m three &&
+	git clone --shallow-since "300000000 +0700" "file://$(pwd)/." ../shallow11 &&
+	git -C ../shallow11 log --pretty=tformat:%s HEAD >actual &&
+	echo three >expected &&
+	test_cmp expected actual
+	)
+'
+
+test_expect_success 'fetch shallow since ...' '
+	git -C shallow11 fetch --shallow-since "200000000 +0700" origin &&
+	git -C shallow11 log --pretty=tformat:%s origin/master >actual &&
+	echo three >expected &&
+	echo two  >>expected &&
+	test_cmp expected actual
+'
+
 test_done
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 37a4335..2a96ab3 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -73,5 +73,29 @@ test_expect_success 'no shallow lines after receiving ACK ready' '
 	)
 '
 
+test_expect_success 'clone shallow since ...' '
+	test_create_repo shallow-since &&
+	(
+	cd shallow-since &&
+	GIT_COMMITTER_DATE="100000000 +0700" git commit --allow-empty -m one &&
+	GIT_COMMITTER_DATE="200000000 +0700" git commit --allow-empty -m two &&
+	GIT_COMMITTER_DATE="300000000 +0700" git commit --allow-empty -m three &&
+	mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-since.git" &&
+	git clone --shallow-since "300000000 +0700" $HTTPD_URL/smart/shallow-since.git ../shallow11 &&
+	git -C ../shallow11 log --pretty=tformat:%s HEAD >actual &&
+	echo three >expected &&
+	test_cmp expected actual
+	)
+'
+
+test_expect_success 'fetch shallow since ...' '
+	git -C shallow11 fetch --shallow-since "200000000 +0700" origin &&
+	git -C shallow11 log --pretty=tformat:%s origin/master >actual &&
+	echo three >expected &&
+	echo two  >>expected &&
+	test_cmp expected actual
+'
+
+
 stop_httpd
 test_done
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 18/25] t5500, t5539: tests for shallow depth since a specific date

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> +	cd shallow-since &&
> +	GIT_COMMITTER_DATE="100000000 +0700" git commit --allow-empty -m one &&
> +	GIT_COMMITTER_DATE="200000000 +0700" git commit --allow-empty -m two &&
> +	GIT_COMMITTER_DATE="300000000 +0700" git commit --allow-empty -m three &&

Didn't --date="@10000000 +0700" work?

Not a complaint but genuinely curious.

> +	git clone --shallow-since "300000000 +0700" "file://$(pwd)/." ../shallow11 &&

Are we required to add TZ to --shallow-since, or we merely tolerate
if there is one (I am hoping that it is the latter)?

> +	git -C ../shallow11 log --pretty=tformat:%s HEAD >actual &&
> +	echo three >expected &&
> +	test_cmp expected actual
> +	)
> +'
> +
> +test_expect_success 'fetch shallow since ...' '
> +	git -C shallow11 fetch --shallow-since "200000000 +0700" origin &&
> +	git -C shallow11 log --pretty=tformat:%s origin/master >actual &&
> +	echo three >expected &&
> +	echo two  >>expected &&

test_write_lines perhaps?

> +	test_cmp expected actual
> +'

Re: [PATCH v2 18/25] t5500, t5539: tests for shallow depth since a specific date

[Duy Nguyen]

On Tue, Feb 9, 2016 at 4:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> +     cd shallow-since &&
>> +     GIT_COMMITTER_DATE="100000000 +0700" git commit --allow-empty -m one &&
>> +     GIT_COMMITTER_DATE="200000000 +0700" git commit --allow-empty -m two &&
>> +     GIT_COMMITTER_DATE="300000000 +0700" git commit --allow-empty -m three &&
>
> Didn't --date="@10000000 +0700" work?
>
> Not a complaint but genuinely curious.

Nope. --date sets author date, but rev-list --since only cares about
committer date.

>> +     git clone --shallow-since "300000000 +0700" "file://$(pwd)/." ../shallow11 &&
>
> Are we required to add TZ to --shallow-since, or we merely tolerate
> if there is one (I am hoping that it is the latter)?

Hmm.. I'm not familiar with date parsing code. I was sure that the
parsing here (to timestamp) is the same as "rev-list --since" done on
the server side. The function used is approxidate() which seems to
ignore timezone.

>> +     git -C ../shallow11 log --pretty=tformat:%s HEAD >actual &&
>> +     echo three >expected &&
>> +     test_cmp expected actual
>> +     )
>> +'
>> +
>> +test_expect_success 'fetch shallow since ...' '
>> +     git -C shallow11 fetch --shallow-since "200000000 +0700" origin &&
>> +     git -C shallow11 log --pretty=tformat:%s origin/master >actual &&
>> +     echo three >expected &&
>> +     echo two  >>expected &&
>
> test_write_lines perhaps?

I was lazy. probably could go with cat <<EOF.
-- 
Duy

[PATCH v2 19/25] refs: add expand_ref()

[Nguyễn Thái Ngọc Duy]

This is basically dwim_ref() without @{} support. To be used on the
server side where we want to expand abbreviated to full ref names and
nothing else.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 8 +++++++-
 refs.h | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index e2d34b2..842e4d8 100644
--- a/refs.c
+++ b/refs.c
@@ -392,6 +392,13 @@ static char *substitute_branch_name(const char **string, int *len)
 int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 {
 	char *last_branch = substitute_branch_name(&str, &len);
+	int   refs_found  = expand_ref(str, len, sha1, ref);
+	free(last_branch);
+	return refs_found;
+}
+
+int expand_ref(const char *str, int len, unsigned char *sha1, char **ref)
+{
 	const char **p, *r;
 	int refs_found = 0;
 
@@ -417,7 +424,6 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 			warning("ignoring broken ref %s.", fullref);
 		}
 	}
-	free(last_branch);
 	return refs_found;
 }
 
diff --git a/refs.h b/refs.h
index 3c3da29..31a2fa6 100644
--- a/refs.h
+++ b/refs.h
@@ -90,6 +90,7 @@ extern int resolve_gitlink_ref(const char *path, const char *refname, unsigned c
  */
 extern int refname_match(const char *abbrev_name, const char *full_name);
 
+extern int expand_ref(const char *str, int len, unsigned char *sha1, char **ref);
 extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
 extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
 
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 19/25] refs: add expand_ref()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This is basically dwim_ref() without @{} support. To be used on the
> server side where we want to expand abbreviated to full ref names and
> nothing else.

It is unclear why we want to have such an expansion on the server,
though.  That is something this commit needs to justify, isn't it?

>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  refs.c | 8 +++++++-
>  refs.h | 1 +
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index e2d34b2..842e4d8 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -392,6 +392,13 @@ static char *substitute_branch_name(const char **string, int *len)
>  int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
>  {
>  	char *last_branch = substitute_branch_name(&str, &len);
> +	int   refs_found  = expand_ref(str, len, sha1, ref);
> +	free(last_branch);
> +	return refs_found;
> +}
> +
> +int expand_ref(const char *str, int len, unsigned char *sha1, char **ref)
> +{
>  	const char **p, *r;
>  	int refs_found = 0;
>  
> @@ -417,7 +424,6 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
>  			warning("ignoring broken ref %s.", fullref);
>  		}
>  	}
> -	free(last_branch);
>  	return refs_found;
>  }
>  
> diff --git a/refs.h b/refs.h
> index 3c3da29..31a2fa6 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -90,6 +90,7 @@ extern int resolve_gitlink_ref(const char *path, const char *refname, unsigned c
>   */
>  extern int refname_match(const char *abbrev_name, const char *full_name);
>  
> +extern int expand_ref(const char *str, int len, unsigned char *sha1, char **ref);
>  extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
>  extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);

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

[Nguyễn Thái Ngọc Duy]

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. 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>
---
 Documentation/technical/pack-protocol.txt         |  3 ++-
 Documentation/technical/protocol-capabilities.txt |  9 +++++++++
 upload-pack.c                                     | 22 ++++++++++++++++++++--
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 9251df1..dee33a6 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -220,7 +220,8 @@ out of what the server said it could do with the first 'want' line.
   shallow-line      =  PKT-LINE("shallow" SP obj-id)
 
   depth-request     =  PKT-LINE("deepen" SP depth) /
-		       PKT-LINE("deepen-since" SP timestamp)
+		       PKT-LINE("deepen-since" SP timestamp) /
+		       PKT-LINE("deepen-not" SP ref)
 
   first-want        =  PKT-LINE("want" SP obj-id SP capability-list)
   additional-want   =  PKT-LINE("want" SP obj-id)
diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index f08cc4e..0e6b57d 100644
--- 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
 "rev-list --max-age=<timestamp>" on the server side. "deepen-since"
 cannot be used with "deepen".
 
+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".
+
 no-progress
 -----------
 
diff --git a/upload-pack.c b/upload-pack.c
index 794736c..a72ffc2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -628,6 +628,7 @@ static void deepen_by_rev_list(int ac, const char **av,
 static void receive_needs(void)
 {
 	struct object_array shallows = OBJECT_ARRAY_INIT;
+	struct string_list deepen_not = STRING_LIST_INIT_DUP;
 	int depth = 0;
 	int has_non_tip = 0;
 	unsigned long deepen_since = 0;
@@ -678,6 +679,16 @@ static void receive_needs(void)
 			deepen_rev_list = 1;
 			continue;
 		}
+		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);
+			string_list_append(&deepen_not, ref);
+			free(ref);
+			deepen_rev_list = 1;
+			continue;
+		}
 		if (!skip_prefix(line, "want ", &arg) ||
 		    get_sha1_hex(arg, sha1_buf))
 			die("git upload-pack: protocol error, "
@@ -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");
 	if (depth > 0)
 		deepen(depth, &shallows);
 	else if (deepen_rev_list) {
@@ -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);
+			}
+		}
 		deepen_by_rev_list(av.argc, av.argv, &shallows);
 		argv_array_clear(&av);
 	}
@@ -797,7 +815,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
 		    int flag, void *cb_data)
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
-		" side-band-64k ofs-delta shallow deepen-since no-progress"
+		" side-band-64k ofs-delta shallow deepen-since deepen-not no-progress"
 		" include-tag multi_ack_detailed";
 	const char *refname_nons = strip_namespace(refname);
 	struct object_id peeled;
-- 
2.7.0.377.g4cd97dd

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

[Eric Sunshine]

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

> 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.

> 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);
>         }

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> 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);
>>         }

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

[Eric Sunshine]

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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) {

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

[Duy Nguyen]

On Fri, Feb 5, 2016 at 12:05 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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

[PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/fetch-options.txt     |  5 +++++
 Documentation/git-fetch-pack.txt    |  5 +++++
 Documentation/gitremote-helpers.txt |  4 ++++
 builtin/fetch-pack.c                | 10 ++++++++++
 builtin/fetch.c                     | 17 ++++++++++++++++-
 fetch-pack.c                        | 15 ++++++++++++++-
 fetch-pack.h                        |  1 +
 remote-curl.c                       |  9 +++++++++
 transport-helper.c                  | 27 +++++++++++++++++++++++++++
 transport.c                         |  4 ++++
 transport.h                         |  6 ++++++
 11 files changed, 101 insertions(+), 2 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 8738d3d..7aa1285 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -18,6 +18,11 @@
 	Deepen or shorten the history of a shallow repository to
 	include all reachable commits after <date>.
 
+--shallow-exclude=<revision>::
+	Deepen or shorten the history of a shallow repository to
+	exclude commits reachable from a specified remote branch or tag.
+	This option can be specified multiple times.
+
 --unshallow::
 	If the source repository is complete, convert a shallow
 	repository to a complete one, removing all the limitations
diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 99e6257..4d15b04 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -91,6 +91,11 @@ be in a separate packet, and the list must end with a flush packet.
 	Deepen or shorten the history of a shallow'repository to
 	include all reachable commits after <date>.
 
+--shallow-exclude=<revision>::
+	Deepen or shorten the history of a shallow repository to
+	exclude commits reachable from a specified remote branch or tag.
+	This option can be specified multiple times.
+
 --no-progress::
 	Do not show the progress.
 
diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index 9971d9a..75bb638 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -418,6 +418,10 @@ set by Git if the remote helper has the 'option' capability.
 'option deepen-since <timestamp>::
 	Deepens the history of a shallow repository based on time.
 
+'option deepen-not <ref>::
+	Deepens the history of a shallow repository excluding ref.
+	Multiple options add up.
+
 'option followtags' {'true'|'false'}::
 	If enabled the helper should automatically fetch annotated
 	tag objects if the object the tag points at was transferred
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 192c1ae..1a49184 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -109,6 +109,16 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			args.deepen_since = xstrdup(value);
 			continue;
 		}
+		if (skip_prefix(arg, "--shallow-exclude=", &value)) {
+			static struct string_list *deepen_not;
+			if (!deepen_not) {
+				deepen_not = xmalloc(sizeof(*deepen_not));
+				string_list_init(deepen_not, 1);
+				args.deepen_not = deepen_not;
+			}
+			string_list_append(deepen_not, value);
+			continue;
+		}
 		if (!strcmp("--no-progress", arg)) {
 			args.no_progress = 1;
 			continue;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7d4d082..11b444b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -41,6 +41,7 @@ static int max_children = 1;
 static const char *depth;
 static const char *deepen_since;
 static const char *upload_pack;
+struct string_list deepen_not = STRING_LIST_INIT_NODUP;
 static struct strbuf default_rla = STRBUF_INIT;
 static struct transport *gtransport;
 static struct transport *gsecondary;
@@ -50,6 +51,13 @@ static int shown_url = 0;
 static int refmap_alloc, refmap_nr;
 static const char **refmap_array;
 
+static int option_parse_deepen_not(const struct option *opt,
+				   const char *arg, int unset)
+{
+	string_list_append(&deepen_not, arg);
+	return 0;
+}
+
 static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
 {
@@ -118,6 +126,9 @@ static struct option builtin_fetch_options[] = {
 		   N_("deepen history of shallow clone")),
 	OPT_STRING(0, "shallow-since", &deepen_since, N_("time"),
 		   N_("deepen history of shallow repository based on time")),
+	{ OPTION_CALLBACK, 0, "shallow-exclude", NULL, N_("revision"),
+		    N_("deepen history of shallow clone by excluding rev"),
+		    PARSE_OPT_NONEG, option_parse_deepen_not },
 	{ OPTION_SET_INT, 0, "unshallow", &unshallow, NULL,
 		   N_("convert to a complete repository"),
 		   PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },
@@ -875,6 +886,9 @@ static struct transport *prepare_transport(struct remote *remote)
 		set_option(transport, TRANS_OPT_DEPTH, depth);
 	if (deepen_since)
 		set_option(transport, TRANS_OPT_DEEPEN_SINCE, deepen_since);
+	if (deepen_not.nr)
+		set_option(transport, TRANS_OPT_DEEPEN_NOT,
+			   (const char *)&deepen_not);
 	if (update_shallow)
 		set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
 	return transport;
@@ -890,6 +904,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_SINCE, NULL);
+	transport_set_option(transport, TRANS_OPT_DEEPEN_NOT, NULL);
 	fetch_refs(transport, ref_map);
 
 	if (gsecondary) {
@@ -1173,7 +1188,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	/* no need to be strict, transport_set_option() will validate it again */
 	if (depth && atoi(depth) < 1)
 		die(_("depth %s is not a positive number"), depth);
-	if (depth || deepen_since)
+	if (depth || deepen_since || deepen_not.nr)
 		deepen = 1;
 
 	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
diff --git a/fetch-pack.c b/fetch-pack.c
index db23998..b3f1775 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -22,6 +22,7 @@ static int unpack_limit = 100;
 static int prefer_ofs_delta = 1;
 static int no_done;
 static int deepen_since_ok;
+static int deepen_not_ok;
 static int fetch_fsck_objects = -1;
 static int transfer_fsck_objects = -1;
 static int agent_supported;
@@ -328,6 +329,7 @@ static int find_common(struct fetch_pack_args *args,
 			if (args->include_tag)   strbuf_addstr(&c, " include-tag");
 			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
 			if (deepen_since_ok)    strbuf_addstr(&c, " deepen-since");
+			if (deepen_not_ok)      strbuf_addstr(&c, " deepen-not");
 			if (agent_supported)    strbuf_addf(&c, " agent=%s",
 							    git_user_agent_sanitized());
 			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
@@ -351,6 +353,13 @@ static int find_common(struct fetch_pack_args *args,
 		unsigned long max_age = approxidate(args->deepen_since);
 		packet_buf_write(&req_buf, "deepen-since %lu", max_age);
 	}
+	if (args->deepen_not) {
+		int i;
+		for (i = 0; i < args->deepen_not->nr; i++) {
+			struct string_list_item *s = args->deepen_not->items + i;
+			packet_buf_write(&req_buf, "deepen-not %s", s->string);
+		}
+	}
 	packet_buf_flush(&req_buf);
 	state_len = req_buf.len;
 
@@ -819,7 +828,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 
 	if ((args->depth > 0 || is_repository_shallow()) && !server_supports("shallow"))
 		die("Server does not support shallow clients");
-	if (args->depth > 0 || args->deepen_since)
+	if (args->depth > 0 || args->deepen_since || args->deepen_not)
 		args->deepen = 1;
 	if (server_supports("multi_ack_detailed")) {
 		print_verbose(args, "Server supports multi_ack_detailed");
@@ -871,6 +880,10 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		deepen_since_ok = 1;
 	else if (args->deepen_since)
 		die("Server does not support --shallow-since");
+	if (server_supports("deepen-not"))
+		deepen_not_ok = 1;
+	else if (args->deepen_not)
+		die("Server does not support --shallow-exclude");
 
 	if (everything_local(args, &ref, sought, nr_sought)) {
 		packet_flush(fd[1]);
diff --git a/fetch-pack.h b/fetch-pack.h
index f7eadb2..144301f 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -11,6 +11,7 @@ struct fetch_pack_args {
 	int unpacklimit;
 	int depth;
 	const char *deepen_since;
+	const struct string_list *deepen_not;
 	unsigned quiet:1;
 	unsigned keep_pack:1;
 	unsigned lock_pack:1;
diff --git a/remote-curl.c b/remote-curl.c
index ce30961..fd80f7e 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -21,6 +21,7 @@ struct options {
 	int verbosity;
 	unsigned long depth;
 	char *deepen_since;
+	struct string_list deepen_not;
 	unsigned progress : 1,
 		check_self_contained_and_connected : 1,
 		cloning : 1,
@@ -65,6 +66,10 @@ static int set_option(const char *name, const char *value)
 		options.deepen_since = xstrdup(value);
 		return 0;
 	}
+	else if (!strcmp(name, "deepen-not")) {
+		string_list_append(&options.deepen_not, value);
+		return 0;
+	}
 	else if (!strcmp(name, "followtags")) {
 		if (!strcmp(value, "true"))
 			options.followtags = 1;
@@ -757,6 +762,9 @@ static int fetch_git(struct discovery *heads,
 		argv_array_pushf(&args, "--depth=%lu", options.depth);
 	if (options.deepen_since)
 		argv_array_pushf(&args, "--shallow-since=%s", options.deepen_since);
+	for (i = 0; i < options.deepen_not.nr; i++)
+		argv_array_pushf(&args, "--shallow-exclude=%s",
+				 options.deepen_not.items[i].string);
 	argv_array_push(&args, url.buf);
 
 	for (i = 0; i < nr_heads; i++) {
@@ -978,6 +986,7 @@ int main(int argc, const char **argv)
 	options.verbosity = 1;
 	options.progress = !!isatty(2);
 	options.thin = 1;
+	string_list_init(&options.deepen_not, 1);
 
 	remote = remote_get(argv[1]);
 
diff --git a/transport-helper.c b/transport-helper.c
index 2e78c4d..cebb71b 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -282,6 +282,29 @@ static int strbuf_set_helper_option(struct helper_data *data,
 	return ret;
 }
 
+static int string_list_set_helper_option(struct helper_data *data,
+					 const char *name,
+					 struct string_list *list)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int i, ret = 0;
+
+	if (!list)
+		return 0;
+
+	for (i = 0; i < list->nr; i++) {
+		strbuf_addf(&buf, "option %s ", name);
+		quote_c_style(list->items[i].string, &buf, NULL, 0);
+		strbuf_addch(&buf, '\n');
+
+		if ((ret = strbuf_set_helper_option(data, &buf)))
+			break;
+		strbuf_reset(&buf);
+	}
+	strbuf_release(&buf);
+	return ret;
+}
+
 static int set_helper_option(struct transport *transport,
 			  const char *name, const char *value)
 {
@@ -294,6 +317,10 @@ static int set_helper_option(struct transport *transport,
 	if (!data->option)
 		return 1;
 
+	if (!strcmp(name, "deepen-not"))
+		return string_list_set_helper_option(data, name,
+						     (struct string_list *)value);
+
 	for (i = 0; i < ARRAY_SIZE(unsupported_options); i++) {
 		if (!strcmp(name, unsupported_options[i]))
 			return 1;
diff --git a/transport.c b/transport.c
index 4902036..3094c6b 100644
--- a/transport.c
+++ b/transport.c
@@ -480,6 +480,9 @@ static int set_git_option(struct git_transport_options *opts,
 	} else if (!strcmp(name, TRANS_OPT_DEEPEN_SINCE)) {
 		opts->deepen_since = value;
 		return 0;
+	} else if (!strcmp(name, TRANS_OPT_DEEPEN_NOT)) {
+		opts->deepen_not = (const struct string_list *)value;
+		return 0;
 	}
 	return 1;
 }
@@ -534,6 +537,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.no_progress = !transport->progress;
 	args.depth = data->options.depth;
 	args.deepen_since = data->options.deepen_since;
+	args.deepen_not = data->options.deepen_not;
 	args.check_self_contained_and_connected =
 		data->options.check_self_contained_and_connected;
 	args.cloning = transport->cloning;
diff --git a/transport.h b/transport.h
index 9c10a44..ab61932 100644
--- a/transport.h
+++ b/transport.h
@@ -5,6 +5,8 @@
 #include "run-command.h"
 #include "remote.h"
 
+struct string_list;
+
 struct git_transport_options {
 	unsigned thin : 1;
 	unsigned keep : 1;
@@ -14,6 +16,7 @@ struct git_transport_options {
 	unsigned update_shallow : 1;
 	int depth;
 	const char *deepen_since;
+	const struct string_list *deepen_not;
 	const char *uploadpack;
 	const char *receivepack;
 	struct push_cas_option *cas;
@@ -175,6 +178,9 @@ int transport_restrict_protocols(void);
 /* Limit the depth of the fetch based on time if not null */
 #define TRANS_OPT_DEEPEN_SINCE "deepen-since"
 
+/* Limit the depth of the fetch based on revs if not null */
+#define TRANS_OPT_DEEPEN_NOT "deepen-not"
+
 /* Aggressively fetch annotated tags if possible */
 #define TRANS_OPT_FOLLOWTAGS "followtags"
 
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Eric Sunshine]

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> @@ -109,6 +109,16 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
> +               if (skip_prefix(arg, "--shallow-exclude=", &value)) {
> +                       static struct string_list *deepen_not;
> +                       if (!deepen_not) {
> +                               deepen_not = xmalloc(sizeof(*deepen_not));
> +                               string_list_init(deepen_not, 1);
> +                               args.deepen_not = deepen_not;
> +                       }
> +                       string_list_append(deepen_not, value);
> +                       continue;
> +               }

Hmm, can't this be simplified to:

    if (skip_prefix(arg, "--shallow-exclude=", &value)) {
        if (!args.deepen_not) {
            args.deepen_not = xmalloc(sizeof(*args.deepen_not));
            string_list_init(args.deepen_not, 1);
        }
        string_list_append(args.deepen_not, value);
        continue;
    }

Or, perhaps even better, declare it as plain 'struct string_list
deepen_not' in struct fetch_pack_args, rather than as a pointer, and
then in cmd_fetch_pack():

    memset(&args, 0, sizeof(args));
    args.uploadpack = "git-upload-pack";
    string_list_init(&args.deepen_not, 1);

    ...

    if (skip_prefix(arg, "--shallow-exclude=", &value)) {
        string_list_append(args.deepen_not, value);
        continue;
    }

Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Duy Nguyen]

On Fri, Feb 5, 2016 at 12:26 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
>> @@ -109,6 +109,16 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>> +               if (skip_prefix(arg, "--shallow-exclude=", &value)) {
>> +                       static struct string_list *deepen_not;
>> +                       if (!deepen_not) {
>> +                               deepen_not = xmalloc(sizeof(*deepen_not));
>> +                               string_list_init(deepen_not, 1);
>> +                               args.deepen_not = deepen_not;
>> +                       }
>> +                       string_list_append(deepen_not, value);
>> +                       continue;
>> +               }
>
> Hmm, can't this be simplified to:
>
>     if (skip_prefix(arg, "--shallow-exclude=", &value)) {
>         if (!args.deepen_not) {
>             args.deepen_not = xmalloc(sizeof(*args.deepen_not));
>             string_list_init(args.deepen_not, 1);
>         }
>         string_list_append(args.deepen_not, value);
>         continue;
>     }

args.deepen_not is const, so no, the compiler will complain at
string_list_init and string_list_append. Dropping "const" is one
option, if you prefer.

> Or, perhaps even better, declare it as plain 'struct string_list
> deepen_not' in struct fetch_pack_args, rather than as a pointer, and
> then in cmd_fetch_pack():
>
>     memset(&args, 0, sizeof(args));
>     args.uploadpack = "git-upload-pack";
>     string_list_init(&args.deepen_not, 1);

There's another place fetch_pack_args variable is declared, in
fetch_refs_via_pack(), and we would need to string_list_copy() from
transport->data->options.deepen_not and then free it afterward. So I
think it's not really worth it.

>     if (skip_prefix(arg, "--shallow-exclude=", &value)) {
>         string_list_append(args.deepen_not, value);
>         continue;
>     }
-- 
Duy

Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Eric Sunshine]

On Sun, Feb 14, 2016 at 10:53 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Fri, Feb 5, 2016 at 12:26 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
>>> @@ -109,6 +109,16 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>>> +               if (skip_prefix(arg, "--shallow-exclude=", &value)) {
>>> +                       static struct string_list *deepen_not;
>>> +                       if (!deepen_not) {
>>> +                               deepen_not = xmalloc(sizeof(*deepen_not));
>>> +                               string_list_init(deepen_not, 1);
>>> +                               args.deepen_not = deepen_not;
>>> +                       }
>>> +                       string_list_append(deepen_not, value);
>>> +                       continue;
>>> +               }
>>
>> Hmm, can't this be simplified to:
>>
>>     if (skip_prefix(arg, "--shallow-exclude=", &value)) {
>>         if (!args.deepen_not) {
>>             args.deepen_not = xmalloc(sizeof(*args.deepen_not));
>>             string_list_init(args.deepen_not, 1);
>>         }
>>         string_list_append(args.deepen_not, value);
>>         continue;
>>     }
>
> args.deepen_not is const, so no, the compiler will complain at
> string_list_init and string_list_append. Dropping "const" is one
> option, if you prefer.

Yes, dropping 'const' was implied. I didn't examine it too deeply, but
it did not appear as if there would be any major fallout from dropping
'const'. It feels a bit cleaner to keep it all self-contained than to
have that somewhat oddball static string_list*, but it's not such a
big deal that I'd insist upon a rewrite.

>> Or, perhaps even better, declare it as plain 'struct string_list
>> deepen_not' in struct fetch_pack_args, rather than as a pointer, and
>> then in cmd_fetch_pack():
>>
>>     memset(&args, 0, sizeof(args));
>>     args.uploadpack = "git-upload-pack";
>>     string_list_init(&args.deepen_not, 1);
>
> There's another place fetch_pack_args variable is declared, in
> fetch_refs_via_pack(), and we would need to string_list_copy() from
> transport->data->options.deepen_not and then free it afterward. So I
> think it's not really worth it.

Okay.

>>     if (skip_prefix(arg, "--shallow-exclude=", &value)) {
>>         string_list_append(args.deepen_not, value);
>>         continue;
>>     }

Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Eric Sunshine]

On Mon, Feb 15, 2016 at 12:52 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Feb 14, 2016 at 10:53 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Fri, Feb 5, 2016 at 12:26 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> Hmm, can't this be simplified to:
>>>
>>>     if (skip_prefix(arg, "--shallow-exclude=", &value)) {
>>>         if (!args.deepen_not) {
>>>             args.deepen_not = xmalloc(sizeof(*args.deepen_not));
>>>             string_list_init(args.deepen_not, 1);
>>>         }
>>>         string_list_append(args.deepen_not, value);
>>>         continue;
>>>     }
>>
>> args.deepen_not is const, so no, the compiler will complain at
>> string_list_init and string_list_append. Dropping "const" is one
>> option, if you prefer.
>
> Yes, dropping 'const' was implied. I didn't examine it too deeply, but
> it did not appear as if there would be any major fallout from dropping
> 'const'. It feels a bit cleaner to keep it all self-contained than to
> have that somewhat oddball static string_list*, but it's not such a
> big deal that I'd insist upon a rewrite.
>
>>> Or, perhaps even better, declare it as plain 'struct string_list
>>> deepen_not' in struct fetch_pack_args, rather than as a pointer, and
>>> then in cmd_fetch_pack():
>>>
>>>     memset(&args, 0, sizeof(args));
>>>     args.uploadpack = "git-upload-pack";
>>>     string_list_init(&args.deepen_not, 1);
>>
>> There's another place fetch_pack_args variable is declared, in
>> fetch_refs_via_pack(), and we would need to string_list_copy() from
>> transport->data->options.deepen_not and then free it afterward. So I
>> think it's not really worth it.

Upon re-reading, I suppose this also is an argument for keeping the
static string_list* rather than dropping the 'const'...(?)

Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Duy Nguyen]

On Mon, Feb 15, 2016 at 12:52:26AM -0500, Eric Sunshine wrote:
> Yes, dropping 'const' was implied. I didn't examine it too deeply, but
> it did not appear as if there would be any major fallout from dropping
> 'const'. It feels a bit cleaner to keep it all self-contained than to
> have that somewhat oddball static string_list*, but it's not such a
> big deal that I'd insist upon a rewrite.

Dropping 'const' is not a big deal. But before we do that, how about
this instead? I think the code looks better, and the compiler can
still catch invalid updates to deepen_not.

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 0402e27..07570be 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -50,6 +50,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	struct child_process *conn;
 	struct fetch_pack_args args;
 	struct sha1_array shallow = SHA1_ARRAY_INIT;
+	struct string_list deepen_not = STRING_LIST_INIT_DUP;
 
 	packet_trace_identity("fetch-pack");
 
@@ -108,6 +109,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			args.deepen_since = xstrdup(arg);
 			continue;
 		}
+		if (skip_prefix(arg, "--shallow-exclude=", &arg)) {
+			string_list_append(&deepen_not, arg);
+			continue;
+		}
 		if (!strcmp("--no-progress", arg)) {
 			args.no_progress = 1;
 			continue;
@@ -135,6 +140,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		}
 		usage(fetch_pack_usage);
 	}
+	if (deepen_not.nr)
+		args.deepen_not = &deepen_not;
 
 	if (i < argc)
 		dest = argv[i++];
--
Duy

Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Eric Sunshine]

On Mon, Feb 15, 2016 at 3:15 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Mon, Feb 15, 2016 at 12:52:26AM -0500, Eric Sunshine wrote:
>> Yes, dropping 'const' was implied. I didn't examine it too deeply, but
>> it did not appear as if there would be any major fallout from dropping
>> 'const'. It feels a bit cleaner to keep it all self-contained than to
>> have that somewhat oddball static string_list*, but it's not such a
>> big deal that I'd insist upon a rewrite.
>
> Dropping 'const' is not a big deal. But before we do that, how about
> this instead? I think the code looks better, and the compiler can
> still catch invalid updates to deepen_not.

I like this better, too.

> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 0402e27..07570be 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -50,6 +50,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>         struct child_process *conn;
>         struct fetch_pack_args args;
>         struct sha1_array shallow = SHA1_ARRAY_INIT;
> +       struct string_list deepen_not = STRING_LIST_INIT_DUP;
>
>         packet_trace_identity("fetch-pack");
>
> @@ -108,6 +109,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>                         args.deepen_since = xstrdup(arg);
>                         continue;
>                 }
> +               if (skip_prefix(arg, "--shallow-exclude=", &arg)) {
> +                       string_list_append(&deepen_not, arg);
> +                       continue;
> +               }
>                 if (!strcmp("--no-progress", arg)) {
>                         args.no_progress = 1;
>                         continue;
> @@ -135,6 +140,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>                 }
>                 usage(fetch_pack_usage);
>         }
> +       if (deepen_not.nr)
> +               args.deepen_not = &deepen_not;
>
>         if (i < argc)
>                 dest = argv[i++];
> --
> Duy

[PATCH v2 22/25] clone: define shallow clone boundary with --shallow-exclude

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-clone.txt |  5 +++++
 builtin/clone.c             | 18 +++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 1b6b639..31e1610 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -197,6 +197,11 @@ objects from the source repository into a pack in the cloned repository.
 --shallow-since=<date>::
 	Create a shallow clone with a history after the specified time.
 
+--shallow-exclude=<revision>::
+	Create a shallow clone with a history, excluding commits
+	reachable from a specified remote branch or tag.  This option
+	can be specified multiple times.
+
 --[no-]single-branch::
 	Clone only the history leading to the tip of a single branch,
 	either specified by the `--branch` option or the primary
diff --git a/builtin/clone.c b/builtin/clone.c
index dc2ef4f..5ccf6b7 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -44,6 +44,7 @@ static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
 static char *option_branch = NULL;
+static struct string_list option_not = STRING_LIST_INIT_NODUP;
 static const char *real_git_dir;
 static char *option_upload_pack = "git-upload-pack";
 static int option_verbosity;
@@ -52,6 +53,13 @@ static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
 
+static int option_parse_deepen_not(const struct option *opt,
+				   const char *arg, int unset)
+{
+	string_list_append(&option_not, arg);
+	return 0;
+}
+
 static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
 	OPT_BOOL(0, "progress", &option_progress,
@@ -89,6 +97,9 @@ static struct option builtin_clone_options[] = {
 		    N_("create a shallow clone of that depth")),
 	OPT_STRING(0, "shallow-since", &option_since, N_("time"),
 		    N_("create a shallow clone since a specific time")),
+	{ OPTION_CALLBACK, 0, "shallow-exclude", NULL, N_("revision"),
+		    N_("deepen history of shallow clone by excluding rev"),
+		    PARSE_OPT_NONEG, option_parse_deepen_not },
 	OPT_BOOL(0, "single-branch", &option_single_branch,
 		    N_("clone only one branch, HEAD or --branch")),
 	OPT_STRING(0, "separate-git-dir", &real_git_dir, N_("gitdir"),
@@ -852,7 +863,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		usage_msg_opt(_("You must specify a repository to clone."),
 			builtin_clone_usage, builtin_clone_options);
 
-	if (option_depth || option_since)
+	if (option_depth || option_since || option_not.nr)
 		deepen = 1;
 	if (option_single_branch == -1)
 		option_single_branch = deepen ? 1 : 0;
@@ -983,6 +994,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			warning(_("--depth is ignored in local clones; use file:// instead."));
 		if (option_since)
 			warning(_("--shallow-since is ignored in local clones; use file:// instead."));
+		if (option_not.nr)
+			warning(_("--shallow-exclude is ignored in local clones; use file:// instead."));
 		if (!access(mkpath("%s/shallow", path), F_OK)) {
 			if (option_local > 0)
 				warning(_("source repository is shallow, ignoring --local"));
@@ -1004,6 +1017,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_since)
 		transport_set_option(transport, TRANS_OPT_DEEPEN_SINCE,
 				     option_since);
+	if (option_not.nr)
+		transport_set_option(transport, TRANS_OPT_DEEPEN_NOT,
+				     (const char *)&option_not);
 	if (option_single_branch)
 		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
 
-- 
2.7.0.377.g4cd97dd

[PATCH v2 23/25] t5500, t5539: tests for shallow depth excluding a ref

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t5500-fetch-pack.sh         | 22 ++++++++++++++++++++++
 t/t5539-fetch-http-shallow.sh | 22 ++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 453c571..53de68c 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -659,4 +659,26 @@ test_expect_success 'fetch shallow since ...' '
 	test_cmp expected actual
 '
 
+test_expect_success 'shallow clone exclude tag two' '
+	test_create_repo shallow-exclude &&
+	(
+	cd shallow-exclude &&
+	test_commit one &&
+	test_commit two &&
+	test_commit three &&
+	git clone --shallow-exclude two "file://$(pwd)/." ../shallow12 &&
+	git -C ../shallow12 log --pretty=tformat:%s HEAD >actual &&
+	echo three >expected &&
+	test_cmp expected actual
+	)
+'
+
+test_expect_success 'fetch exclude tag one' '
+	git -C shallow12 fetch --shallow-exclude one origin &&
+	git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
+	echo three >expected &&
+	echo two  >>expected &&
+	test_cmp expected actual
+'
+
 test_done
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 2a96ab3..95eaaff 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -96,6 +96,28 @@ test_expect_success 'fetch shallow since ...' '
 	test_cmp expected actual
 '
 
+test_expect_success 'shallow clone exclude tag two' '
+	test_create_repo shallow-exclude &&
+	(
+	cd shallow-exclude &&
+	test_commit one &&
+	test_commit two &&
+	test_commit three &&
+	mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-exclude.git" &&
+	git clone --shallow-exclude two $HTTPD_URL/smart/shallow-exclude.git ../shallow12 &&
+	git -C ../shallow12 log --pretty=tformat:%s HEAD >actual &&
+	echo three >expected &&
+	test_cmp expected actual
+	)
+'
+
+test_expect_success 'fetch exclude tag one' '
+	git -C shallow12 fetch --shallow-exclude one origin &&
+	git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
+	echo three >expected &&
+	echo two  >>expected &&
+	test_cmp expected actual
+'
 
 stop_httpd
 test_done
-- 
2.7.0.377.g4cd97dd

[PATCH v2 24/25] upload-pack: make check_reachable_object() return unreachable list if asked

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 object.h      |  2 +-
 upload-pack.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/object.h b/object.h
index f8b6442..614a006 100644
--- a/object.h
+++ b/object.h
@@ -31,7 +31,7 @@ struct object_array {
  * revision.h:      0---------10                                26
  * fetch-pack.c:    0---4
  * walker.c:        0-2
- * upload-pack.c:               11----------------19
+ * upload-pack.c:       4       11----------------19
  * builtin/blame.c:               12-13
  * bisect.c:                               16
  * bundle.c:                               16
diff --git a/upload-pack.c b/upload-pack.c
index a72ffc2..1270aa3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -451,8 +451,16 @@ static int is_our_ref(struct object *o)
 			(ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));
 	return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
-
-static int check_unreachable(struct object_array *src)
+/*
+ * If reachable is NULL, return 1 if there is no unreachable object,
+ * zero otherwise.
+ *
+ * If reachable is not NULL, it's filled with reachable objects.
+ * Return value is irrelevant. The caller has to compare reachable and
+ * src to find out if there's any unreachable object.
+ */
+static int check_unreachable(struct object_array *reachable,
+			     struct object_array *src)
 {
 	static const char *argv[] = {
 		"rev-list", "--stdin", NULL,
@@ -484,6 +492,8 @@ static int check_unreachable(struct object_array *src)
 		o = get_indexed_object(--i);
 		if (!o)
 			continue;
+		if (reachable && o->type == OBJ_COMMIT)
+			o->flags &= ~TMP_MARK;
 		if (!is_our_ref(o))
 			continue;
 		memcpy(namebuf + 1, oid_to_hex(&o->oid), GIT_SHA1_HEXSZ);
@@ -493,8 +503,13 @@ static int check_unreachable(struct object_array *src)
 	namebuf[40] = '\n';
 	for (i = 0; i < src->nr; i++) {
 		o = src->objects[i].item;
-		if (is_our_ref(o))
+		if (is_our_ref(o)) {
+			if (reachable)
+				add_object_array(o, NULL, reachable);
 			continue;
+		}
+		if (reachable && o->type == OBJ_COMMIT)
+			o->flags |= TMP_MARK;
 		memcpy(namebuf, oid_to_hex(&o->oid), GIT_SHA1_HEXSZ);
 		if (write_in_full(cmd.in, namebuf, 41) < 0)
 			return 0;
@@ -507,9 +522,31 @@ static int check_unreachable(struct object_array *src)
 	 * The commits out of the rev-list are not ancestors of
 	 * our ref.
 	 */
-	i = read_in_full(cmd.out, namebuf, 1);
-	if (i)
-		return 0;
+	if (!reachable) {
+		i = read_in_full(cmd.out, namebuf, 1);
+		if (i)
+			return 0;
+	} else {
+		while ((i = read_in_full(cmd.out, namebuf, 41)) == 41) {
+			struct object_id sha1;
+
+			if (namebuf[40] != '\n' || get_oid_hex(namebuf, &sha1))
+				break;
+
+			o = lookup_object(sha1.hash);
+			if (o && o->type == OBJ_COMMIT) {
+				o->flags &= ~TMP_MARK;
+			}
+		}
+		for (i = get_max_object_index(); 0 < i; ) {
+			o = get_indexed_object(--i);
+			if (o && o->type == OBJ_COMMIT &&
+			    (o->flags & TMP_MARK)) {
+				add_object_array(o, NULL, reachable);
+				o->flags &= ~TMP_MARK;
+			}
+		}
+	}
 	close(cmd.out);
 
 	/*
@@ -535,7 +572,7 @@ static void check_non_tip(void)
 	 */
 	if (!stateless_rpc && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))
 		;		/* error */
-	else if (check_unreachable(&want_obj))
+	else if (check_unreachable(NULL, &want_obj))
 		return;
 
 	/* Pick one of them (we know there at least is one) */
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 24/25] upload-pack: make check_reachable_object() return unreachable list if asked

[Eric Sunshine]

On Thu, Feb 4, 2016 at 4:04 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/upload-pack.c b/upload-pack.c
> @@ -451,8 +451,16 @@ static int is_our_ref(struct object *o)
> -static int check_unreachable(struct object_array *src)
> +/*
> + * If reachable is NULL, return 1 if there is no unreachable object,
> + * zero otherwise.
> + *
> + * If reachable is not NULL, it's filled with reachable objects.
> + * Return value is irrelevant. The caller has to compare reachable and
> + * src to find out if there's any unreachable object.
> + */

This dual-purpose function serving up two entirely orthogonal modes
feel strange. Would it make sense to split this  into two functions,
one for each paragraph in the above documentation? (Of course, they
could share common implementation behind-the-scenes as needed.)

More below...

> +static int check_unreachable(struct object_array *reachable,
> +                            struct object_array *src)
>  {
>         static const char *argv[] = {
>                 "rev-list", "--stdin", NULL,
> @@ -507,9 +522,31 @@ static int check_unreachable(struct object_array *src)
>          * The commits out of the rev-list are not ancestors of
>          * our ref.
>          */
> -       i = read_in_full(cmd.out, namebuf, 1);
> -       if (i)
> -               return 0;
> +       if (!reachable) {
> +               i = read_in_full(cmd.out, namebuf, 1);
> +               if (i)
> +                       return 0;
> +       } else {
> +               while ((i = read_in_full(cmd.out, namebuf, 41)) == 41) {
> +                       struct object_id sha1;
> +
> +                       if (namebuf[40] != '\n' || get_oid_hex(namebuf, &sha1))
> +                               break;
> +
> +                       o = lookup_object(sha1.hash);
> +                       if (o && o->type == OBJ_COMMIT) {
> +                               o->flags &= ~TMP_MARK;
> +                       }
> +               }
> +               for (i = get_max_object_index(); 0 < i; ) {
> +                       o = get_indexed_object(--i);

Perhaps code this as:

    for (i = get_max_object_index(); 0 < i; i--) {
        o = get_indexed_object(i - 1);

in order to keep the loop control within the 'for' itself?

> +                       if (o && o->type == OBJ_COMMIT &&
> +                           (o->flags & TMP_MARK)) {
> +                               add_object_array(o, NULL, reachable);
> +                               o->flags &= ~TMP_MARK;
> +                       }
> +               }
> +       }

[PATCH v2 25/25] fetch, upload-pack: --deepen=N extends shallow boundary by N commits

[Nguyễn Thái Ngọc Duy]

In git-fetch, --depth argument is always relative with the latest
remote refs. This makes it a bit difficult to cover this use case,
where the user wants to make the shallow history, say 3 levels
deeper. It would work if remote refs have not moved yet, but nobody
can guarantee that, especially when that use case is performed a
couple months after the last clone or "git fetch --depth". Also,
modifying shallow boundary using --depth does not work well with
clones created by --since or --not.

This patch fixes that. A new argument --deepen=<N> will add <N> more (*)
parent commits to the current history regardless of where remote refs
are.

Have/Want negotiation is still respected. So if remote refs move, the
server will send two chunks: one between "have" and "want" and another
to extend shallow history. In theory, the client could send no "want"s
in order to get the second chunk only. But the protocol does not allow
that. Either you send no want lines, which means ls-remote; or you
have to send at least one want line that carries deep-relative to the
server..

The main work was done by Dongcan Jiang. I fixed it up here and there.
And of course all the bugs belong to me.

(*) We could even support --deepen=<N> where <N> is negative. In that
case we can cut some history from the shallow clone. This operation
(and --depth=<shorter depth>) does not require interaction with remote
side (and more complicated to implement as a result).

Helped-by: Duy Nguyen <pclouds@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/fetch-options.txt                   |  5 +++++
 Documentation/git-fetch-pack.txt                  |  5 +++++
 Documentation/gitremote-helpers.txt               |  4 ++++
 Documentation/technical/protocol-capabilities.txt |  7 ++++++
 builtin/fetch-pack.c                              |  4 ++++
 builtin/fetch.c                                   | 14 +++++++++++-
 fetch-pack.c                                      |  3 +++
 fetch-pack.h                                      |  1 +
 remote-curl.c                                     | 14 +++++++++++-
 t/t5500-fetch-pack.sh                             | 23 ++++++++++++++++++++
 t/t5539-fetch-http-shallow.sh                     | 26 +++++++++++++++++++++++
 transport-helper.c                                |  1 +
 transport.c                                       |  3 +++
 transport.h                                       |  4 ++++
 upload-pack.c                                     | 21 +++++++++++++-----
 15 files changed, 128 insertions(+), 7 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 7aa1285..3b91f15 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -14,6 +14,11 @@
 	linkgit:git-clone[1]), deepen or shorten the history to the specified
 	number of commits. Tags for the deepened commits are not fetched.
 
+--deepen=<depth>::
+	Similar to --depth, except it specifies the number of commits
+	from the current shallow boundary instead of from the tip of
+	each remote branch history.
+
 --shallow-since=<date>::
 	Deepen or shorten the history of a shallow repository to
 	include all reachable commits after <date>.
diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 4d15b04..c20958f 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -96,6 +96,11 @@ be in a separate packet, and the list must end with a flush packet.
 	exclude commits reachable from a specified remote branch or tag.
 	This option can be specified multiple times.
 
+--deepen-relative::
+	Argument --depth specifies the number of commits from the
+	current shallow boundary instead of from the tip of each
+	remote branch history.
+
 --no-progress::
 	Do not show the progress.
 
diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index 75bb638..6fca268 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -422,6 +422,10 @@ set by Git if the remote helper has the 'option' capability.
 	Deepens the history of a shallow repository excluding ref.
 	Multiple options add up.
 
+'option deepen-relative {'true'|'false'}::
+	Deepens the history of a shallow repository relative to
+	current boundary. Only valid when used with "option depth".
+
 'option followtags' {'true'|'false'}::
 	If enabled the helper should automatically fetch annotated
 	tag objects if the object the tag points at was transferred
diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index 0e6b57d..4fd6dcc 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -197,6 +197,13 @@ 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".
 
+deepen-relative
+---------------
+
+If this capability is requested by the client, the semantics of
+"deepen" command is changed. The "depth" argument is the depth from
+the current shallow boundary, instead of the depth from remote refs.
+
 no-progress
 -----------
 
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 1a49184..67a13af 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -119,6 +119,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			string_list_append(deepen_not, value);
 			continue;
 		}
+		if (!strcmp(arg, "--deepen-relative")) {
+			args.deepen_relative = 1;
+			continue;
+		}
 		if (!strcmp("--no-progress", arg)) {
 			args.no_progress = 1;
 			continue;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 11b444b..c5f0e9e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -34,7 +34,7 @@ static int fetch_prune_config = -1; /* unspecified */
 static int prune = -1; /* unspecified */
 #define PRUNE_BY_DEFAULT 0 /* do we prune by default? */
 
-static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;
+static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity, deepen_relative;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
 static int max_children = 1;
@@ -129,6 +129,8 @@ static struct option builtin_fetch_options[] = {
 	{ OPTION_CALLBACK, 0, "shallow-exclude", NULL, N_("revision"),
 		    N_("deepen history of shallow clone by excluding rev"),
 		    PARSE_OPT_NONEG, option_parse_deepen_not },
+	OPT_INTEGER(0, "deepen", &deepen_relative,
+		    N_("deepen history of shallow clone")),
 	{ OPTION_SET_INT, 0, "unshallow", &unshallow, NULL,
 		   N_("convert to a complete repository"),
 		   PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },
@@ -889,6 +891,8 @@ static struct transport *prepare_transport(struct remote *remote)
 	if (deepen_not.nr)
 		set_option(transport, TRANS_OPT_DEEPEN_NOT,
 			   (const char *)&deepen_not);
+	if (deepen_relative)
+		set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, "yes");
 	if (update_shallow)
 		set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
 	return transport;
@@ -905,6 +909,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_SINCE, NULL);
 	transport_set_option(transport, TRANS_OPT_DEEPEN_NOT, NULL);
+	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
 	fetch_refs(transport, ref_map);
 
 	if (gsecondary) {
@@ -1176,6 +1181,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix,
 			     builtin_fetch_options, builtin_fetch_usage, 0);
 
+	if (deepen_relative) {
+		if (deepen_relative < 0)
+			die(_("Negative depth in --deepen is not supported"));
+		if (depth)
+			die(_("--deepen and --depth are mutually exclusive"));
+		depth = xstrfmt("%d", deepen_relative);
+	}
 	if (unshallow) {
 		if (depth)
 			die(_("--depth and --unshallow cannot be used together"));
diff --git a/fetch-pack.c b/fetch-pack.c
index b3f1775..69a99e5 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -324,6 +324,7 @@ static int find_common(struct fetch_pack_args *args,
 			if (no_done)            strbuf_addstr(&c, " no-done");
 			if (use_sideband == 2)  strbuf_addstr(&c, " side-band-64k");
 			if (use_sideband == 1)  strbuf_addstr(&c, " side-band");
+			if (args->deepen_relative) strbuf_addstr(&c, " deepen-relative");
 			if (args->use_thin_pack) strbuf_addstr(&c, " thin-pack");
 			if (args->no_progress)   strbuf_addstr(&c, " no-progress");
 			if (args->include_tag)   strbuf_addstr(&c, " include-tag");
@@ -884,6 +885,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		deepen_not_ok = 1;
 	else if (args->deepen_not)
 		die("Server does not support --shallow-exclude");
+	if (!server_supports("deepen-relative") && args->deepen_relative)
+		die("Server does not support --deepen");
 
 	if (everything_local(args, &ref, sought, nr_sought)) {
 		packet_flush(fd[1]);
diff --git a/fetch-pack.h b/fetch-pack.h
index 144301f..c912e3d 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -12,6 +12,7 @@ struct fetch_pack_args {
 	int depth;
 	const char *deepen_since;
 	const struct string_list *deepen_not;
+	unsigned deepen_relative:1;
 	unsigned quiet:1;
 	unsigned keep_pack:1;
 	unsigned lock_pack:1;
diff --git a/remote-curl.c b/remote-curl.c
index fd80f7e..ea06c5d 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -30,7 +30,8 @@ struct options {
 		dry_run : 1,
 		thin : 1,
 		/* One of the SEND_PACK_PUSH_CERT_* constants. */
-		push_cert : 2;
+		push_cert : 2,
+		deepen_relative : 1;
 };
 static struct options options;
 static struct string_list cas_options = STRING_LIST_INIT_DUP;
@@ -70,6 +71,15 @@ static int set_option(const char *name, const char *value)
 		string_list_append(&options.deepen_not, value);
 		return 0;
 	}
+	else if (!strcmp(name, "deepen-relative")) {
+		if (!strcmp(value, "true"))
+			options.deepen_relative = 1;
+		else if (!strcmp(value, "false"))
+			options.deepen_relative = 0;
+		else
+			return -1;
+		return 0;
+	}
 	else if (!strcmp(name, "followtags")) {
 		if (!strcmp(value, "true"))
 			options.followtags = 1;
@@ -765,6 +775,8 @@ static int fetch_git(struct discovery *heads,
 	for (i = 0; i < options.deepen_not.nr; i++)
 		argv_array_pushf(&args, "--shallow-exclude=%s",
 				 options.deepen_not.items[i].string);
+	if (options.deepen_relative && options.depth)
+		argv_array_push(&args, "--deepen-relative");
 	argv_array_push(&args, url.buf);
 
 	for (i = 0; i < nr_heads; i++) {
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 53de68c..ed8d18c 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -681,4 +681,27 @@ test_expect_success 'fetch exclude tag one' '
 	test_cmp expected actual
 '
 
+test_expect_success 'fetching deepen' '
+	test_create_repo shallow-deepen &&
+	(
+	cd shallow-deepen &&
+	test_commit one &&
+	test_commit two &&
+	test_commit three &&
+	git clone --depth 1 "file://$(pwd)/." deepen &&
+	test_commit four &&
+	git -C deepen log --pretty=tformat:%s master >actual &&
+	echo three >expected &&
+	test_cmp expected actual &&
+	git -C deepen fetch --deepen=1 &&
+	git -C deepen log --pretty=tformat:%s origin/master >actual &&
+	cat >expected <<-\EOF &&
+	four
+	three
+	two
+	EOF
+	test_cmp expected actual
+	)
+'
+
 test_done
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 95eaaff..06389eb 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -119,5 +119,31 @@ test_expect_success 'fetch exclude tag one' '
 	test_cmp expected actual
 '
 
+test_expect_success 'fetching deepen' '
+	test_create_repo shallow-deepen &&
+	(
+	cd shallow-deepen &&
+	test_commit one &&
+	test_commit two &&
+	test_commit three &&
+	mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" &&
+	git clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&
+	mv "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" .git &&
+	test_commit four &&
+	git -C deepen log --pretty=tformat:%s master >actual &&
+	echo three >expected &&
+	test_cmp expected actual &&
+	mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" &&
+	git -C deepen fetch --deepen=1 &&
+	git -C deepen log --pretty=tformat:%s origin/master >actual &&
+	cat >expected <<-\EOF &&
+	four
+	three
+	two
+	EOF
+	test_cmp expected actual
+	)
+'
+
 stop_httpd
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index cebb71b..3c1ae6f 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -258,6 +258,7 @@ static const char *boolean_options[] = {
 	TRANS_OPT_THIN,
 	TRANS_OPT_KEEP,
 	TRANS_OPT_FOLLOWTAGS,
+	TRANS_OPT_DEEPEN_RELATIVE
 	};
 
 static int strbuf_set_helper_option(struct helper_data *data,
diff --git a/transport.c b/transport.c
index 3094c6b..2f3823a 100644
--- a/transport.c
+++ b/transport.c
@@ -482,6 +482,8 @@ static int set_git_option(struct git_transport_options *opts,
 		return 0;
 	} else if (!strcmp(name, TRANS_OPT_DEEPEN_NOT)) {
 		opts->deepen_not = (const struct string_list *)value;
+	} else if (!strcmp(name, TRANS_OPT_DEEPEN_RELATIVE)) {
+		opts->deepen_relative = !!value;
 		return 0;
 	}
 	return 1;
@@ -538,6 +540,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.depth = data->options.depth;
 	args.deepen_since = data->options.deepen_since;
 	args.deepen_not = data->options.deepen_not;
+	args.deepen_relative = data->options.deepen_relative;
 	args.check_self_contained_and_connected =
 		data->options.check_self_contained_and_connected;
 	args.cloning = transport->cloning;
diff --git a/transport.h b/transport.h
index ab61932..bdc3518 100644
--- a/transport.h
+++ b/transport.h
@@ -14,6 +14,7 @@ struct git_transport_options {
 	unsigned check_self_contained_and_connected : 1;
 	unsigned self_contained_and_connected : 1;
 	unsigned update_shallow : 1;
+	unsigned deepen_relative : 1;
 	int depth;
 	const char *deepen_since;
 	const struct string_list *deepen_not;
@@ -181,6 +182,9 @@ int transport_restrict_protocols(void);
 /* Limit the depth of the fetch based on revs if not null */
 #define TRANS_OPT_DEEPEN_NOT "deepen-not"
 
+/* Limit the deepen of the fetch if not null */
+#define TRANS_OPT_DEEPEN_RELATIVE "deepen-relative"
+
 /* Aggressively fetch annotated tags if possible */
 #define TRANS_OPT_FOLLOWTAGS "followtags"
 
diff --git a/upload-pack.c b/upload-pack.c
index 1270aa3..f05971e 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -32,6 +32,7 @@ static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<
 
 static unsigned long oldest_have;
 
+static int deepen_relative;
 static int multi_ack;
 static int no_done;
 static int use_thin_pack, use_ofs_delta, use_include_tag;
@@ -632,7 +633,8 @@ static void send_unshallow(const struct object_array *shallows)
 	}
 }
 
-static void deepen(int depth, const struct object_array *shallows)
+static void deepen(int depth, int deepen_relative,
+		   struct object_array *shallows)
 {
 	struct commit_list *result = NULL;
 	int i;
@@ -641,7 +643,14 @@ static void deepen(int depth, const struct object_array *shallows)
 			struct object *object = shallows->objects[i].item;
 			object->flags |= NOT_SHALLOW;
 		}
-	else
+	else if (deepen_relative) {
+		struct object_array reachable_shallows = OBJECT_ARRAY_INIT;
+		(void)check_unreachable(&reachable_shallows, shallows);
+		result = get_shallow_commits(&reachable_shallows,
+					     depth + 1,
+					     SHALLOW, NOT_SHALLOW);
+		object_array_clear(&reachable_shallows);
+	} else
 		result = get_shallow_commits(&want_obj, depth,
 					     SHALLOW, NOT_SHALLOW);
 	send_shallow(result);
@@ -733,6 +742,8 @@ static void receive_needs(void)
 
 		features = arg + 40;
 
+		if (parse_feature_request(features, "deepen-relative"))
+			deepen_relative = 1;
 		if (parse_feature_request(features, "multi_ack_detailed"))
 			multi_ack = 2;
 		else if (parse_feature_request(features, "multi_ack"))
@@ -782,7 +793,7 @@ static void receive_needs(void)
 	if (depth > 0 && deepen_rev_list)
 		die("--depth and --shallow-since (or --shallow-exclude) cannot be used together");
 	if (depth > 0)
-		deepen(depth, &shallows);
+		deepen(depth, deepen_relative, &shallows);
 	else if (deepen_rev_list) {
 		struct argv_array av = ARGV_ARRAY_INIT;
 		int i;
@@ -852,8 +863,8 @@ static int send_ref(const char *refname, const struct object_id *oid,
 		    int flag, void *cb_data)
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
-		" side-band-64k ofs-delta shallow deepen-since deepen-not no-progress"
-		" include-tag multi_ack_detailed";
+		" side-band-64k ofs-delta shallow deepen-since deepen-not"
+		" deepen-relative no-progress include-tag multi_ack_detailed";
 	const char *refname_nons = strip_namespace(refname);
 	struct object_id peeled;
 
-- 
2.7.0.377.g4cd97dd

Re: [PATCH v2 00/25] More flexibility in making shallow clones

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This series brings three new options to shallow clone/fetch, to let
> you specify cut point by time, or by excluding some refs, or to let
> you extend shallow boundary by <N> commits.
>
> The series is now complete. Changes since v1 [1]
>
>  - smart http support
>  - option names --not and --since are changed to --shallow-exclude
>    and --shallow-since
>  - fix the last patch per Eric's comments (the tests were totally
>    broken but I didn't realize)
>
> The meat starts since 14/25. Before that is just cleanups and stuff.
> Happy (shallowly) cloning!

Nicely done.  While I had a few "Huh?" moments, and I still feel
some changes are under-justified, it was a pleasant read overall.

How extensively have you been using this, or is this hot off the
press?

Re: [PATCH v2 00/25] More flexibility in making shallow clones

[Duy Nguyen]

On Tue, Feb 9, 2016 at 4:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> This series brings three new options to shallow clone/fetch, to let
>> you specify cut point by time, or by excluding some refs, or to let
>> you extend shallow boundary by <N> commits.
>>
>> The series is now complete. Changes since v1 [1]
>>
>>  - smart http support
>>  - option names --not and --since are changed to --shallow-exclude
>>    and --shallow-since
>>  - fix the last patch per Eric's comments (the tests were totally
>>    broken but I didn't realize)
>>
>> The meat starts since 14/25. Before that is just cleanups and stuff.
>> Happy (shallowly) cloning!
>
> Nicely done.  While I had a few "Huh?" moments, and I still feel
> some changes are under-justified, it was a pleasant read overall.

Thanks. Will resend some time later.

> How extensively have you been using this, or is this hot off the
> press?

No I have not used it at all (can't use it for real when I can't
control the server side of github ;)
-- 
Duy