{"thread":{"id":"65682","subject":"[PATCH] fetch: pass transport to post-fetch connectivity check","startedAt":"2026-05-24T12:28:15Z","lastAt":"2026-05-27T10:39:32Z","messageCount":6,"participants":["Kristofer Karlsson via GitGitGadget","Junio C Hamano","Kristofer Karlsson","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"543997","messageId":"pull.2123.git.1779625693328.gitgitgadget@gmail.com","threadId":"65682","inReplyTo":null,"subject":"[PATCH] fetch: pass transport to post-fetch connectivity check","fromName":"Kristofer Karlsson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-05-24T12:28:12Z","receivedAt":"2026-05-24T12:28:15Z","isPatch":true,"body":"From: Kristofer Karlsson <krka@spotify.com>\n\nWhen fetching with a transport that sets `self_contained_and_connected`\n(as index-pack does for self-contained packs), check_connected() can\nuse find_pack_entry_one() to skip connectivity verification for refs\nwhose objects exist in the new pack. This avoids sending those OIDs to\nthe rev-list child process.\n\nHowever, store_updated_refs() never passed the transport to\ncheck_connected(), so opt.transport was always NULL and this\noptimization was dead code for post-fetch connectivity checks.\n\nThread the transport parameter through store_updated_refs() and set\nopt.transport so that check_connected() can take advantage of\nself-contained packs.\n\nOn a large repository (2.4M commits, 374K files, 10.9K local refs),\nfetching 200 new commits:\n\n  Before: rev-list connectivity check  22s,  total fetch  36s\n  After:  rev-list connectivity check   5s,  total fetch  14s\n\nThe remaining 5s is spent verifying refs not contained in the new pack.\n\nSigned-off-by: Kristofer Karlsson <krka@spotify.com>\n---\n    fetch: pass transport to post-fetch connectivity check\n    \n    We're working on reducing git fetch times on a large monorepo (2.4M\n    commits, 374K files, 10.9K local refs). Profiling showed the post-fetch\n    connectivity check (rev-list --objects --stdin --not --all) dominating\n    wall time when there are new objects.\n    \n    While investigating, I noticed that check_connected() already has a fast\n    path for self-contained packs — it uses find_pack_entry_one() to skip\n    refs whose objects are in the new pack. builtin/clone.c passes the\n    transport to enable this, but store_updated_refs() in builtin/fetch.c\n    does not, making the optimization dead code for fetches.\n    \n    The fix is a three-line change to thread the transport through.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2123%2Fspkrka%2Ffetch-transport-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2123/spkrka/fetch-transport-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2123\n\n builtin/fetch.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a22c319467..647fd1c30c 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1213,6 +1213,7 @@ N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\n    \"to avoid this check\\n\");\n \n static int store_updated_refs(struct display_state *display_state,\n+\t\t\t      struct transport *transport,\n \t\t\t      int connectivity_checked,\n \t\t\t      struct ref_transaction *transaction, struct ref *ref_map,\n \t\t\t      struct fetch_head *fetch_head,\n@@ -1228,6 +1229,7 @@ static int store_updated_refs(struct display_state *display_state,\n \tif (!connectivity_checked) {\n \t\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n \n+\t\topt.transport = transport;\n \t\topt.exclude_hidden_refs_section = \"fetch\";\n \t\trm = ref_map;\n \t\tif (check_connected(iterate_ref_map, &rm, &opt)) {\n@@ -1432,7 +1434,7 @@ static int fetch_and_consume_refs(struct display_state *display_state,\n \t}\n \n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n-\tret = store_updated_refs(display_state, connectivity_checked,\n+\tret = store_updated_refs(display_state, transport, connectivity_checked,\n \t\t\t\t transaction, ref_map, fetch_head, config,\n \t\t\t\t display_array);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n\nbase-commit: 6a4418c36d6bad69a599044b3cf49dcbd049cb45\n-- \ngitgitgadget\n"},{"id":"543999","messageId":"xmqq4ijxhst9.fsf@gitster.g","threadId":"65682","inReplyTo":"pull.2123.git.1779625693328.gitgitgadget@gmail.com","subject":"Re: [PATCH] fetch: pass transport to post-fetch connectivity check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-24T12:53:54Z","receivedAt":"2026-05-24T12:53:57Z","isPatch":true,"body":"\"Kristofer Karlsson via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Kristofer Karlsson <krka@spotify.com>\n>\n> When fetching with a transport that sets `self_contained_and_connected`\n> (as index-pack does for self-contained packs), check_connected() can\n> use find_pack_entry_one() to skip connectivity verification for refs\n> whose objects exist in the new pack. This avoids sending those OIDs to\n> the rev-list child process.\n>\n> However, store_updated_refs() never passed the transport to\n> check_connected(), so opt.transport was always NULL and this\n> optimization was dead code for post-fetch connectivity checks.\n>\n> Thread the transport parameter through store_updated_refs() and set\n> opt.transport so that check_connected() can take advantage of\n> self-contained packs.\n>\n> On a large repository (2.4M commits, 374K files, 10.9K local refs),\n> fetching 200 new commits:\n>\n>   Before: rev-list connectivity check  22s,  total fetch  36s\n>   After:  rev-list connectivity check   5s,  total fetch  14s\n>\n> The remaining 5s is spent verifying refs not contained in the new pack.\n\nImpressive.\n\nThe check_connected() function itself is a battle tested helper\nfunction, with the optimization that originates in c6807a40 (clone:\nopen a shortcut for connectivity check, 2013-05-26), and then\npolished in 26b974b3 (check_connected(): delay opening new_pack,\n2026-03-05), allowing available \"transport\" to be taken into account\ndoes make very good sense.\n\nThe other call to check_connected() that appear in builtin/fetch.c\ndoes not pass opt.transport, either, but this one checks before we\neven fetch any packs over any transport, so a tweak similar to this\npatch would not help that code path, I guess.  In fact, many calls\nto check_connected() elsewhere use opt that is often local to the\nscope, that do not have transport at all.  I wonder if there are\nsome of them that benefit from a similar tweak?\n\nThanks.\n\n\n>\n> Signed-off-by: Kristofer Karlsson <krka@spotify.com>\n> ---\n>     fetch: pass transport to post-fetch connectivity check\n>     \n>     We're working on reducing git fetch times on a large monorepo (2.4M\n>     commits, 374K files, 10.9K local refs). Profiling showed the post-fetch\n>     connectivity check (rev-list --objects --stdin --not --all) dominating\n>     wall time when there are new objects.\n>     \n>     While investigating, I noticed that check_connected() already has a fast\n>     path for self-contained packs — it uses find_pack_entry_one() to skip\n>     refs whose objects are in the new pack. builtin/clone.c passes the\n>     transport to enable this, but store_updated_refs() in builtin/fetch.c\n>     does not, making the optimization dead code for fetches.\n>     \n>     The fix is a three-line change to thread the transport through.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2123%2Fspkrka%2Ffetch-transport-fix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2123/spkrka/fetch-transport-fix-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/2123\n>\n>  builtin/fetch.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index a22c319467..647fd1c30c 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1213,6 +1213,7 @@ N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\n>     \"to avoid this check\\n\");\n>  \n>  static int store_updated_refs(struct display_state *display_state,\n> +\t\t\t      struct transport *transport,\n>  \t\t\t      int connectivity_checked,\n>  \t\t\t      struct ref_transaction *transaction, struct ref *ref_map,\n>  \t\t\t      struct fetch_head *fetch_head,\n> @@ -1228,6 +1229,7 @@ static int store_updated_refs(struct display_state *display_state,\n>  \tif (!connectivity_checked) {\n>  \t\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n>  \n> +\t\topt.transport = transport;\n>  \t\topt.exclude_hidden_refs_section = \"fetch\";\n>  \t\trm = ref_map;\n>  \t\tif (check_connected(iterate_ref_map, &rm, &opt)) {\n> @@ -1432,7 +1434,7 @@ static int fetch_and_consume_refs(struct display_state *display_state,\n>  \t}\n>  \n>  \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n> -\tret = store_updated_refs(display_state, connectivity_checked,\n> +\tret = store_updated_refs(display_state, transport, connectivity_checked,\n>  \t\t\t\t transaction, ref_map, fetch_head, config,\n>  \t\t\t\t display_array);\n>  \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n>\n> base-commit: 6a4418c36d6bad69a599044b3cf49dcbd049cb45\n"},{"id":"544000","messageId":"CAL71e4Oct7SHEi+=Xx8Q9LxrRKXi_oov=wm86VyWwioyNCGoaA@mail.gmail.com","threadId":"65682","inReplyTo":"xmqq4ijxhst9.fsf@gitster.g","subject":"Re: [PATCH] fetch: pass transport to post-fetch connectivity check","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-05-24T13:04:57Z","receivedAt":"2026-05-24T13:05:09Z","isPatch":true,"body":"Good catch! After finding this case, I looked into the other related\ncall sites but found that they are already correct as-is:\n- builtin/clone.c - already passes opt.transport (this is where I\ncopied it from)\n- builtin/receive-pack.c (3 calls) - no transport object available to propagate\n- fetch-pack.c - only used for the --deepen path, which sets\nconnectivity_checked when it passes,\n  so the store_updated_refs() check is skipped entirely and transport\nis not needed\n- bundle.c - no need for transport\n\nI am not 100% sure, but I suppose it's always possible to follow up\nwith more reuse of this later.\n\n- Kristofer\n\nOn Sun, 24 May 2026 at 14:53, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Kristofer Karlsson via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n>\n> > From: Kristofer Karlsson <krka@spotify.com>\n> >\n> > When fetching with a transport that sets `self_contained_and_connected`\n> > (as index-pack does for self-contained packs), check_connected() can\n> > use find_pack_entry_one() to skip connectivity verification for refs\n> > whose objects exist in the new pack. This avoids sending those OIDs to\n> > the rev-list child process.\n> >\n> > However, store_updated_refs() never passed the transport to\n> > check_connected(), so opt.transport was always NULL and this\n> > optimization was dead code for post-fetch connectivity checks.\n> >\n> > Thread the transport parameter through store_updated_refs() and set\n> > opt.transport so that check_connected() can take advantage of\n> > self-contained packs.\n> >\n> > On a large repository (2.4M commits, 374K files, 10.9K local refs),\n> > fetching 200 new commits:\n> >\n> >   Before: rev-list connectivity check  22s,  total fetch  36s\n> >   After:  rev-list connectivity check   5s,  total fetch  14s\n> >\n> > The remaining 5s is spent verifying refs not contained in the new pack.\n>\n> Impressive.\n>\n> The check_connected() function itself is a battle tested helper\n> function, with the optimization that originates in c6807a40 (clone:\n> open a shortcut for connectivity check, 2013-05-26), and then\n> polished in 26b974b3 (check_connected(): delay opening new_pack,\n> 2026-03-05), allowing available \"transport\" to be taken into account\n> does make very good sense.\n>\n> The other call to check_connected() that appear in builtin/fetch.c\n> does not pass opt.transport, either, but this one checks before we\n> even fetch any packs over any transport, so a tweak similar to this\n> patch would not help that code path, I guess.  In fact, many calls\n> to check_connected() elsewhere use opt that is often local to the\n> scope, that do not have transport at all.  I wonder if there are\n> some of them that benefit from a similar tweak?\n>\n> Thanks.\n>\n>\n> >\n> > Signed-off-by: Kristofer Karlsson <krka@spotify.com>\n> > ---\n> >     fetch: pass transport to post-fetch connectivity check\n> >\n> >     We're working on reducing git fetch times on a large monorepo (2.4M\n> >     commits, 374K files, 10.9K local refs). Profiling showed the post-fetch\n> >     connectivity check (rev-list --objects --stdin --not --all) dominating\n> >     wall time when there are new objects.\n> >\n> >     While investigating, I noticed that check_connected() already has a fast\n> >     path for self-contained packs — it uses find_pack_entry_one() to skip\n> >     refs whose objects are in the new pack. builtin/clone.c passes the\n> >     transport to enable this, but store_updated_refs() in builtin/fetch.c\n> >     does not, making the optimization dead code for fetches.\n> >\n> >     The fix is a three-line change to thread the transport through.\n> >\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2123%2Fspkrka%2Ffetch-transport-fix-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2123/spkrka/fetch-transport-fix-v1\n> > Pull-Request: https://github.com/gitgitgadget/git/pull/2123\n> >\n> >  builtin/fetch.c | 4 +++-\n> >  1 file changed, 3 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n> > index a22c319467..647fd1c30c 100644\n> > --- a/builtin/fetch.c\n> > +++ b/builtin/fetch.c\n> > @@ -1213,6 +1213,7 @@ N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\n> >     \"to avoid this check\\n\");\n> >\n> >  static int store_updated_refs(struct display_state *display_state,\n> > +                           struct transport *transport,\n> >                             int connectivity_checked,\n> >                             struct ref_transaction *transaction, struct ref *ref_map,\n> >                             struct fetch_head *fetch_head,\n> > @@ -1228,6 +1229,7 @@ static int store_updated_refs(struct display_state *display_state,\n> >       if (!connectivity_checked) {\n> >               struct check_connected_options opt = CHECK_CONNECTED_INIT;\n> >\n> > +             opt.transport = transport;\n> >               opt.exclude_hidden_refs_section = \"fetch\";\n> >               rm = ref_map;\n> >               if (check_connected(iterate_ref_map, &rm, &opt)) {\n> > @@ -1432,7 +1434,7 @@ static int fetch_and_consume_refs(struct display_state *display_state,\n> >       }\n> >\n> >       trace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n> > -     ret = store_updated_refs(display_state, connectivity_checked,\n> > +     ret = store_updated_refs(display_state, transport, connectivity_checked,\n> >                                transaction, ref_map, fetch_head, config,\n> >                                display_array);\n> >       trace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n> >\n> > base-commit: 6a4418c36d6bad69a599044b3cf49dcbd049cb45\n"},{"id":"544141","messageId":"20260527083216.GA981444@coredump.intra.peff.net","threadId":"65682","inReplyTo":"pull.2123.git.1779625693328.gitgitgadget@gmail.com","subject":"Re: [PATCH] fetch: pass transport to post-fetch connectivity check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-27T08:32:16Z","receivedAt":"2026-05-27T08:32:23Z","isPatch":true,"body":"On Sun, May 24, 2026 at 12:28:12PM +0000, Kristofer Karlsson via GitGitGadget wrote:\n\n> From: Kristofer Karlsson <krka@spotify.com>\n> \n> When fetching with a transport that sets `self_contained_and_connected`\n> (as index-pack does for self-contained packs), check_connected() can\n> use find_pack_entry_one() to skip connectivity verification for refs\n> whose objects exist in the new pack. This avoids sending those OIDs to\n> the rev-list child process.\n> \n> However, store_updated_refs() never passed the transport to\n> check_connected(), so opt.transport was always NULL and this\n> optimization was dead code for post-fetch connectivity checks.\n> \n> Thread the transport parameter through store_updated_refs() and set\n> opt.transport so that check_connected() can take advantage of\n> self-contained packs.\n\nThat makes sense in principle, but one thing puzzles me. We only turn on\nthe optimization in check_connected() if the transport's smart_options\nhas the self_contained_and_connected bit set. And we set that only when\nwe were told via check_self_contained_and_connected to do so (and we\npass the appropriate option to index-pack, which tells us the result is\nOK).\n\nAnd the only place that turns on check_self_contained_and_connected is\nin builtin/clone.c. So how does this optimization work for a non-clone\nfetch? Am I missing some code path?\n\n-Peff\n"},{"id":"544148","messageId":"CAL71e4MrVqC1=AR6x0_8S=8kVqPdDkhgCZRb4etFsxTzd6s_8Q@mail.gmail.com","threadId":"65682","inReplyTo":"20260527083216.GA981444@coredump.intra.peff.net","subject":"Re: [PATCH] fetch: pass transport to post-fetch connectivity check","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-05-27T10:04:19Z","receivedAt":"2026-05-27T10:04:31Z","isPatch":true,"body":"You're right. I dug into this further and realized the problem is deeper\nthan just the flag not being set in builtin/fetch.c.\n\nEven if we add:\ntransport->smart_options->check_self_contained_and_connected = 1;\nto prepare_transport(), the optimization still won't work for fetches.\n\nThe optimization is fundamentally clone-only.\n\nI was unable to reproduce the benchmark numbers from my original commit\nmessage. The patch as submitted is indeed inert for non-clone fetches.\nIt looked like a simple improvement, but it's clear that it was incorrect.\nI'll drop it, and I apologize for the noise here.\n\n-- Kristofer\n\nOn Wed, 27 May 2026 at 10:32, Jeff King <peff@peff.net> wrote:\n>\n> On Sun, May 24, 2026 at 12:28:12PM +0000, Kristofer Karlsson via GitGitGadget wrote:\n>\n> > From: Kristofer Karlsson <krka@spotify.com>\n> >\n> > When fetching with a transport that sets `self_contained_and_connected`\n> > (as index-pack does for self-contained packs), check_connected() can\n> > use find_pack_entry_one() to skip connectivity verification for refs\n> > whose objects exist in the new pack. This avoids sending those OIDs to\n> > the rev-list child process.\n> >\n> > However, store_updated_refs() never passed the transport to\n> > check_connected(), so opt.transport was always NULL and this\n> > optimization was dead code for post-fetch connectivity checks.\n> >\n> > Thread the transport parameter through store_updated_refs() and set\n> > opt.transport so that check_connected() can take advantage of\n> > self-contained packs.\n>\n> That makes sense in principle, but one thing puzzles me. We only turn on\n> the optimization in check_connected() if the transport's smart_options\n> has the self_contained_and_connected bit set. And we set that only when\n> we were told via check_self_contained_and_connected to do so (and we\n> pass the appropriate option to index-pack, which tells us the result is\n> OK).\n>\n> And the only place that turns on check_self_contained_and_connected is\n> in builtin/clone.c. So how does this optimization work for a non-clone\n> fetch? Am I missing some code path?\n>\n> -Peff\n"},{"id":"544151","messageId":"20260527103930.GJ981444@coredump.intra.peff.net","threadId":"65682","inReplyTo":"CAL71e4MrVqC1=AR6x0_8S=8kVqPdDkhgCZRb4etFsxTzd6s_8Q@mail.gmail.com","subject":"Re: [PATCH] fetch: pass transport to post-fetch connectivity check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-27T10:39:30Z","receivedAt":"2026-05-27T10:39:32Z","isPatch":true,"body":"On Wed, May 27, 2026 at 12:04:19PM +0200, Kristofer Karlsson wrote:\n\n> You're right. I dug into this further and realized the problem is deeper\n> than just the flag not being set in builtin/fetch.c.\n> \n> Even if we add:\n> transport->smart_options->check_self_contained_and_connected = 1;\n> to prepare_transport(), the optimization still won't work for fetches.\n> \n> The optimization is fundamentally clone-only.\n\nI have wondered if the transport could do the same thing for:\n\n  git init\n  git fetch ...\n\nWhen we do not send any \"want\", then we'd expect the pack we receive to\nbe self-contained.\n\nBut in practice that is not that exciting, as it is a special case that\ndoes not come up that often.\n\nI suspect there's some hybrid mode where we could save some work. It is\neasy for index-pack to come up with a list of \"edges\" from the pack it\ngot that point outside of the pack. We just need to know that those\nedges are reachable from existing refs. So really we could be checking\nthe connectivity of those edges, rather than the actual ref tips.\n\nWould that be less work? I'm not sure. It saves walking over the\nnewly-fetched history, but in practice that is probably not that\nexpensive. It potentially saves a lot when the edge is a ref tip; for a\ntrue fast-forward we'd see a ref going from A..B, and if index-pack\ntells us that it just needs A, we can skip the traversal entirely.\n\nBut index-pack isn't really thinking in terms of commits, but rather the\nwhole object graph. So you're going to find that commit A is needed, but\nalso all of the tree entries in the existing history that weren't\ntouched by the new history (e.g., B touched path \"foo\" but not \"bar\", so\nit gets a new top-level tree, a new blob for \"foo\", but still references\nthe existing blob for \"bar\"). I guess you could speculatively load A^{tree}\nto cull the list.\n\nSo I dunno. I think there is some room for speedup here, and in many\ncommon cases you could skip the rev-list invocation entirely. But it's\nnot trivial, and I think is far afield from what your patch was\noriginally trying to do. ;)\n\n> I was unable to reproduce the benchmark numbers from my original commit\n> message.\n\nYeah, I wondered where the numbers came from. It is very easy to fool\nyourself with fetch benchmarks, because even \"fetch --dry-run\" will\ntransfer objects, and under the hood we try to optimize out as much\nobject transfer as possible. So you really have to start from the exact\nsame on-disk state for each trial.\n\n> The patch as submitted is indeed inert for non-clone fetches.\n> It looked like a simple improvement, but it's clear that it was incorrect.\n> I'll drop it, and I apologize for the noise here.\n\nNo problem. You've been generating some interesting optimization work\nlately, so I can't complain. :)\n\nI'll probably have more comments on your other topics, but I'm out of\ntime for tonight.\n\n-Peff\n"}]}