[PATCH] add warning for depth=0 in git clone.

[PATCH] add warning for depth=0 in git clone.

[Stefan Beller]

---
 builtin/clone.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index ec2f75b..5e91c1e 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -818,6 +818,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	remote = remote_get(option_origin);
 	transport = transport_get(remote, remote->url[0]);
 
+	if (option_depth && transport->smart_options->depth < 1)
+		die(_("--depth less or equal 0 makes no sense; read manpage."));
+
 	if (!is_local) {
 		if (!transport->get_refs_list || !transport->fetch)
 			die(_("Don't know how to clone %s"), transport->url);
-- 
1.8.1.166.g27cbb96

Re: [PATCH] add warning for depth=0 in git clone.

[Stefan Beller]

Hi,

I am struggling a little with the development process,
is a sign-off strictly required for git as it is for kernel development?
If so here would be my sign-off:

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>

This adds a warning and the previous patch adds the documentation.

However I agree to Junio, that the meaning should actually
depth=0 -> just the tip and no (0) other commits before.

depth=n -> the tip and n other commits before.


On 01/08/2013 09:07 AM, Stefan Beller wrote:
> ---
>  builtin/clone.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index ec2f75b..5e91c1e 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -818,6 +818,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	remote = remote_get(option_origin);
>  	transport = transport_get(remote, remote->url[0]);
>  
> +	if (option_depth && transport->smart_options->depth < 1)
> +		die(_("--depth less or equal 0 makes no sense; read manpage."));
> +
>  	if (!is_local) {
>  		if (!transport->get_refs_list || !transport->fetch)
>  			die(_("Don't know how to clone %s"), transport->url);
> 

Re: [PATCH] add warning for depth=0 in git clone.

[Jonathan Nieder]

Stefan Beller wrote:

> I am struggling a little with the development process,
> is a sign-off strictly required for git as it is for kernel development?

Yes.  Documentation/SubmittingPatches has more hints.

> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
>
> This adds a warning and the previous patch adds the documentation.

Thanks for your work on this.

Re: [PATCH] add warning for depth=0 in git clone.

[Jonathan Nieder]

Stefan Beller wrote:

> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -818,6 +818,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	remote = remote_get(option_origin);
>  	transport = transport_get(remote, remote->url[0]);
>  
> +	if (option_depth && transport->smart_options->depth < 1)
> +		die(_("--depth less or equal 0 makes no sense; read manpage."));

"Makes no sense" is a little harsh.  Presumably it made sense to the
user, or she wouldn't have passed that option.  The relevant point is
that git was not able to make sense of it.

How about something like

	fatal: shallow clone must have positive depth

?  The "see manpage" is always implied by die(), and I don't see any
particular reason to point to a specific section here.

Hope that helps,
Jonathan

Re: [PATCH] add warning for depth=0 in git clone.

[Duy Nguyen]

On Tue, Jan 8, 2013 at 3:07 PM, Stefan Beller
<stefanbeller@googlemail.com> wrote:
> ---
>  builtin/clone.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index ec2f75b..5e91c1e 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -818,6 +818,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>         remote = remote_get(option_origin);
>         transport = transport_get(remote, remote->url[0]);
>
> +       if (option_depth && transport->smart_options->depth < 1)
> +               die(_("--depth less or equal 0 makes no sense; read manpage."));
> +

Isn't this too early for the check? The following code is

>         if (!is_local) {
>                 if (!transport->get_refs_list || !transport->fetch)
>                         die(_("Don't know how to clone %s"), transport->url);

		transport_set_option(transport, TRANS_OPT_KEEP, "yes");

		if (option_depth)
			transport_set_option(transport, TRANS_OPT_DEPTH,
					     option_depth);


where transport_set_option() calls set_git_option() to initialize
transport->smart_options->depth. A check should be done after this
point.
-- 
Duy

On --depth=funny value

[Junio C Hamano]

Here to outline my current thinking.  Note that this is unrelated to
the "git clone --bottom=v1.2.3" to say "I do not care about anything
that happened before that version".

 * First, let's *not* do "git fetch --depth=inf"; if you want to
   unplug the bottom of your shallow clone, be more explicit and
   introduce a new option, e.g. "git fetch --unshallow", or
   something.

 * Make "git fetch" and "git clone" die() when zero or negative
   number is given with --depth=$N, for the following reasons:

   - The --depth option is describe as:

         ("git clone") ... a 'shallow' clone with a history
         truncated to the specified number of revisions.

         ("git fetch") Limit fetching to ancestor-chains not longer
         than n.

     It is fairly clear from the above that negative $N does not
     make any sense.

     Technically, fetching the commits that were explicitly asked
     for and not there parents is the only possible ancestor-chain
     that is not longer than -4, so "git fetch --depth=-4" ought to
     behave just like "git fetch --depth=0", but you have to be very
     sick to read the documentation and expect things to work that
     way.  Also there is no way to misread the "git clone"
     documentation and expect "git clone --depth=-4" to create a
     history truncated to negative number of revisions.

     Which means that it is the right thing to do to die() when a
     negative number is given to --depth for these commands.

   - As people count from one, the natural way to ask for the tip
     commit without any history ought to be "--depth=1".  Let's
     declare the current behaviour of "--depth=1" that gives the tip
     and one commit behind it a bug.

     Which means that these commands should be updated to die() when
     zero is given to their --depth option.  "Do not give me any
     history" is inherenty incompatibe with "clone" or "fetch".

     Because there is no configuration variable "fetch.depth" (or
     "clone.depth") that forces all your cloned repositories to be
     shallow, "git clone --depth=0" or "git fetch --depth=0"
     couldn't have been ways for existing users to ask to defeat any
     funny configured depth value and clone or fetch everything.
     When they wanted to clone or fetch everything, they would have
     just used the command without any "--depth" option instead.

     Which means that nobody gets hurt if we change these commands
     to die() when zero is given to their --depth option.

 * We would like to update "clone --depth=1" to end up with a tip
   only repository, but let's not to touch "git fetch" (and "git
   clone") and make them send 0 over the wire when the command line
   tells them to use "--depth=1" (i.e. let's not do the "off-by-one"
   thing).

   Instead, fix "upload-pack" (the bug is in get_shallow_commits()
   in shallow.c, I think), so that it counts correctly.  When the
   other end asks for a history with 1-commit deep, it should return
   a history that is 1-commit deep, and tell the other end about the
   parents of the returned history, instead of returning a history
   that is 2 commmits deep.  So when talking with a newer server,
   clients will get correct number of commits; when talkng with an
   older server, clients will get a bit more than they asked, but
   nothing will break.

Can people sanity check the reasoning outlined here?  Anything I
missed?

The above outline identifies three concrete tasks that different
people can tackle more or less independently, each with updated
code, documentation and test:

 1. "git fetch --unshallow" that gives a pretty surface on Duy's
    "--depth=inf";

 2. Making "git fetch" and "git clone" die on "--depth=0" or
    "--depth=-4";

 3 Updating "upload-pack" to count correctly.

I'll refrain from saying "Any takers?" for now.

Re: On --depth=funny value

[Duy Nguyen]

On Wed, Jan 9, 2013 at 9:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
>  * First, let's *not* do "git fetch --depth=inf"; if you want to
>    unplug the bottom of your shallow clone, be more explicit and
>    introduce a new option, e.g. "git fetch --unshallow", or
>    something.

No problem. "Something" could be --no-depth or --no-shallow. I think
any of them is better than --unshallow.

>  * We would like to update "clone --depth=1" to end up with a tip
>    only repository, but let's not to touch "git fetch" (and "git
>    clone") and make them send 0 over the wire when the command line
>    tells them to use "--depth=1" (i.e. let's not do the "off-by-one"
>    thing).

You can't anyway. Depth 0 on the wire is considered invalid by upload-pack.

>    Instead, fix "upload-pack" (the bug is in get_shallow_commits()
>    in shallow.c, I think), so that it counts correctly.  When the
>    other end asks for a history with 1-commit deep, it should return
>    a history that is 1-commit deep, and tell the other end about the
>    parents of the returned history, instead of returning a history
>    that is 2 commmits deep.  So when talking with a newer server,
>    clients will get correct number of commits; when talkng with an
>    older server, clients will get a bit more than they asked, but
>    nothing will break.

I'll need to look at get_shallow_commits() anyway for the unshallow
patch. I'll probably do this too.
-- 
Duy

Re: On --depth=funny value

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Jan 9, 2013 at 9:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>>  * We would like to update "clone --depth=1" to end up with a tip
>>    only repository, but let's not to touch "git fetch" (and "git
>>    clone") and make them send 0 over the wire when the command line
>>    tells them to use "--depth=1" (i.e. let's not do the "off-by-one"
>>    thing).
>
> You can't anyway. Depth 0 on the wire is considered invalid by upload-pack.

Yes, that is a good point that we say "if (0 < opt->depth) do the
shallow thing" everywhere, so 0 is spcial in that sense.

Which suggests that if we wanted to, we could update the fetch side
to do the off-by-one thing against the current upload-pack when the
given depth is two or more, and still send 1 when depth=1.  When
talking with an updated upload-pack that advertises exact-shallow
protocol extension, it can disable that off-by-one for all values of
depth.  That way, the updated client gets history of wrong depth
only for --depth=1 when talking with the current upload-pack; all
other cases, it will get history of correct depth.

Hmm?

Re: On --depth=funny value

[Duy Nguyen]

On Wed, Jan 9, 2013 at 12:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Wed, Jan 9, 2013 at 9:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>>  * We would like to update "clone --depth=1" to end up with a tip
>>>    only repository, but let's not to touch "git fetch" (and "git
>>>    clone") and make them send 0 over the wire when the command line
>>>    tells them to use "--depth=1" (i.e. let's not do the "off-by-one"
>>>    thing).
>>
>> You can't anyway. Depth 0 on the wire is considered invalid by upload-pack.
>
> Yes, that is a good point that we say "if (0 < opt->depth) do the
> shallow thing" everywhere, so 0 is spcial in that sense.
>
> Which suggests that if we wanted to, we could update the fetch side
> to do the off-by-one thing against the current upload-pack when the
> given depth is two or more, and still send 1 when depth=1.  When
> talking with an updated upload-pack that advertises exact-shallow
> protocol extension, it can disable that off-by-one for all values of
> depth.  That way, the updated client gets history of wrong depth
> only for --depth=1 when talking with the current upload-pack; all
> other cases, it will get history of correct depth.
>
> Hmm?

I haven't checked because frankly I have never run JGit, but are we
sure this off-by-one thing applies to JGit server as well? So far I'm
only aware of three sever implementations: C Git, JGit and Dulwich.
The last one does not support shallow extension so it's out of
question.
-- 
Duy

Re: On --depth=funny value

[Duy Nguyen]

On Wed, Jan 9, 2013 at 9:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
>  * Make "git fetch" and "git clone" die() when zero or negative
>    number is given with --depth=$N, for the following reasons:
>    ...

For Stefan when you update the patch. If "git fetch --depth=0" is
considered invalid too as Junio outlined, then the proper place for
the check is transport.c:set_git_option(), not clone.c. It already
catches --depth=random-string. Adding "depth < 1" check should be
trivial. You may want to update builtin/fetch-pack.c too because it
does not share this code.
-- 
Duy

Re: On --depth=funny value

[Stefan Beller]

On 01/09/2013 03:53 AM, Junio C Hamano wrote:
> Can people sanity check the reasoning outlined here?  Anything I
> missed?
> 
> The above outline identifies three concrete tasks that different
> people can tackle more or less independently, each with updated
> code, documentation and test:
> 
>  1. "git fetch --unshallow" that gives a pretty surface on Duy's
>     "--depth=inf";
> 
>  2. Making "git fetch" and "git clone" die on "--depth=0" or
>     "--depth=-4";
> 
>  3 Updating "upload-pack" to count correctly.
> 
> I'll refrain from saying "Any takers?" for now.

Sorry for answering with delay, I am just contributing to git in my
spare time.
So if I understood Duy correctly, he is going to solve 1. and 3 by his
patches.
I'll try to come up with a solution for 2. within the next days.

[PATCH 1/2] fetch, upload-pack: add --no-shallow for infinite depth

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

The user can do --depth=2147483647 (*) for infinite depth now. But
it's hard to remember. Any other numbers larger than the longest
commit chain in the repository would also do, but some guessing may
be involved. Make easy-to-remember --no-shallow an alias for
--depth=2147483647.

Make upload-pack recognize this special number as infinite depth. The
effect is essentially the same as before, except that upload-pack is
more efficient because it does not have to traverse to the bottom
any more. The chance of a user actually wanting exactly 2147483647
commits depth, not infinite, on a repository with a history that long,
is probably too small to consider.

(*) This is the largest positive number a 32-bit signed integer can
    contain. JGit and older C Git store depth as "int" so both are OK
    with this number. Dulwich does not support shallow clone.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/fetch-options.txt     |  4 ++++
 Documentation/git-fetch-pack.txt    |  2 ++
 Documentation/technical/shallow.txt |  3 +++
 builtin/fetch.c                     | 15 ++++++++++++++-
 commit.h                            |  3 +++
 t/t5500-fetch-pack.sh               | 16 ++++++++++++++++
 upload-pack.c                       | 13 ++++++++++---
 7 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 6e98bdf..012d1b2 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -13,6 +13,10 @@
 	to the specified number of commits from the tip of each remote
 	branch history. Tags for the deepened commits are not fetched.
 
+--no-shallow::
+	Deepen to the roots of the repository's history (i.e. the
+	result repository is no longer shallow).
+
 ifndef::git-pull[]
 --dry-run::
 	Show what would be done, without making any changes.
diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 8c75120..b81e90d 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -84,6 +84,8 @@ be in a separate packet, and the list must end with a flush packet.
 
 --depth=<n>::
 	Limit fetching to ancestor-chains not longer than n.
+	'git-upload-pack' treats the special depth 2147483647 as
+	infinite even if there is an ancestor-chain that long.
 
 --no-progress::
 	Do not show the progress.
diff --git a/Documentation/technical/shallow.txt b/Documentation/technical/shallow.txt
index 0502a54..ea2f69f 100644
--- a/Documentation/technical/shallow.txt
+++ b/Documentation/technical/shallow.txt
@@ -53,3 +53,6 @@ It also writes an appropriate $GIT_DIR/shallow.
 You can deepen a shallow repository with "git-fetch --depth 20
 repo branch", which will fetch branch from repo, but stop at depth
 20, updating $GIT_DIR/shallow.
+
+The special depth 2147483647 (or 0x7fffffff, the largest positive
+number a signed 32-bit integer can contain) means infinite depth.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4b5a898..bf7b5c5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -32,7 +32,7 @@ enum {
 
 static int all, append, dry_run, force, keep, multiple, prune, update_head_ok, verbosity;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
-static int tags = TAGS_DEFAULT;
+static int tags = TAGS_DEFAULT, no_shallow;
 static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
@@ -82,6 +82,9 @@ 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")),
+	{ OPTION_SET_INT, 0, "no-shallow", &no_shallow, NULL,
+		   N_("deepen history to the bottom"),
+		   PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },
 	{ OPTION_STRING, 0, "submodule-prefix", &submodule_prefix, N_("dir"),
 		   N_("prepend this to submodule path output"), PARSE_OPT_HIDDEN },
 	{ OPTION_STRING, 0, "recurse-submodules-default",
@@ -970,6 +973,16 @@ 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 (no_shallow) {
+		if (depth)
+			die(_("--depth and --no-shallow cannot be used together"));
+		else {
+			static char inf_depth[12];
+			sprintf(inf_depth, "%d", INFINITE_DEPTH);
+			depth = inf_depth;
+		}
+	}
+
 	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
 		if (recurse_submodules_default) {
 			int arg = parse_fetch_recurse_submodules_arg("--recurse-submodules-default", recurse_submodules_default);
diff --git a/commit.h b/commit.h
index 0f469e5..fbde106 100644
--- a/commit.h
+++ b/commit.h
@@ -162,6 +162,9 @@ extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *r
 extern struct commit_list *get_merge_bases_many(struct commit *one, int n, struct commit **twos, int cleanup);
 extern struct commit_list *get_octopus_merge_bases(struct commit_list *in);
 
+/* largest postive number a signed 32-bit integer can contain */
+#define INFINITE_DEPTH 0x7fffffff
+
 extern int register_shallow(const unsigned char *sha1);
 extern int unregister_shallow(const unsigned char *sha1);
 extern int for_each_commit_graft(each_commit_graft_fn, void *);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 6322e8a..6a6e672 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -264,6 +264,22 @@ test_expect_success 'clone shallow object count' '
 	grep "^count: 52" count.shallow
 '
 
+test_expect_success 'fetch --depth --no-shallow' '
+	(
+		cd shallow &&
+		test_must_fail git fetch --depth=1 --no-shallow
+	)
+'
+
+test_expect_success 'infinite deepening (full repo)' '
+	(
+		cd shallow &&
+		git fetch --no-shallow &&
+		git fsck --full &&
+		! test -f .git/shallow
+	)
+'
+
 test_expect_success 'clone shallow without --no-single-branch' '
 	git clone --depth 1 "file://$(pwd)/." shallow2
 '
diff --git a/upload-pack.c b/upload-pack.c
index 6142421..88f0029 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -670,10 +670,17 @@ static void receive_needs(void)
 	if (depth == 0 && shallows.nr == 0)
 		return;
 	if (depth > 0) {
-		struct commit_list *result, *backup;
+		struct commit_list *result = NULL, *backup = NULL;
 		int i;
-		backup = result = get_shallow_commits(&want_obj, depth,
-			SHALLOW, NOT_SHALLOW);
+		if (depth == INFINITE_DEPTH)
+			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))) {
-- 
1.8.0.rc2.23.g1fb49df

[PATCH 2/2] upload-pack: fix off-by-one depth calculation in shallow clone

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

get_shallow_commits() is used to determine the cut points at a given
depth (i.e. the number of commits in a chain that the user likes to
get). However we count current depth up to the commit "commit" but we
do the cutting at its parents (i.e. current depth + 1). This makes
upload-pack always return one commit more than requested. This patch
fixes it.

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

diff --git a/shallow.c b/shallow.c
index a0363de..6be915f 100644
--- a/shallow.c
+++ b/shallow.c
@@ -72,8 +72,14 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 		}
 		if (parse_commit(commit))
 			die("invalid commit");
-		commit->object.flags |= not_shallow_flag;
 		cur_depth++;
+		if (cur_depth >= depth) {
+			commit_list_insert(commit, &result);
+			commit->object.flags |= shallow_flag;
+			commit = NULL;
+			continue;
+		}
+		commit->object.flags |= not_shallow_flag;
 		for (p = commit->parents, commit = NULL; p; p = p->next) {
 			if (!p->item->util) {
 				int *pointer = xmalloc(sizeof(int));
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 6a6e672..58d3bdf 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -130,16 +130,25 @@ test_expect_success 'single given branch clone' '
 	test_must_fail git --git-dir=branch-a/.git rev-parse origin/B
 '
 
+test_expect_success 'clone shallow depth 1' '
+	git clone --no-single-branch --depth 1 "file://$(pwd)/." shallow0
+	test "`git --git-dir=shallow0/.git rev-list --count HEAD`" = 1
+'
+
 test_expect_success 'clone shallow' '
 	git clone --no-single-branch --depth 2 "file://$(pwd)/." shallow
 '
 
+test_expect_success 'clone shallow depth count' '
+	test "`git --git-dir=shallow/.git rev-list --count HEAD`" = 2
+'
+
 test_expect_success 'clone shallow object count' '
 	(
 		cd shallow &&
 		git count-objects -v
 	) > count.shallow &&
-	grep "^in-pack: 18" count.shallow
+	grep "^in-pack: 12" count.shallow
 '
 
 test_expect_success 'clone shallow object count (part 2)' '
@@ -256,12 +265,16 @@ test_expect_success 'additional simple shallow deepenings' '
 	)
 '
 
+test_expect_success 'clone shallow depth count' '
+	test "`git --git-dir=shallow/.git rev-list --count HEAD`" = 11
+'
+
 test_expect_success 'clone shallow object count' '
 	(
 		cd shallow &&
 		git count-objects -v
 	) > count.shallow &&
-	grep "^count: 52" count.shallow
+	grep "^count: 55" count.shallow
 '
 
 test_expect_success 'fetch --depth --no-shallow' '
@@ -289,7 +302,7 @@ test_expect_success 'clone shallow object count' '
 		cd shallow2 &&
 		git count-objects -v
 	) > count.shallow2 &&
-	grep "^in-pack: 6" count.shallow2
+	grep "^in-pack: 3" count.shallow2
 '
 
 test_expect_success 'clone shallow with --branch' '
@@ -297,7 +310,7 @@ test_expect_success 'clone shallow with --branch' '
 '
 
 test_expect_success 'clone shallow object count' '
-	echo "in-pack: 6" > count3.expected &&
+	echo "in-pack: 3" > count3.expected &&
 	GIT_DIR=shallow3/.git git count-objects -v |
 		grep "^in-pack" > count3.actual &&
 	test_cmp count3.expected count3.actual
@@ -326,7 +339,7 @@ EOF
 	GIT_DIR=shallow6/.git git tag -l >taglist.actual &&
 	test_cmp taglist.expected taglist.actual &&
 
-	echo "in-pack: 7" > count6.expected &&
+	echo "in-pack: 4" > count6.expected &&
 	GIT_DIR=shallow6/.git git count-objects -v |
 		grep "^in-pack" > count6.actual &&
 	test_cmp count6.expected count6.actual
@@ -341,7 +354,7 @@ EOF
 	GIT_DIR=shallow7/.git git tag -l >taglist.actual &&
 	test_cmp taglist.expected taglist.actual &&
 
-	echo "in-pack: 7" > count7.expected &&
+	echo "in-pack: 4" > count7.expected &&
 	GIT_DIR=shallow7/.git git count-objects -v |
 		grep "^in-pack" > count7.actual &&
 	test_cmp count7.expected count7.actual
-- 
1.8.0.rc2.23.g1fb49df

Re: [PATCH 2/2] upload-pack: fix off-by-one depth calculation in shallow clone

[Junio C Hamano]

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

> get_shallow_commits() is used to determine the cut points at a given
> depth (i.e. the number of commits in a chain that the user likes to
> get). However we count current depth up to the commit "commit" but we
> do the cutting at its parents (i.e. current depth + 1). This makes
> upload-pack always return one commit more than requested. This patch
> fixes it.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  shallow.c             |  8 +++++++-
>  t/t5500-fetch-pack.sh | 25 +++++++++++++++++++------
>  2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/shallow.c b/shallow.c
> index a0363de..6be915f 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -72,8 +72,14 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
>  		}
>  		if (parse_commit(commit))
>  			die("invalid commit");
> -		commit->object.flags |= not_shallow_flag;
>  		cur_depth++;
> +		if (cur_depth >= depth) {
> +			commit_list_insert(commit, &result);
> +			commit->object.flags |= shallow_flag;
> +			commit = NULL;
> +			continue;
> +		}
> +		commit->object.flags |= not_shallow_flag;
>  		for (p = commit->parents, commit = NULL; p; p = p->next) {
>  			if (!p->item->util) {
>  				int *pointer = xmalloc(sizeof(int));

Nice.  No protocol extensions, you update the server side, and an
old client starts getting the right number of generations.

> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 6a6e672..58d3bdf 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -130,16 +130,25 @@ test_expect_success 'single given branch clone' '
>  	test_must_fail git --git-dir=branch-a/.git rev-parse origin/B
>  '
>  
> +test_expect_success 'clone shallow depth 1' '
> +	git clone --no-single-branch --depth 1 "file://$(pwd)/." shallow0
> +	test "`git --git-dir=shallow0/.git rev-list --count HEAD`" = 1
> +'

No &&-chaining?

Thanks.

Re: [PATCH 1/2] fetch, upload-pack: add --no-shallow for infinite depth

[Junio C Hamano]

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

> The user can do --depth=2147483647 (*) for infinite depth now. But
> it's hard to remember. Any other numbers larger than the longest
> commit chain in the repository would also do, but some guessing may
> be involved. Make easy-to-remember --no-shallow an alias for
> --depth=2147483647.

I think "no shallow" makes sense in a much more important way than
"infinite depth", and this patch is a good idea for a reason
entirely different from the justification your log message makes ;-)

We explain that "clone --depth=<number>" is a way to end up with a
partial repository that contains only the recent history, and while
this may give users a smaller repository, in return the result will
be a "shallow repository" with certain limitations (i.e. fetching or
cloning from such a repository will be refused).

We also explain that the reason to use --depth=<number> is to grab
some history behind what you originally acquired into your "shallow
repository" so that you have deeper history than your original
"shallow repository".

A reader who does not know how this shallowness is implemented
cannot be blamed if she thinks the shallowness is a permanent
attribute of a repository, and once a repository is tainted by
shallowness, it cannot be wiped away by deepening it, because
nowhere in the documentation we say the "shallowness" will go away
once you grabbed enough number of older commits with it.

Calling the option "--no-shallow" (or even better, "--unshallow",
meaning "make it a repository that is no longer shallow") makes it
crystal clear that the option is about wiping away the shallowness.
Of course, the result has to contain an untruncted history, but that
is a mere side effect and an implementation detail from the end
user's point of view.

> Make upload-pack recognize this special number as infinite depth. The
> effect is essentially the same as before, except that upload-pack is
> more efficient because it does not have to traverse to the bottom
> any more. The chance of a user actually wanting exactly 2147483647
> commits depth, not infinite, on a repository with a history that long,
> is probably too small to consider.
>
> (*) This is the largest positive number a 32-bit signed integer can
>     contain. JGit and older C Git store depth as "int" so both are OK
>     with this number. Dulwich does not support shallow clone.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/fetch-options.txt     |  4 ++++
>  Documentation/git-fetch-pack.txt    |  2 ++
>  Documentation/technical/shallow.txt |  3 +++
>  builtin/fetch.c                     | 15 ++++++++++++++-
>  commit.h                            |  3 +++
>  t/t5500-fetch-pack.sh               | 16 ++++++++++++++++
>  upload-pack.c                       | 13 ++++++++++---
>  7 files changed, 52 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 6e98bdf..012d1b2 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -13,6 +13,10 @@
>  	to the specified number of commits from the tip of each remote
>  	branch history. Tags for the deepened commits are not fetched.
>  
> +--no-shallow::
> +	Deepen to the roots of the repository's history (i.e. the
> +	result repository is no longer shallow).
> +

Mentioning both is a good thing, but I think "no longer shallow" is
more important aspect of this operation than "deepen to the roots".

> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 6322e8a..6a6e672 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -264,6 +264,22 @@ test_expect_success 'clone shallow object count' '
>  	grep "^count: 52" count.shallow
>  '
>  
> +test_expect_success 'fetch --depth --no-shallow' '
> +	(
> +		cd shallow &&
> +		test_must_fail git fetch --depth=1 --no-shallow
> +	)
> +'
> +
> +test_expect_success 'infinite deepening (full repo)' '
> +	(
> +		cd shallow &&
> +		git fetch --no-shallow &&
> +		git fsck --full &&
> +		! test -f .git/shallow

This looks as if fsck is what removes .git/shallow but I do not
think that is what is being tested...

> diff --git a/upload-pack.c b/upload-pack.c
> index 6142421..88f0029 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -670,10 +670,17 @@ static void receive_needs(void)
>  	if (depth == 0 && shallows.nr == 0)
>  		return;
>  	if (depth > 0) {
> -		struct commit_list *result, *backup;
> +		struct commit_list *result = NULL, *backup = NULL;
>  		int i;
> -		backup = result = get_shallow_commits(&want_obj, depth,
> -			SHALLOW, NOT_SHALLOW);
> +		if (depth == INFINITE_DEPTH)
> +			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);

Nice.  Thanks.

[PATCH v2 1/3] fetch: add --unshallow for turning shallow repo into complete one

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

The user can do --depth=2147483647 (*) for restoring complete repo
now. But it's hard to remember. Any other numbers larger than the
longest commit chain in the repository would also do, but some
guessing may be involved. Make easy-to-remember --unshallow an alias
for --depth=2147483647.

Make upload-pack recognize this special number as infinite depth. The
effect is essentially the same as before, except that upload-pack is
more efficient because it does not have to traverse to the bottom
anymore.

The chance of a user actually wanting exactly 2147483647 commits
depth, not infinite, on a repository with a history that long, is
probably too small to consider. The client can learn to add or
subtract one commit to avoid the special treatment when that actually
happens.

(*) This is the largest positive number a 32-bit signed integer can
    contain. JGit and older C Git store depth as "int" so both are OK
    with this number. Dulwich does not support shallow clone.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Fri, Jan 11, 2013 at 10:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
 > I think "no shallow" makes sense in a much more important way than
 > "infinite depth", and this patch is a good idea for a reason
 > entirely different from the justification your log message makes ;-)
 > [snip]
 > Calling the option "--no-shallow" (or even better, "--unshallow",
 > meaning "make it a repository that is no longer shallow") makes it
 > crystal clear that the option is about wiping away the shallowness.
 > Of course, the result has to contain an untruncted history, but that
 > is a mere side effect and an implementation detail from the end
 > user's point of view.

 Very well said. --unshallow it is.

 Documentation/fetch-options.txt     |  4 ++++
 Documentation/git-fetch-pack.txt    |  2 ++
 Documentation/technical/shallow.txt |  3 +++
 builtin/fetch.c                     | 17 ++++++++++++++++-
 commit.h                            |  3 +++
 t/t5500-fetch-pack.sh               | 20 ++++++++++++++++++++
 upload-pack.c                       | 13 ++++++++++---
 7 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 6e98bdf..8a0449c 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -13,6 +13,10 @@
 	to the specified number of commits from the tip of each remote
 	branch history. Tags for the deepened commits are not fetched.
 
+--unshallow::
+	Convert a shallow repository to a complete one, removing all
+	the limitations imposed by shallow repositories.
+
 ifndef::git-pull[]
 --dry-run::
 	Show what would be done, without making any changes.
diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 8c75120..b81e90d 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -84,6 +84,8 @@ be in a separate packet, and the list must end with a flush packet.
 
 --depth=<n>::
 	Limit fetching to ancestor-chains not longer than n.
+	'git-upload-pack' treats the special depth 2147483647 as
+	infinite even if there is an ancestor-chain that long.
 
 --no-progress::
 	Do not show the progress.
diff --git a/Documentation/technical/shallow.txt b/Documentation/technical/shallow.txt
index 0502a54..ea2f69f 100644
--- a/Documentation/technical/shallow.txt
+++ b/Documentation/technical/shallow.txt
@@ -53,3 +53,6 @@ It also writes an appropriate $GIT_DIR/shallow.
 You can deepen a shallow repository with "git-fetch --depth 20
 repo branch", which will fetch branch from repo, but stop at depth
 20, updating $GIT_DIR/shallow.
+
+The special depth 2147483647 (or 0x7fffffff, the largest positive
+number a signed 32-bit integer can contain) means infinite depth.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4b5a898..2b15ced 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -32,7 +32,7 @@ enum {
 
 static int all, append, dry_run, force, keep, multiple, prune, update_head_ok, verbosity;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
-static int tags = TAGS_DEFAULT;
+static int tags = TAGS_DEFAULT, unshallow;
 static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
@@ -82,6 +82,9 @@ 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")),
+	{ OPTION_SET_INT, 0, "unshallow", &unshallow, NULL,
+		   N_("convert to a complete repository"),
+		   PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },
 	{ OPTION_STRING, 0, "submodule-prefix", &submodule_prefix, N_("dir"),
 		   N_("prepend this to submodule path output"), PARSE_OPT_HIDDEN },
 	{ OPTION_STRING, 0, "recurse-submodules-default",
@@ -970,6 +973,18 @@ 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 (unshallow) {
+		if (depth)
+			die(_("--depth and --unshallow cannot be used together"));
+		else if (!is_repository_shallow())
+			die(_("--unshallow on a complete repository does not make sense"));
+		else {
+			static char inf_depth[12];
+			sprintf(inf_depth, "%d", INFINITE_DEPTH);
+			depth = inf_depth;
+		}
+	}
+
 	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
 		if (recurse_submodules_default) {
 			int arg = parse_fetch_recurse_submodules_arg("--recurse-submodules-default", recurse_submodules_default);
diff --git a/commit.h b/commit.h
index 0f469e5..fbde106 100644
--- a/commit.h
+++ b/commit.h
@@ -162,6 +162,9 @@ extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *r
 extern struct commit_list *get_merge_bases_many(struct commit *one, int n, struct commit **twos, int cleanup);
 extern struct commit_list *get_octopus_merge_bases(struct commit_list *in);
 
+/* largest postive number a signed 32-bit integer can contain */
+#define INFINITE_DEPTH 0x7fffffff
+
 extern int register_shallow(const unsigned char *sha1);
 extern int unregister_shallow(const unsigned char *sha1);
 extern int for_each_commit_graft(each_commit_graft_fn, void *);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 6322e8a..426027e 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -264,6 +264,26 @@ test_expect_success 'clone shallow object count' '
 	grep "^count: 52" count.shallow
 '
 
+test_expect_success 'fetch --no-shallow on full repo' '
+	test_must_fail git fetch --noshallow
+'
+
+test_expect_success 'fetch --depth --no-shallow' '
+	(
+		cd shallow &&
+		test_must_fail git fetch --depth=1 --noshallow
+	)
+'
+
+test_expect_success 'turn shallow to complete repository' '
+	(
+		cd shallow &&
+		git fetch --unshallow &&
+		! test -f .git/shallow &&
+		git fsck --full
+	)
+'
+
 test_expect_success 'clone shallow without --no-single-branch' '
 	git clone --depth 1 "file://$(pwd)/." shallow2
 '
diff --git a/upload-pack.c b/upload-pack.c
index 6142421..88f0029 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -670,10 +670,17 @@ static void receive_needs(void)
 	if (depth == 0 && shallows.nr == 0)
 		return;
 	if (depth > 0) {
-		struct commit_list *result, *backup;
+		struct commit_list *result = NULL, *backup = NULL;
 		int i;
-		backup = result = get_shallow_commits(&want_obj, depth,
-			SHALLOW, NOT_SHALLOW);
+		if (depth == INFINITE_DEPTH)
+			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))) {
-- 
1.8.0.rc2.23.g1fb49df

[PATCH v2 2/3] upload-pack: fix off-by-one depth calculation in shallow clone

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

get_shallow_commits() is used to determine the cut points at a given
depth (i.e. the number of commits in a chain that the user likes to
get). However we count current depth up to the commit "commit" but we
do the cutting at its parents (i.e. current depth + 1). This makes
upload-pack always return one commit more than requested. This patch
fixes it.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Fixed the && command chain in t5500.

 shallow.c             |  8 +++++++-
 t/t5500-fetch-pack.sh | 25 +++++++++++++++++++------
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/shallow.c b/shallow.c
index a0363de..6be915f 100644
--- a/shallow.c
+++ b/shallow.c
@@ -72,8 +72,14 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 		}
 		if (parse_commit(commit))
 			die("invalid commit");
-		commit->object.flags |= not_shallow_flag;
 		cur_depth++;
+		if (cur_depth >= depth) {
+			commit_list_insert(commit, &result);
+			commit->object.flags |= shallow_flag;
+			commit = NULL;
+			continue;
+		}
+		commit->object.flags |= not_shallow_flag;
 		for (p = commit->parents, commit = NULL; p; p = p->next) {
 			if (!p->item->util) {
 				int *pointer = xmalloc(sizeof(int));
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 426027e..354d32c 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -130,16 +130,25 @@ test_expect_success 'single given branch clone' '
 	test_must_fail git --git-dir=branch-a/.git rev-parse origin/B
 '
 
+test_expect_success 'clone shallow depth 1' '
+	git clone --no-single-branch --depth 1 "file://$(pwd)/." shallow0 &&
+	test "`git --git-dir=shallow0/.git rev-list --count HEAD`" = 1
+'
+
 test_expect_success 'clone shallow' '
 	git clone --no-single-branch --depth 2 "file://$(pwd)/." shallow
 '
 
+test_expect_success 'clone shallow depth count' '
+	test "`git --git-dir=shallow/.git rev-list --count HEAD`" = 2
+'
+
 test_expect_success 'clone shallow object count' '
 	(
 		cd shallow &&
 		git count-objects -v
 	) > count.shallow &&
-	grep "^in-pack: 18" count.shallow
+	grep "^in-pack: 12" count.shallow
 '
 
 test_expect_success 'clone shallow object count (part 2)' '
@@ -256,12 +265,16 @@ test_expect_success 'additional simple shallow deepenings' '
 	)
 '
 
+test_expect_success 'clone shallow depth count' '
+	test "`git --git-dir=shallow/.git rev-list --count HEAD`" = 11
+'
+
 test_expect_success 'clone shallow object count' '
 	(
 		cd shallow &&
 		git count-objects -v
 	) > count.shallow &&
-	grep "^count: 52" count.shallow
+	grep "^count: 55" count.shallow
 '
 
 test_expect_success 'fetch --no-shallow on full repo' '
@@ -293,7 +306,7 @@ test_expect_success 'clone shallow object count' '
 		cd shallow2 &&
 		git count-objects -v
 	) > count.shallow2 &&
-	grep "^in-pack: 6" count.shallow2
+	grep "^in-pack: 3" count.shallow2
 '
 
 test_expect_success 'clone shallow with --branch' '
@@ -301,7 +314,7 @@ test_expect_success 'clone shallow with --branch' '
 '
 
 test_expect_success 'clone shallow object count' '
-	echo "in-pack: 6" > count3.expected &&
+	echo "in-pack: 3" > count3.expected &&
 	GIT_DIR=shallow3/.git git count-objects -v |
 		grep "^in-pack" > count3.actual &&
 	test_cmp count3.expected count3.actual
@@ -330,7 +343,7 @@ EOF
 	GIT_DIR=shallow6/.git git tag -l >taglist.actual &&
 	test_cmp taglist.expected taglist.actual &&
 
-	echo "in-pack: 7" > count6.expected &&
+	echo "in-pack: 4" > count6.expected &&
 	GIT_DIR=shallow6/.git git count-objects -v |
 		grep "^in-pack" > count6.actual &&
 	test_cmp count6.expected count6.actual
@@ -345,7 +358,7 @@ EOF
 	GIT_DIR=shallow7/.git git tag -l >taglist.actual &&
 	test_cmp taglist.expected taglist.actual &&
 
-	echo "in-pack: 7" > count7.expected &&
+	echo "in-pack: 4" > count7.expected &&
 	GIT_DIR=shallow7/.git git count-objects -v |
 		grep "^in-pack" > count7.actual &&
 	test_cmp count7.expected count7.actual
-- 
1.8.0.rc2.23.g1fb49df

[PATCH v2 3/3] fetch: elaborate --depth action

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

--depth is explained as deepen, but the way it's applied, it can
shorten the history as well. Keen users may have noticed the
implication by the phrase "the specified number of commits from the
tip of each remote branch". Put "shorten" in the description to make
it clearer.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/fetch-options.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 8a0449c..fb92b02 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -8,7 +8,7 @@
 	option old data in `.git/FETCH_HEAD` will be overwritten.
 
 --depth=<depth>::
-	Deepen the history of a 'shallow' repository created by
+	Deepen or shorten the history of a 'shallow' repository created by
 	`git clone` with `--depth=<depth>` option (see linkgit:git-clone[1])
 	to the specified number of commits from the tip of each remote
 	branch history. Tags for the deepened commits are not fetched.
-- 
1.8.0.rc2.23.g1fb49df