[PATCH/GSoC/RFC] fetch: git fetch --deepen

[PATCH/GSoC/RFC] fetch: git fetch --deepen

[Dongcan Jiang]

This patch is just for discusstion. An option --deepen is added to
'git fetch'. When it comes to '--deepen', git should fetch N more
commits ahead the local shallow commit, where N is indicated by
'--depth=N'. [1]

e.g.

>  (upstream)
>   ---o---o---o---A---B
>
>  (you)
>                  A---B

After excuting "git fetch --depth=1 --deepen", (you) get one more
tip and it becomes

>  (you)
>              o---A---B

'--deepen' is designed to be a boolean option in this patch, which
is a little different from [1]. It's designed in this way, because
it can reuse '--depth' in the program, and just costs one more bit
in some data structure, such as fetch_pack_args,
git_transport_options.

Of course, as a patch for discussion, it remains a long way to go
before being complete.

	1) Documents should be completed.
	2) More test cases, expecially corner cases, should be added.
	3) No need to get remote refs when it comes to '--deepen' option.
	4) Validity on options combination should be checked.
	5) smart-http protocol remains to be supported. [2]

Besides, I still have one question:
What does (you) exactly mean in [1]? The local branch or the local
remote ref?

I hope you could give me some comments and suggestions on this
patch. I would like to know whether I'm off the right way.

[1] http://article.gmane.org/gmane.comp.version-control.git/212950
[2] http://article.gmane.org/gmane.comp.version-control.git/265050

Helped-by: Duy Nguyen <pclouds@gmail.com>
Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
---
 builtin/fetch.c  |  5 ++++-
 fetch-pack.c     |  3 ++-
 fetch-pack.h     |  1 +
 t/t5510-fetch.sh | 11 +++++++++++
 transport.c      |  4 ++++
 transport.h      |  4 ++++
 upload-pack.c    | 15 +++++++++++----
 7 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f951265..6861207 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -33,7 +33,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;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow;
 static const char *depth;
@@ -111,6 +111,7 @@ 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_BOOL(0, "deepen", &deepen, N_("deepen")),
 	{ OPTION_SET_INT, 0, "unshallow", &unshallow, NULL,
 		   N_("convert to a complete repository"),
 		   PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },
@@ -853,6 +854,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)
+		set_option(transport, TRANS_OPT_DEEPEN, "yes");
 	if (update_shallow)
 		set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
 	return transport;
diff --git a/fetch-pack.c b/fetch-pack.c
index 655ee64..6f4adca 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -295,6 +295,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->depth_deepen)  strbuf_addstr(&c, " depth_deepen");
 			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");
@@ -317,7 +318,7 @@ static int find_common(struct fetch_pack_args *args,
 	if (is_repository_shallow())
 		write_shallow_commits(&req_buf, 1, NULL);
 	if (args->depth > 0)
-		packet_buf_write(&req_buf, "deepen %d", args->depth);
+		packet_buf_write(&req_buf, "depth %d", args->depth);
 	packet_buf_flush(&req_buf);
 	state_len = req_buf.len;

diff --git a/fetch-pack.h b/fetch-pack.h
index bb7fd76..200ac78 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -10,6 +10,7 @@ struct fetch_pack_args {
 	const char *uploadpack;
 	int unpacklimit;
 	int depth;
+	unsigned depth_deepen:1;
 	unsigned quiet:1;
 	unsigned keep_pack:1;
 	unsigned lock_pack:1;
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index d78f320..6738006 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -708,4 +708,15 @@ test_expect_success 'fetching a one-level ref works' '
 	)
 '

+test_expect_success 'fetching deepen' '
+	git clone . deepen --depth=1 &&
+	cd deepen &&
+	git fetch .. foo --depth=1
+	git show foo
+	test_must_fail git show foo~
+	git fetch .. foo --depth=1 --deepen
+	git show foo~
+	test_must_fail git show foo~2
+'
+
 test_done
diff --git a/transport.c b/transport.c
index 0694a7c..53ee7b3 100644
--- a/transport.c
+++ b/transport.c
@@ -478,6 +478,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)) {
+		opts->depth_deepen = !!value;
+		return 0;
 	} else if (!strcmp(name, TRANS_OPT_PUSH_CERT)) {
 		opts->push_cert = !!value;
 		return 0;
@@ -534,6 +537,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.depth_deepen = data->options.depth_deepen;
 	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 18d2cf8..4f432fd 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;
 	unsigned push_cert : 1;
+	unsigned depth_deepen : 1;
 	int depth;
 	const char *uploadpack;
 	const char *receivepack;
@@ -153,6 +154,9 @@ struct transport *transport_get(struct remote *, const char *);
 /* Limit the depth of the fetch if not null */
 #define TRANS_OPT_DEPTH "depth"

+/* Limit the deepen of the fetch if not null */
+#define TRANS_OPT_DEEPEN "deepen"
+
 /* Aggressively fetch annotated tags if possible */
 #define TRANS_OPT_FOLLOWTAGS "followtags"

diff --git a/upload-pack.c b/upload-pack.c
index b531a32..8004f2f 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -31,6 +31,7 @@ static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<

 static unsigned long oldest_have;

+static int depth_deepen;
 static int multi_ack;
 static int no_done;
 static int use_thin_pack, use_ofs_delta, use_include_tag;
@@ -563,11 +564,11 @@ static void receive_needs(void)
 			}
 			continue;
 		}
