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