{"thread":{"id":"61819","subject":"[PATCH] fetch: use bundle URIs when having creationToken heuristic","startedAt":"2024-07-22T08:07:37Z","lastAt":"2024-09-27T09:05:32Z","messageCount":17,"participants":["Toon Claes","Junio C Hamano","Karthik Nayak","Patrick Steinhardt","Justin Tobler","Toon claes"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"499049","messageId":"20240722080705.2614195-1-toon@iotcl.com","threadId":"61819","inReplyTo":null,"subject":"[PATCH] fetch: use bundle URIs when having creationToken heuristic","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-07-22T08:07:05Z","receivedAt":"2024-07-22T08:07:37Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"At the moment, bundle URIs are only used on git-clone(1). For a clone\nthe use of bundle URI is trivial, because repository is empty so\ndownloading bundles will never result in downloading objects that are in\nthe repository already.\n\nFor git-fetch(1), this more complicated to use bundle URI. We want to\navoid downloading bundles that only contains objects that are in the\nlocal repository already.\n\nOne way to achieve this is possible when the \"creationToken\" heuristic\nis used for bundle URIs. With this heuristic, the server sends along a\ncreationToken for each bundle. When a client downloads bundles with such\ncreationToken, the highest known value is written to\n`fetch.bundleCreationToken` in the git-config. The next time bundles are\nadvertised by the server, bundles with a lower creationToken value are\nignored. This was already implemented by 7903efb717 (bundle-uri:\ndownload in creationToken order, 2023-01-31) in\nfetch_bundles_by_token().\n\nUsing the creationToken heuristic is optional, but without it the\nclient has no idea which bundles are new and which only have objects the\nclient already has.\n\nWith this knowledge, make git-fetch(1) get bundle URI information from\nthe server, but only use bundle URIs if the creationToken heuristic is\nused.\n\nThe code added in builtin/fetch.c is very similar to the code in\nbuiltin/clone.c, so while at it make sure we always clean up the bundles\nlist advertised by the server.\n\nSigned-off-by: Toon Claes \n---\n builtin/clone.c | 13 +++++-----\n builtin/fetch.c | 17 +++++++++++++\n t/t5584-fetch-bundle-uri.sh | 49 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 73 insertions(+), 6 deletions(-)\n create mode 100755 t/t5584-fetch-bundle-uri.sh\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex af6017d41a..29f0470aea 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1406,9 +1406,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tgit_config_set_gently(\"fetch.bundleuri\", bundle_uri);\n \t} else {\n \t\t/*\n-\t\t* Populate transport->got_remote_bundle_uri and\n-\t\t* transport->bundle_uri. We might get nothing.\n-\t\t*/\n+\t\t * Populate transport->bundles. We might get nothing or fail\n+\t\t * trying, but clone can continue without bundles as well.\n+\t\t */\n \t\ttransport_get_remote_bundle_uri(transport);\n\n \t\tif (transport->bundles &&\n@@ -1419,10 +1419,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\telse if (fetch_bundle_list(the_repository,\n \t\t\t\t\t\t transport->bundles))\n \t\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n-\t\t} else {\n-\t\t\tclear_bundle_list(transport->bundles);\n-\t\t\tFREE_AND_NULL(transport->bundles);\n \t\t}\n+\n+\t\tclear_bundle_list(transport->bundles);\n+\t\tif (transport->bundles)\n+\t\t\tFREE_AND_NULL(transport->bundles);\n \t}\n\n \tif (refs)\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 693f02b958..1e944f81af 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1694,6 +1694,23 @@ static int do_fetch(struct transport *transport,\n \t\t\tretcode = 1;\n \t}\n\n+\t/*\n+\t * Populate transport->bundles. We might get nothing or fail\n+\t * trying, but fetch can continue without bundles as well.\n+\t */\n+\ttransport_get_remote_bundle_uri(transport);\n+\n+\tif (transport->bundles &&\n+\t hashmap_get_size(&transport->bundles->bundles) &&\n+\t (transport->bundles->heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN)) {\n+\t\tif (fetch_bundle_list(the_repository, transport->bundles))\n+\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n+\t}\n+\n+\tclear_bundle_list(transport->bundles);\n+\tif (transport->bundles)\n+\t\tFREE_AND_NULL(transport->bundles);\n+\n \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n \t\t\t\t &fetch_head, config)) {\n \t\tretcode = 1;\ndiff --git a/t/t5584-fetch-bundle-uri.sh b/t/t5584-fetch-bundle-uri.sh\nnew file mode 100755\nindex 0000000000..6c2383646e\n--- /dev/null\n+++ b/t/t5584-fetch-bundle-uri.sh\n@@ -0,0 +1,49 @@\n+#!/bin/sh\n+\n+test_description='test use of bundle URI in \"git fetch\"'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'set up repos and bundles' '\n+\tgit init source &&\n+\ttest_commit -C source A &&\n+\tgit clone --no-local source go-A-to-C &&\n+\ttest_commit -C source B &&\n+\tgit clone --no-local source go-B-to-C &&\n+\tgit clone --no-local source go-B-to-D &&\n+\tgit -C source bundle create B.bundle main &&\n+\ttest_commit -C source C &&\n+\tgit -C source bundle create B-to-C.bundle B..main &&\n+\tgit -C source config uploadpack.advertiseBundleURIs true &&\n+\tgit -C source config bundle.version 1 &&\n+\tgit -C source config bundle.mode all &&\n+\tgit -C source config bundle.heuristic creationToken &&\n+\tgit -C source config bundle.bundle-B.uri \"file://$(pwd)/source/B.bundle\" &&\n+\tgit -C source config bundle.bundle-B.creationToken 1 &&\n+\tgit -C source config bundle.bundle-B-to-C.uri \"file://$(pwd)/source/B-to-C.bundle\" &&\n+\tgit -C source config bundle.bundle-B-to-C.creationToken 2\n+'\n+\n+test_expect_success 'fetches one bundle URI to get up-to-date' '\n+\tgit -C go-B-to-C -c transfer.bundleURI=true fetch origin &&\n+\ttest 1 = $(ls go-B-to-C/.git/objects/bundles | wc -l) &&\n+\ttest 2 = $(git -C go-B-to-C config fetch.bundleCreationToken)\n+'\n+\n+test_expect_success 'fetches two bundle URIs to get up-to-date' '\n+\tgit -C go-A-to-C -c transfer.bundleURI=true fetch origin &&\n+\ttest 2 = $(ls go-A-to-C/.git/objects/bundles | wc -l) &&\n+\ttest 2 = $(git -C go-A-to-C config fetch.bundleCreationToken)\n+'\n+\n+test_expect_success 'fetches one bundle URI and objects from remote' '\n+\ttest_commit -C source D &&\n+\tgit -C go-B-to-D -c transfer.bundleURI=true fetch origin &&\n+\ttest 1 = $(ls go-B-to-D/.git/objects/bundles | wc -l) &&\n+\ttest 2 = $(git -C go-B-to-D config fetch.bundleCreationToken)\n+'\n+\n+test_done\n--\n2.45.2\n"},{"id":"499073","messageId":"xmqqmsm9b77v.fsf@gitster.g","threadId":"61819","inReplyTo":"20240722080705.2614195-1-toon@iotcl.com","subject":"Re: [PATCH] fetch: use bundle URIs when having creationToken heuristic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-22T18:40:52Z","receivedAt":"2024-07-22T18:40:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Toon Claes writes:\n\n> At the moment, bundle URIs are only used on git-clone(1). For a clone\n> the use of bundle URI is trivial, because repository is empty so\n> downloading bundles will never result in downloading objects that are in\n> the repository already.\n>\n> For git-fetch(1), this more complicated to use bundle URI. We want to\n> avoid downloading bundles that only contains objects that are in the\n> local repository already.\n>\n> One way to achieve this is possible when the \"creationToken\" heuristic\n> is used for bundle URIs. With this heuristic, the server sends along a\n> creationToken for each bundle. When a client downloads bundles with such\n> creationToken, the highest known value is written to\n> `fetch.bundleCreationToken` in the git-config. The next time bundles are\n> advertised by the server, bundles with a lower creationToken value are\n> ignored. This was already implemented by 7903efb717 (bundle-uri:\n> download in creationToken order, 2023-01-31) in\n> fetch_bundles_by_token().\n>\n> Using the creationToken heuristic is optional, but without it the\n> client has no idea which bundles are new and which only have objects the\n> client already has.\n\nThe above explains that you can see which bundle is newer than which\nother ones, but ...\n\n> With this knowledge, make git-fetch(1) get bundle URI information from\n> the server, but only use bundle URIs if the creationToken heuristic is\n> used.\n\n... it does not explain how it helps for the repository that runs\n\"fetch\" to tell which bundles do not give any new objects it does\nnot have. \"clone\" starts from an emptyness, so for it it is enough\nif it can figure out among N bundles the other end has, which ones\nare sufficient to cover the entire set of objects the other end has,\nbut \"fetch\" needs to be able to exclude a lot more---the objects\nthis end has need to be excluded\". The logic to do so may need to\nbe explained a bit better than the above to make it more convincing.\n\nOn the other hand...\n\n> The code added in builtin/fetch.c is very similar to the code in\n> builtin/clone.c, so while at it make sure we always clean up the bundles\n> list advertised by the server.\n\n... I do understand that clean-up code. Please make it a separate\npreliminary clean-up patch before the main thing.\n\nIf the \"clone\" and \"fetch\" codepath uses very similar logic, can the\ncode in \"clone\" the preliminary clean-up patch would fix be\nrefactored in another separate patch on top of the clean-up patch to\na helper function that can be called from outside? And then can\n\"fetch\" be taught, on top of those two preliminary patches, to call\nthat helper function to implement what this single patch wanted to\ndo?\n\nThanks.\n\n> Signed-off-by: Toon Claes \n> ---\n> builtin/clone.c | 13 +++++-----\n> builtin/fetch.c | 17 +++++++++++++\n> t/t5584-fetch-bundle-uri.sh | 49 +++++++++++++++++++++++++++++++++++++\n> 3 files changed, 73 insertions(+), 6 deletions(-)\n> create mode 100755 t/t5584-fetch-bundle-uri.sh\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index af6017d41a..29f0470aea 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1406,9 +1406,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \t\t\tgit_config_set_gently(\"fetch.bundleuri\", bundle_uri);\n> \t} else {\n> \t\t/*\n> -\t\t* Populate transport->got_remote_bundle_uri and\n> -\t\t* transport->bundle_uri. We might get nothing.\n> -\t\t*/\n> +\t\t * Populate transport->bundles. We might get nothing or fail\n> +\t\t * trying, but clone can continue without bundles as well.\n> +\t\t */\n> \t\ttransport_get_remote_bundle_uri(transport);\n>\n> \t\tif (transport->bundles &&\n> @@ -1419,10 +1419,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \t\t\telse if (fetch_bundle_list(the_repository,\n> \t\t\t\t\t\t transport->bundles))\n> \t\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n> -\t\t} else {\n> -\t\t\tclear_bundle_list(transport->bundles);\n> -\t\t\tFREE_AND_NULL(transport->bundles);\n> \t\t}\n> +\n> +\t\tclear_bundle_list(transport->bundles);\n> +\t\tif (transport->bundles)\n> +\t\t\tFREE_AND_NULL(transport->bundles);\n> \t}\n>\n> \tif (refs)\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 693f02b958..1e944f81af 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1694,6 +1694,23 @@ static int do_fetch(struct transport *transport,\n> \t\t\tretcode = 1;\n> \t}\n>\n> +\t/*\n> +\t * Populate transport->bundles. We might get nothing or fail\n> +\t * trying, but fetch can continue without bundles as well.\n> +\t */\n> +\ttransport_get_remote_bundle_uri(transport);\n> +\n> +\tif (transport->bundles &&\n> +\t hashmap_get_size(&transport->bundles->bundles) &&\n> +\t (transport->bundles->heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN)) {\n> +\t\tif (fetch_bundle_list(the_repository, transport->bundles))\n> +\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n> +\t}\n> +\n> +\tclear_bundle_list(transport->bundles);\n> +\tif (transport->bundles)\n> +\t\tFREE_AND_NULL(transport->bundles);\n> +\n> \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n> \t\t\t\t &fetch_head, config)) {\n> \t\tretcode = 1;\n> diff --git a/t/t5584-fetch-bundle-uri.sh b/t/t5584-fetch-bundle-uri.sh\n> new file mode 100755\n> index 0000000000..6c2383646e\n> --- /dev/null\n> +++ b/t/t5584-fetch-bundle-uri.sh\n> @@ -0,0 +1,49 @@\n> +#!/bin/sh\n> +\n> +test_description='test use of bundle URI in \"git fetch\"'\n> +\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'set up repos and bundles' '\n> +\tgit init source &&\n> +\ttest_commit -C source A &&\n> +\tgit clone --no-local source go-A-to-C &&\n> +\ttest_commit -C source B &&\n> +\tgit clone --no-local source go-B-to-C &&\n> +\tgit clone --no-local source go-B-to-D &&\n> +\tgit -C source bundle create B.bundle main &&\n> +\ttest_commit -C source C &&\n> +\tgit -C source bundle create B-to-C.bundle B..main &&\n> +\tgit -C source config uploadpack.advertiseBundleURIs true &&\n> +\tgit -C source config bundle.version 1 &&\n> +\tgit -C source config bundle.mode all &&\n> +\tgit -C source config bundle.heuristic creationToken &&\n> +\tgit -C source config bundle.bundle-B.uri \"file://$(pwd)/source/B.bundle\" &&\n> +\tgit -C source config bundle.bundle-B.creationToken 1 &&\n> +\tgit -C source config bundle.bundle-B-to-C.uri \"file://$(pwd)/source/B-to-C.bundle\" &&\n> +\tgit -C source config bundle.bundle-B-to-C.creationToken 2\n> +'\n> +\n> +test_expect_success 'fetches one bundle URI to get up-to-date' '\n> +\tgit -C go-B-to-C -c transfer.bundleURI=true fetch origin &&\n> +\ttest 1 = $(ls go-B-to-C/.git/objects/bundles | wc -l) &&\n> +\ttest 2 = $(git -C go-B-to-C config fetch.bundleCreationToken)\n> +'\n> +\n> +test_expect_success 'fetches two bundle URIs to get up-to-date' '\n> +\tgit -C go-A-to-C -c transfer.bundleURI=true fetch origin &&\n> +\ttest 2 = $(ls go-A-to-C/.git/objects/bundles | wc -l) &&\n> +\ttest 2 = $(git -C go-A-to-C config fetch.bundleCreationToken)\n> +'\n> +\n> +test_expect_success 'fetches one bundle URI and objects from remote' '\n> +\ttest_commit -C source D &&\n> +\tgit -C go-B-to-D -c transfer.bundleURI=true fetch origin &&\n> +\ttest 1 = $(ls go-B-to-D/.git/objects/bundles | wc -l) &&\n> +\ttest 2 = $(git -C go-B-to-D config fetch.bundleCreationToken)\n> +'\n> +\n> +test_done\n> --\n> 2.45.2\n"},{"id":"499262","messageId":"20240724144957.3033840-1-toon@iotcl.com","threadId":"61819","inReplyTo":"20240722080705.2614195-1-toon@iotcl.com","subject":"[PATCH v2 0/3] fetch: use bundle URIs when having creationToken heuristic","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-07-24T14:49:54Z","receivedAt":"2024-07-24T14:50:30Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"This is version 2 of teaching git-fetch(1) to use bundle URIs. For this second\nrevision I've split up the changes in multliple commits as Junio suggested.\n\n - The first patch cleans up some unneccessary code.\n\n - The second patch refactor the code to make it possible to reuse.\n\n - The third patch makes git-fetch(1) use bundle URIs when creationToken\n heuristic is used. This time I've added some more information about how\n bundle URIs are handled when they have a creationToken heuristic.\n\nToon Claes (3):\n clone: remove double bundle list clear code\n transport: introduce transport_has_remote_bundle_uri()\n fetch: use bundle URIs when having creationToken heuristic\n\n builtin/clone.c | 26 ++++++--------------\n builtin/fetch.c | 13 ++++++++++\n t/helper/test-bundle-uri.c | 2 +-\n t/t5558-clone-bundle-uri.sh | 28 ++++++++++++++++++++-\n t/t5584-fetch-bundle-uri.sh | 49 +++++++++++++++++++++++++++++++++++++\n transport.c | 14 ++++++++++-\n transport.h | 7 +++---\n 7 files changed, 114 insertions(+), 25 deletions(-)\n create mode 100755 t/t5584-fetch-bundle-uri.sh\n\nRange-diff against v1:\n-: ---------- > 1: fb556a76d3 clone: remove double bundle list clear code\n1: 00f6017ab0 ! 2: 0f7c53bbe1 fetch: use bundle URIs when having creationToken heuristic\n @@ Metadata\n Author: Toon Claes \n\n ## Commit message ##\n - fetch: use bundle URIs when having creationToken heuristic\n + transport: introduce transport_has_remote_bundle_uri()\n\n - At the moment, bundle URIs are only used on git-clone(1). For a clone\n - the use of bundle URI is trivial, because repository is empty so\n - downloading bundles will never result in downloading objects that are in\n - the repository already.\n + The public function transport_get_remote_bundle_uri() exists to fetch\n + the bundle URI(s) from the remote. This function is only called from\n + builtin/clone.c (not taking test-tool into account). There it ignores\n + the return value, because it doesn't matter whether the server didn't\n + return any bundles or if it failed trying to fetch them, clone can\n + continue without bundle URIs. After calling it, it checks if anything\n + is collected in the bundle list and starts fetching them.\n\n - For git-fetch(1), this more complicated to use bundle URI. We want to\n - avoid downloading bundles that only contains objects that are in the\n - local repository already.\n -\n - One way to achieve this is possible when the \"creationToken\" heuristic\n - is used for bundle URIs. With this heuristic, the server sends along a\n - creationToken for each bundle. When a client downloads bundles with such\n - creationToken, the highest known value is written to\n - `fetch.bundleCreationToken` in the git-config. The next time bundles are\n - advertised by the server, bundles with a lower creationToken value are\n - ignored. This was already implemented by 7903efb717 (bundle-uri:\n - download in creationToken order, 2023-01-31) in\n - fetch_bundles_by_token().\n -\n - Using the creationToken heuristic is optional, but without it the\n - client has no idea which bundles are new and which only have objects the\n - client already has.\n -\n - With this knowledge, make git-fetch(1) get bundle URI information from\n - the server, but only use bundle URIs if the creationToken heuristic is\n - used.\n -\n - The code added in builtin/fetch.c is very similar to the code in\n - builtin/clone.c, so while at it make sure we always clean up the bundles\n - list advertised by the server.\n + Add public function transport_has_remote_bundle_uri() instead. This\n + calls the (now made private) transport_get_remote_bundle_uri() function\n + and returns whether any bundle URI is received. This makes reuse of the\n + code easier and avoids code duplication when we add bundle URI support\n + to git-fetch(1).\n\n Signed-off-by: Toon Claes \n\n ## builtin/clone.c ##\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n + \t\t\t\tbundle_uri);\n + \t\telse if (has_heuristic)\n \t\t\tgit_config_set_gently(\"fetch.bundleuri\", bundle_uri);\n - \t} else {\n - \t\t/*\n +-\t} else {\n +-\t\t/*\n -\t\t* Populate transport->got_remote_bundle_uri and\n -\t\t* transport->bundle_uri. We might get nothing.\n -\t\t*/\n -+\t\t * Populate transport->bundles. We might get nothing or fail\n -+\t\t * trying, but clone can continue without bundles as well.\n -+\t\t */\n - \t\ttransport_get_remote_bundle_uri(transport);\n -\n - \t\tif (transport->bundles &&\n -@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n - \t\t\telse if (fetch_bundle_list(the_repository,\n - \t\t\t\t\t\t transport->bundles))\n - \t\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n --\t\t} else {\n --\t\t\tclear_bundle_list(transport->bundles);\n --\t\t\tFREE_AND_NULL(transport->bundles);\n - \t\t}\n -+\n -+\t\tclear_bundle_list(transport->bundles);\n -+\t\tif (transport->bundles)\n -+\t\t\tFREE_AND_NULL(transport->bundles);\n +-\t\ttransport_get_remote_bundle_uri(transport);\n +-\n +-\t\tif (transport->bundles &&\n +-\t\t hashmap_get_size(&transport->bundles->bundles)) {\n +-\t\t\t/* At this point, we need the_repository to match the cloned repo. */\n +-\t\t\tif (repo_init(the_repository, git_dir, work_tree))\n +-\t\t\t\twarning(_(\"failed to initialize the repo, skipping bundle URI\"));\n +-\t\t\telse if (fetch_bundle_list(the_repository,\n +-\t\t\t\t\t\t transport->bundles))\n +-\t\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n +-\t\t}\n ++\t} else if (transport_has_remote_bundle_uri(transport)) {\n ++\t\t/* At this point, we need the_repository to match the cloned repo. */\n ++\t\tif (repo_init(the_repository, git_dir, work_tree))\n ++\t\t\twarning(_(\"failed to initialize the repo, skipping bundle URI\"));\n ++\t\telse if (fetch_bundle_list(the_repository,\n ++\t\t\t\t\t transport->bundles))\n ++\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n \t}\n\n \tif (refs)\n\n - ## builtin/fetch.c ##\n -@@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n - \t\t\tretcode = 1;\n + ## t/helper/test-bundle-uri.c ##\n +@@ t/helper/test-bundle-uri.c: static int cmd_ls_remote(int argc, const char **argv)\n \t}\n\n -+\t/*\n -+\t * Populate transport->bundles. We might get nothing or fail\n -+\t * trying, but fetch can continue without bundles as well.\n -+\t */\n -+\ttransport_get_remote_bundle_uri(transport);\n -+\n -+\tif (transport->bundles &&\n -+\t hashmap_get_size(&transport->bundles->bundles) &&\n -+\t (transport->bundles->heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN)) {\n -+\t\tif (fetch_bundle_list(the_repository, transport->bundles))\n -+\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n -+\t}\n -+\n -+\tclear_bundle_list(transport->bundles);\n -+\tif (transport->bundles)\n -+\t\tFREE_AND_NULL(transport->bundles);\n -+\n - \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n - \t\t\t\t &fetch_head, config)) {\n - \t\tretcode = 1;\n + \ttransport = transport_get(remote, NULL);\n +-\tif (transport_get_remote_bundle_uri(transport) < 0) {\n ++\tif (!transport_has_remote_bundle_uri(transport)) {\n + \t\terror(_(\"could not get the bundle-uri list\"));\n + \t\tstatus = 1;\n + \t\tgoto cleanup;\n\n - ## t/t5584-fetch-bundle-uri.sh (new) ##\n -@@\n -+#!/bin/sh\n -+\n -+test_description='test use of bundle URI in \"git fetch\"'\n -+\n -+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n -+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n -+\n -+. ./test-lib.sh\n -+\n -+test_expect_success 'set up repos and bundles' '\n -+\tgit init source &&\n -+\ttest_commit -C source A &&\n -+\tgit clone --no-local source go-A-to-C &&\n -+\ttest_commit -C source B &&\n -+\tgit clone --no-local source go-B-to-C &&\n -+\tgit clone --no-local source go-B-to-D &&\n -+\tgit -C source bundle create B.bundle main &&\n -+\ttest_commit -C source C &&\n -+\tgit -C source bundle create B-to-C.bundle B..main &&\n -+\tgit -C source config uploadpack.advertiseBundleURIs true &&\n -+\tgit -C source config bundle.version 1 &&\n -+\tgit -C source config bundle.mode all &&\n -+\tgit -C source config bundle.heuristic creationToken &&\n -+\tgit -C source config bundle.bundle-B.uri \"file://$(pwd)/source/B.bundle\" &&\n -+\tgit -C source config bundle.bundle-B.creationToken 1 &&\n -+\tgit -C source config bundle.bundle-B-to-C.uri \"file://$(pwd)/source/B-to-C.bundle\" &&\n -+\tgit -C source config bundle.bundle-B-to-C.creationToken 2\n -+'\n + ## transport.c ##\n +@@ transport.c: int transport_fetch_refs(struct transport *transport, struct ref *refs)\n + \treturn rc;\n + }\n +\n +-int transport_get_remote_bundle_uri(struct transport *transport)\n ++static int transport_get_remote_bundle_uri(struct transport *transport)\n + {\n + \tint value = 0;\n + \tconst struct transport_vtable *vtable = transport->vtable;\n +@@ transport.c: int transport_get_remote_bundle_uri(struct transport *transport)\n +\n + \tif (vtable->get_bundle_uri(transport) < 0)\n + \t\treturn error(_(\"could not retrieve server-advertised bundle-uri list\"));\n +\n -+test_expect_success 'fetches one bundle URI to get up-to-date' '\n -+\tgit -C go-B-to-C -c transfer.bundleURI=true fetch origin &&\n -+\ttest 1 = $(ls go-B-to-C/.git/objects/bundles | wc -l) &&\n -+\ttest 2 = $(git -C go-B-to-C config fetch.bundleCreationToken)\n -+'\n ++\treturn 0;\n ++}\n +\n -+test_expect_success 'fetches two bundle URIs to get up-to-date' '\n -+\tgit -C go-A-to-C -c transfer.bundleURI=true fetch origin &&\n -+\ttest 2 = $(ls go-A-to-C/.git/objects/bundles | wc -l) &&\n -+\ttest 2 = $(git -C go-A-to-C config fetch.bundleCreationToken)\n -+'\n ++int transport_has_remote_bundle_uri(struct transport *transport)\n ++{\n ++\ttransport_get_remote_bundle_uri(transport);\n +\n -+test_expect_success 'fetches one bundle URI and objects from remote' '\n -+\ttest_commit -C source D &&\n -+\tgit -C go-B-to-D -c transfer.bundleURI=true fetch origin &&\n -+\ttest 1 = $(ls go-B-to-D/.git/objects/bundles | wc -l) &&\n -+\ttest 2 = $(git -C go-B-to-D config fetch.bundleCreationToken)\n -+'\n ++\tif (transport->bundles &&\n ++\t hashmap_get_size(&transport->bundles->bundles))\n ++\t\treturn 1;\n +\n -+test_done\n + \treturn 0;\n + }\n +\n +\n + ## transport.h ##\n +@@ transport.h: const struct ref *transport_get_remote_refs(struct transport *transport,\n + \t\t\t\t\t struct transport_ls_refs_options *transport_options);\n +\n + /**\n +- * Retrieve bundle URI(s) from a remote. Populates \"struct\n +- * transport\"'s \"bundle_uri\" and \"got_remote_bundle_uri\".\n ++ * Try fetch bundle URI(s) from a remote and returns 1 if one or more\n ++ * bundle URI(s) are received from the server.\n ++ * Populates \"struct transport\"'s \"bundles\" and \"got_remote_bundle_uri\".\n + */\n +-int transport_get_remote_bundle_uri(struct transport *transport);\n ++int transport_has_remote_bundle_uri(struct transport *transport);\n +\n + /*\n + * Fetch the hash algorithm used by a remote.\n-: ---------- > 3: db8f303dde fetch: use bundle URIs when having creationToken heuristic\n--\n2.45.2\n"},{"id":"499263","messageId":"20240724144957.3033840-2-toon@iotcl.com","threadId":"61819","inReplyTo":"20240724144957.3033840-1-toon@iotcl.com","subject":"[PATCH v2 1/3] clone: remove double bundle list clear code","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-07-24T14:49:55Z","receivedAt":"2024-07-24T14:50:40Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"The bundle list transport->bundles is filled by\ntransport_get_remote_bundle_uri(). Only when the list is not used, it is\ncleared right away by calling clear_bundle_list().\n\nThis looks like we leak memory allocated for the list when\ntransport->bundles *is* used. But in fact, transport->bundles is cleaned\nup in transport_disconnect() near the end of cmd_clone().\n\nRemove the double clean up of transport->bundles, and depend solely on\ntransport_disconnect() to take care of it.\n\nAlso add a test case that hits this code, but due to other leaks we\ncannot mark it as leak-free.\n\nSigned-off-by: Toon Claes \n---\n builtin/clone.c | 3 ---\n t/t5558-clone-bundle-uri.sh | 28 +++++++++++++++++++++++++++-\n 2 files changed, 27 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex af6017d41a..aa507395a0 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1419,9 +1419,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\telse if (fetch_bundle_list(the_repository,\n \t\t\t\t\t\t transport->bundles))\n \t\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n-\t\t} else {\n-\t\t\tclear_bundle_list(transport->bundles);\n-\t\t\tFREE_AND_NULL(transport->bundles);\n \t\t}\n \t}\n\ndiff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh\nindex cd05321e17..2d6e690fbe 100755\n--- a/t/t5558-clone-bundle-uri.sh\n+++ b/t/t5558-clone-bundle-uri.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n\n-test_description='test fetching bundles with --bundle-uri'\n+test_description='test clone with use of bundle-uri'\n\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-bundle.sh\n@@ -438,6 +438,32 @@ test_expect_success 'negotiation: bundle list with all wanted commits' '\n \ttest_grep ! \"clone> want \" trace-packet.txt\n '\n\n+test_expect_success 'bundles advertised by the server' '\n+\ttest_when_finished rm -f trace*.txt &&\n+\tgit clone clone-from clone-advertiser &&\n+\tgit -C clone-advertiser config uploadpack.advertiseBundleURIs true &&\n+\tgit -C clone-advertiser config bundle.version 1 &&\n+\tgit -C clone-advertiser config bundle.mode all &&\n+\tgit -C clone-advertiser config bundle.bundle-1.uri \"file://$(pwd)/clone-from/bundle-1.bundle\" &&\n+\tgit -C clone-advertiser config bundle.bundle-2.uri \"file://$(pwd)/clone-from/bundle-2.bundle\" &&\n+\tgit -C clone-advertiser config bundle.bundle-3.uri \"file://$(pwd)/clone-from/bundle-3.bundle\" &&\n+\tgit -C clone-advertiser config bundle.bundle-4.uri \"file://$(pwd)/clone-from/bundle-4.bundle\" &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace-packet.txt\" \\\n+\tgit -c transfer.bundleURI=true clone clone-advertiser clone-advertised &&\n+\tgit -C clone-advertised for-each-ref --format=\"%(refname)\" >refs &&\n+\tgrep \"refs/bundles/\" refs >actual &&\n+\tcat >expect <<-\\EOF &&\n+\trefs/bundles/base\n+\trefs/bundles/left\n+\trefs/bundles/merge\n+\trefs/bundles/right\n+\tEOF\n+\ttest_cmp expect actual &&\n+\t# We already have all needed commits so no \"want\" needed.\n+\ttest_grep ! \"clone> want \" trace-packet.txt\n+'\n+\n #########################################################################\n # HTTP tests begin here\n\n--\n2.45.2\n"},{"id":"499264","messageId":"20240724144957.3033840-3-toon@iotcl.com","threadId":"61819","inReplyTo":"20240724144957.3033840-1-toon@iotcl.com","subject":"[PATCH v2 2/3] transport: introduce transport_has_remote_bundle_uri()","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-07-24T14:49:56Z","receivedAt":"2024-07-24T14:50:43Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"The public function transport_get_remote_bundle_uri() exists to fetch\nthe bundle URI(s) from the remote. This function is only called from\nbuiltin/clone.c (not taking test-tool into account). There it ignores\nthe return value, because it doesn't matter whether the server didn't\nreturn any bundles or if it failed trying to fetch them, clone can\ncontinue without bundle URIs. After calling it, it checks if anything\nis collected in the bundle list and starts fetching them.\n\nAdd public function transport_has_remote_bundle_uri() instead. This\ncalls the (now made private) transport_get_remote_bundle_uri() function\nand returns whether any bundle URI is received. This makes reuse of the\ncode easier and avoids code duplication when we add bundle URI support\nto git-fetch(1).\n\nSigned-off-by: Toon Claes \n---\n builtin/clone.c | 23 +++++++----------------\n t/helper/test-bundle-uri.c | 2 +-\n transport.c | 14 +++++++++++++-\n transport.h | 7 ++++---\n 4 files changed, 25 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex aa507395a0..25535c1814 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1404,22 +1404,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t\tbundle_uri);\n \t\telse if (has_heuristic)\n \t\t\tgit_config_set_gently(\"fetch.bundleuri\", bundle_uri);\n-\t} else {\n-\t\t/*\n-\t\t* Populate transport->got_remote_bundle_uri and\n-\t\t* transport->bundle_uri. We might get nothing.\n-\t\t*/\n-\t\ttransport_get_remote_bundle_uri(transport);\n-\n-\t\tif (transport->bundles &&\n-\t\t hashmap_get_size(&transport->bundles->bundles)) {\n-\t\t\t/* At this point, we need the_repository to match the cloned repo. */\n-\t\t\tif (repo_init(the_repository, git_dir, work_tree))\n-\t\t\t\twarning(_(\"failed to initialize the repo, skipping bundle URI\"));\n-\t\t\telse if (fetch_bundle_list(the_repository,\n-\t\t\t\t\t\t transport->bundles))\n-\t\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n-\t\t}\n+\t} else if (transport_has_remote_bundle_uri(transport)) {\n+\t\t/* At this point, we need the_repository to match the cloned repo. */\n+\t\tif (repo_init(the_repository, git_dir, work_tree))\n+\t\t\twarning(_(\"failed to initialize the repo, skipping bundle URI\"));\n+\t\telse if (fetch_bundle_list(the_repository,\n+\t\t\t\t\t transport->bundles))\n+\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n \t}\n\n \tif (refs)\ndiff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c\nindex 0c5fa723d8..bd558d5e57 100644\n--- a/t/helper/test-bundle-uri.c\n+++ b/t/helper/test-bundle-uri.c\n@@ -90,7 +90,7 @@ static int cmd_ls_remote(int argc, const char **argv)\n \t}\n\n \ttransport = transport_get(remote, NULL);\n-\tif (transport_get_remote_bundle_uri(transport) < 0) {\n+\tif (!transport_has_remote_bundle_uri(transport)) {\n \t\terror(_(\"could not get the bundle-uri list\"));\n \t\tstatus = 1;\n \t\tgoto cleanup;\ndiff --git a/transport.c b/transport.c\nindex 12cc5b4d96..1a7d86fa40 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1536,7 +1536,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)\n \treturn rc;\n }\n\n-int transport_get_remote_bundle_uri(struct transport *transport)\n+static int transport_get_remote_bundle_uri(struct transport *transport)\n {\n \tint value = 0;\n \tconst struct transport_vtable *vtable = transport->vtable;\n@@ -1561,6 +1561,18 @@ int transport_get_remote_bundle_uri(struct transport *transport)\n\n \tif (vtable->get_bundle_uri(transport) < 0)\n \t\treturn error(_(\"could not retrieve server-advertised bundle-uri list\"));\n+\n+\treturn 0;\n+}\n+\n+int transport_has_remote_bundle_uri(struct transport *transport)\n+{\n+\ttransport_get_remote_bundle_uri(transport);\n+\n+\tif (transport->bundles &&\n+\t hashmap_get_size(&transport->bundles->bundles))\n+\t\treturn 1;\n+\n \treturn 0;\n }\n\ndiff --git a/transport.h b/transport.h\nindex 6393cd9823..5ea9641558 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -294,10 +294,11 @@ const struct ref *transport_get_remote_refs(struct transport *transport,\n \t\t\t\t\t struct transport_ls_refs_options *transport_options);\n\n /**\n- * Retrieve bundle URI(s) from a remote. Populates \"struct\n- * transport\"'s \"bundle_uri\" and \"got_remote_bundle_uri\".\n+ * Try fetch bundle URI(s) from a remote and returns 1 if one or more\n+ * bundle URI(s) are received from the server.\n+ * Populates \"struct transport\"'s \"bundles\" and \"got_remote_bundle_uri\".\n */\n-int transport_get_remote_bundle_uri(struct transport *transport);\n+int transport_has_remote_bundle_uri(struct transport *transport);\n\n /*\n * Fetch the hash algorithm used by a remote.\n--\n2.45.2\n"},{"id":"499265","messageId":"20240724144957.3033840-4-toon@iotcl.com","threadId":"61819","inReplyTo":"20240724144957.3033840-1-toon@iotcl.com","subject":"[PATCH v2 3/3] fetch: use bundle URIs when having creationToken heuristic","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-07-24T14:49:57Z","receivedAt":"2024-07-24T14:50:48Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"At the moment, bundle URIs are only used by git-clone(1). For a clone\nthe use of bundle URI is trivial, because the repository is empty so\ndownloading bundles will never result in downloading objects that are in\nthe repository already.\n\nFor git-fetch(1), this more complicated to use bundle URI. We want to\navoid downloading bundles that only contains objects that are in the\nlocal repository already.\n\nOne way to achieve this is possible when the \"creationToken\" heuristic\nis used for bundle URIs. We attempt to download and unbundle the minimum\nnumber of bundles by creationToken in decreasing order. If we fail to\nunbundle (after a successful download) then move to the next\nnon-downloaded bundle and attempt downloading. Once we succeed in\napplying a bundle, move to the previous unapplied bundle and attempt to\nunbundle it again. At the end the highest applied creationToken is\nwritten to `fetch.bundleCreationToken` in the git-config. The next time\nbundles are advertised by the server, bundles with a lower creationToken\nvalue are ignored. This was already implemented by\n7903efb717 (bundle-uri: download in creationToken order, 2023-01-31) in\nfetch_bundles_by_token().\n\nUsing the creationToken heuristic is optional, but without it the client\nhas no idea which bundles are new, how to sort them, and which only have\nobjects the client already has.\n\nWith this knowledge, make git-fetch(1) use bundle URIs from the server,\nbut only when the creationToken heuristic is used.\n\nSigned-off-by: Toon Claes \n---\n builtin/fetch.c | 13 ++++++++++\n t/t5584-fetch-bundle-uri.sh | 49 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 62 insertions(+)\n create mode 100755 t/t5584-fetch-bundle-uri.sh\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 693f02b958..98e811f438 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1694,6 +1694,19 @@ static int do_fetch(struct transport *transport,\n \t\t\tretcode = 1;\n \t}\n\n+\tif (transport_has_remote_bundle_uri(transport)) {\n+\t\t/*\n+\t\t * Only use bundle-URIs when they use the creationToken\n+\t\t * heuristic, this allows us to ensure not downloading bundles\n+\t\t * we don't need. You can read the comments in\n+\t\t * fetch_bundles_by_token() to understand how this works.\n+\t\t */\n+\t\tif (transport->bundles->heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN) {\n+\t\t\tif (fetch_bundle_list(the_repository, transport->bundles))\n+\t\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n+\t\t}\n+\t}\n+\n \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n \t\t\t\t &fetch_head, config)) {\n \t\tretcode = 1;\ndiff --git a/t/t5584-fetch-bundle-uri.sh b/t/t5584-fetch-bundle-uri.sh\nnew file mode 100755\nindex 0000000000..6c2383646e\n--- /dev/null\n+++ b/t/t5584-fetch-bundle-uri.sh\n@@ -0,0 +1,49 @@\n+#!/bin/sh\n+\n+test_description='test use of bundle URI in \"git fetch\"'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'set up repos and bundles' '\n+\tgit init source &&\n+\ttest_commit -C source A &&\n+\tgit clone --no-local source go-A-to-C &&\n+\ttest_commit -C source B &&\n+\tgit clone --no-local source go-B-to-C &&\n+\tgit clone --no-local source go-B-to-D &&\n+\tgit -C source bundle create B.bundle main &&\n+\ttest_commit -C source C &&\n+\tgit -C source bundle create B-to-C.bundle B..main &&\n+\tgit -C source config uploadpack.advertiseBundleURIs true &&\n+\tgit -C source config bundle.version 1 &&\n+\tgit -C source config bundle.mode all &&\n+\tgit -C source config bundle.heuristic creationToken &&\n+\tgit -C source config bundle.bundle-B.uri \"file://$(pwd)/source/B.bundle\" &&\n+\tgit -C source config bundle.bundle-B.creationToken 1 &&\n+\tgit -C source config bundle.bundle-B-to-C.uri \"file://$(pwd)/source/B-to-C.bundle\" &&\n+\tgit -C source config bundle.bundle-B-to-C.creationToken 2\n+'\n+\n+test_expect_success 'fetches one bundle URI to get up-to-date' '\n+\tgit -C go-B-to-C -c transfer.bundleURI=true fetch origin &&\n+\ttest 1 = $(ls go-B-to-C/.git/objects/bundles | wc -l) &&\n+\ttest 2 = $(git -C go-B-to-C config fetch.bundleCreationToken)\n+'\n+\n+test_expect_success 'fetches two bundle URIs to get up-to-date' '\n+\tgit -C go-A-to-C -c transfer.bundleURI=true fetch origin &&\n+\ttest 2 = $(ls go-A-to-C/.git/objects/bundles | wc -l) &&\n+\ttest 2 = $(git -C go-A-to-C config fetch.bundleCreationToken)\n+'\n+\n+test_expect_success 'fetches one bundle URI and objects from remote' '\n+\ttest_commit -C source D &&\n+\tgit -C go-B-to-D -c transfer.bundleURI=true fetch origin &&\n+\ttest 1 = $(ls go-B-to-D/.git/objects/bundles | wc -l) &&\n+\ttest 2 = $(git -C go-B-to-D config fetch.bundleCreationToken)\n+'\n+\n+test_done\n--\n2.45.2\n"},{"id":"499407","messageId":"CAOLa=ZRJDoy=n=ZTULc9rBT=mQPyWRUBW7E5-JeQsPUYaDV0=Q@mail.gmail.com","threadId":"61819","inReplyTo":"20240724144957.3033840-2-toon@iotcl.com","subject":"Re: [PATCH v2 1/3] clone: remove double bundle list clear code","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-26T08:51:57Z","receivedAt":"2024-07-26T08:51:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Toon Claes writes:\n\n> The bundle list transport->bundles is filled by\n> transport_get_remote_bundle_uri(). Only when the list is not used, it is\n> cleared right away by calling clear_bundle_list().\n>\n> This looks like we leak memory allocated for the list when\n> transport->bundles *is* used. But in fact, transport->bundles is cleaned\n> up in transport_disconnect() near the end of cmd_clone().\n>\n> Remove the double clean up of transport->bundles, and depend solely on\n> transport_disconnect() to take care of it.\n>\n> Also add a test case that hits this code, but due to other leaks we\n> cannot mark it as leak-free.\n>\n> Signed-off-by: Toon Claes \n> ---\n> builtin/clone.c | 3 ---\n> t/t5558-clone-bundle-uri.sh | 28 +++++++++++++++++++++++++++-\n> 2 files changed, 27 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index af6017d41a..aa507395a0 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1419,9 +1419,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \t\t\telse if (fetch_bundle_list(the_repository,\n> \t\t\t\t\t\t transport->bundles))\n> \t\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n> -\t\t} else {\n> -\t\t\tclear_bundle_list(transport->bundles);\n> -\t\t\tFREE_AND_NULL(transport->bundles);\n>\n\nThere is a small difference that here we also set `transport->bundles`\nto NULL, whereas in `transport_disconnect()` we only do `free(...)`. I\ndon't see any issues because of it though. This cleanup looks good.\n\n[snip]\n"},{"id":"499408","messageId":"CAOLa=ZTuM9XsroDKizvtOB0d1mJ4t8ffi6_bMuX+R8qzUEW6Rw@mail.gmail.com","threadId":"61819","inReplyTo":"20240724144957.3033840-3-toon@iotcl.com","subject":"Re: [PATCH v2 2/3] transport: introduce transport_has_remote_bundle_uri()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-26T08:58:21Z","receivedAt":"2024-07-26T08:58:23Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Toon Claes writes:\n\n[snip]\n\n> diff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c\n> index 0c5fa723d8..bd558d5e57 100644\n> --- a/t/helper/test-bundle-uri.c\n> +++ b/t/helper/test-bundle-uri.c\n> @@ -90,7 +90,7 @@ static int cmd_ls_remote(int argc, const char **argv)\n> \t}\n>\n> \ttransport = transport_get(remote, NULL);\n> -\tif (transport_get_remote_bundle_uri(transport) < 0) {\n> +\tif (!transport_has_remote_bundle_uri(transport)) {\n> \t\terror(_(\"could not get the bundle-uri list\"));\n> \t\tstatus = 1;\n> \t\tgoto cleanup;\n> diff --git a/transport.c b/transport.c\n> index 12cc5b4d96..1a7d86fa40 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1536,7 +1536,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)\n> \treturn rc;\n> }\n>\n> -int transport_get_remote_bundle_uri(struct transport *transport)\n> +static int transport_get_remote_bundle_uri(struct transport *transport)\n>\n\nWhy make it static?\n\n> {\n> \tint value = 0;\n> \tconst struct transport_vtable *vtable = transport->vtable;\n> @@ -1561,6 +1561,18 @@ int transport_get_remote_bundle_uri(struct transport *transport)\n>\n> \tif (vtable->get_bundle_uri(transport) < 0)\n> \t\treturn error(_(\"could not retrieve server-advertised bundle-uri list\"));\n> +\n> +\treturn 0;\n> +}\n> +\n> +int transport_has_remote_bundle_uri(struct transport *transport)\n> +{\n> +\ttransport_get_remote_bundle_uri(transport);\n> +\n> +\tif (transport->bundles &&\n> +\t hashmap_get_size(&transport->bundles->bundles))\n> +\t\treturn 1;\n> +\n> \treturn 0;\n> }\n>\n> diff --git a/transport.h b/transport.h\n> index 6393cd9823..5ea9641558 100644\n> --- a/transport.h\n> +++ b/transport.h\n> @@ -294,10 +294,11 @@ const struct ref *transport_get_remote_refs(struct transport *transport,\n> \t\t\t\t\t struct transport_ls_refs_options *transport_options);\n>\n> /**\n> - * Retrieve bundle URI(s) from a remote. Populates \"struct\n> - * transport\"'s \"bundle_uri\" and \"got_remote_bundle_uri\".\n> + * Try fetch bundle URI(s) from a remote and returns 1 if one or more\n\nNit: s/Try/Try to/\n\n> + * bundle URI(s) are received from the server.\n> + * Populates \"struct transport\"'s \"bundles\" and \"got_remote_bundle_uri\".\n> */\n> -int transport_get_remote_bundle_uri(struct transport *transport);\n> +int transport_has_remote_bundle_uri(struct transport *transport);\n>\n\nShouldn't this now be renamed to `transport_has_bundle_uri`? Earlier, we\nwere 'getting' bundle URIs from the remote. Now, we are abstracting that\naway and simply asking, 'do we have bundle URIs'.\n\n> /*\n> * Fetch the hash algorithm used by a remote.\n> --\n> 2.45.2\n"},{"id":"499409","messageId":"CAOLa=ZQ5bvWuBVC7FuT6+C2tB9FXr0mmexvBycRwUf6Ak_c_rA@mail.gmail.com","threadId":"61819","inReplyTo":"20240724144957.3033840-4-toon@iotcl.com","subject":"Re: [PATCH v2 3/3] fetch: use bundle URIs when having creationToken heuristic","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-26T09:06:32Z","receivedAt":"2024-07-26T09:06:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Toon Claes writes:\n\n> At the moment, bundle URIs are only used by git-clone(1). For a clone\n> the use of bundle URI is trivial, because the repository is empty so\n> downloading bundles will never result in downloading objects that are in\n> the repository already.\n>\n> For git-fetch(1), this more complicated to use bundle URI. We want to\n\nNit: s/this/it is/\n\n> avoid downloading bundles that only contains objects that are in the\n> local repository already.\n>\n> One way to achieve this is possible when the \"creationToken\" heuristic\n> is used for bundle URIs. We attempt to download and unbundle the minimum\n\nThis first sentence reads a bit weird. Perhaps, \"One way to achieve\nthis is to restrict the usage of bundle URIs to the 'creationToken'\nheuristic for git-fetch(1).\"?\n\n> number of bundles by creationToken in decreasing order. If we fail to\n> unbundle (after a successful download) then move to the next\n> non-downloaded bundle and attempt downloading. Once we succeed in\n> applying a bundle, move to the previous unapplied bundle and attempt to\n> unbundle it again. At the end the highest applied creationToken is\n> written to `fetch.bundleCreationToken` in the git-config. The next time\n> bundles are advertised by the server, bundles with a lower creationToken\n> value are ignored. This was already implemented by\n> 7903efb717 (bundle-uri: download in creationToken order, 2023-01-31) in\n> fetch_bundles_by_token().\n>\n> Using the creationToken heuristic is optional, but without it the client\n> has no idea which bundles are new, how to sort them, and which only have\n> objects the client already has.\n>\n> With this knowledge, make git-fetch(1) use bundle URIs from the server,\n> but only when the creationToken heuristic is used.\n>\n\nI would say that apart from all this it is also worth noting that bundle\nURIs are still opt-in from the client side.\n\nRest of the patch looks good to me.\n\nThanks\n\n[snip]\n"},{"id":"499439","messageId":"ZqObobw8FsDMkllm@tanuki","threadId":"61819","inReplyTo":"20240724144957.3033840-4-toon@iotcl.com","subject":"Re: [PATCH v2 3/3] fetch: use bundle URIs when having creationToken heuristic","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-26T12:50:41Z","receivedAt":"2024-07-26T12:51:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 24, 2024 at 04:49:57PM +0200, Toon Claes wrote:\n> One way to achieve this is possible when the \"creationToken\" heuristic\n> is used for bundle URIs. We attempt to download and unbundle the minimum\n> number of bundles by creationToken in decreasing order. If we fail to\n> unbundle (after a successful download) then move to the next\n> non-downloaded bundle and attempt downloading. Once we succeed in\n> applying a bundle, move to the previous unapplied bundle and attempt to\n> unbundle it again. At the end the highest applied creationToken is\n> written to `fetch.bundleCreationToken` in the git-config. The next time\n> bundles are advertised by the server, bundles with a lower creationToken\n> value are ignored. This was already implemented by\n> 7903efb717 (bundle-uri: download in creationToken order, 2023-01-31) in\n> fetch_bundles_by_token().\n\nI think Junio essentially asked this already, but I'm still missing the\nbigger picture here. When the \"creationToken\" heuristic is applied, the\neffect of your change is that we'll always favor bundle URIs now over\nperforming proper fetches, right?\n\nNow suppose that the server creates new bundled whenever somebody pushes\na new change to the default branch. We do not really have information\nhow this bundle is structured. It _could_ be an incremental bundle, and\nin that case it might be sensible to fetch that bundle. But it could\nalso be that the server generates a full bundle including all objects\ntransitively reachable from that default branch. Now if we started to\nrely on the \"creationToken\" heuristic, we would basically end up\nre-downloading the complete repository, which is a strict regression.\n\nNow that scenario is of course hypothetical. But the problem is that the\nstrategy for how bundle URIs are generated are determined by the hosting\nprovider. So ultimately, I expect that the reality will lie somewhere in\nbetween and be different depending on which hosting solution you use.\n\nAll of this to me means that the \"creationToken\" heuristic is not really\na good signal, unless I'm missing something about the way it works. Is\nthere any additional signal provided by the server except for the time\nwhen the bundle was created? If so, is that information sufficient to\ndetermine whether it makes sense for a client to fetch a bundle instead\nof performing a \"proper\" fetch? If not, what is the additional info that\nwe would need to make this schema work properly?\n\nSo unless I'm missing something, I feel like we need to think bigger and\ndesign a heuristic that gives us the information needed. Without such a\nheuristic, default-enabling may or may not do the right thing, and we\nhave no way to really argue whether it will do as we now depend on\nserver operators to do the right thing.\n\nPatrick\n"},{"id":"499448","messageId":"xmqq5xssyy2z.fsf@gitster.g","threadId":"61819","inReplyTo":"CAOLa=ZTuM9XsroDKizvtOB0d1mJ4t8ffi6_bMuX+R8qzUEW6Rw@mail.gmail.com","subject":"Re: [PATCH v2 2/3] transport: introduce transport_has_remote_bundle_uri()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-26T15:25:40Z","receivedAt":"2024-07-26T15:25:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>> diff --git a/transport.c b/transport.c\n>> index 12cc5b4d96..1a7d86fa40 100644\n>> --- a/transport.c\n>> +++ b/transport.c\n>> @@ -1536,7 +1536,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)\n>> \treturn rc;\n>> }\n>>\n>> -int transport_get_remote_bundle_uri(struct transport *transport)\n>> +static int transport_get_remote_bundle_uri(struct transport *transport)\n>>\n>\n> Why make it static?\n\nThe reason is rather well described in the proposed log message, I\nthink.\n\n>> + * bundle URI(s) are received from the server.\n>> + * Populates \"struct transport\"'s \"bundles\" and \"got_remote_bundle_uri\".\n>> */\n>> -int transport_get_remote_bundle_uri(struct transport *transport);\n>> +int transport_has_remote_bundle_uri(struct transport *transport);\n>>\n>\n> Shouldn't this now be renamed to `transport_has_bundle_uri`? Earlier, we\n> were 'getting' bundle URIs from the remote. Now, we are abstracting that\n> away and simply asking, 'do we have bundle URIs'.\n\nA good question.\n"},{"id":"499473","messageId":"ypkmgjgobu2tn36o7qzultltknaz35qysrfe4y4kce67uysdxk@edmxls7p36mx","threadId":"61819","inReplyTo":"20240724144957.3033840-2-toon@iotcl.com","subject":"Re: [PATCH v2 1/3] clone: remove double bundle list clear code","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-07-26T21:52:14Z","receivedAt":"2024-07-26T21:52:57Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/07/24 04:49PM, Toon Claes wrote:\n> The bundle list transport->bundles is filled by\n> transport_get_remote_bundle_uri(). Only when the list is not used, it is\n> cleared right away by calling clear_bundle_list().\n> \n> This looks like we leak memory allocated for the list when\n> transport->bundles *is* used. But in fact, transport->bundles is cleaned\n> up in transport_disconnect() near the end of cmd_clone().\n\nThe `transport->bundles` is setup and initialized by `transport_get()`\nand populated by `transport_get_remote_bundle_uri()`. In its current\nform, we clean up `transport->bundles` if it is empty immediately after\nchecking the remote for bundles. This is redundant though because the\ncleanup already occurs as part of `transport_disconnect()`.\n\nSince `transport_disconnect()` is already responsible for freeing other\nparts of `transport`, it makes sense to let it be the one to handle it.\n\n> \n> Remove the double clean up of transport->bundles, and depend solely on\n> transport_disconnect() to take care of it.\n> \n> Also add a test case that hits this code, but due to other leaks we\n> cannot mark it as leak-free.\n> \n> Signed-off-by: Toon Claes \n> ---\n> builtin/clone.c | 3 ---\n> t/t5558-clone-bundle-uri.sh | 28 +++++++++++++++++++++++++++-\n> 2 files changed, 27 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index af6017d41a..aa507395a0 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1419,9 +1419,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \t\t\telse if (fetch_bundle_list(the_repository,\n> \t\t\t\t\t\t transport->bundles))\n> \t\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n> -\t\t} else {\n> -\t\t\tclear_bundle_list(transport->bundles);\n> -\t\t\tFREE_AND_NULL(transport->bundles);\n> \t\t}\n> \t}\n> \n> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh\n> index cd05321e17..2d6e690fbe 100755\n> --- a/t/t5558-clone-bundle-uri.sh\n> +++ b/t/t5558-clone-bundle-uri.sh\n> @@ -1,6 +1,6 @@\n> #!/bin/sh\n> \n> -test_description='test fetching bundles with --bundle-uri'\n> +test_description='test clone with use of bundle-uri'\n\nI assume this description was changed because the test pertains to\nclones and not fetches. Might be worth mentioning in the commit message.\n\n> \n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-bundle.sh\n> @@ -438,6 +438,32 @@ test_expect_success 'negotiation: bundle list with all wanted commits' '\n> \ttest_grep ! \"clone> want \" trace-packet.txt\n> '\n> \n> +test_expect_success 'bundles advertised by the server' '\n> +\ttest_when_finished rm -f trace*.txt &&\n> +\tgit clone clone-from clone-advertiser &&\n> +\tgit -C clone-advertiser config uploadpack.advertiseBundleURIs true &&\n> +\tgit -C clone-advertiser config bundle.version 1 &&\n> +\tgit -C clone-advertiser config bundle.mode all &&\n> +\tgit -C clone-advertiser config bundle.bundle-1.uri \"file://$(pwd)/clone-from/bundle-1.bundle\" &&\n> +\tgit -C clone-advertiser config bundle.bundle-2.uri \"file://$(pwd)/clone-from/bundle-2.bundle\" &&\n> +\tgit -C clone-advertiser config bundle.bundle-3.uri \"file://$(pwd)/clone-from/bundle-3.bundle\" &&\n> +\tgit -C clone-advertiser config bundle.bundle-4.uri \"file://$(pwd)/clone-from/bundle-4.bundle\" &&\n> +\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace-packet.txt\" \\\n> +\tgit -c transfer.bundleURI=true clone clone-advertiser clone-advertised &&\n> +\tgit -C clone-advertised for-each-ref --format=\"%(refname)\" >refs &&\n> +\tgrep \"refs/bundles/\" refs >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\trefs/bundles/base\n> +\trefs/bundles/left\n> +\trefs/bundles/merge\n> +\trefs/bundles/right\n> +\tEOF\n> +\ttest_cmp expect actual &&\n> +\t# We already have all needed commits so no \"want\" needed.\n> +\ttest_grep ! \"clone> want \" trace-packet.txt\n> +'\n> +\n\nLooks like this test is validating that a clone is retrieving bundles\nfrom a remote repository advertising said bundles. We to do so by\nchecking for the `refs/bundles/*` references which should only exist if\nthe advertised bundles were fetched. This makes sense to me.\n\nFrom the commit message though, I thought the added test might have\nsomething to do with the changes to when `transport->bundles` was freed.\nI wonder if it would be a good idea to expland on this further in the\ncommit message. Or since the added test is somewhat unrelated to this\npatch, maybe it should be a separate patch?\n\n-Justin\n\n> #########################################################################\n> # HTTP tests begin here\n> \n> --\n> 2.45.2\n> \n"},{"id":"499946","messageId":"87a5hv9gw0.fsf@gitlab.com","threadId":"61819","inReplyTo":"ZqObobw8FsDMkllm@tanuki","subject":"Re: [PATCH v2 3/3] fetch: use bundle URIs when having creationToken heuristic","fromName":"Toon claes","fromEmail":"toon@iotcl.com","sentAt":"2024-08-02T13:46:39Z","receivedAt":"2024-08-02T13:47:00Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Patrick Steinhardt writes:\n\n> I think Junio essentially asked this already, but I'm still missing the\n> bigger picture here. When the \"creationToken\" heuristic is applied, the\n> effect of your change is that we'll always favor bundle URIs now over\n> performing proper fetches, right?\n\nYes, sort of. Bundle URIs are a step before \"proper\" fetches. A \"proper\"\nfetch might still happen after downloading bundles.\n(I would rather call them \"negotiated fetches\" or something, but let's\njust stick with \"proper\" fetch for now)\n\n> Now suppose that the server creates new bundled whenever somebody pushes\n> a new change to the default branch. We do not really have information\n> how this bundle is structured. It _could_ be an incremental bundle, and\n> in that case it might be sensible to fetch that bundle. But it could\n> also be that the server generates a full bundle including all objects\n> transitively reachable from that default branch. Now if we started to\n> rely on the \"creationToken\" heuristic, we would basically end up\n> re-downloading the complete repository, which is a strict regression.\n>\n> Now that scenario is of course hypothetical. But the problem is that the\n> strategy for how bundle URIs are generated are determined by the hosting\n> provider. So ultimately, I expect that the reality will lie somewhere in\n> between and be different depending on which hosting solution you use.\n\nThat is true. The mechanism of bundle URIs is mostly outside the control\nof Git itself. It's up to the Git hosting provider how they use it. This\ngives them a lot of flexibility, like where to store bundles, how\nincremental bundles are used, and how often bundles are regenerated. But\nthis also comes with a huge responsibility, due to the scenario you\ndescribe above.\n\nAt GitLab we want to do an initial roll-out of bundle URIs with just one\nbundle advertised to the client. This bundle is generated every week,\nand only contains the whole history of default branch (up to the newest\ncommit). But, we'll advertise this bundle with creationToken \"1\",\nalways. This will cause the client to fetch any bundle for the\nrepository only once. Even when the bundle is updated by the server, the\nclient will not fetch it because the creationToken did not change.\n\n> All of this to me means that the \"creationToken\" heuristic is not really\n> a good signal, unless I'm missing something about the way it works. Is\n> there any additional signal provided by the server except for the time\n> when the bundle was created?\n\nThe Git hosting provider can use the \"creationToken\" however they want,\nat the moment it's up to them to decide on a good strategy.\n\nFor example, assume you decide to create bundle URIs only for the\ndefault branch, then you can choose to use the committer date of the\ntopmost commit of that branch as the creationToken.\n\nImagine you have the following example commit history (committer date of\nsome commits are indicated with a caret below those commits):\n\n\n A --- B --- C --- D --- E --- F --- G\n ^ ^ ^\n 2024-08-02 2024-08-04 2024-08-09\n\nToday (on 2024-08-02, at this point commits D to G don't exist yet) the\nGit host decides to create a bundle:\n\n- git bundle create first.bundle main\n\nThe server will advertise this bundle with the creationToken being the\nUnix timestamp of the topmost commit in the bundle:\n\n- first.bundle = 2024-08-02 or in Unix 1722549600\n\nWhen the client clones/fetches, it will download this bundle and store\ncreationToken 1722549600 (= 2024-08-02) in the repo config as\n`fetch.bundleCreationToken`.\n\nA few days later (on 2024-08-04, now D & E are added) the client fetches\nagain, and there are no new bundles, so a \"proper\" fetch happens.\n\nA week after the first bundle (on 2024-08-09, D to G were added since)\nthe Git host decides to create a new (incremental) bundle:\n\n- git bundle create second.bundle C..main\n\nThe server then advertises the following bundles:\n\n- first.bundle = 2024-08-02 or in Unix 1722549600\n- second.bundle = 2024-08-09 1723154400\n\nNow when the client fetches again, it sees a new bundle \"second.bundle\",\nand will download it. This bundle contains the history from C..G. And\nyou might have noticed, the client already has C..E, while it only needs\nE..G. This is a slight inefficiency, it's a pitfall of bundle URI I\ndon't think we can avoid. By design bundle URIs are used to pre-generate\nfiles for clients to use, so they are never built-to-order. It's almost\nimpossible to avoid clients to download bundles that have objects they\nalready have.\n\nWell, maybe it is possible. But then the question arises, if the current\nmain branch is at G, do you want the client to download a bundle which\nhas a little bit too much objects, or do you want to have the client do\na \"proper\" fetch and skip any bundle?\n\nBut all the above assumes there's only one branch in the bundle. With\nmore branches another strategy might be required.\n\n> If so, is that information sufficient to determine whether it makes\n> sense for a client to fetch a bundle instead of performing a \"proper\"\n> fetch? If not, what is the additional info that we would need to make\n> this schema work properly?\n\nI think my example above indicates the \"creationToken\" _can_ be\nsufficient information for the client to determine if downloading a\nbundle makes sense, but it depends on a well-though-out strategy of the\nGit host. So one the hand it gives the host the flexibility of using a\nstrategy, on the other hand it puts a lot of responsibility on them.\n\n> So unless I'm missing something, I feel like we need to think bigger and\n> design a heuristic that gives us the information needed. Without such a\n> heuristic, default-enabling may or may not do the right thing, and we\n> have no way to really argue whether it will do as we now depend on\n> server operators to do the right thing.\n\nI think we're absolutely not ready to default-enabling the bundle URI\nfeature in Git. Here at GitLab we're trying pilot bundle URI to figure\nout how it should/can work. Roll-out to CI runners is our first target\nand because we have the flexibility to toggle use of bundle URI on the\nclient side, we can test things safely, without affecting any other\nuser.\n\nTalking about heuristics, any heuristic will be a _hack_ if you ask me.\nIf you think about it, the bundles them self should be the single source\nof truth. In fact, a bundle is a packfile with a header prepended that\ncontains refs and their OID in git-show-ref(1) format, and includes\nwhich prerequisite OIDs it has.\n\nSo another approach I think of is to have the client partially download\nbundles (so it pauses the download once they have received the complete\nheader) and make them parse the header to determine if they continue the\ndownload. But this approach feels quite complicated to me.\n\nLuckily, with the bundle.heuristic config we can always introduce new\nvalues if we discover there is a flaw in the \"creationToken\" heuristic.\n\n\n-- \nToon\n"},{"id":"499953","messageId":"875xsj9bdt.fsf@gitlab.com","threadId":"61819","inReplyTo":"ypkmgjgobu2tn36o7qzultltknaz35qysrfe4y4kce67uysdxk@edmxls7p36mx","subject":"Re: [PATCH v2 1/3] clone: remove double bundle list clear code","fromName":"Toon claes","fromEmail":"toon@iotcl.com","sentAt":"2024-08-02T15:45:34Z","receivedAt":"2024-08-02T15:45:50Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Justin Tobler writes:\n\n> From the commit message though, I thought the added test might have\n> something to do with the changes to when `transport->bundles` was freed.\n> I wonder if it would be a good idea to expland on this further in the\n> commit message. Or since the added test is somewhat unrelated to this\n> patch, maybe it should be a separate patch?\n\nI added this test case because I wanted to debug the code path where the\nremote advertises bundle URIs, all other test cases use --bundle-uri.\nThis helped me to debug the code and understand where the\n`transport->bundles` are freed. Although this test case does not fully\ntouch the code path of the code that's removed, but it was close enough\nto hack a few extra lines and understand what happens. Since I wrote\nthat test anyway, I felt I might as well submit it.\n\n-- \nToon\n"},{"id":"501489","messageId":"ZsbkyaLFnsz1Xik9@tanuki","threadId":"61819","inReplyTo":"87a5hv9gw0.fsf@gitlab.com","subject":"Re: [PATCH v2 3/3] fetch: use bundle URIs when having creationToken heuristic","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-22T07:12:09Z","receivedAt":"2024-08-22T07:12:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 02, 2024 at 03:46:39PM +0200, Toon claes wrote:\n> Patrick Steinhardt writes:\n> \n> > I think Junio essentially asked this already, but I'm still missing the\n> > bigger picture here. When the \"creationToken\" heuristic is applied, the\n> > effect of your change is that we'll always favor bundle URIs now over\n> > performing proper fetches, right?\n> \n> Yes, sort of. Bundle URIs are a step before \"proper\" fetches. A \"proper\"\n> fetch might still happen after downloading bundles.\n> (I would rather call them \"negotiated fetches\" or something, but let's\n> just stick with \"proper\" fetch for now)\n> \n> > Now suppose that the server creates new bundled whenever somebody pushes\n> > a new change to the default branch. We do not really have information\n> > how this bundle is structured. It _could_ be an incremental bundle, and\n> > in that case it might be sensible to fetch that bundle. But it could\n> > also be that the server generates a full bundle including all objects\n> > transitively reachable from that default branch. Now if we started to\n> > rely on the \"creationToken\" heuristic, we would basically end up\n> > re-downloading the complete repository, which is a strict regression.\n> >\n> > Now that scenario is of course hypothetical. But the problem is that the\n> > strategy for how bundle URIs are generated are determined by the hosting\n> > provider. So ultimately, I expect that the reality will lie somewhere in\n> > between and be different depending on which hosting solution you use.\n> \n> That is true. The mechanism of bundle URIs is mostly outside the control\n> of Git itself. It's up to the Git hosting provider how they use it. This\n> gives them a lot of flexibility, like where to store bundles, how\n> incremental bundles are used, and how often bundles are regenerated. But\n> this also comes with a huge responsibility, due to the scenario you\n> describe above.\n> \n> At GitLab we want to do an initial roll-out of bundle URIs with just one\n> bundle advertised to the client. This bundle is generated every week,\n> and only contains the whole history of default branch (up to the newest\n> commit). But, we'll advertise this bundle with creationToken \"1\",\n> always. This will cause the client to fetch any bundle for the\n> repository only once. Even when the bundle is updated by the server, the\n> client will not fetch it because the creationToken did not change.\n\nOkay. Are details like this documented anywhere in the Git project?\nBecause when we start to build on the preexisting heuristics I think\nthat we should give server operators a guide for how things come\ntogether and what the client's expectations can and should be.\n\n> > All of this to me means that the \"creationToken\" heuristic is not really\n> > a good signal, unless I'm missing something about the way it works. Is\n> > there any additional signal provided by the server except for the time\n> > when the bundle was created?\n> \n> The Git hosting provider can use the \"creationToken\" however they want,\n> at the moment it's up to them to decide on a good strategy.\n> \n> For example, assume you decide to create bundle URIs only for the\n> default branch, then you can choose to use the committer date of the\n> topmost commit of that branch as the creationToken.\n> \n> Imagine you have the following example commit history (committer date of\n> some commits are indicated with a caret below those commits):\n> \n> \n> A --- B --- C --- D --- E --- F --- G\n> ^ ^ ^\n> 2024-08-02 2024-08-04 2024-08-09\n> \n> Today (on 2024-08-02, at this point commits D to G don't exist yet) the\n> Git host decides to create a bundle:\n> \n> - git bundle create first.bundle main\n> \n> The server will advertise this bundle with the creationToken being the\n> Unix timestamp of the topmost commit in the bundle:\n> \n> - first.bundle = 2024-08-02 or in Unix 1722549600\n> \n> When the client clones/fetches, it will download this bundle and store\n> creationToken 1722549600 (= 2024-08-02) in the repo config as\n> `fetch.bundleCreationToken`.\n> \n> A few days later (on 2024-08-04, now D & E are added) the client fetches\n> again, and there are no new bundles, so a \"proper\" fetch happens.\n> \n> A week after the first bundle (on 2024-08-09, D to G were added since)\n> the Git host decides to create a new (incremental) bundle:\n> \n> - git bundle create second.bundle C..main\n> \n> The server then advertises the following bundles:\n> \n> - first.bundle = 2024-08-02 or in Unix 1722549600\n> - second.bundle = 2024-08-09 1723154400\n> \n> Now when the client fetches again, it sees a new bundle \"second.bundle\",\n> and will download it. This bundle contains the history from C..G. And\n> you might have noticed, the client already has C..E, while it only needs\n> E..G. This is a slight inefficiency, it's a pitfall of bundle URI I\n> don't think we can avoid. By design bundle URIs are used to pre-generate\n> files for clients to use, so they are never built-to-order. It's almost\n> impossible to avoid clients to download bundles that have objects they\n> already have.\n> \n> Well, maybe it is possible. But then the question arises, if the current\n> main branch is at G, do you want the client to download a bundle which\n> has a little bit too much objects, or do you want to have the client do\n> a \"proper\" fetch and skip any bundle?\n> \n> But all the above assumes there's only one branch in the bundle. With\n> more branches another strategy might be required.\n\nOkay. What I'm still missing though is the ability for a client do\ndiscern whether bundles are incremental or full.\n\nIn the schema you describe, my assumption is that the server operator\nwill eventually regenerate the first bundle to become a full bundle.\nWhich means that if there was any activity in the repository, then the\nUnix timestamp of that regenerated bundle will likely be bigger than\nwhat the client has stored on disk. Consequently, without information\nthat this is in fact a full bundle, the client would end up fetching it.\nThis is a potentially big waste of time and resources.\n\n> > If so, is that information sufficient to determine whether it makes\n> > sense for a client to fetch a bundle instead of performing a \"proper\"\n> > fetch? If not, what is the additional info that we would need to make\n> > this schema work properly?\n> \n> I think my example above indicates the \"creationToken\" _can_ be\n> sufficient information for the client to determine if downloading a\n> bundle makes sense, but it depends on a well-though-out strategy of the\n> Git host. So one the hand it gives the host the flexibility of using a\n> strategy, on the other hand it puts a lot of responsibility on them.\n\nIt does put a big burden on the hoster to get it right, that much is\ntrue. But I think the bigger problem is that the hoster _cannot_ get it\nright, because they have no ability to distribute enough information to\nthe client.\n\nFor a schema like this to work I think we need to advertise at least two\nthings for each bundle: a unique bundle token, and the token of the base\nbundle that a bundle may refer to. This would allow us to advertise\ndependencies between bundles to a client rather easily: a full bundle\ndoes not have a base, whereas incremental bundles would point to the\nprevious bundle.\n\nA theoretical flow would thus go like this:\n\n 1. The server announces the full bundle A.bundle. The client downloads\n it and stores the unique token Tok(A) in its config.\n\n 2. Subsequent fetches notice that Tok(A) announced by the server and\n stored in the config remain the same.\n\n 3. The server creates incremental bundles B.bundle and C.bundle.\n B.bundle builds on top of A, C builds on top of B. The server\n announces all three of those bundles. The client determines that it\n only has A, so to get to C it needs to fetch both B and C. It then\n stores Tok(C) in its config.\n\nOne scenario not yet covered is when the server regenerates the full\nbundle to get A' = A + B + C + D. Ideally, the server would temporarily\nannounce a new bundle D = C + D while also announcing that A' and the\nresult from D are equivalent. Like this, the client knows that it does\nnot have to refetch A', but already has its contents and can thus update\nits config to point to Tok(A').\n\nBut even a schema that is more involved like this one is rather\nsuboptimal. Eventually, the equivalence of Tok(A') and D will not be\nadvertised by the server anymore. So the client will not be able to\nlearn about which incremental bundles to fetch anymore, and would be\nforced to re-download everything.\n\n> > So unless I'm missing something, I feel like we need to think bigger and\n> > design a heuristic that gives us the information needed. Without such a\n> > heuristic, default-enabling may or may not do the right thing, and we\n> > have no way to really argue whether it will do as we now depend on\n> > server operators to do the right thing.\n> \n> I think we're absolutely not ready to default-enabling the bundle URI\n> feature in Git. Here at GitLab we're trying pilot bundle URI to figure\n> out how it should/can work. Roll-out to CI runners is our first target\n> and because we have the flexibility to toggle use of bundle URI on the\n> client side, we can test things safely, without affecting any other\n> user.\n> \n> Talking about heuristics, any heuristic will be a _hack_ if you ask me.\n> If you think about it, the bundles them self should be the single source\n> of truth. In fact, a bundle is a packfile with a header prepended that\n> contains refs and their OID in git-show-ref(1) format, and includes\n> which prerequisite OIDs it has.\n\nYes, that is true indeed.\n\n> So another approach I think of is to have the client partially download\n> bundles (so it pauses the download once they have received the complete\n> header) and make them parse the header to determine if they continue the\n> download. But this approach feels quite complicated to me.\n\nIt certainly is more complicated. But it's also the only mechanism that\ngives you all the information you need, I guess.\n\n> Luckily, with the bundle.heuristic config we can always introduce new\n> values if we discover there is a flaw in the \"creationToken\" heuristic.\n\nWell, I'd claim we have already shown that it's flawed. I think before\nwe enable incremental bundle fetches by default, even if it's only with\nthe \"creationToken\" set, we need to show and document a way for server\noperators to make use of this feature. In other words, we need to\ndocument how they have to generate bundles in a way that does not cause\nclients to re-download everything whenever a new bundle is announced.\n\nPatrick\n"},{"id":"503607","messageId":"20240927090455.1014896-1-toon@iotcl.com","threadId":"61819","inReplyTo":"20240722080705.2614195-1-toon@iotcl.com","subject":"Re: [PATCH] builtin/clone: teach git-clone(1) the --ref= argument","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-09-27T09:04:54Z","receivedAt":"2024-09-27T09:05:26Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Hi,\n\nMy apologies, instantly after sending my v1 I noticed some lines in the test\nweren't indented with tabs. So I'm sending v2 straight away. Sorry for the\nnoise.\n\n--\nToon\n"},{"id":"503608","messageId":"20240927090455.1014896-2-toon@iotcl.com","threadId":"61819","inReplyTo":"20240927090455.1014896-1-toon@iotcl.com","subject":"[PATCH v2] builtin/clone: teach git-clone(1) the --ref= argument","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-09-27T09:04:55Z","receivedAt":"2024-09-27T09:05:32Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Add option `--ref` to git-clone(1). This enables the user to clone and\ncheckout the given named reference. It's pretty similar to --branch and\nwhile --branch takes a branch name or tag name, it doesn't take a fully\nqualified reference. This allows the user to clone a reference that\ndoesn't start with refs/heads or refs/tags. This can be useful when the\nserver uses custom references.\n\nThis new argument can be used in conjuction with --single-branch to only\nclone the given ref.\n\nSigned-off-by: Toon Claes \n---\n Documentation/git-clone.txt | 9 ++++-\n builtin/clone.c | 67 ++++++++++++++++++++++++++++++++-----\n t/t5612-clone-refspec.sh | 35 +++++++++++++++++++\n 3 files changed, 102 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex 8e925db7e9..ae82f1c1c3 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -215,6 +215,13 @@ objects from the source repository into a pack in the cloned repository.\n \t`--branch` can also take tags and detaches the HEAD at that commit\n \tin the resulting repository.\n\n+`--ref` __::\n+\tThis detaches HEAD and makes it point to the commit where the __\n+\treference is pointing to. In a non-bare repository, this is the ref that\n+\twill be checked out.\n+\tCan be used in conjunction with `--single-branch` and `--no-tags` to\n+\tclone only the given ref. Cannot be combined with `--branch`.\n+\n `-u` __::\n `--upload-pack` __::\n \tWhen given, and the repository to clone from is accessed\n@@ -259,7 +266,7 @@ corresponding `--mirror` and `--no-tags` options instead.\n\n `--`[`no-`]`single-branch`::\n \tClone only the history leading to the tip of a single branch,\n-\teither specified by the `--branch` option or the primary\n+\teither specified by the `--branch` or `--ref` option or the primary\n \tbranch remote's `HEAD` points at.\n \tFurther fetches into the resulting repository will only update the\n \tremote-tracking branch for the branch this option was used for the\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex e77339c847..1081478905 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -69,6 +69,7 @@ static char *option_template, *option_depth, *option_since;\n static char *option_origin = NULL;\n static char *remote_name = NULL;\n static char *option_branch = NULL;\n+static char *option_ref = NULL;\n static struct string_list option_not = STRING_LIST_INIT_NODUP;\n static const char *real_git_dir;\n static const char *ref_format;\n@@ -141,6 +142,8 @@ static struct option builtin_clone_options[] = {\n \t\t N_(\"use instead of 'origin' to track upstream\")),\n \tOPT_STRING('b', \"branch\", &option_branch, N_(\"branch\"),\n \t\t N_(\"checkout instead of the remote's HEAD\")),\n+\tOPT_STRING(0, \"ref\", &option_ref, N_(\"ref\"),\n+\t\t N_(\"checkout (detached) instead of the remote's HEAD\")),\n \tOPT_STRING('u', \"upload-pack\", &option_upload_pack, N_(\"path\"),\n \t\t N_(\"path to git-upload-pack on the remote\")),\n \tOPT_STRING(0, \"depth\", &option_depth, N_(\"depth\"),\n@@ -531,32 +534,64 @@ static struct ref *wanted_peer_refs(const struct ref *refs,\n \tif (option_single_branch) {\n \t\tstruct ref *remote_head = NULL;\n\n-\t\tif (!option_branch)\n+\t\tif (!option_branch && !option_ref)\n \t\t\tremote_head = guess_remote_head(head, refs, 0);\n \t\telse {\n \t\t\tfree_one_ref(head);\n \t\t\tlocal_refs = head = NULL;\n \t\t\ttail = &local_refs;\n-\t\t\tremote_head = copy_ref(find_remote_branch(refs, option_branch));\n+\t\t\tif (option_branch)\n+\t\t\t\tremote_head = copy_ref(find_remote_branch(refs, option_branch));\n+\t\t\telse\n+\t\t\t\tremote_head = copy_ref(find_ref_by_name(refs, option_ref));\n \t\t}\n\n \t\tif (!remote_head && option_branch)\n \t\t\twarning(_(\"Could not find remote branch %s to clone.\"),\n \t\t\t\toption_branch);\n+\t\telse if (!remote_head && option_ref)\n+\t\t\twarning(_(\"Could not find remote ref %s to clone.\"),\n+\t\t\t\toption_ref);\n \t\telse {\n \t\t\tint i;\n \t\t\tfor (i = 0; i < refspec->nr; i++)\n \t\t\t\tget_fetch_map(remote_head, &refspec->items[i],\n \t\t\t\t\t &tail, 0);\n\n-\t\t\t/* if --branch=tag, pull the requested tag explicitly */\n-\t\t\tget_fetch_map(remote_head, &tag_refspec, &tail, 0);\n+\t\t\tif (option_ref) {\n+\t\t\t\tstruct strbuf spec = STRBUF_INIT;\n+\t\t\t\tstruct refspec_item ref_refspec;\n+\n+\t\t\t\tstrbuf_addf(&spec, \"%s:%s\", option_ref, option_ref);\n+\t\t\t\trefspec_item_init(&ref_refspec, spec.buf, 0);\n+\n+\t\t\t\tget_fetch_map(remote_head, &ref_refspec, &tail, 0);\n+\n+\t\t\t\trefspec_item_clear(&ref_refspec);\n+\t\t\t\tstrbuf_release(&spec);\n+\t\t\t} else {\n+\t\t\t\t/* if --branch=tag, pull the requested tag explicitly */\n+\t\t\t\tget_fetch_map(remote_head, &tag_refspec, &tail, 0);\n+\t\t\t}\n \t\t}\n \t\tfree_refs(remote_head);\n \t} else {\n \t\tint i;\n \t\tfor (i = 0; i < refspec->nr; i++)\n \t\t\tget_fetch_map(refs, &refspec->items[i], &tail, 0);\n+\n+\t\tif (option_ref) {\n+\t\t\tstruct strbuf spec = STRBUF_INIT;\n+\t\t\tstruct refspec_item ref_refspec;\n+\n+\t\t\tstrbuf_addf(&spec, \"%s:%s\", option_ref, option_ref);\n+\t\t\trefspec_item_init(&ref_refspec, spec.buf, 0);\n+\n+\t\t\tget_fetch_map(refs, &ref_refspec, &tail, 0);\n+\n+\t\t\trefspec_item_clear(&ref_refspec);\n+\t\t\tstrbuf_release(&spec);\n+\t\t}\n \t}\n\n \tif (!option_mirror && !option_single_branch && !option_no_tags)\n@@ -684,10 +719,15 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t} else if (our) {\n \t\tstruct commit *c = lookup_commit_reference(the_repository,\n \t\t\t\t\t\t\t &our->old_oid);\n-\t\t/* --branch specifies a non-branch (i.e. tags), detach HEAD */\n-\t\trefs_update_ref(get_main_ref_store(the_repository), msg,\n-\t\t\t\t\"HEAD\", &c->object.oid, NULL, REF_NO_DEREF,\n-\t\t\t\tUPDATE_REFS_DIE_ON_ERR);\n+\t\tif (c)\n+\t\t\t/* --branch specifies a non-branch (i.e. tags), detach HEAD */\n+\t\t\trefs_update_ref(get_main_ref_store(the_repository), msg,\n+\t\t\t\t\t\"HEAD\", &c->object.oid, NULL, REF_NO_DEREF,\n+\t\t\t\t\tUPDATE_REFS_DIE_ON_ERR);\n+\t\telse\n+\t\t\trefs_update_ref(get_main_ref_store(the_repository), msg,\n+\t\t\t\t\t\"HEAD\", &our->old_oid, NULL, REF_NO_DEREF,\n+\t\t\t\t\tUPDATE_REFS_DIE_ON_ERR);\n \t} else if (remote) {\n \t\t/*\n \t\t * We know remote HEAD points to a non-branch, or\n@@ -898,6 +938,9 @@ static void write_refspec_config(const char *src_ref_prefix,\n \t\t\t\telse\n \t\t\t\t\tstrbuf_addf(&value, \"+%s:%s%s\", our_head_points_at->name,\n \t\t\t\t\t\tbranch_top->buf, option_branch);\n+\t\t\t} else if (option_ref) {\n+\t\t\t\tstrbuf_addf(&value, \"+%s:%s\", our_head_points_at->name,\n+\t\t\t\t\tour_head_points_at->name);\n \t\t\t} else if (remote_head_points_at) {\n \t\t\t\tconst char *head = remote_head_points_at->name;\n \t\t\t\tif (!skip_prefix(head, \"refs/heads/\", &head))\n@@ -1383,6 +1426,9 @@ int cmd_clone(int argc,\n \tif (option_branch)\n \t\texpand_ref_prefix(&transport_ls_refs_options.ref_prefixes,\n \t\t\t\t option_branch);\n+\tif (option_ref)\n+\t\tstrvec_push(&transport_ls_refs_options.ref_prefixes,\n+\t\t\t option_ref);\n \tif (!option_no_tags)\n \t\tstrvec_push(&transport_ls_refs_options.ref_prefixes,\n \t\t\t \"refs/tags/\");\n@@ -1468,6 +1514,11 @@ int cmd_clone(int argc,\n \t\tif (!our_head_points_at)\n \t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n \t\t\t option_branch, remote_name);\n+\t} else if (option_ref) {\n+\t\tour_head_points_at = find_ref_by_name(mapped_refs, option_ref);\n+\t\tif (!our_head_points_at)\n+\t\t\tdie(_(\"Remote ref %s not found in upstream %s\"),\n+\t\t\t option_ref, remote_name);\n \t} else if (remote_head_points_at) {\n \t\tour_head_points_at = remote_head_points_at;\n \t} else if (remote_head) {\ndiff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh\nindex 72762de977..bbdf66b7cb 100755\n--- a/t/t5612-clone-refspec.sh\n+++ b/t/t5612-clone-refspec.sh\n@@ -17,6 +17,7 @@ test_expect_success 'setup' '\n \tgit tag two &&\n \techo three >file &&\n \tgit commit -a -m three &&\n+\tgit update-ref refs/some/three HEAD &&\n \tgit checkout -b side &&\n \techo four >file &&\n \tgit commit -a -m four &&\n@@ -236,4 +237,38 @@ test_expect_success '--single-branch with detached' '\n \ttest_must_be_empty actual\n '\n\n+test_expect_success 'with --ref' '\n+\tgit clone --ref=refs/some/three . dir_ref &&\n+\tgit -C dir_ref for-each-ref refs > refs &&\n+\tsed -e \"/HEAD$/d\" \\\n+\t -e \"s|/remotes/origin/|/heads/|\" refs >actual &&\n+\tgit for-each-ref refs >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'with --ref and --no-tags' '\n+\tgit clone --ref=refs/some/three --no-tags . dir_ref_notags &&\n+\tgit -C dir_ref_notags for-each-ref refs > refs &&\n+\tsed -e \"/HEAD$/d\" \\\n+\t -e \"s|/remotes/origin/|/heads/|\" refs >actual &&\n+\tgit for-each-ref refs/heads >expect &&\n+\tgit for-each-ref refs/some >>expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--single-branch with --ref' '\n+\tgit clone --single-branch --ref=refs/some/three . dir_single_ref &&\n+\tgit -C dir_single_ref for-each-ref refs > actual &&\n+\tgit for-each-ref refs/some >expect &&\n+\tgit for-each-ref refs/tags >>expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--single-branch with --ref and --no-tags' '\n+\tgit clone --single-branch --ref=refs/some/three --no-tags . dir_single_ref_notags &&\n+\tgit -C dir_single_ref_notags for-each-ref refs > actual &&\n+\tgit for-each-ref refs/some >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nRange-diff against v1:\n1: 8ee6a42431 ! 1: 07363b1de5 builtin/clone: teach git-clone(1) the --ref= argument\n @@ Commit message\n checkout the given named reference. It's pretty similar to --branch and\n while --branch takes a branch name or tag name, it doesn't take a fully\n qualified reference. This allows the user to clone a reference that\n - doesn't start with refs/heads/ or refs/tags. This can be useful when the\n + doesn't start with refs/heads or refs/tags. This can be useful when the\n server uses custom references.\n\n - Allow the user to clone a certain ref, similar to --branch.\n + This new argument can be used in conjuction with --single-branch to only\n + clone the given ref.\n\n Signed-off-by: Toon Claes \n\n @@ Documentation/git-clone.txt: objects from the source repository into a pack in t\n +\treference is pointing to. In a non-bare repository, this is the ref that\n +\twill be checked out.\n +\tCan be used in conjunction with `--single-branch` and `--no-tags` to\n -+\tclone only given ref. Cannot be combined with `--branch`.\n ++\tclone only the given ref. Cannot be combined with `--branch`.\n +\n `-u` __::\n `--upload-pack` __::\n @@ builtin/clone.c: static void update_head(const struct ref *our, const struct ref\n +\t\telse\n +\t\t\trefs_update_ref(get_main_ref_store(the_repository), msg,\n +\t\t\t\t\t\"HEAD\", &our->old_oid, NULL, REF_NO_DEREF,\n -+\t\t\t\t\tUPDATE_REFS_MSG_ON_ERR);\n ++\t\t\t\t\tUPDATE_REFS_DIE_ON_ERR);\n \t} else if (remote) {\n \t\t/*\n \t\t * We know remote HEAD points to a non-branch, or\n @@ t/t5612-clone-refspec.sh: test_expect_success '--single-branch with detached' '\n\n +test_expect_success 'with --ref' '\n +\tgit clone --ref=refs/some/three . dir_ref &&\n -+ git -C dir_ref for-each-ref refs > refs &&\n -+ sed -e \"/HEAD$/d\" \\\n ++\tgit -C dir_ref for-each-ref refs > refs &&\n ++\tsed -e \"/HEAD$/d\" \\\n +\t -e \"s|/remotes/origin/|/heads/|\" refs >actual &&\n +\tgit for-each-ref refs >expect &&\n +\ttest_cmp expect actual\n @@ t/t5612-clone-refspec.sh: test_expect_success '--single-branch with detached' '\n +\n +test_expect_success 'with --ref and --no-tags' '\n +\tgit clone --ref=refs/some/three --no-tags . dir_ref_notags &&\n -+ git -C dir_ref_notags for-each-ref refs > refs &&\n -+ sed -e \"/HEAD$/d\" \\\n ++\tgit -C dir_ref_notags for-each-ref refs > refs &&\n ++\tsed -e \"/HEAD$/d\" \\\n +\t -e \"s|/remotes/origin/|/heads/|\" refs >actual &&\n +\tgit for-each-ref refs/heads >expect &&\n +\tgit for-each-ref refs/some >>expect &&\n @@ t/t5612-clone-refspec.sh: test_expect_success '--single-branch with detached' '\n +\n +test_expect_success '--single-branch with --ref' '\n +\tgit clone --single-branch --ref=refs/some/three . dir_single_ref &&\n -+ git -C dir_single_ref for-each-ref refs > actual &&\n ++\tgit -C dir_single_ref for-each-ref refs > actual &&\n +\tgit for-each-ref refs/some >expect &&\n +\tgit for-each-ref refs/tags >>expect &&\n +\ttest_cmp expect actual\n @@ t/t5612-clone-refspec.sh: test_expect_success '--single-branch with detached' '\n +\n +test_expect_success '--single-branch with --ref and --no-tags' '\n +\tgit clone --single-branch --ref=refs/some/three --no-tags . dir_single_ref_notags &&\n -+ git -C dir_single_ref_notags for-each-ref refs > actual &&\n ++\tgit -C dir_single_ref_notags for-each-ref refs > actual &&\n +\tgit for-each-ref refs/some >expect &&\n +\ttest_cmp expect actual\n +'\n--\n2.46.0\n"}]}