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

From: Jonathan Nieder <jrnieder@gmail.com>
To: Xin Li <delphij@google.com>
Cc: git@vger.kernel.org, gitster@pobox.com,
	sandals@crustytoothpaste.net, iankaz@google.com,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v4] fetch: allow adding a filter after initial clone.
Date: Thu, 28 May 2020 18:01:19 -0700	[thread overview]
Message-ID: <20200529010119.GE114915@google.com> (raw)
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. ;-)

[...]
> --- 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).

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

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

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

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