Re: [PATCH v2 9/9] clone: unbundle the advertised bundles

From: Victoria Dye <vdye@github.com>
Date: Nov 29, 2022, 01:59 UTC
Message-ID: <9522d667-da4d-6eb0-8f1c-1396ebc0351c@github.com>
In-Reply-To: <b62b4b1748194f0c7b81536701f15aa0df8e1d9b.1668628303.git.gitgitgadget@gmail.com>

Derrick Stolee via GitGitGadget wrote:

Show 46 quoted lines

> From: Derrick Stolee <derrickstolee@github.com>
> 
> A previous change introduced the transport methods to acquire a bundle
> list from the 'bundle-uri' protocol v2 command, when advertised _and_
> when the client has chosen to enable the feature.
> 
> Teach Git to download and unbundle the data advertised by those bundles
> during 'git clone'.
> 
> Also, since the --bundle-uri option exists, we do not want to mix the
> advertised bundles with the user-specified bundles.
> 
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  builtin/clone.c  | 26 +++++++++++++++++----
>  t/t5601-clone.sh | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 22b1e506452..09f10477ed6 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1267,11 +1267,27 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	if (refs)
>  		mapped_refs = wanted_peer_refs(refs, &remote->fetch);
>  
> -	/*
> -	 * Populate transport->got_remote_bundle_uri and
> -	 * transport->bundle_uri. We might get nothing.
> -	 */
> -	transport_get_remote_bundle_uri(transport, 1);
> +	if (!bundle_uri) {
> +		/*
> +		* Populate transport->got_remote_bundle_uri and
> +		* transport->bundle_uri. We might get nothing.
> +		*/
> +		transport_get_remote_bundle_uri(transport, 1);
> +
> +		if (transport->bundles &&
> +		    hashmap_get_size(&transport->bundles->bundles)) {
> +			/* At this point, we need the_repository to match the cloned repo. */
> +			if (repo_init(the_repository, git_dir, work_tree))
> +				warning(_("failed to initialize the repo, skipping bundle URI"));
> +			if (fetch_bundle_list(the_repository,
> +					      remote->url[0],
> +					      transport->bundles))

If the repo initialization fails, this line is still executed. Should the condition be 'else if' to avoid that?

Otherwise, all of the added logic looks good to me.

Show 13 quoted lines

> +				warning(_("failed to fetch advertised bundles"));
> +		} else {
> +			clear_bundle_list(transport->bundles);
> +			FREE_AND_NULL(transport->bundles);
> +		}
> +	}
>  
>  	if (mapped_refs) {
>  		int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 45f0803ed4d..d1d8139751e 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh

Per the commit message:

> Also, since the --bundle-uri option exists, we do not want to mix the
> advertised bundles with the user-specified bundles.

Could you add a test verifying that '--bundle-uri' causes 'clone' to skip bundle URI auto-discovery? It's clear from the implementation above that 'clone' is currently doing that as-expected, but it might be nice to have the test for regression testing purposes.

Show 66 quoted lines

> @@ -795,6 +795,65 @@ test_expect_success 'reject cloning shallow repository using HTTP' '
>  	git clone --no-reject-shallow $HTTPD_URL/smart/repo.git repo
>  '
>  
> +test_expect_success 'auto-discover bundle URI from HTTP clone' '
> +	test_when_finished rm -rf trace.txt repo2 "$HTTPD_DOCUMENT_ROOT_PATH/repo2.git" &&
> +	git -C src bundle create "$HTTPD_DOCUMENT_ROOT_PATH/everything.bundle" --all &&
> +	git clone --bare --no-local src "$HTTPD_DOCUMENT_ROOT_PATH/repo2.git" &&
> +
> +	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo2.git" config \
> +		uploadpack.advertiseBundleURIs true &&
> +	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo2.git" config \
> +		bundle.version 1 &&
> +	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo2.git" config \
> +		bundle.mode all &&
> +	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo2.git" config \
> +		bundle.everything.uri "$HTTPD_URL/everything.bundle" &&
> +
> +	GIT_TEST_BUNDLE_URI=1 \
> +	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
> +		git -c protocol.version=2 clone \
> +		$HTTPD_URL/smart/repo2.git repo2 &&
> +	cat >pattern <<-EOF &&
> +	"event":"child_start".*"argv":\["git-remote-https","$HTTPD_URL/everything.bundle"\]
> +	EOF
> +	grep -f pattern trace.txt
> +'
> +
> +test_expect_success 'auto-discover multiple bundles from HTTP clone' '
> +	test_when_finished rm -rf trace.txt repo3 "$HTTPD_DOCUMENT_ROOT_PATH/repo3.git" &&
> +
> +	test_commit -C src new &&
> +	git -C src bundle create "$HTTPD_DOCUMENT_ROOT_PATH/new.bundle" HEAD~1..HEAD &&
> +	git clone --bare --no-local src "$HTTPD_DOCUMENT_ROOT_PATH/repo3.git" &&
> +
> +	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo3.git" config \
> +		uploadpack.advertiseBundleURIs true &&
> +	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo3.git" config \
> +		bundle.version 1 &&
> +	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo3.git" config \
> +		bundle.mode all &&
> +
> +	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo3.git" config \
> +		bundle.everything.uri "$HTTPD_URL/everything.bundle" &&
> +	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo3.git" config \
> +		bundle.new.uri "$HTTPD_URL/new.bundle" &&
> +
> +	GIT_TEST_BUNDLE_URI=1 \
> +	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
> +		git -c protocol.version=2 clone \
> +		$HTTPD_URL/smart/repo3.git repo3 &&
> +
> +	# We should fetch _both_ bundles
> +	cat >pattern <<-EOF &&
> +	"event":"child_start".*"argv":\["git-remote-https","$HTTPD_URL/everything.bundle"\]
> +	EOF
> +	grep -f pattern trace.txt &&
> +	cat >pattern <<-EOF &&
> +	"event":"child_start".*"argv":\["git-remote-https","$HTTPD_URL/new.bundle"\]
> +	EOF
> +	grep -f pattern trace.txt
> +'
> +
>  # DO NOT add non-httpd-specific tests here, because the last part of this
>  # test script is only executed when httpd is available and enabled.
>