{"thread":{"id":"66067","subject":"[PATCH] fetch-pack: trace packfile URI downloads","startedAt":"2026-07-26T08:33:12Z","lastAt":"2026-09-08T03:27:03Z","messageCount":4,"participants":["Ted Nyman","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"548989","messageId":"20260726083310.16180-2-tnyman@openai.com","threadId":"66067","inReplyTo":null,"subject":"[PATCH] fetch-pack: trace packfile URI downloads","fromName":"Ted Nyman","fromEmail":"tnyman@openai.com","sentAt":"2026-07-26T08:33:11Z","receivedAt":"2026-07-26T08:33:12Z","isPatch":true,"body":"When a protocol v2 fetch includes packfile URIs, the client downloads\neach advertised pack in a separate http-fetch process. Existing Trace2\nregions cover negotiation, but not the time spent downloading these\npacks or the number of advertised URIs.\n\nAdd a Trace2 region around the packfile URI download loop and record the\nnumber of URIs. This makes the cost of downloading external packs\nvisible without emitting an event for each pack.\n\nExtend the existing packfile URI test to verify the region and count.\n\nSigned-off-by: Ted Nyman <tnyman@openai.com>\n---\n fetch-pack.c           | 12 ++++++++++++\n t/t5702-protocol-v2.sh |  7 ++++++-\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 29c41132ee..701a23f808 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1886,6 +1886,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t}\n \t}\n \n+\tif (packfile_uris.nr) {\n+\t\ttrace2_region_enter(\"fetch-pack\", \"packfile-uris\",\n+\t\t\t\t    the_repository);\n+\t\ttrace2_data_intmax(\"fetch-pack\", the_repository,\n+\t\t\t\t   \"packfile-uris/count\", packfile_uris.nr);\n+\t}\n+\n \tfor (i = 0; i < packfile_uris.nr; i++) {\n \t\tint j;\n \t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n@@ -1936,6 +1943,11 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t\t\t repo_get_object_directory(the_repository),\n \t\t\t\t\t\t packname));\n \t}\n+\n+\tif (packfile_uris.nr)\n+\t\ttrace2_region_leave(\"fetch-pack\", \"packfile-uris\",\n+\t\t\t\t    the_repository);\n+\n \tstring_list_clear(&packfile_uris, 0);\n \tstrvec_clear(&index_pack_args);\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 74a2b7730b..537deff7b3 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -1223,7 +1223,7 @@ configure_exclusion () {\n \n test_expect_success 'part of packfile response provided as URI' '\n \tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n-\trm -rf \"$P\" http_child log &&\n+\trm -rf \"$P\" http_child log trace2 &&\n \n \tgit init \"$P\" &&\n \tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n@@ -1238,10 +1238,15 @@ test_expect_success 'part of packfile response provided as URI' '\n \tconfigure_exclusion \"$P\" other-blob >h2 &&\n \n \tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace2\" \\\n \tgit -c protocol.version=2 \\\n \t\t-c fetch.uriprotocols=http,https \\\n \t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n \n+\ttest_grep \\\"event\\\":\\\"region_enter\\\".*\\\"label\\\":\\\"packfile-uris\\\" trace2 &&\n+\ttest_grep \\\"key\\\":\\\"packfile-uris/count\\\",\\\"value\\\":\\\"2\\\" trace2 &&\n+\ttest_grep \\\"event\\\":\\\"region_leave\\\".*\\\"label\\\":\\\"packfile-uris\\\" trace2 &&\n+\n \t# Ensure that my-blob and other-blob are in separate packfiles.\n \tfor idx in http_child/.git/objects/pack/*.idx\n \tdo\n"},{"id":"550286","messageId":"anskQP_xB-Xw3nug@pks.im","threadId":"66067","inReplyTo":"20260726083310.16180-2-tnyman@openai.com","subject":"Re: [PATCH] fetch-pack: trace packfile URI downloads","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-11T13:31:44Z","receivedAt":"2026-08-11T13:31:53Z","isPatch":true,"body":"On Sun, Jul 26, 2026 at 01:33:11AM -0700, Ted Nyman wrote:\n> When a protocol v2 fetch includes packfile URIs, the client downloads\n> each advertised pack in a separate http-fetch process. Existing Trace2\n> regions cover negotiation, but not the time spent downloading these\n> packs or the number of advertised URIs.\n> \n> Add a Trace2 region around the packfile URI download loop and record the\n> number of URIs. This makes the cost of downloading external packs\n> visible without emitting an event for each pack.\n\nRight, by having a region we can verify how long downloading the\npackfiles took, and by tracking the number of packfiles we know how many\nwe fetched. What we don't know is how long fetching each of the\nindividual packs took, but I think that omission makes sense. After all,\nwe can reasonably expect all packs to be served by the same infra, and\nas such they should usually have similar download speeds.\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 29c41132ee..701a23f808 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1886,6 +1886,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n>  \t\t}\n>  \t}\n>  \n> +\tif (packfile_uris.nr) {\n> +\t\ttrace2_region_enter(\"fetch-pack\", \"packfile-uris\",\n> +\t\t\t\t    the_repository);\n> +\t\ttrace2_data_intmax(\"fetch-pack\", the_repository,\n> +\t\t\t\t   \"packfile-uris/count\", packfile_uris.nr);\n\nWe don't have a repository available in our context, so we have to use\n`the_repository`.\n\n> +\t}\n> +\n>  \tfor (i = 0; i < packfile_uris.nr; i++) {\n>  \t\tint j;\n>  \t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\nSensible. We don't need to track fetching if we don't have any packfiles\nat all.\n\n> @@ -1936,6 +1943,11 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n>  \t\t\t\t\t\t repo_get_object_directory(the_repository),\n>  \t\t\t\t\t\t packname));\n>  \t}\n> +\n> +\tif (packfile_uris.nr)\n> +\t\ttrace2_region_leave(\"fetch-pack\", \"packfile-uris\",\n> +\t\t\t\t    the_repository);\n> +\n>  \tstring_list_clear(&packfile_uris, 0);\n>  \tstrvec_clear(&index_pack_args);\n>  \n\nAnd likewise, we don't have to leave the region, either in that case.\n\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 74a2b7730b..537deff7b3 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -1223,7 +1223,7 @@ configure_exclusion () {\n>  \n>  test_expect_success 'part of packfile response provided as URI' '\n>  \tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n> -\trm -rf \"$P\" http_child log &&\n> +\trm -rf \"$P\" http_child log trace2 &&\n>  \n>  \tgit init \"$P\" &&\n>  \tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n> @@ -1238,10 +1238,15 @@ test_expect_success 'part of packfile response provided as URI' '\n>  \tconfigure_exclusion \"$P\" other-blob >h2 &&\n>  \n>  \tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/trace2\" \\\n>  \tgit -c protocol.version=2 \\\n>  \t\t-c fetch.uriprotocols=http,https \\\n>  \t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n>  \n> +\ttest_grep \\\"event\\\":\\\"region_enter\\\".*\\\"label\\\":\\\"packfile-uris\\\" trace2 &&\n> +\ttest_grep \\\"key\\\":\\\"packfile-uris/count\\\",\\\"value\\\":\\\"2\\\" trace2 &&\n> +\ttest_grep \\\"event\\\":\\\"region_leave\\\".*\\\"label\\\":\\\"packfile-uris\\\" trace2 &&\n> +\n>  \t# Ensure that my-blob and other-blob are in separate packfiles.\n>  \tfor idx in http_child/.git/objects/pack/*.idx\n>  \tdo\n\nIt does feel a tiny bit off to piggy-back on an existing test that has\nnothing to do with tracing except that it requires the traces to... I\ndunno, what does the existing test even do with the written logfile?\nDoesn't seem like it's using it at all.\n\nAnyway, having this in a separate test would've been nice, but that\ndoesn't warrant a reroll in my eyes. So overall, this patch looks good\nto me, thanks!\n\nPatrick\n"},{"id":"552172","messageId":"20260907212454.80627-1-tnyman@openai.com","threadId":"66067","inReplyTo":"anskQP_xB-Xw3nug@pks.im","subject":"Re: [PATCH] fetch-pack: trace packfile URI downloads","fromName":"Ted Nyman","fromEmail":"tnyman@openai.com","sentAt":"2026-09-07T21:24:54Z","receivedAt":"2026-09-07T21:24:58Z","isPatch":true,"body":"On Tue, Aug 11, 2026 at 03:31:44PM +0200, Patrick Steinhardt wrote:\n> Anyway, having this in a separate test would've been nice, but that\n> doesn't warrant a reroll in my eyes. So overall, this patch looks good\n> to me, thanks!\n\nThanks for reviewing, Patrick.\n\nJunio, a gentle ping on this patch. Patrick was positive on it and didn't\nthink the test organization warranted a reroll. Is there anything else\nyou'd like me to address before picking it up? I'm happy to split out the\ntracing checks into a separate test or refresh the patch if that helps.\n\nThanks,\nTed\n"},{"id":"552177","messageId":"xmqqse3kzam2.fsf@gitster.g","threadId":"66067","inReplyTo":"20260907212454.80627-1-tnyman@openai.com","subject":"Re: [PATCH] fetch-pack: trace packfile URI downloads","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-08T03:27:01Z","receivedAt":"2026-09-08T03:27:03Z","isPatch":true,"body":"Ted Nyman <tnyman@openai.com> writes:\n\n> On Tue, Aug 11, 2026 at 03:31:44PM +0200, Patrick Steinhardt wrote:\n>> Anyway, having this in a separate test would've been nice, but that\n>> doesn't warrant a reroll in my eyes. So overall, this patch looks good\n>> to me, thanks!\n>\n> Thanks for reviewing, Patrick.\n>\n> Junio, a gentle ping on this patch. Patrick was positive on it and didn't\n> think the test organization warranted a reroll. Is there anything else\n> you'd like me to address before picking it up? I'm happy to split out the\n> tracing checks into a separate test or refresh the patch if that helps.\n\nThanks.  This slipped below my radar.  It does not look to me that\nthe placement of the test is too bad.\n\nWill queue.\n"}]}