-		if (starts_with(line, "deepen ")) {
+		if (starts_with(line, "depth ")) {
 			char *end;
-			depth = strtol(line + 7, &end, 0);
-			if (end == line + 7 || depth <= 0)
-				die("Invalid deepen: %s", line);
+			depth = strtol(line + 6, &end, 0);
+			if (end == line + 6 || depth <= 0)
+				die("Invalid depth: %s", line);
 			continue;
 		}
 		if (!starts_with(line, "want ") ||
@@ -577,6 +578,8 @@ static void receive_needs(void)

 		features = line + 45;

+		if (parse_feature_request(features, "depth_deepen"))
+			depth_deepen = 1;
 		if (parse_feature_request(features, "multi_ack_detailed"))
 			multi_ack = 2;
 		else if (parse_feature_request(features, "multi_ack"))
@@ -631,6 +634,10 @@ static void receive_needs(void)
 				struct object *object = shallows.objects[i].item;
 				object->flags |= NOT_SHALLOW;
 			}
+		else if (depth_deepen)
+			backup = result =
+				get_shallow_commits(&shallows, depth + 1,
+						    SHALLOW, NOT_SHALLOW);
 		else
 			backup = result =
 				get_shallow_commits(&want_obj, depth,
--
2.3.1.253.gb3fd89e

Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen

[Eric Sunshine]

On Fri, Mar 13, 2015 at 9:04 AM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
> This patch is just for discusstion. An option --deepen is added to
> 'git fetch'. When it comes to '--deepen', git should fetch N more
> commits ahead the local shallow commit, where N is indicated by
> '--depth=N'. [1]
> [...]
> Of course, as a patch for discussion, it remains a long way to go
> before being complete.
> [...]
> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
> ---
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 655ee64..6f4adca 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -295,6 +295,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->depth_deepen)  strbuf_addstr(&c, " depth_deepen");

For consistency, should this be "depth-deepen" rather than "depth_deepen"?

>                         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");
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index d78f320..6738006 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -708,4 +708,15 @@ test_expect_success 'fetching a one-level ref works' '
>         )
>  '
>
> +test_expect_success 'fetching deepen' '
> +       git clone . deepen --depth=1 &&
> +       cd deepen &&

When this tests ends, the working directory will still be 'deepen',
which will likely break tests added after this one. If you wrap the
'cd' and following statements in a subshell via '(' and ')', then the
'cd' will affect the subshell but leave the parent shell's working
directory alone, and thus not negatively impact subsequent tests.

> +       git fetch .. foo --depth=1
> +       git show foo
> +       test_must_fail git show foo~
> +       git fetch .. foo --depth=1 --deepen
> +       git show foo~
> +       test_must_fail git show foo~2

Broken &&-chain throughout.

> +'
> +
>  test_done

Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen

[Junio C Hamano]

Dongcan Jiang <dongcan.jiang@gmail.com> writes:

> This patch is just for discusstion. An option --deepen is added to
> 'git fetch'. When it comes to '--deepen', git should fetch N more
> commits ahead the local shallow commit, where N is indicated by
> '--depth=N'. [1]
>
> e.g.
>
>>  (upstream)
>>   ---o---o---o---A---B
>>
>>  (you)
>>                  A---B
>
> After excuting "git fetch --depth=1 --deepen", (you) get one more
> tip and it becomes
>
>>  (you)
>>              o---A---B
>
> '--deepen' is designed to be a boolean option in this patch, which
> is a little different from [1]. It's designed in this way, because
> it can reuse '--depth' in the program, and just costs one more bit
> in some data structure, such as fetch_pack_args,
> git_transport_options.
>
> Of course, as a patch for discussion, it remains a long way to go
> before being complete.
>
> 	1) Documents should be completed.
> 	2) More test cases, expecially corner cases, should be added.
> 	3) No need to get remote refs when it comes to '--deepen' option.
> 	4) Validity on options combination should be checked.
> 	5) smart-http protocol remains to be supported. [2]
>
> Besides, I still have one question:
> What does (you) exactly mean in [1]? The local branch or the local
> remote ref?

As this operation is not about moving _any_ refs, whether local
branches or remote-tracking branches, any ref that used to point at
commit B before you executed "fetch --deepen" would point at the
same commit after the command finishes.

The "you" does not explicitly depict any ref, because the picture is
meant to illustrate _everything_ the repository at the receiving end
of "fetch" has.  It used to have two commits, A and B (and the tree
and blob objects necessary to complete these two commits).  After
deepening the history by one commit, it then will have commit A^ and
its trees and blobs.

