{"thread":{"id":"59510","subject":"[PATCH] fetch: download bundles once, even with --all","startedAt":"2023-03-31T15:59:11Z","lastAt":"2023-04-03T18:32:18Z","messageCount":5,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"474510","messageId":"pull.1508.git.1680278344173.gitgitgadget@gmail.com","threadId":"59510","inReplyTo":null,"subject":"[PATCH] fetch: download bundles once, even with --all","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-31T15:59:04Z","receivedAt":"2023-03-31T15:59:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nWhen fetch.bundleURI is set, 'git fetch' downloads bundles from the\ngiven bundle URI before fetching from the specified remote. However,\nwhen using non-file remotes, 'git fetch --all' will launch 'git fetch'\nsubprocesses which then read fetch.bundleURI and fetch the bundle list\nagain. We do not expect the bundle list to have new information during\nthese multiple runs, so avoid these extra calls by un-setting\nfetch.bundleURI in the subprocess arguments.\n\nBe careful to skip fetching bundles for the empty bundle string.\nFetching bundles from the empty list presents some interesting test\nfailures.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n    fetch: download bundles once, even with --all\n    \n    I noticed this in local testing where I had set up a bundle server for\n    my Git repo, but wrote the wrong port number so I saw multiple warning\n    messages during a git fetch --all run.\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1508%2Fderrickstolee%2Ffetch-bundles-multiple-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1508/derrickstolee/fetch-bundles-multiple-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1508\n\n builtin/fetch.c             |  7 ++++++-\n bundle-uri.c                |  9 +++++++++\n t/t5558-clone-bundle-uri.sh | 34 ++++++++++++++++++++++++++++++++++\n 3 files changed, 49 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7221e57f352..8d8b2e0c26f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1955,7 +1955,12 @@ static int fetch_multiple(struct string_list *list, int max_children)\n \t\t\treturn errcode;\n \t}\n \n-\tstrvec_pushl(&argv, \"fetch\", \"--append\", \"--no-auto-gc\",\n+\t/*\n+\t * Cancel out the fetch.bundleURI config when running subprocesses,\n+\t * to avoid fetching from the same bundle list multiple times.\n+\t */\n+\tstrvec_pushl(&argv, \"-c\", \"fetch.bundleURI=\",\n+\t\t     \"fetch\", \"--append\", \"--no-auto-gc\",\n \t\t     \"--no-write-commit-graph\", NULL);\n \tadd_options_to_argv(&argv);\n \ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex 177c1810402..56390651b92 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -792,6 +792,15 @@ int fetch_bundle_uri(struct repository *r, const char *uri,\n \n \tinit_bundle_list(&list);\n \n+\t/*\n+\t * Do not fetch a NULL or empty bundle URI. An empty bundle URI\n+\t * could signal that a configured bundle URI has been disabled.\n+\t */\n+\tif (!uri || !*uri) {\n+\t\tresult = 0;\n+\t\tgoto cleanup;\n+\t}\n+\n \t/* If a bundle is added to this global list, then it is required. */\n \tlist.mode = BUNDLE_MODE_ALL;\n \ndiff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh\nindex afd56926c53..996a08e90c9 100755\n--- a/t/t5558-clone-bundle-uri.sh\n+++ b/t/t5558-clone-bundle-uri.sh\n@@ -1018,6 +1018,40 @@ test_expect_success 'creationToken heuristic with failed downloads (fetch)' '\n \ttest_cmp expect refs\n '\n \n+test_expect_success 'bundles are downloaded once during fetch --all' '\n+\ttest_when_finished rm -rf download-* trace*.txt fetch-mult &&\n+\n+\tcat >\"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list\" <<-EOF &&\n+\t[bundle]\n+\t\tversion = 1\n+\t\tmode = all\n+\t\theuristic = creationToken\n+\n+\t[bundle \"bundle-1\"]\n+\t\turi = bundle-1.bundle\n+\t\tcreationToken = 1\n+\n+\t[bundle \"bundle-2\"]\n+\t\turi = bundle-2.bundle\n+\t\tcreationToken = 2\n+\n+\t[bundle \"bundle-3\"]\n+\t\turi = bundle-3.bundle\n+\t\tcreationToken = 3\n+\tEOF\n+\n+\tgit clone --single-branch --branch=left \\\n+\t\t--bundle-uri=\"$HTTPD_URL/bundle-list\" \\\n+\t\t\"$HTTPD_URL/smart/fetch.git\" fetch-mult &&\n+\tgit -C fetch-mult remote add dup1 \"$HTTPD_URL/smart/fetch.git\" &&\n+\tgit -C fetch-mult remote add dup2 \"$HTTPD_URL/smart/fetch.git\" &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace-mult.txt\" \\\n+\t\tgit -C fetch-mult fetch --all &&\n+\tgrep \"\\\"child_start\\\".*\\\"git-remote-https\\\",\\\"$HTTPD_URL/bundle-list\\\"\" \\\n+\t\ttrace-mult.txt >bundle-fetches &&\n+\ttest_line_count = 1 bundle-fetches\n+'\n # Do not add tests here unless they use the HTTP server, as they will\n # not run unless the HTTP dependencies exist.\n \n\nbase-commit: 6369acd968d02899973a9a853c48029b92cea401\n-- \ngitgitgadget\n"},{"id":"474516","messageId":"xmqqo7o8g89a.fsf@gitster.g","threadId":"59510","inReplyTo":"pull.1508.git.1680278344173.gitgitgadget@gmail.com","subject":"Re: [PATCH] fetch: download bundles once, even with --all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-31T17:06:57Z","receivedAt":"2023-03-31T17:07:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <derrickstolee@github.com>\n>\n> When fetch.bundleURI is set, 'git fetch' downloads bundles from the\n> given bundle URI before fetching from the specified remote. However,\n> when using non-file remotes, 'git fetch --all' will launch 'git fetch'\n> subprocesses which then read fetch.bundleURI and fetch the bundle list\n> again. We do not expect the bundle list to have new information during\n> these multiple runs, so avoid these extra calls by un-setting\n> fetch.bundleURI in the subprocess arguments.\n\nIt is a good observation, if we assume that all these remotes want\nto share the same expectation on what the list of bundles described\nin bundleURI should be.\n\nI expected that people use multiple remotes and do a \"fetch --all\"\nbefore starting work from a cronjob or while fetching coffee first\ntime in the morning to fetch from repositories holding work from\ndifferent folks and manged by different groups, and these groups do\nnot tightly share the object management recipes at what bundles to\npre-package and list in the list served at a bundleURI.  If there is\nsuch an arrangement between repositires to share the object\nmanagement, even if the repositories fetched with the \"--all\" option\nare truly multiple places, it may make sense to assume that these\nrepositories you are fetching from want you to use the same set of\nbundles that are managed the same way to be used.  But I am not sure\nif that assumption holds true.\n\nWhere does fetch.bundleURI come originally?  If we set only one\nglobally for the local repository at \"git clone\" time, perhaps that\nis a problem?  IOW, instead of fetch.bundleURI, we would want to\nhave something per remote, e.g. remote.<name>.bundleURI, instead?\n\nPutting that design level tangent aside, everything I see in this\npatch makes sense, assuming that the fetch_bundle_uri() call done\nfairly early in the parent process is sufficient to tell the child\nprocesses that deal with individual repositories to reuse the info\nthat was already retrieved that call.\n\nThanks.\n\n"},{"id":"474519","messageId":"780308b8-6e61-fe20-78c2-47df1c1737c9@github.com","threadId":"59510","inReplyTo":"xmqqo7o8g89a.fsf@gitster.g","subject":"Re: [PATCH] fetch: download bundles once, even with --all","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-03-31T18:09:47Z","receivedAt":"2023-03-31T18:09:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/31/2023 1:06 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Derrick Stolee <derrickstolee@github.com>\n>>\n>> When fetch.bundleURI is set, 'git fetch' downloads bundles from the\n>> given bundle URI before fetching from the specified remote. However,\n>> when using non-file remotes, 'git fetch --all' will launch 'git fetch'\n>> subprocesses which then read fetch.bundleURI and fetch the bundle list\n>> again. We do not expect the bundle list to have new information during\n>> these multiple runs, so avoid these extra calls by un-setting\n>> fetch.bundleURI in the subprocess arguments.\n> \n> It is a good observation, if we assume that all these remotes want\n> to share the same expectation on what the list of bundles described\n> in bundleURI should be.\n> \n> I expected that people use multiple remotes and do a \"fetch --all\"\n> before starting work from a cronjob or while fetching coffee first\n> time in the morning to fetch from repositories holding work from\n> different folks and manged by different groups, and these groups do\n> not tightly share the object management recipes at what bundles to\n> pre-package and list in the list served at a bundleURI.  If there is\n> such an arrangement between repositires to share the object\n> management, even if the repositories fetched with the \"--all\" option\n> are truly multiple places, it may make sense to assume that these\n> repositories you are fetching from want you to use the same set of\n> bundles that are managed the same way to be used.  But I am not sure\n> if that assumption holds true.\n> \n> Where does fetch.bundleURI come originally?  If we set only one\n> globally for the local repository at \"git clone\" time, perhaps that\n> is a problem?  IOW, instead of fetch.bundleURI, we would want to\n> have something per remote, e.g. remote.<name>.bundleURI, instead?\n\nfetch.bundleURI is either set by 'git clone --bundle-uri' when the\nlist advertises a bundle.heuristic value, or can be set manually by\nthe user.\n\nThis indicates the use of a \"standalone\" bundle server that is\nmanaged independently from a Git remote. The bundle list is downloaded\nfrom a static URI instead of during protocol v2 in the 'bundle-uri'\ncommand.\n\nIn that sense, the static bundle URI is not paired with a remote,\nbut instead presents a place where objects can be downloaded perhaps\nfaster than any Git remote (by downloading from a static file, or\nbecause the bundle server has a faster link to the client machine).\nThe objects added to the client object store can then reduce the\nsize of the fetch from each other Git server remote, of course only\nif the objects in the bundle(s) are also present on those remotes.\n\nIf there are remote-specific bundle lists, then the protocol v2\ncommand will be a good way to communicate bundle lists that are\ncustom to those remotes. However, even in that case it is unlikely\nthat sourcing bundles from multiple sources will be useful, since\nthe bundle downloads will likely have significant common data.\n \n> Putting that design level tangent aside, everything I see in this\n> patch makes sense, assuming that the fetch_bundle_uri() call done\n> fairly early in the parent process is sufficient to tell the child\n> processes that deal with individual repositories to reuse the info\n> that was already retrieved that call.\n\nThanks for looking closely.\n\n-Stolee\n"},{"id":"474549","messageId":"20230401082233.GA3503828@coredump.intra.peff.net","threadId":"59510","inReplyTo":"pull.1508.git.1680278344173.gitgitgadget@gmail.com","subject":"Re: [PATCH] fetch: download bundles once, even with --all","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-01T08:22:33Z","receivedAt":"2023-04-01T08:22:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 31, 2023 at 03:59:04PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> diff --git a/bundle-uri.c b/bundle-uri.c\n> index 177c1810402..56390651b92 100644\n> --- a/bundle-uri.c\n> +++ b/bundle-uri.c\n> @@ -792,6 +792,15 @@ int fetch_bundle_uri(struct repository *r, const char *uri,\n>  \n>  \tinit_bundle_list(&list);\n>  \n> +\t/*\n> +\t * Do not fetch a NULL or empty bundle URI. An empty bundle URI\n> +\t * could signal that a configured bundle URI has been disabled.\n> +\t */\n> +\tif (!uri || !*uri) {\n> +\t\tresult = 0;\n> +\t\tgoto cleanup;\n> +\t}\n\nCoverity flagged this \"!uri\"; it can never be true since we\nunconditionally xstrdup(uri) at the top of the function, and we'd\nsegfault in that case.\n\nI think the existing code that assumes a non-NULL uri is reasonable.\nEven before the xstrdup() we'd have segfaulted when it was handed to\nfetch_bundle_uri_internal().\n\nIt's not a huge deal, in the sense that you're just being overly\ndefensive, but the comment and conditional here are a little misleading.\n\n-Peff\n"},{"id":"474703","messageId":"xmqqy1n84y1l.fsf@gitster.g","threadId":"59510","inReplyTo":"20230401082233.GA3503828@coredump.intra.peff.net","subject":"Re: [PATCH] fetch: download bundles once, even with --all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-03T18:32:06Z","receivedAt":"2023-04-03T18:32:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> +\tif (!uri || !*uri) {\n>> +\t\tresult = 0;\n>> +\t\tgoto cleanup;\n>> +\t}\n>\n> Coverity flagged this \"!uri\"; it can never be true since we\n> unconditionally xstrdup(uri) at the top of the function, and we'd\n> segfault in that case.\n>\n> I think the existing code that assumes a non-NULL uri is reasonable.\n> Even before the xstrdup() we'd have segfaulted when it was handed to\n> fetch_bundle_uri_internal().\n>\n> It's not a huge deal, in the sense that you're just being overly\n> defensive, but the comment and conditional here are a little misleading.\n\nNice.  Thanks for commenting on what you spotted.\n"}]}