threads / patch / 41326

v2, 25 partsMore flexibility in making shallow clones

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

## tl;dr

63 messages between Feb 4, 2016 and Feb 19, 2016. Diffs are folded; open one to read it.

replies: 62people: 3as markdown or json

Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · lore

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
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 remote-curl.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)
Show changes to remote-curl.c +18 −23
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
Junio C Hamano· Feb 4, 2016, 22:59 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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 ;-)
Show 20 quoted lines
> 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.
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 transport-helper.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)
Show changes to transport-helper.c +23 −14
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
Junio C Hamano· Feb 4, 2016, 23:18 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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

Show 58 quoted lines
>  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;
>  }
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 transport-helper.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to transport-helper.c +5 −1
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
Junio C Hamano· Feb 4, 2016, 23:22 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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.

Show 21 quoted lines
>  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);
Duy Nguyen· Feb 6, 2016, 09:38 UTC · re: Junio C Hamano · lore

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

On Fri, Feb 5, 2016 at 6:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> 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
Junio C Hamano· Feb 8, 2016, 20:53 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
Show 27 quoted lines
> 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.

Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to upload-pack.c +52 −47
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
Junio C Hamano· Feb 4, 2016, 23:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 5 quoted lines
> 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.
Show 120 quoted lines
>  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++)
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 upload-pack.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)
Show changes to upload-pack.c +15 −10
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
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to upload-pack.c +4 −5
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
Junio C Hamano· Feb 4, 2016, 23:32 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 6 quoted lines
> 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.

Show 31 quoted lines
>  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) {
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to upload-pack.c +26 −13
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
Junio C Hamano· Feb 4, 2016, 23:39 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 6 quoted lines
> 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.
Show 68 quoted lines
>  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);
>  }
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 upload-pack.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)
Show changes to upload-pack.c +15 −11
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
Junio C Hamano· Feb 4, 2016, 23:42 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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.

Show 81 quoted lines
>  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;
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 upload-pack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to upload-pack.c +2 −2
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
Junio C Hamano· Feb 4, 2016, 23:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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.

Show 19 quoted lines
>  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;
>  		}
Duy Nguyen· Feb 15, 2016, 03:07 UTC · re: Junio C Hamano · lore

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

On Fri, Feb 5, 2016 at 6:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
> 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
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 upload-pack.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)
Show changes to upload-pack.c +25 −19
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
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to builtin/fetch-pack.c +7 −6
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
Junio C Hamano· Feb 4, 2016, 23:56 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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.

