threads / patch / 24796

patch, 3 partsclone: do not accept --depth on local clones

Subject: [PATCH 1/3] clone: do not accept --depth on local clones

## tl;dr

8 messages between Aug 20, 2010 and Aug 22, 2010. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Nguyễn Thái Ngọc Duy· Aug 20, 2010, 01:51 UTC · lore

clone_local() function disregards --depth. Make it more apparent. Also hint users that file:// works with --depth.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/clone.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
Show changes to builtin/clone.c +6 −2
diff --git a/builtin/clone.c b/builtin/clone.c
index efb1e6f..e787cf2 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -361,7 +361,7 @@ static void write_remote_refs(const struct ref *local_refs)
 
 int cmd_clone(int argc, const char **argv, const char *prefix)
 {
-	int is_bundle = 0;
+	int is_bundle = 0, is_local;
 	struct stat buf;
 	const char *repo_name, *repo, *work_tree, *git_dir;
 	char *path, *dir;
@@ -414,6 +414,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		repo = xstrdup(make_absolute_path(repo_name));
 	else
 		repo = repo_name;
+	is_local = path && !is_bundle;
+	if (is_local && option_depth)
+		die("local clone with --depth does not make sense\n"
+		    "hint: use file:// instead");
 
 	if (argc == 2)
 		dir = xstrdup(argv[1]);
@@ -514,7 +518,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	strbuf_reset(&value);
 
-	if (path && !is_bundle) {
+	if (is_local) {
 		refs = clone_local(path, git_dir);
 		mapped_refs = wanted_peer_refs(refs, refspec);
 	} else {
-- 
1.7.1.rc1.69.g24c2f7
Nguyễn Thái Ngọc Duy· Aug 20, 2010, 01:51 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/3] fetch-pack: use args.shallow to detect shallow clone instead of args.depth

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/fetch-pack.c |   17 ++++++++++-------
 fetch-pack.h         |    3 ++-
 transport.c          |    9 +++++++--
 transport.h          |    1 +
 4 files changed, 20 insertions(+), 10 deletions(-)
Show changes to 4 files +20 −10

builtin/fetch-pack.c, fetch-pack.h, transport.c, transport.h

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index dbd8b7b..45d1824 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -167,7 +167,7 @@ enum ack_type {
 
 static void consume_shallow_list(int fd)
 {
-	if (args.stateless_rpc && args.depth > 0) {
+	if (args.stateless_rpc && args.shallow) {
 		/* If we sent a depth we will get back "duplicate"
 		 * shallow and unshallow commands every time there
 		 * is a block of have lines exchanged.
@@ -283,12 +283,12 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 
 	if (is_repository_shallow())
 		write_shallow_commits(&req_buf, 1);
-	if (args.depth > 0)
+	if (args.shallow)
 		packet_buf_write(&req_buf, "deepen %d", args.depth);
 	packet_buf_flush(&req_buf);
 	state_len = req_buf.len;
 
-	if (args.depth > 0) {
+	if (args.shallow) {
 		char line[1024];
 		unsigned char sha1[20];
 
@@ -476,7 +476,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
 		    check_ref_format(ref->name + 5))
 			; /* trash */
 		else if (args.fetch_all &&
-			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
+			 (!args.shallow || prefixcmp(ref->name, "refs/tags/") )) {
 			*newtail = ref;
 			ref->next = NULL;
 			newtail = &ref->next;
@@ -534,7 +534,7 @@ static int everything_local(struct ref **refs, int nr_match, char **match)
 		}
 	}
 
-	if (!args.depth) {
+	if (!args.shallow) {
 		for_each_ref(mark_complete, NULL);
 		if (cutoff)
 			mark_recent_complete_commits(cutoff);
@@ -845,6 +845,9 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			}
 			if (!prefixcmp(arg, "--depth=")) {
 				args.depth = strtol(arg + 8, NULL, 0);
+				if (args.depth <= 0)
+					die("Invalid depth %d", args.depth);
+				args.shallow = 1;
 				continue;
 			}
 			if (!strcmp("--no-progress", arg)) {
@@ -928,7 +931,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 	fetch_pack_setup();
 	if (&args != my_args)
 		memcpy(&args, my_args, sizeof(args));
-	if (args.depth > 0) {
+	if (args.shallow) {
 		if (stat(git_path("shallow"), &st))
 			st.st_mtime = 0;
 	}
@@ -941,7 +944,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 	}
 	ref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);
 
-	if (args.depth > 0) {
+	if (args.shallow) {
 		struct cache_time mtime;
 		struct strbuf sb = STRBUF_INIT;
 		char *shallow = git_path("shallow");
diff --git a/fetch-pack.h b/fetch-pack.h
index fbe85ac..abc1ab7 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -14,7 +14,8 @@ struct fetch_pack_args
 		verbose:1,
 		no_progress:1,
 		include_tag:1,
-		stateless_rpc:1;
+		stateless_rpc:1,
+		shallow:1;
 };
 
 struct ref *fetch_pack(struct fetch_pack_args *args,
diff --git a/transport.c b/transport.c
index 4dba6f8..cf7030b 100644
--- a/transport.c
+++ b/transport.c
@@ -470,10 +470,14 @@ static int set_git_option(struct git_transport_options *opts,
 		opts->keep = !!value;
 		return 0;
 	} else if (!strcmp(name, TRANS_OPT_DEPTH)) {
-		if (!value)
+		if (!value) {
 			opts->depth = 0;
-		else
+			opts->shallow = 0;
+		}
+		else {
 			opts->depth = atoi(value);
+			opts->shallow = 1;
+		}
 		return 0;
 	}
 	return 1;
@@ -529,6 +533,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.quiet = (transport->verbose < 0);
 	args.no_progress = !transport->progress;
 	args.depth = data->options.depth;
+	args.shallow = data->options.shallow;
 
 	for (i = 0; i < nr_heads; i++)
 		origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
diff --git a/transport.h b/transport.h
index c59d973..e6ac4b1 100644
--- a/transport.h
+++ b/transport.h
@@ -8,6 +8,7 @@ struct git_transport_options {
 	unsigned thin : 1;
 	unsigned keep : 1;
 	unsigned followtags : 1;
+	unsigned shallow : 1;
 	int depth;
 	const char *uploadpack;
 	const char *receivepack;
-- 
1.7.1.rc1.69.g24c2f7
Nguyễn Thái Ngọc Duy· Aug 20, 2010, 01:51 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 3/3] {fetch,upload}-pack: allow --depth=0 for infinite depth

Users can do --depth=2147483648 for infinite depth now. It just looks ugly. So make "0" special (i.e. infinite depth) at plumbing/protocol level.

To make it even more user friendly, "git fetch" accepts --depth=inf as an alternative to --depth=0. "git clone" also can. It just does not make much sense for doing "git clone --depth=inf"

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/fetch-options.txt     |    3 ++-
 Documentation/git-fetch-pack.txt    |    1 +
 Documentation/technical/shallow.txt |    2 ++
 builtin/fetch-pack.c                |    2 +-
 shallow.c                           |    2 +-
 t/t5500-fetch-pack.sh               |    8 ++++++++
 transport.c                         |    2 +-
 upload-pack.c                       |    8 ++++----
 8 files changed, 20 insertions(+), 8 deletions(-)
Show changes to 8 files +20 −8

Documentation/fetch-options.txt, Documentation/git-fetch-pack.txt, Documentation/technical/shallow.txt, builtin/fetch-pack.c, shallow.c, t/t5500-fetch-pack.sh, transport.c, upload-pack.c

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 9333c42..a22d839 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -10,7 +10,8 @@
 --depth=<depth>::
 	Deepen the history of a 'shallow' repository created by
 	`git clone` with `--depth=<depth>` option (see linkgit:git-clone[1])
-	by the specified number of commits.
+	by the specified number of commits. Specify "inf" for
+	infinite depth.
 
 ifndef::git-pull[]
 --dry-run::
diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 4a8487c..75e4669 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -70,6 +70,7 @@ OPTIONS
 
 --depth=<n>::
 	Limit fetching to ancestor-chains not longer than n.
+	Specify 0 for infinite depth.
 
 --no-progress::
 	Do not show the progress.
diff --git a/Documentation/technical/shallow.txt b/Documentation/technical/shallow.txt
index 559263a..405170f 100644
--- a/Documentation/technical/shallow.txt
+++ b/Documentation/technical/shallow.txt
@@ -47,3 +47,5 @@ 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.
+
+"--depth 0" (or "--depth inf" for git-fetch) means infinite depth.
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 45d1824..7d7af2e 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -845,7 +845,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			}
 			if (!prefixcmp(arg, "--depth=")) {
 				args.depth = strtol(arg + 8, NULL, 0);
-				if (args.depth <= 0)
+				if (args.depth < 0)
 					die("Invalid depth %d", args.depth);
 				args.shallow = 1;
 				continue;
diff --git a/shallow.c b/shallow.c
index 4d90eda..eab97c6 100644
--- a/shallow.c
+++ b/shallow.c
@@ -85,7 +85,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 					continue;
 				*pointer = cur_depth;
 			}
-			if (cur_depth < depth) {
+			if (!depth || cur_depth < depth) {
 				if (p->next)
 					add_object_array(&p->item->object,
 							NULL, &stack);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 18376d6..47fd87c 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -248,4 +248,12 @@ test_expect_success 'clone shallow object count' '
 	grep "^count: 52" count.shallow
 '
 
+test_expect_success 'infinite deepening (full repo)' '
+	(
+		cd shallow &&
+		git fetch --depth=inf &&
+		! test -f .git/shallow
+	)
+'
+
 test_done
diff --git a/transport.c b/transport.c
index cf7030b..9303aef 100644
--- a/transport.c
+++ b/transport.c
@@ -475,7 +475,7 @@ static int set_git_option(struct git_transport_options *opts,
 			opts->shallow = 0;
 		}
 		else {
-			opts->depth = atoi(value);
+			opts->depth = !strcmp(value, "inf") ? 0 : atoi(value);
 			opts->shallow = 1;
 		}
 		return 0;
diff --git a/upload-pack.c b/upload-pack.c
index fc79dde..7b004b9 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -484,7 +484,7 @@ static void receive_needs(void)
 {
 	struct object_array shallows = {0, 0, NULL};
 	static char line[1000];
-	int len, depth = 0;
+	int len, depth = -1;
 
 	shallow_nr = 0;
 	if (debug_fd)
@@ -514,7 +514,7 @@ static void receive_needs(void)
 		if (!prefixcmp(line, "deepen ")) {
 			char *end;
 			depth = strtol(line + 7, &end, 0);
-			if (end == line + 7 || depth <= 0)
+			if (end == line + 7 || depth < 0)
 				die("Invalid deepen: %s", line);
 			continue;
 		}
@@ -562,9 +562,9 @@ static void receive_needs(void)
 	if (!use_sideband && daemon_mode)
 		no_progress = 1;
 
-	if (depth == 0 && shallows.nr == 0)
+	if (depth == -1 && shallows.nr == 0)
 		return;
-	if (depth > 0) {
+	if (depth >= 0) {
 		struct commit_list *result, *backup;
 		int i;
 		backup = result = get_shallow_commits(&want_obj, depth,
-- 
1.7.1.rc1.69.g24c2f7
Jakub Narebski· Aug 20, 2010, 22:11 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 3/3] {fetch,upload}-pack: allow --depth=0 for infinite depth

On Fri, 20 Aug 2010, Nguyễn Thái Ngọc Duy wrote:
Show 11 quoted lines
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 9333c42..a22d839 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -10,7 +10,8 @@
>  --depth=<depth>::
>  	Deepen the history of a 'shallow' repository created by
>  	`git clone` with `--depth=<depth>` option (see linkgit:git-clone[1])
> -	by the specified number of commits.
> +	by the specified number of commits. Specify "inf" for
> +	infinite depth.
It should probably be 'Specify 0 or "inf" for infinite (unlimited) depth',
which means making repository non-shallow.
  
Show 11 quoted lines
>  ifndef::git-pull[]
>  --dry-run::
> diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
> index 4a8487c..75e4669 100644
> --- a/Documentation/git-fetch-pack.txt
> +++ b/Documentation/git-fetch-pack.txt
> @@ -70,6 +70,7 @@ OPTIONS
>  
>  --depth=<n>::
>  	Limit fetching to ancestor-chains not longer than n.
> +	Specify 0 for infinite depth.
Again '0 or "inf"'.
  
Show 12 quoted lines
>  --no-progress::
>  	Do not show the progress.
> diff --git a/Documentation/technical/shallow.txt b/Documentation/technical/shallow.txt
> index 559263a..405170f 100644
> --- a/Documentation/technical/shallow.txt
> +++ b/Documentation/technical/shallow.txt
> @@ -47,3 +47,5 @@ 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.
> +
> +"--depth 0" (or "--depth inf" for git-fetch) means infinite depth.
???
Show 17 quoted lines
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 18376d6..47fd87c 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -248,4 +248,12 @@ test_expect_success 'clone shallow object count' '
>  	grep "^count: 52" count.shallow
>  '
>  
> +test_expect_success 'infinite deepening (full repo)' '
> +	(
> +		cd shallow &&
> +		git fetch --depth=inf &&
> +		! test -f .git/shallow
> +	)
> +'
> +
>  test_done
It's very good that you included test of making repository non-shallow.
-- 
Jakub Narebski
Poland
Nguyen Thai Ngoc Duy· Aug 22, 2010, 22:51 UTC · re: Jakub Narebski · lore

Re: [PATCH 3/3] {fetch,upload}-pack: allow --depth=0 for infinite depth

On Sat, Aug 21, 2010 at 8:11 AM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 17 quoted lines
> On Fri, 20 Aug 2010, Nguyễn Thái Ngọc Duy wrote:
>
>> diff --git a/Documentation/fetch-options.txt
>> b/Documentation/fetch-options.txt
>> index 9333c42..a22d839 100644
>> --- a/Documentation/fetch-options.txt
>> +++ b/Documentation/fetch-options.txt
>> @@ -10,7 +10,8 @@
>>  --depth=<depth>::
>>       Deepen the history of a 'shallow' repository created by
>>       `git clone` with `--depth=<depth>` option (see linkgit:git-clone[1])
>> -     by the specified number of commits.
>> +     by the specified number of commits. Specify "inf" for
>> +     infinite depth.
>
> It should probably be 'Specify 0 or "inf" for infinite (unlimited) depth',
> which means making repository non-shallow.

It would say "... which _may_ make the repository non-shallow". Or I have to redo the patch and make sure tags are fetched automatically too. Hmm..

Show 14 quoted lines
>>  ifndef::git-pull[]
>>  --dry-run::
>> diff --git a/Documentation/git-fetch-pack.txt
>> b/Documentation/git-fetch-pack.txt
>> index 4a8487c..75e4669 100644
>> --- a/Documentation/git-fetch-pack.txt
>> +++ b/Documentation/git-fetch-pack.txt
>> @@ -70,6 +70,7 @@ OPTIONS
>>
>>  --depth=<n>::
>>       Limit fetching to ancestor-chains not longer than n.
>> +     Specify 0 for infinite depth.
>
> Again '0 or "inf"'.

OK. "inf" was intented for user pleasure, which was the reason I did not implement it in git-fetch-pack (a plumbing). But Jonathan was brave enough to make OPT_INT_INF, I'll rebase this series on that patch.

-- 
Duy
Junio C Hamano· Aug 21, 2010, 00:27 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 3/3] {fetch,upload}-pack: allow --depth=0 for infinite depth

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 24 quoted lines
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 9333c42..a22d839 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -10,7 +10,8 @@
>  --depth=<depth>::
>  	Deepen the history of a 'shallow' repository created by
>  	`git clone` with `--depth=<depth>` option (see linkgit:git-clone[1])
> -	by the specified number of commits.
> +	by the specified number of commits. Specify "inf" for
> +	infinite depth.
>  
>  ifndef::git-pull[]
>  --dry-run::
> diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
> index 4a8487c..75e4669 100644
> --- a/Documentation/git-fetch-pack.txt
> +++ b/Documentation/git-fetch-pack.txt
> @@ -70,6 +70,7 @@ OPTIONS
>  
>  --depth=<n>::
>  	Limit fetching to ancestor-chains not longer than n.
> +	Specify 0 for infinite depth.
>  
Should we force users to remember which one is which?
Show 10 quoted lines
> diff --git a/transport.c b/transport.c
> index cf7030b..9303aef 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -475,7 +475,7 @@ static int set_git_option(struct git_transport_options *opts,
>  			opts->shallow = 0;
>  		}
>  		else {
> -			opts->depth = atoi(value);
> +			opts->depth = !strcmp(value, "inf") ? 0 : atoi(value);
Doesn't atoi("inf") give you 0 anyway ;-)?
Junio C Hamano· Aug 21, 2010, 00:27 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 1/3] clone: do not accept --depth on local clones

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> clone_local() function disregards --depth. Make it more apparent.
> Also hint users that file:// works with --depth.
Hmm---why?

The "--depth" option is an ugly hack that tries to conserve network bandwidth and disk usage, which is not necessary if you are doing a local hardlinking clone. Without this patch we allowed people to clone with the option without a downside (allowing it didn't result in broken repository, for example), but with it, what has long been allowed suddenly stops working. Is the breakage justifiable?

Nguyen Thai Ngoc Duy· Aug 22, 2010, 22:43 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] clone: do not accept --depth on local clones

On Sat, Aug 21, 2010 at 10:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> clone_local() function disregards --depth. Make it more apparent.
>> Also hint users that file:// works with --depth.
>
> Hmm---why?
>
> The "--depth" option is an ugly hack that tries to conserve network
> bandwidth and disk usage, which is not necessary if you are doing a local
> hardlinking clone.  Without this patch we allowed people to clone with the
> option without a downside (allowing it didn't result in broken repository,
> for example), but with it, what has long been allowed suddenly stops
> working.  Is the breakage justifiable?

I also expect it to cut down history too. Maybe it's just me, when I say --depth=3, 'git log' should give me more or less 3 commits regardless what kind of transport I use to clone. Or we could make it a warning, "--depth is ignored in local clone"?

-- 
Duy

← back to recent threads