> diff --git a/upload-pack.c b/upload-pack.c
> index b531a32..8004f2f 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -31,6 +31,7 @@ static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<
>
>  static unsigned long oldest_have;
>
> +static int depth_deepen;
>  static int multi_ack;
>  static int no_done;
>  static int use_thin_pack, use_ofs_delta, use_include_tag;
> @@ -563,11 +564,11 @@ static void receive_needs(void)
>  			}
>  			continue;
>  		}
> -		if (starts_with(line, "deepen ")) {
> +		if (starts_with(line, "depth ")) {
>  			char *end;
> -			depth = strtol(line + 7, &end, 0);
> -			if (end == line + 7 || depth <= 0)
> -				die("Invalid deepen: %s", line);
> +			depth = strtol(line + 6, &end, 0);
> +			if (end == line + 6 || depth <= 0)
> +				die("Invalid depth: %s", line);
>  			continue;
>  		}
>  		if (!starts_with(line, "want ") ||

I do not quite understand this hunk.  What happens when this program
is talking to an existing fetch-pack that requested "deepen"?

> @@ -577,6 +578,8 @@ static void receive_needs(void)
>
>  		features = line + 45;
>
> +		if (parse_feature_request(features, "depth_deepen"))
> +			depth_deepen = 1;
>  		if (parse_feature_request(features, "multi_ack_detailed"))
>  			multi_ack = 2;
>  		else if (parse_feature_request(features, "multi_ack"))
> @@ -631,6 +634,10 @@ static void receive_needs(void)
>  				struct object *object = shallows.objects[i].item;
>  				object->flags |= NOT_SHALLOW;
>  			}
> +		else if (depth_deepen)
> +			backup = result =
> +				get_shallow_commits(&shallows, depth + 1,
> +						    SHALLOW, NOT_SHALLOW);
>  		else
>  			backup = result =
>  				get_shallow_commits(&want_obj, depth,
> --
> 2.3.1.253.gb3fd89e

Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen

[Duy Nguyen]

On Sat, Mar 14, 2015 at 12:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dongcan Jiang <dongcan.jiang@gmail.com> writes:
>> What does (you) exactly mean in [1]? The local branch or the local
>> remote ref?
>
> As this operation is not about moving _any_ refs, whether local
> branches or remote-tracking branches, any ref that used to point at
> commit B before you executed "fetch --deepen" would point at the
> same commit after the command finishes.

That would make it harder to implement "fetch --deepen" than the
version that moves refs if they are updated. And I think what Dongcan
implemented moves refs. From the user point of view, I think it's ok
with either version, so the one that's easier to implement wins.

> The "you" does not explicitly depict any ref, because the picture is
> meant to illustrate _everything_ the repository at the receiving end
> of "fetch" has.  It used to have two commits, A and B (and the tree
> and blob objects necessary to complete these two commits).  After
> deepening the history by one commit, it then will have commit A^ and
> its trees and blobs.
>
>> [1] http://article.gmane.org/gmane.comp.version-control.git/212950
-- 
Duy

Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen

[Duy Nguyen]

On Fri, Mar 13, 2015 at 8:04 PM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
> @@ -317,7 +318,7 @@ static int find_common(struct fetch_pack_args *args,
>         if (is_repository_shallow())
>                 write_shallow_commits(&req_buf, 1, NULL);
>         if (args->depth > 0)
> -               packet_buf_write(&req_buf, "deepen %d", args->depth);
> +               packet_buf_write(&req_buf, "depth %d", args->depth);
>         packet_buf_flush(&req_buf);
>         state_len = req_buf.len;

Junio already questioned about your replacing starts_with("deepen "..)
with starts_with("depth "..), this is part of that question. If you
run "make test" you should find some breakages, or we need to improve
our test suite.

I think you just need to keep the old "if (args->depth > 0) send
"depth" unchanged and add two new statements that check
args->depth_deepen and sends "depth ". BTW, I think "deepen-more" or
"deepen-relative" would be better than "depth" (*), which is a noun.
But that's a minor point at this stage.

And because --depth and --deepen are mutually exclusive, you need to
make sure that the user must not specify that. The "user" includes the
"client" from the server perspective, we need to make sure that
upload-pack barf if some client sends both "deepen" and "depth".

(*) I was about to suggest you use "deepen" for both --depth and
--deepen: --depth sends "deepen NUM" while --deepen sends "deepen
+NUM". The protocol as described in pack-protocol.txt may allow this
extension. Unfortunately the current implementation is too relaxed. We
use strtol() to parse "NUM" and it would accept the leading "+"
instead of barfing out. So old clients would mistakes --deepen as
--depth, not good. And it even accepts base 8 and 16!! We should fix
this.
-- 
Duy

Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sat, Mar 14, 2015 at 12:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Dongcan Jiang <dongcan.jiang@gmail.com> writes:
>>> What does (you) exactly mean in [1]? The local branch or the local
>>> remote ref?
>>
>> As this operation is not about moving _any_ refs, whether local
>> branches or remote-tracking branches, any ref that used to point at
>> commit B before you executed "fetch --deepen" would point at the
>> same commit after the command finishes.
>
> That would make it harder to implement "fetch --deepen" than the
> version that moves refs if they are updated.

The comment you are responding to was in the context of "What does
the illustrated 'your' history mean?  Is it a history of a single
ref (if so in which repository)?" to clarify that the example was
not fetching _new_ history (and with the RFC/POC design that was
posted, my understanding is that deepen was meant to only deepen).

The paragraph you are reacting to is not an endorsement for that
"only deepen, never advance" design. It merely is a clarification of
the explanation that was in the paragraph that follows.

> And I think what Dongcan
> implemented moves refs. From the user point of view, I think it's ok
> with either version, so the one that's easier to implement wins.
>
>> The "you" does not explicitly depict any ref, because the picture is
>> meant to illustrate _everything_ the repository at the receiving end
>> of "fetch" has.  It used to have two commits, A and B (and the tree
>> and blob objects necessary to complete these two commits).  After
>> deepening the history by one commit, it then will have commit A^ and
>> its trees and blobs.
>>
>>> [1] http://article.gmane.org/gmane.comp.version-control.git/212950

Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen

[Dongcan Jiang]

Hi Eric,

Sorry for my late response. Thank you for your suggestions! I will try
to use them in my next patch version.

Best Regards,
Dongcan

2015-03-14 3:42 GMT+08:00 Eric Sunshine <sunshine@sunshineco.com>:
> On Fri, Mar 13, 2015 at 9:04 AM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
>> This patch is just for discusstion. An option --deepen is added to
>> 'git fetch'. When it comes to '--deepen', git should fetch N more
>> commits ahead the local shallow commit, where N is indicated by
>> '--depth=N'. [1]
>> [...]
>> Of course, as a patch for discussion, it remains a long way to go
>> before being complete.
>> [...]
>> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
>> ---
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index 655ee64..6f4adca 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -295,6 +295,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->depth_deepen)  strbuf_addstr(&c, " depth_deepen");
>
> For consistency, should this be "depth-deepen" rather than "depth_deepen"?
>
>>                         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");
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index d78f320..6738006 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -708,4 +708,15 @@ test_expect_success 'fetching a one-level ref works' '
>>         )
>>  '
>>
>> +test_expect_success 'fetching deepen' '
>> +       git clone . deepen --depth=1 &&
>> +       cd deepen &&
>
> When this tests ends, the working directory will still be 'deepen',
> which will likely break tests added after this one. If you wrap the
> 'cd' and following statements in a subshell via '(' and ')', then the
> 'cd' will affect the subshell but leave the parent shell's working
> directory alone, and thus not negatively impact subsequent tests.
>
>> +       git fetch .. foo --depth=1
>> +       git show foo
>> +       test_must_fail git show foo~
>> +       git fetch .. foo --depth=1 --deepen
>> +       git show foo~
>> +       test_must_fail git show foo~2
>
> Broken &&-chain throughout.
>
>> +'
>> +
>>  test_done



-- 
江东灿（Dongcan Jiang）
Team of Search Engine & Web Mining
School of Electronic Engineering & Computer Science
Peking University, Beijing, 100871, P.R.China

Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen

[Dongcan Jiang]

Hi Junio,

Sorry for my late response.

> As this operation is not about moving _any_ refs, whether local
> branches or remote-tracking branches, any ref that used to point at
> commit B before you executed "fetch --deepen" would point at the
> same commit after the command finishes.

Thanks for clarifying the definition of "you". In this patch, it
actually would update the remote-tracking branches to the newest history.
I will keep working on it to figure out the reason.

It looks that it would be more efficient if the new history is not fetched,
as it transports less objects. Is this your main consideration on not
updating any refs?

>> -             if (starts_with(line, "deepen ")) {
>> +             if (starts_with(line, "depth ")) {
>>                       char *end;
>> -                     depth = strtol(line + 7, &end, 0);
>> -                     if (end == line + 7 || depth <= 0)
>> -                             die("Invalid deepen: %s", line);
>> +                     depth = strtol(line + 6, &end, 0);
>> +                     if (end == line + 6 || depth <= 0)
>> +                             die("Invalid depth: %s", line);
>>                       continue;
>>               }
>>               if (!starts_with(line, "want ") ||
>
> I do not quite understand this hunk.  What happens when this program
> is talking to an existing fetch-pack that requested "deepen"?

In this hunk, I actually just changed the one command name in upload-pack
service from "deepen" to "depth". I made this change because the name
"deepen" here is quite misleading, as it just means "depth". Of course,
I've changed the caller's sent pack from "deepen" to "depth" too (See below).

> @@ -317,7 +318,7 @@ static int find_common(struct fetch_pack_args *args,
>         if (is_repository_shallow())
>                 write_shallow_commits(&req_buf, 1, NULL);
>         if (args->depth > 0)
> -               packet_buf_write(&req_buf, "deepen %d", args->depth);
> +               packet_buf_write(&req_buf, "depth %d", args->depth);
>         packet_buf_flush(&req_buf);
>         state_len = req_buf.len;

If the user wants "deepen", he should send a "depth_deepen" signal as well as
"depth N". When the upload-pack service in this patch receive "depth_deepen"
and "depth N", it collects N more commits ahead of the shallow commit and send
back to the caller.

Thanks,
Dongcan

2015-03-14 13:35 GMT+08:00 Junio C Hamano <gitster@pobox.com>:
> Dongcan Jiang <dongcan.jiang@gmail.com> writes:
>
>> This patch is just for discusstion. An option --deepen is added to
>> 'git fetch'. When it comes to '--deepen', git should fetch N more
>> commits ahead the local shallow commit, where N is indicated by
>> '--depth=N'. [1]
>>
>> e.g.
>>
>>>  (upstream)
>>>   ---o---o---o---A---B
>>>
>>>  (you)
>>>                  A---B
>>
>> After excuting "git fetch --depth=1 --deepen", (you) get one more
>> tip and it becomes
>>
>>>  (you)
>>>              o---A---B
>>
>> '--deepen' is designed to be a boolean option in this patch, which
>> is a little different from [1]. It's designed in this way, because
>> it can reuse '--depth' in the program, and just costs one more bit
>> in some data structure, such as fetch_pack_args,
>> git_transport_options.
>>
>> Of course, as a patch for discussion, it remains a long way to go
>> before being complete.
>>
>>       1) Documents should be completed.
>>       2) More test cases, expecially corner cases, should be added.
>>       3) No need to get remote refs when it comes to '--deepen' option.
>>       4) Validity on options combination should be checked.
>>       5) smart-http protocol remains to be supported. [2]
>>
>> Besides, I still have one question:
>> What does (you) exactly mean in [1]? The local branch or the local
>> remote ref?
>
> As this operation is not about moving _any_ refs, whether local
> branches or remote-tracking branches, any ref that used to point at
> commit B before you executed "fetch --deepen" would point at the
> same commit after the command finishes.
>
> The "you" does not explicitly depict any ref, because the picture is
> meant to illustrate _everything_ the repository at the receiving end
> of "fetch" has.  It used to have two commits, A and B (and the tree
> and blob objects necessary to complete these two commits).  After
> deepening the history by one commit, it then will have commit A^ and
> its trees and blobs.
>
>> diff --git a/upload-pack.c b/upload-pack.c
>> index b531a32..8004f2f 100644
>> --- a/upload-pack.c
>> +++ b/upload-pack.c
>> @@ -31,6 +31,7 @@ static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<
>>
>>  static unsigned long oldest_have;
>>
>> +static int depth_deepen;
>>  static int multi_ack;
>>  static int no_done;
>>  static int use_thin_pack, use_ofs_delta, use_include_tag;
>> @@ -563,11 +564,11 @@ static void receive_needs(void)
>>                       }
>>                       continue;
>>               }
>> -             if (starts_with(line, "deepen ")) {
>> +             if (starts_with(line, "depth ")) {
>>                       char *end;
>> -                     depth = strtol(line + 7, &end, 0);
>> -                     if (end == line + 7 || depth <= 0)
>> -                             die("Invalid deepen: %s", line);
>> +                     depth = strtol(line + 6, &end, 0);
>> +                     if (end == line + 6 || depth <= 0)
>> +                             die("Invalid depth: %s", line);
>>                       continue;
>>               }
>>               if (!starts_with(line, "want ") ||
>
> I do not quite understand this hunk.  What happens when this program
> is talking to an existing fetch-pack that requested "deepen"?
>
>> @@ -577,6 +578,8 @@ static void receive_needs(void)
>>
>>               features = line + 45;
>>
>> +             if (parse_feature_request(features, "depth_deepen"))
>> +                     depth_deepen = 1;
>>               if (parse_feature_request(features, "multi_ack_detailed"))
>>                       multi_ack = 2;
>>               else if (parse_feature_request(features, "multi_ack"))
>> @@ -631,6 +634,10 @@ static void receive_needs(void)
>>                               struct object *object = shallows.objects[i].item;
>>                               object->flags |= NOT_SHALLOW;
>>                       }
>> +             else if (depth_deepen)
>> +                     backup = result =
>> +                             get_shallow_commits(&shallows, depth + 1,
>> +                                                 SHALLOW, NOT_SHALLOW);
>>               else
>>                       backup = result =
>>                               get_shallow_commits(&want_obj, depth,
>> --
>> 2.3.1.253.gb3fd89e



-- 
江东灿（Dongcan Jiang）
Team of Search Engine & Web Mining
School of Electronic Engineering & Computer Science
Peking University, Beijing, 100871, P.R.China

Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen

[Dongcan Jiang]

Hi Duy,

Sorry for my late response.

> we need to make sure that upload-pack barf if some client sends both "deepen" and
> "depth".

Actually, in my current design, when client just wants "depth", it
sends "depth N";
when it want "deepen", it sends "depth N" as well as "depth_deepen".
For the latter
case, it tells upload-pack to collect objects for "deepen N".

Thus, I'm not so sure whether upload-pack needs to check their exclusion.

> I was about to suggest you use "deepen" for both --depth and
> --deepen: --depth sends "deepen NUM" while --deepen sends "deepen
> +NUM". The protocol as described in pack-protocol.txt may allow this
> extension.

This suggestion looks neat! However, I'm afraid it may involves quite
a bit changes
as you've mentioned, which would be better to take in next stage.

Thanks,
Dongcan

2015-03-14 18:56 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:
> On Fri, Mar 13, 2015 at 8:04 PM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
>> @@ -317,7 +318,7 @@ static int find_common(struct fetch_pack_args *args,
>>         if (is_repository_shallow())
>>                 write_shallow_commits(&req_buf, 1, NULL);
>>         if (args->depth > 0)
>> -               packet_buf_write(&req_buf, "deepen %d", args->depth);
>> +               packet_buf_write(&req_buf, "depth %d", args->depth);
>>         packet_buf_flush(&req_buf);
>>         state_len = req_buf.len;
>
> Junio already questioned about your replacing starts_with("deepen "..)
> with starts_with("depth "..), this is part of that question. If you
> run "make test" you should find some breakages, or we need to improve
> our test suite.
>
> I think you just need to keep the old "if (args->depth > 0) send
> "depth" unchanged and add two new statements that check
> args->depth_deepen and sends "depth ". BTW, I think "deepen-more" or
> "deepen-relative" would be better than "depth" (*), which is a noun.
> But that's a minor point at this stage.
>
> And because --depth and --deepen are mutually exclusive, you need to
> make sure that the user must not specify that. The "user" includes the
> "client" from the server perspective, we need to make sure that
> upload-pack barf if some client sends both "deepen" and "depth".
>
> (*) I was about to suggest you use "deepen" for both --depth and
> --deepen: --depth sends "deepen NUM" while --deepen sends "deepen
> +NUM". The protocol as described in pack-protocol.txt may allow this
> extension. Unfortunately the current implementation is too relaxed. We
> use strtol() to parse "NUM" and it would accept the leading "+"
> instead of barfing out. So old clients would mistakes --deepen as
> --depth, not good. And it even accepts base 8 and 16!! We should fix
> this.
> --
> Duy



-- 
江东灿（Dongcan Jiang）
Team of Search Engine & Web Mining
School of Electronic Engineering & Computer Science
Peking University, Beijing, 100871, P.R.China

Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen

[Duy Nguyen]

On Mon, Mar 16, 2015 at 11:10 PM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
> Hi Duy,
>
> Sorry for my late response.
>
>> we need to make sure that upload-pack barf if some client sends both "deepen" and
>> "depth".
>
> Actually, in my current design, when client just wants "depth", it
> sends "depth N";
> when it want "deepen", it sends "depth N" as well as "depth_deepen".
> For the latter
> case, it tells upload-pack to collect objects for "deepen N".
>
> Thus, I'm not so sure whether upload-pack needs to check their exclusion.

C Git is not the only client that can talk to upload-pack. A buggy
client may send both. The least we need is make sure it would not
crash or break the server security in any way. But if we have to
consider that, we may as well reject with a clear message, which would
help the client writer. And speaking of clients..

On Mon, Mar 16, 2015 at 11:08 PM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
>>> -             if (starts_with(line, "deepen ")) {
>>> +             if (starts_with(line, "depth ")) {
>>>                       char *end;
>>> -                     depth = strtol(line + 7, &end, 0);
>>> -                     if (end == line + 7 || depth <= 0)
>>> -                             die("Invalid deepen: %s", line);
>>> +                     depth = strtol(line + 6, &end, 0);
>>> +                     if (end == line + 6 || depth <= 0)
>>> +                             die("Invalid depth: %s", line);
>>>                       continue;
>>>               }
>>>               if (!starts_with(line, "want ") ||
>>
>> I do not quite understand this hunk.  What happens when this program
>> is talking to an existing fetch-pack that requested "deepen"?
>
> In this hunk, I actually just changed the one command name in upload-pack
> service from "deepen" to "depth". I made this change because the name
> "deepen" here is quite misleading, as it just means "depth". Of course,
> I've changed the caller's sent pack from "deepen" to "depth" too (See below).

You missed Junio's keyword "existing". Your new client will work well
with your new server. But it breaks "git clone --depth" (or "git fetch
--depth") for all existing git installations.
-- 
Duy

Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen

[Dongcan Jiang]

> C Git is not the only client that can talk to upload-pack. A buggy
> client may send both. The least we need is make sure it would not
> crash or break the server security in any way. But if we have to
> consider that, we may as well reject with a clear message, which would
> help the client writer. And speaking of clients..

Actually, I mean that the upload-pack in this patch will not crash,
whatever it receives.
e.g. "depth N", "depth_deepen", "depth N"+"depth_deepen"

Because "depth_deepen" is just a boolean signal to tell the upload-pack
that "depth N" means "deepen N", it takes effect only when "depth N"
is given. Otherwise, "depth_deepen" will be ignored.


> You missed Junio's keyword "existing". Your new client will work well
> with your new server. But it breaks "git clone --depth" (or "git fetch
> --depth") for all existing git installations.

Oh, thank you for pointing out this. And sorry Junio. I misunderstood
what you said. I haven't realized the problem of existing git server.
You've reminded me of the importance of compatibility. Thank you!

2015-03-17 18:44 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:
> On Mon, Mar 16, 2015 at 11:10 PM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
>> Hi Duy,
>>
>> Sorry for my late response.
>>
>>> we need to make sure that upload-pack barf if some client sends both "deepen" and
>>> "depth".
>>
>> Actually, in my current design, when client just wants "depth", it
>> sends "depth N";
>> when it want "deepen", it sends "depth N" as well as "depth_deepen".
>> For the latter
>> case, it tells upload-pack to collect objects for "deepen N".
>>
>> Thus, I'm not so sure whether upload-pack needs to check their exclusion.
>
> C Git is not the only client that can talk to upload-pack. A buggy
> client may send both. The least we need is make sure it would not
> crash or break the server security in any way. But if we have to
> consider that, we may as well reject with a clear message, which would
> help the client writer. And speaking of clients..
>
> On Mon, Mar 16, 2015 at 11:08 PM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
>>>> -             if (starts_with(line, "deepen ")) {
>>>> +             if (starts_with(line, "depth ")) {
>>>>                       char *end;
>>>> -                     depth = strtol(line + 7, &end, 0);
>>>> -                     if (end == line + 7 || depth <= 0)
>>>> -                             die("Invalid deepen: %s", line);
>>>> +                     depth = strtol(line + 6, &end, 0);
>>>> +                     if (end == line + 6 || depth <= 0)
>>>> +                             die("Invalid depth: %s", line);
>>>>                       continue;
>>>>               }
>>>>               if (!starts_with(line, "want ") ||
>>>
>>> I do not quite understand this hunk.  What happens when this program
>>> is talking to an existing fetch-pack that requested "deepen"?
>>
>> In this hunk, I actually just changed the one command name in upload-pack
>> service from "deepen" to "depth". I made this change because the name
>> "deepen" here is quite misleading, as it just means "depth". Of course,
>> I've changed the caller's sent pack from "deepen" to "depth" too (See below).
>
> You missed Junio's keyword "existing". Your new client will work well
> with your new server. But it breaks "git clone --depth" (or "git fetch
> --depth") for all existing git installations.
> --
> Duy



-- 
江东灿（Dongcan Jiang）
Team of Search Engine & Web Mining
School of Electronic Engineering & Computer Science
Peking University, Beijing, 100871, P.R.China

[PATCH v2/GSoC/RFC] fetch: git fetch --deepen

[Dongcan Jiang]

This patch is just for discusstion. An option --deepen is added to
'git fetch'. When it comes to '--deepen', git should fetch N more
commits ahead the local shallow commit, where N is indicated by
'--depth=N'. [1]

e.g.

>  (upstream)
>   ---o---o---o---A---B
>
>  (you)
>                  A---B

After excuting "git fetch --depth=1 --deepen", (you) get one more
tip and it becomes

>  (you)
>              o---A---B

'--deepen' is designed to be a boolean option in this patch, which
is a little different from [1]. It's designed in this way, because
it can reuse '--depth' in the program, and just costs one more bit
in some data structure, such as fetch_pack_args,
git_transport_options.

Of course, as a patch for discussion, it remains a long way to go
before being complete.

	1) Documents should be completed.
	2) More test cases, expecially corner cases, should be added.
	3) No need to get remote refs when it comes to '--deepen' option.
	4) Validity on options combination should be checked.
	5) smart-http protocol remains to be supported. [2]

[1] http://article.gmane.org/gmane.comp.version-control.git/212950
[2] http://article.gmane.org/gmane.comp.version-control.git/265050

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

-Background-

At present the command "git-fetch" has one option "depth" which deepens or shortens the history of a shallow repository created by git clone with --depth=<depth> option (see git-clone[1]) to the specified number of commits from the tip of each remote branch history [2].

The drawback is that the starting point for deepening history is limited to the tip of each remote branch history. However, it's not able to satisfy the needs of users in some cases:
1) If the user wants to know the process of how a function was created and developed. What he expects is that he goes to the point of latest modification about the function and deepens the history further back from this current cut-off point by N commit directly, rather than having to guess what the depth is from tip of remote branch.
2) What's more, if the user only cares about the revisions of this function, the other commits after the latest modification are redundant for him, but he has to fetch them concomitantly, which wastes unnecessary time and space.


For the convenience of users, a new option should be added as "deepen" to allow users to just get history commits deepened by N commits from current cut-off point, irrespective of location where the tips are [3].


-Goals-

1) Supports for command "git fetch --deepen";
2) Conflict Detection for this option to guarantee robustness;
3) Sufficient tests;
4) Guarantee for compatibility;
5) Sufficient documents;

