On 24/07/24 04:49PM, Toon Claes wrote:
> The bundle list transport->bundles is filled by
> transport_get_remote_bundle_uri(). Only when the list is not used, it is
> cleared right away by calling clear_bundle_list().
>
> This looks like we leak memory allocated for the list when
> transport->bundles *is* used. But in fact, transport->bundles is cleaned
> up in transport_disconnect() near the end of cmd_clone().
The `transport->bundles` is setup and initialized by `transport_get()`
and populated by `transport_get_remote_bundle_uri()`. In its current
form, we clean up `transport->bundles` if it is empty immediately after
checking the remote for bundles. This is redundant though because the
cleanup already occurs as part of `transport_disconnect()`.
Since `transport_disconnect()` is already responsible for freeing other
parts of `transport`, it makes sense to let it be the one to handle it.
>
> Remove the double clean up of transport->bundles, and depend solely on
> transport_disconnect() to take care of it.
>
> Also add a test case that hits this code, but due to other leaks we
> cannot mark it as leak-free.
>
> Signed-off-by: Toon Claes <toon@xxxxxxxxx>
> ---
> builtin/clone.c | 3 ---
> t/t5558-clone-bundle-uri.sh | 28 +++++++++++++++++++++++++++-
> 2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index af6017d41a..aa507395a0 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1419,9 +1419,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> else if (fetch_bundle_list(the_repository,
> transport->bundles))
> warning(_("failed to fetch advertised bundles"));
> - } else {
> - clear_bundle_list(transport->bundles);
> - FREE_AND_NULL(transport->bundles);
> }
> }
>
> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
> index cd05321e17..2d6e690fbe 100755
> --- a/t/t5558-clone-bundle-uri.sh
> +++ b/t/t5558-clone-bundle-uri.sh
> @@ -1,6 +1,6 @@
> #!/bin/sh
>
> -test_description='test fetching bundles with --bundle-uri'
> +test_description='test clone with use of bundle-uri'
I assume this description was changed because the test pertains to
clones and not fetches. Might be worth mentioning in the commit message.
>
> . ./test-lib.sh
> . "$TEST_DIRECTORY"/lib-bundle.sh
> @@ -438,6 +438,32 @@ test_expect_success 'negotiation: bundle list with all wanted commits' '
> test_grep ! "clone> want " trace-packet.txt
> '
>
> +test_expect_success 'bundles advertised by the server' '
> + test_when_finished rm -f trace*.txt &&
> + git clone clone-from clone-advertiser &&
> + git -C clone-advertiser config uploadpack.advertiseBundleURIs true &&
> + git -C clone-advertiser config bundle.version 1 &&
> + git -C clone-advertiser config bundle.mode all &&
> + git -C clone-advertiser config bundle.bundle-1.uri "file://$(pwd)/clone-from/bundle-1.bundle" &&
> + git -C clone-advertiser config bundle.bundle-2.uri "file://$(pwd)/clone-from/bundle-2.bundle" &&
> + git -C clone-advertiser config bundle.bundle-3.uri "file://$(pwd)/clone-from/bundle-3.bundle" &&
> + git -C clone-advertiser config bundle.bundle-4.uri "file://$(pwd)/clone-from/bundle-4.bundle" &&
> +
> + GIT_TRACE_PACKET="$(pwd)/trace-packet.txt" \
> + git -c transfer.bundleURI=true clone clone-advertiser clone-advertised &&
> + git -C clone-advertised for-each-ref --format="%(refname)" >refs &&
> + grep "refs/bundles/" refs >actual &&
> + cat >expect <<-\EOF &&
> + refs/bundles/base
> + refs/bundles/left
> + refs/bundles/merge
> + refs/bundles/right
> + EOF
> + test_cmp expect actual &&
> + # We already have all needed commits so no "want" needed.
> + test_grep ! "clone> want " trace-packet.txt
> +'
> +
Looks like this test is validating that a clone is retrieving bundles
from a remote repository advertising said bundles. We to do so by
checking for the `refs/bundles/*` references which should only exist if
the advertised bundles were fetched. This makes sense to me.
