Volume XXII, number 279Tuesday, October 6, 2026Latest message 45 minutes ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

patchfetch-pack: trace packfile URI downloads

4 messages between Jul 26, 2026 and Sep 8, 2026, from Ted Nyman, Patrick Steinhardt, Junio C Hamano.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Ted NymanJul 26, 2026, 08:33 UTC on lore

When a protocol v2 fetch includes packfile URIs, the client downloads each advertised pack in a separate http-fetch process. Existing Trace2 regions cover negotiation, but not the time spent downloading these packs or the number of advertised URIs.

Add a Trace2 region around the packfile URI download loop and record the number of URIs. This makes the cost of downloading external packs visible without emitting an event for each pack.

Extend the existing packfile URI test to verify the region and count.
Signed-off-by: Ted Nyman <tnyman@openai.com>
---
 fetch-pack.c           | 12 ++++++++++++
 t/t5702-protocol-v2.sh |  7 ++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)
Show changes to 2 files +18 −1

fetch-pack.c, t/t5702-protocol-v2.sh

diff --git a/fetch-pack.c b/fetch-pack.c
index 29c41132ee..701a23f808 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1886,6 +1886,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		}
 	}
 
+	if (packfile_uris.nr) {
+		trace2_region_enter("fetch-pack", "packfile-uris",
+				    the_repository);
+		trace2_data_intmax("fetch-pack", the_repository,
+				   "packfile-uris/count", packfile_uris.nr);
+	}
+
 	for (i = 0; i < packfile_uris.nr; i++) {
 		int j;
 		struct child_process cmd = CHILD_PROCESS_INIT;
@@ -1936,6 +1943,11 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 						 repo_get_object_directory(the_repository),
 						 packname));
 	}
