| Message ID | 20240724144957.3033840-2-toon@iotcl.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fetch: use bundle URIs when having creationToken heuristic | expand |
Toon Claes <toon@iotcl.com> writes: > 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(). > > 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@iotcl.com> > --- > 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); > There is a small difference that here we also set `transport->bundles` to NULL, whereas in `transport_disconnect()` we only do `free(...)`. I don't see any issues because of it though. This cleanup looks good. [snip]
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@iotcl.com> > --- > 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. From the commit message though, I thought the added test might have something to do with the changes to when `transport->bundles` was freed. I wonder if it would be a good idea to expland on this further in the commit message. Or since the added test is somewhat unrelated to this patch, maybe it should be a separate patch? -Justin > ######################################################################### > # HTTP tests begin here > > -- > 2.45.2 >
Justin Tobler <jltobler@gmail.com> writes: > From the commit message though, I thought the added test might have > something to do with the changes to when `transport->bundles` was freed. > I wonder if it would be a good idea to expland on this further in the > commit message. Or since the added test is somewhat unrelated to this > patch, maybe it should be a separate patch? I added this test case because I wanted to debug the code path where the remote advertises bundle URIs, all other test cases use --bundle-uri. This helped me to debug the code and understand where the `transport->bundles` are freed. Although this test case does not fully touch the code path of the code that's removed, but it was close enough to hack a few extra lines and understand what happens. Since I wrote that test anyway, I felt I might as well submit it.
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' . ./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 +' + ######################################################################### # HTTP tests begin here
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(). 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@iotcl.com> --- builtin/clone.c | 3 --- t/t5558-clone-bundle-uri.sh | 28 +++++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 4 deletions(-) -- 2.45.2