More Details are shown in next section.


-Details-

--1. Current Workflow of git-fetch--

         +-+-+-+-+-+-+-+-+-+                      +-+-+-+-+-+-+-+-+-+-+-+
         | git-upload-pack |                      | git-unpack-objects  |
         +-+-+-+-+-+-+-+-+-+                      +-+-+-+-+-+-+-+-+-+-+-+
                |   ^                                      |   ^
                v   |                                      v   |
 fetch   +-+-+-+-+-+-+-+-+-+      +-+-+-+-+-+-+-+      +-+-+-+-+-+-+
-------> | get remote refs | ---> | fetch refs  | ---> | get_pack  | -+
         +-+-+-+-+-+-+-+-+-+      +-+-+-+-+-+-+-+      +-+-+-+-+-+-+  |
                                       |     ^                        |
                                       v     |                        v
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+  +-+-+-+-+-+-+-+
| git-upload-pack                                       |  |   update    |
+  +-+-+-+-+-+-+-+-+-+-+-+-+      +-+-+-+-+-+-+-+-+-+-+ +  | local refs  |
|  | generate wanted objs  | ---> | create_pack_file  | |  +-+-+-+-+-+-+-+
+  |    update shallows    |      +-+-+-+-+-+-+-+-+-+-+ +
|  +-+-+-+-+-+-+-+-+-+-+-+-+            |     ^         |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
                                        |     |
                                        v     |
                                  +-+-+-+-+-+-+-+-+-+-+
                                  | git-pack-objects  |
                                  +-+-+-+-+-+-+-+-+-+-+
  Figure 1. Workflow of git-fetch (Please view it in fixed-width fonts)

