Re: [PATCH v4] fetch: allow adding a filter after initial clone.

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 29, 2020, 01:01 UTC
Message-ID: <20200529010119.GE114915@google.com>
In-Reply-To: <20200529000432.146618-1-delphij@google.com>

(+cc: Jonathan Tan, partial clone expert) Xin Li wrote:

> Subject: fetch: allow adding a filter after initial clone.

nit: should be no period at the end of the subject line

> Signed-off-by: Xin Li <delphij@google.com>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Let's see whether I still stand by that. ;-)

[...]

Show 8 quoted lines

> --- a/setup.c
> +++ b/setup.c
> @@ -13,6 +13,9 @@ static int work_tree_config_is_bogus;
>  static struct startup_info the_startup_info;
>  struct startup_info *startup_info = &the_startup_info;
>  
> +static int verify_repository_format_eligibility(const struct repository_format *,
> +    struct strbuf *, int);

Odd wrapping, but after Junio's suggestion this goes away.

[...]

> +		warning("unable to upgrade repository format from %d to %d: %s",
> +		    repo_fmt.version, target_version, err.buf);

Same wrapping nit: this should use a tab to line up right after the parenthesis (Git uses 8-space tabs).

[...]

Show 7 quoted lines

> --- a/t/t1090-sparse-checkout-scope.sh
> +++ b/t/t1090-sparse-checkout-scope.sh
> @@ -63,7 +63,6 @@ test_expect_success 'in partial clone, sparse checkout only fetches needed blobs
>  	git -C server commit -m message &&
>  
>  	test_config -C client core.sparsecheckout 1 &&
> -	test_config -C client extensions.partialclone origin &&

Nice.

Show 13 quoted lines

>  	echo "!/*" >client/.git/info/sparse-checkout &&
>  	echo "/a" >>client/.git/info/sparse-checkout &&
>  	git -C client fetch --filter=blob:none origin &&
> diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh
> index 286121d8de..9536d10919 100755
> --- a/t/t2404-worktree-config.sh
> +++ b/t/t2404-worktree-config.sh
> @@ -23,8 +23,10 @@ test_expect_success 'config --worktree without extension' '
>  '
>  
>  test_expect_success 'enable worktreeConfig extension' '
> +	git config core.repositoryformatversion 1 &&
>  	git config extensions.worktreeConfig true &&

Yes, makes sense. Does this patch need it, or could this go in a separate patch?

> -	test_cmp_config true extensions.worktreeConfig
> +	test_cmp_config true extensions.worktreeConfig &&
> +	test_cmp_config 1 core.repositoryformatversion

This (both the existing code and the modified version) is strange: we just set the config, so why are we checking it?

[...]

Show 8 quoted lines

> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -999,7 +999,6 @@ fetch_filter_blob_limit_zero () {
>  	test_config -C "$SERVER" uploadpack.allowfilter 1 &&
>  
>  	git clone "$URL" client &&
> -	test_config -C client extensions.partialclone origin &&
>

Nice.

[...]

Show 10 quoted lines

> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -348,7 +348,6 @@ test_expect_success 'partial fetch' '
>  	rm -rf client "$(pwd)/trace" &&
>  	git init client &&
>  	SERVER="file://$(pwd)/server" &&
> -	test_config -C client extensions.partialClone "$SERVER" &&
>  
>  	GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
>  		fetch --filter=blob:none "$SERVER" master:refs/heads/other &&

This test is a bit unusual, since it's fetching by URL instead of from a remote. Looks like it comes from

	commit ba95710a3bdcb2a80495b1d93a0e482dd69905e1
	Author: Jonathan Tan <jonathantanmy@google.com>
	Date:   Thu May 3 16:46:56 2018 -0700

	    {fetch,upload}-pack: support filter in protocol v2

Jonathan, thoughts about this one? Is making it set extensions.partialClone implicitly like the change above a good change?

With the nits above and the bits Junio described addressed, this would
indeed be
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.