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

[Derrick Stolee <derrickstolee@github.com>]

On 11/28/2022 8:59 PM, Victoria Dye wrote:
> Derrick Stolee via GitGitGadget wrote:
>> 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.

Yes, it should. An earlier version of this follows the correct if/else if
pattern.

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

I can add that to the lib-bundle-uri-protocol.sh tests pretty easily.

Thanks,
-Stolee