Figure 1 illustrates the workflow of git-fetch. When it comes to git-fetch, a series of operations are performed in cooperation with the server side via `transport` module.

At first, it gets remote refs from git-upload-pack, and then it fetches refs, i.e. generating wanted objs, updating shallows and creating pack file. Finally, it unpacks objects from the pack file and updates the local refs.

Actually, there are several protocols running the workflow above, which is showed in Table 1. It is determined in transport.c/transport_get().

Table 1. Protocols supporting `fetch` (Please view it in fixed-width fonts)
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| *protocol*  |   *get remote refs*   | *fetch refs / get_pack* |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|   rsync     |  get_refs_via_rsync   |  fetch_objs_via_rsync   |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|   bundle    | get_refs_from_bundle  | fetch_refs_from_bundle  |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|   normal    | get_refs_via_connect  |  fetch_refs_via_pack    |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| foreign_vsc |     get_refs_list     |          fetch          |
| and others  |  in transport-helper  |   in transport-helper   |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

In this project, "normal" and "foreign_vsc and others" protocols will be mainly concerned, because they can support fetching with "--depth".

More details will be discussed in following parts.


--2. How to Support "fetch --deepen" for Normal Protocol--

Normal protocol will be used when the remote address is not a url or when it starts with "files://", "git://", "ssh://", "git+ssh://" or "ssh+git://".

