Re: [PATCH] pack-objects: trace pack bytes written
From: Junio C Hamano <hidden>
Date: 2026-08-18 01:08:27
friel@openai.com writes:
quoted hunk ↗ jump to hunk
From: Friel <redacted> We want to measure how compression settings affect push performance on the client. Different settings can produce different-sized packs from the same objects. Trace2 records the object count, but we also need the pack size to compare those settings. Add a write_pack_file/wrote_bytes Trace2 datum alongside write_pack_file/wrote. Count packs written to stdout or disk, including each pack's header and trailing checksum. When pack.packSizeLimit splits the output, report the sum of the pack sizes. Signed-off-by: Friel <redacted> --- builtin/pack-objects.c | 7 +++++++ t/t5300-pack-object.sh | 24 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+)diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 1ec5b6f206..bbf1adb437 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c@@ -1337,6 +1337,7 @@ static void write_pack_file(void) uint32_t nr_remaining = nr_result; time_t last_mtime = 0; struct object_entry **write_order; + off_t bytes_written = 0; if (progress > pack_to_stdout) progress_state = start_progress(the_repository,@@ -1347,6 +1348,7 @@ static void write_pack_file(void) do { unsigned char hash[GIT_MAX_RAWSZ]; char *pack_tmp_name = NULL; + off_t pack_bytes; if (pack_to_stdout) { /*@@ -1389,6 +1391,8 @@ static void write_pack_file(void) display_progress(progress_state, written); } + pack_bytes = hashfile_total(f) + + the_repository->hash_algo->rawsz; if (pack_to_stdout) { /* * We never fsync when writing to stdout since we may@@ -1419,6 +1423,7 @@ static void write_pack_file(void) write_bitmap_index = 0; } } + bytes_written += pack_bytes;
I may very well be misreading the code, but it is unclear to me what role pack_bytes is playing, why we want to compute it before the finialization if/else cascade above, and increment bytes_written after that finalization if/else cascade above. IOW, wouldn't it be equivalent to get rid of hunks 1347 and 1419, and in hunk 1389 to this instead? bytes_written += hashfile_total(f) + the_hash_algo->rawsz; The numbers for non stdout case are not that interesting (we can see how bit the on-disk files are very easily), but counting in the common code path (i.e., hunk 1389) sounds like the cleanest approach. I just found that the code with two variables confusing. Thanks.