+
+	if (packfile_uris.nr)
+		trace2_region_leave("fetch-pack", "packfile-uris",
+				    the_repository);
+
 	string_list_clear(&packfile_uris, 0);
 	strvec_clear(&index_pack_args);
 
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 74a2b7730b..537deff7b3 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -1223,7 +1223,7 @@ configure_exclusion () {
 
 test_expect_success 'part of packfile response provided as URI' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
-	rm -rf "$P" http_child log &&
+	rm -rf "$P" http_child log trace2 &&
 
 	git init "$P" &&
 	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
@@ -1238,10 +1238,15 @@ test_expect_success 'part of packfile response provided as URI' '
 	configure_exclusion "$P" other-blob >h2 &&
 
 	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
+	GIT_TRACE2_EVENT="$(pwd)/trace2" \
 	git -c protocol.version=2 \
 		-c fetch.uriprotocols=http,https \
 		clone "$HTTPD_URL/smart/http_parent" http_child &&
 
+	test_grep \"event\":\"region_enter\".*\"label\":\"packfile-uris\" trace2 &&
+	test_grep \"key\":\"packfile-uris/count\",\"value\":\"2\" trace2 &&
+	test_grep \"event\":\"region_leave\".*\"label\":\"packfile-uris\" trace2 &&
+
 	# Ensure that my-blob and other-blob are in separate packfiles.
 	for idx in http_child/.git/objects/pack/*.idx
 	do
Patrick SteinhardtAug 11, 2026, 13:31 UTC in reply to Ted Nyman on lore

Re: [PATCH] fetch-pack: trace packfile URI downloads

On Sun, Jul 26, 2026 at 01:33:11AM -0700, Ted Nyman wrote:
Show 8 quoted lines
> When a protocol v2 fetch includes packfile URIs, the client downloads
> each advertised pack in a separate http-fetch process. Existing Trace2
> regions cover negotiation, but not the time spent downloading these
> packs or the number of advertised URIs.
> 
> Add a Trace2 region around the packfile URI download loop and record the
> number of URIs. This makes the cost of downloading external packs
> visible without emitting an event for each pack.

Right, by having a region we can verify how long downloading the packfiles took, and by tracking the number of packfiles we know how many we fetched. What we don't know is how long fetching each of the individual packs took, but I think that omission makes sense. After all, we can reasonably expect all packs to be served by the same infra, and as such they should usually have similar download speeds.

Show 13 quoted lines
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 29c41132ee..701a23f808 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1886,6 +1886,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  		}
>  	}
>  
> +	if (packfile_uris.nr) {
> +		trace2_region_enter("fetch-pack", "packfile-uris",
> +				    the_repository);
> +		trace2_data_intmax("fetch-pack", the_repository,
> +				   "packfile-uris/count", packfile_uris.nr);

We don't have a repository available in our context, so we have to use `the_repository`.

Show 5 quoted lines
> +	}
> +
>  	for (i = 0; i < packfile_uris.nr; i++) {
>  		int j;
>  		struct child_process cmd = CHILD_PROCESS_INIT;

Sensible. We don't need to track fetching if we don't have any packfiles at all.

Show 12 quoted lines
> @@ -1936,6 +1943,11 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  						 repo_get_object_directory(the_repository),
>  						 packname));
>  	}
> +
> +	if (packfile_uris.nr)
> +		trace2_region_leave("fetch-pack", "packfile-uris",
> +				    the_repository);
> +
>  	string_list_clear(&packfile_uris, 0);
>  	strvec_clear(&index_pack_args);
>  
And likewise, we don't have to leave the region, either in that case.
Show 29 quoted lines
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 74a2b7730b..537deff7b3 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -1223,7 +1223,7 @@ configure_exclusion () {
>  
>  test_expect_success 'part of packfile response provided as URI' '
>  	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
> -	rm -rf "$P" http_child log &&
> +	rm -rf "$P" http_child log trace2 &&
>  
>  	git init "$P" &&
>  	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
> @@ -1238,10 +1238,15 @@ test_expect_success 'part of packfile response provided as URI' '
>  	configure_exclusion "$P" other-blob >h2 &&
>  
>  	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
> +	GIT_TRACE2_EVENT="$(pwd)/trace2" \
>  	git -c protocol.version=2 \
>  		-c fetch.uriprotocols=http,https \
>  		clone "$HTTPD_URL/smart/http_parent" http_child &&
>  
> +	test_grep \"event\":\"region_enter\".*\"label\":\"packfile-uris\" trace2 &&
> +	test_grep \"key\":\"packfile-uris/count\",\"value\":\"2\" trace2 &&
> +	test_grep \"event\":\"region_leave\".*\"label\":\"packfile-uris\" trace2 &&
> +
>  	# Ensure that my-blob and other-blob are in separate packfiles.
>  	for idx in http_child/.git/objects/pack/*.idx
>  	do

It does feel a tiny bit off to piggy-back on an existing test that has nothing to do with tracing except that it requires the traces to... I dunno, what does the existing test even do with the written logfile? Doesn't seem like it's using it at all.

Anyway, having this in a separate test would've been nice, but that doesn't warrant a reroll in my eyes. So overall, this patch looks good to me, thanks!

Patrick
Ted NymanSep 7, 2026, 21:24 UTC in reply to Patrick Steinhardt on lore

Re: [PATCH] fetch-pack: trace packfile URI downloads

On Tue, Aug 11, 2026 at 03:31:44PM +0200, Patrick Steinhardt wrote:
> Anyway, having this in a separate test would've been nice, but that
> doesn't warrant a reroll in my eyes. So overall, this patch looks good
> to me, thanks!
Thanks for reviewing, Patrick.

Junio, a gentle ping on this patch. Patrick was positive on it and didn't think the test organization warranted a reroll. Is there anything else you'd like me to address before picking it up? I'm happy to split out the tracing checks into a separate test or refresh the patch if that helps.

Thanks, Ted

Junio C HamanoSep 8, 2026, 03:27 UTC in reply to Ted Nyman on lore

Re: [PATCH] fetch-pack: trace packfile URI downloads

Ted Nyman <tnyman@openai.com> writes:
Show 11 quoted lines
> On Tue, Aug 11, 2026 at 03:31:44PM +0200, Patrick Steinhardt wrote:
>> Anyway, having this in a separate test would've been nice, but that
>> doesn't warrant a reroll in my eyes. So overall, this patch looks good
>> to me, thanks!
>
> Thanks for reviewing, Patrick.
>
> Junio, a gentle ping on this patch. Patrick was positive on it and didn't
> think the test organization warranted a reroll. Is there anything else
> you'd like me to address before picking it up? I'm happy to split out the
> tracing checks into a separate test or refresh the patch if that helps.

Thanks. This slipped below my radar. It does not look to me that the placement of the test is too bad.

Will queue.

Back to recent threads