An intuitive way to implement this feature is as follows,
1) Add "--deepen" option throughout the workflow shown in Figure 1;
2) Generate wanted objs and update shallows according to local shallows and "--deepen" value, but not the newest tips;
3) Make git not to update local refs when it's processing with "--deepen".

Actually, my current patch on this issue is implemented in this intuitive way. However, it has several drawbacks:
1) It gets remote refs, which are useless in its workflow;
2) For each ref, it interacts with git-upload-pack, while only one interaction is needed;
2) It's not able to fetch refs, when the useless "get remote refs" step is forced to skip.

Therefore, several stuffs have to be improved:
1) Not to get remote refs when processing with "--deepen";
2) Just send shallows to git-upload-pack without "want" objects;
3) Make git-upload-pack able to process "--deepen" without "want" objects;


--3. How to Support "fetch --deepen" for foreign_vsc And Others Protocol--

This protocol is just a bit different from above. Actually it shares most logics with the normal one.

The biggest difference is that it does the whole process through git-remote-curl and git-fetch-pack [4], which is illustrated in Figure 2.

                                   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+
                                   | git-fetch-pack            |
 fetch   +-+-+-+-+-+-+-+-+-+       +           ...             +
-------> | git-remote-curl | <---> |  +-+-+-+-+-+-+-+-+-+      |
         +-+-+-+-+-+-+-+-+-+       +  | get remote refs | ...  +
                                   |  +-+-+-+-+-+-+-+-+-+      |
                                   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Figure 2. fetch through git-remote-curl (Please view it in fixed-width fonts)