Show 37 quoted lines
>  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)) {
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to fetch-pack.c +42 −46
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
Junio C Hamano· Feb 5, 2016, 00:02 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 6 quoted lines
> 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>
> ---
Show 25 quoted lines
>  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.
Eric Sunshine· Feb 5, 2016, 04:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 24 quoted lines
> 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"))
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +9 −6

fetch-pack.c, fetch-pack.h

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
Junio C Hamano· Feb 5, 2016, 00:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 11 quoted lines
> 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.
Show 83 quoted lines
>  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;
>  };
>  
>  /*
Eric Sunshine· Feb 5, 2016, 04:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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/
Show 9 quoted lines
> --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>
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(+)
Show changes to 2 files +87 −0

commit.h, shallow.c

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
Junio C Hamano· Feb 8, 2016, 21:09 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 12 quoted lines
> 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.

Show 25 quoted lines
> +/*
> + * 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.

Show 20 quoted lines
> +	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?

Show 25 quoted lines
> +		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)
Duy Nguyen· Feb 15, 2016, 08:00 UTC · re: Junio C Hamano · lore

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

On Tue, Feb 9, 2016 at 4:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
>> +     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?

Show 18 quoted lines
>> +             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
Duy Nguyen· Feb 19, 2016, 09:30 UTC · re: Junio C Hamano · lore

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

On Mon, Feb 08, 2016 at 01:09:24PM -0800, Junio C Hamano wrote:
Show 17 quoted lines
> 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

Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 3 files +54 −3

Documentation/technical/pack-protocol.txt, Documentation/technical/protocol-capabilities.txt, upload-pack.c

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
Junio C Hamano· Feb 8, 2016, 21:14 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 11 quoted lines
> 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.
Show 131 quoted lines
>  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;
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 10 files +54 −5

Documentation/fetch-options.txt, Documentation/git-fetch-pack.txt, Documentation/gitremote-helpers.txt, builtin/fetch-pack.c, builtin/fetch.c, fetch-pack.c, fetch-pack.h, remote-curl.c, transport.c, transport.h

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
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +16 −3

Documentation/git-clone.txt, builtin/clone.c

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
Junio C Hamano· Feb 8, 2016, 21:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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...
Show 81 quoted lines
>  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);
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(+)
Show changes to 2 files +46 −0

t/t5500-fetch-pack.sh, t/t5539-fetch-http-shallow.sh

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
Junio C Hamano· Feb 8, 2016, 21:24 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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)?

Show 11 quoted lines
> +	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
> +'
Duy Nguyen· Feb 15, 2016, 07:17 UTC · re: Junio C Hamano · lore

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

On Tue, Feb 9, 2016 at 4:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> 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.

Show 13 quoted lines
>> +     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
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +8 −1

refs.c, refs.h

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
Junio C Hamano· Feb 8, 2016, 21:27 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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?

Show 44 quoted lines
>
> 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);
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 3 files +31 −3

Documentation/technical/pack-protocol.txt, Documentation/technical/protocol-capabilities.txt, upload-pack.c

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
Eric Sunshine· Feb 5, 2016, 05:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 5 quoted lines
> This should allow the user to say "create a shallow clone of this branch
> after version <some-tag>".
>
> deepen-not cannot be used with deepen the same way deepen-since cannot
> be used with deepen.

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

    Like deepen-since, deepen-not cannot be used with deepen.
or:
    Like deepen-since, deepen-not is incompatible with deepen.
Show 16 quoted lines
> But deepen-not can be mixed with deepen-since. The
> result is exactly how you do the command "git rev-list --since=... --not
> ref".
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
> @@ -188,6 +188,15 @@ specific time, instead of depth. Internally it's equivalent of doing
> +deepen-not
> +----------
> +
> +This capability adds "deepen-not" command to fetch-pack/upload-pack
> +protocol so the client can request shallow clones that are cut at a
> +specific revision, instead of depth. Internally it's equivalent of
> +doing "rev-list --not <rev>" on the server side. "deepen-not"
> +cannot be used with "deepen", but can be used with "deepen-since".
This description, on the other hand, is easy to grasp.
Show 7 quoted lines
> diff --git a/upload-pack.c b/upload-pack.c
> @@ -678,6 +679,16 @@ static void receive_needs(void)
> +               if (skip_prefix(line, "deepen-not ", &arg)) {
> +                       char *ref = NULL;
> +                       unsigned char sha1[20];
> +                       if (expand_ref(arg, strlen(arg), sha1, &ref) != 1)
> +                               die("Ambiguous deepen-not: %s", line);

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

    die("git upload-pack: ambiguous deepen-not: %s", line);
Show 15 quoted lines
> +                       string_list_append(&deepen_not, ref);
> +                       free(ref);
> +                       deepen_rev_list = 1;
> +                       continue;
> +               }
> @@ -746,6 +757,13 @@ static void receive_needs(void)
>                         struct object *o = want_obj.objects[i].item;
>                         argv_array_push(&av, oid_to_hex(&o->oid));
>                 }
> +               if (deepen_not.nr) {
> +                       argv_array_push(&av, "--not");
> +                       for (i = 0; i < deepen_not.nr; i++) {
> +                               struct string_list_item *s = deepen_not.items + i;
> +                               argv_array_push(&av, s->string);
> +                       }

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

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

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

> +               }
>                 deepen_by_rev_list(av.argc, av.argv, &shallows);
>                 argv_array_clear(&av);
>         }
Junio C Hamano· Feb 8, 2016, 21:34 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 18 quoted lines
>> @@ -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);
>>         }
Eric Sunshine· Feb 5, 2016, 05:05 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 6 quoted lines
> @@ -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) {
Duy Nguyen· Feb 15, 2016, 03:31 UTC · re: Eric Sunshine · lore

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

On Fri, Feb 5, 2016 at 12:05 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 9 quoted lines
> 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
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 11 files +101 −2

Documentation/fetch-options.txt, Documentation/git-fetch-pack.txt, Documentation/gitremote-helpers.txt, builtin/fetch-pack.c, builtin/fetch.c, fetch-pack.c, fetch-pack.h, remote-curl.c, transport-helper.c, transport.c, transport.h

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
Eric Sunshine· Feb 5, 2016, 05:26 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 14 quoted lines
> 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;
    }
Duy Nguyen· Feb 15, 2016, 03:53 UTC · re: Eric Sunshine · lore

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

On Fri, Feb 5, 2016 at 12:26 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 26 quoted lines
> 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.

Show 7 quoted lines
> 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
Eric Sunshine· Feb 15, 2016, 05:52 UTC · re: Duy Nguyen · lore

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

On Sun, Feb 14, 2016 at 10:53 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 31 quoted lines
> 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.

Show 12 quoted lines
>> 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;
>>     }
Eric Sunshine· Feb 15, 2016, 05:56 UTC · re: Eric Sunshine · lore

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

On Mon, Feb 15, 2016 at 12:52 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 35 quoted lines
> 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'...(?)

Duy Nguyen· Feb 15, 2016, 08:15 UTC · re: Eric Sunshine · lore

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

On Mon, Feb 15, 2016 at 12:52:26AM -0500, Eric Sunshine wrote:
Show 5 quoted lines
> 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.

Show changes to builtin/fetch-pack.c +7 −1
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
Eric Sunshine· Feb 19, 2016, 01:35 UTC · re: Duy Nguyen · lore

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

On Mon, Feb 15, 2016 at 3:15 AM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 10 quoted lines
> 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.
Show 34 quoted lines
> 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
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +22 −1

Documentation/git-clone.txt, builtin/clone.c

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
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(+)
Show changes to 2 files +44 −0

t/t5500-fetch-pack.sh, t/t5539-fetch-http-shallow.sh

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
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:04 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +45 −8

object.h, upload-pack.c

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
Eric Sunshine· Feb 5, 2016, 05:41 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Thu, Feb 4, 2016 at 4:04 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 13 quoted lines
> 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...
Show 30 quoted lines
> +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?
Show 7 quoted lines
> +                       if (o && o->type == OBJ_COMMIT &&
> +                           (o->flags & TMP_MARK)) {
> +                               add_object_array(o, NULL, reachable);
> +                               o->flags &= ~TMP_MARK;
> +                       }
> +               }
> +       }
Nguyễn Thái Ngọc Duy· Feb 4, 2016, 09:04 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 15 files +128 −7

Documentation/fetch-options.txt, Documentation/git-fetch-pack.txt, Documentation/gitremote-helpers.txt, Documentation/technical/protocol-capabilities.txt, builtin/fetch-pack.c, builtin/fetch.c, fetch-pack.c, fetch-pack.h, remote-curl.c, t/t5500-fetch-pack.sh, t/t5539-fetch-http-shallow.sh, transport-helper.c, transport.c, transport.h, upload-pack.c

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
Junio C Hamano· Feb 8, 2016, 21:45 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 14 quoted lines
> 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?

Duy Nguyen· Feb 12, 2016, 00:24 UTC · re: Junio C Hamano · lore

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

On Tue, Feb 9, 2016 at 4:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> 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

← back to recent threads