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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 11, 2013, 03:55 UTC
Message-ID: <7vtxqocr0u.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1357875005-21956-1-git-send-email-pclouds@gmail.com>

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

Show 5 quoted lines

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

Show 34 quoted lines

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

Show 21 quoted lines

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

Show 22 quoted lines

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