The key to make it work with "--deepen" is to add the option throughout remote-curl.c and builtin/fetch-pack.c.


--4. Tests--

As git is a product demanding high robustness. Sufficient tests have to be done to make sure the new feature's correctness.

Cases needed to be tested are listed in Table 2.

Table 2. Cases needed to be tested (Please view it in fixed-width fonts)
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|  *module*   |                       *cases*                             |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|             | should fail when used with some options like --unshallow  |
|   fetch,    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|             |   work correctly when deepen exceeds the remote shallow   |
|    pull,    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|             |         the updated shallows by deepen are correct        |
| fetch-pack  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|             |          local remote refs should not be updated          |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Every module on the left needs to test all cases on the right side.


--5. Compatibility--

It's impossible to request users to install the software again for new changes, so all changes shouldn't go against the existing installations. For this case, neither existing options nor their behaviors in code should be changed to guarantee compatibility [5,6].


[1] http://git-scm.com/docs/git-clone
[2] http://git-scm.com/docs/git-fetch
[3] http://article.gmane.org/gmane.comp.version-control.git/212950
[4] http://marc.info/?l=git&m=142580732110033&w=2
[5] http://marc.info/?l=git&m=142631134424268&w=2
[6] http://marc.info/?l=git&m=142658909625290&w=2

Above is my draft proposal for "git fetch --deepen".

However, I'm still not quite following when would it go through the git-remote-curl workflow shown in Figure 2. Are there any examples I could follow in the test set?

Have I missed anything else in my draft? Look forward to your comments and suggestions.


 builtin/fetch.c  |  7 +++++--
 fetch-pack.c     |  1 +
 fetch-pack.h     |  1 +
 t/t5510-fetch.sh | 12 ++++++++++++
 transport.c      |  4 ++++
 transport.h      |  4 ++++
 upload-pack.c    | 27 ++++++++++++++++++---------
 7 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f951265..d8b6504 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -33,7 +33,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;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow;
 static const char *depth;
@@ -111,6 +111,7 @@ 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_BOOL(0, "deepen", &deepen, N_("deepen")),
 	{ OPTION_SET_INT, 0, "unshallow", &unshallow, NULL,
 		   N_("convert to a complete repository"),
 		   PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },
@@ -756,7 +757,7 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
 	int ret = quickfetch(ref_map);
 	if (ret)
 		ret = transport_fetch_refs(transport, ref_map);
-	if (!ret)
+	if (!ret && !deepen)
 		ret |= store_updated_refs(transport->url,
 				transport->remote->name,
 				ref_map);
@@ -853,6 +854,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)
+		set_option(transport, TRANS_OPT_DEEPEN, "yes");
 	if (update_shallow)
 		set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
 	return transport;
diff --git a/fetch-pack.c b/fetch-pack.c
index 655ee64..3c6f17a 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -295,6 +295,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_mode)  strbuf_addstr(&c, " deepen-mode");
 			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");
diff --git a/fetch-pack.h b/fetch-pack.h
index bb7fd76..f575c8b 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -10,6 +10,7 @@ struct fetch_pack_args {
 	const char *uploadpack;
 	int unpacklimit;
 	int depth;
+	unsigned deepen_mode:1;
 	unsigned quiet:1;
 	unsigned keep_pack:1;
 	unsigned lock_pack:1;
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index d78f320..3b960c8 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -708,4 +708,16 @@ test_expect_success 'fetching a one-level ref works' '
 	)
 '

+test_expect_success 'fetching deepen' '
+	git clone . deepen --depth=1 && (
+		cd deepen &&
+		git fetch .. foo --depth=1
+		git show foo
+		test_must_fail git show foo~
+		git fetch .. foo --depth=1 --deepen
+		git show foo~
+		test_must_fail git show foo~2
+	)
+'
+
 test_done
diff --git a/transport.c b/transport.c
index 0694a7c..0a43474 100644
--- a/transport.c
+++ b/transport.c
@@ -478,6 +478,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)) {
+		opts->deepen_mode = !!value;
+		return 0;
 	} else if (!strcmp(name, TRANS_OPT_PUSH_CERT)) {
 		opts->push_cert = !!value;
 		return 0;
@@ -534,6 +537,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_mode = data->options.deepen_mode;
 	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 18d2cf8..357a57b 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;
 	unsigned push_cert : 1;
+	unsigned deepen_mode : 1;
 	int depth;
 	const char *uploadpack;
 	const char *receivepack;
@@ -153,6 +154,9 @@ struct transport *transport_get(struct remote *, const char *);
 /* Limit the depth of the fetch if not null */
 #define TRANS_OPT_DEPTH "depth"

+/* Limit the deepen of the fetch if not null */
+#define TRANS_OPT_DEEPEN "deepen"
+
 /* Aggressively fetch annotated tags if possible */
 #define TRANS_OPT_FOLLOWTAGS "followtags"

diff --git a/upload-pack.c b/upload-pack.c
index b531a32..fbe095a 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -31,6 +31,7 @@ static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<

 static unsigned long oldest_have;

+static int deepen_mode;
 static int multi_ack;
 static int no_done;
 static int use_thin_pack, use_ofs_delta, use_include_tag;
@@ -577,6 +578,8 @@ static void receive_needs(void)

 		features = line + 45;

+		if (parse_feature_request(features, "deepen-mode"))
+			deepen_mode = 1;
 		if (parse_feature_request(features, "multi_ack_detailed"))
 			multi_ack = 2;
 		else if (parse_feature_request(features, "multi_ack"))
@@ -596,15 +599,17 @@ static void receive_needs(void)
 		if (parse_feature_request(features, "include-tag"))
 			use_include_tag = 1;

-		o = parse_object(sha1_buf);
-		if (!o)
-			die("git upload-pack: not our ref %s",
-			    sha1_to_hex(sha1_buf));
-		if (!(o->flags & WANTED)) {
-			o->flags |= WANTED;
-			if (!is_our_ref(o))
-				has_non_tip = 1;
-			add_object_array(o, NULL, &want_obj);
+		if (!deepen_mode) {
+			o = parse_object(sha1_buf);
+			if (!o)
+				die("git upload-pack: not our ref %s",
+						sha1_to_hex(sha1_buf));
+			if (!(o->flags & WANTED)) {
+				o->flags |= WANTED;
+				if (!is_our_ref(o))
+					has_non_tip = 1;
+				add_object_array(o, NULL, &want_obj);
+			}
 		}
 	}

@@ -631,6 +636,10 @@ static void receive_needs(void)
 				struct object *object = shallows.objects[i].item;
 				object->flags |= NOT_SHALLOW;
 			}
+		else if (deepen_mode)
+			backup = result =
+				get_shallow_commits(&shallows, depth + 1,
+						    SHALLOW, NOT_SHALLOW);
 		else
 			backup = result =
 				get_shallow_commits(&want_obj, depth,
--
2.3.3.221.g72ad08d.dirty

Re: [PATCH v2/GSoC/RFC] fetch: git fetch --deepen

[Eric Sunshine]

On Sun, Mar 22, 2015 at 11:24 AM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
> This patch is just for discusstion. An option --deepen is added to
> 'git fetch'. When it comes to '--deepen', git should fetch N more
> commits ahead the local shallow commit, where N is indicated by
> '--depth=N'. [1]
> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
> ---
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index d78f320..3b960c8 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -708,4 +708,16 @@ test_expect_success 'fetching a one-level ref works' '
>         )
>  '
>
> +test_expect_success 'fetching deepen' '
> +       git clone . deepen --depth=1 && (

Quoting Junio[1]: "...make sure tests serve as good examples, tests
should stick to 'options first and then arguments'..."

> +               cd deepen &&
> +               git fetch .. foo --depth=1

See [1].

> +               git show foo
> +               test_must_fail git show foo~
> +               git fetch .. foo --depth=1 --deepen

See [1].

> +               git show foo~
> +               test_must_fail git show foo~2

Mentioned previously[2]: Broken &&-chain throughout this test.

> +       )
> +'
> +
>  test_done

[1]: http://article.gmane.org/gmane.comp.version-control.git/265248
[2]: http://article.gmane.org/gmane.comp.version-control.git/265419

Re: [PATCH v2/GSoC/RFC] fetch: git fetch --deepen

[Duy Nguyen]

On Sun, Mar 22, 2015 at 10:24 PM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
> This patch is just for discusstion. An option --deepen is added to
> 'git fetch'. When it comes to '--deepen', git should fetch N more
> commits ahead the local shallow commit, where N is indicated by
> '--depth=N'. [1]
>
> e.g.
>
>>  (upstream)
>>   ---o---o---o---A---B
>>
>>  (you)
>>                  A---B
>
> After excuting "git fetch --depth=1 --deepen", (you) get one more
> tip and it becomes
>
>>  (you)
>>              o---A---B
>
> '--deepen' is designed to be a boolean option in this patch, which
> is a little different from [1]. It's designed in this way, because
> it can reuse '--depth' in the program, and just costs one more bit
> in some data structure, such as fetch_pack_args,
> git_transport_options.
>
> Of course, as a patch for discussion, it remains a long way to go
> before being complete.
>
>         1) Documents should be completed.
>         2) More test cases, expecially corner cases, should be added.
>         3) No need to get remote refs when it comes to '--deepen' option.
>         4) Validity on options combination should be checked.
>         5) smart-http protocol remains to be supported. [2]

Quick notes before $DAYJOB starts. Cool pictures, perhaps they could
be part of the commit message too.

Personally i still don't think not moving the refs is worth the effort
(and it's a waste if we have to send then drop objects for these
updated refs, but I didn't check carefully). So if you we don't needs
ref updates, we probably don't need to send "want" lines and sort of
simplify processing at upload-pack side.

And it makes me realise, we're loosing security a bit here. We
normally don't send anything that's not reachable from the visible ref
set. But we now would accept any shallow sha-1 and send some objects
regardless if these sha-1 are connected to any refs. We may need some
more checking in place to avoid this. See check_non_sha1_tip() for a
way to do it. Pack bitmaps may help as well, but I think that's behind
the scene (i.e. behind rev-list and we already can take advantage of
it).
-- 
Duy