> From: Friel <friel@openai.com>
>
> 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 <friel@openai.com>
> ---
>  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;

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 1ec5b6f206..252530172c 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1389,6 +1390,8 @@ static void write_pack_file(void)
>  			display_progress(progress_state, written);
>  		}
>  
> +		bytes_written += hashfile_total(f) +
> +			the_repository->hash_algo->rawsz;
>  		if (pack_to_stdout) {
>  			/*
>  			 * We never fsync when writing to stdout since we may

> @@ -1510,6 +1513,8 @@ static void write_pack_file(void)
>  		    written, nr_result);
>  	trace2_data_intmax("pack-objects", the_repository,
>  			   "write_pack_file/wrote", nr_result);
> +	trace2_data_intmax("pack-objects", the_repository,
> +			   "write_pack_file/wrote_bytes", bytes_written);
>  }
>  
>  static int no_try_delta(const char *path)

> On Wed, Aug 19, 2026 at 04:28:10PM -0700, friel@openai.com wrote:
> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index 1ec5b6f206..252530172c 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -1389,6 +1390,8 @@ static void write_pack_file(void)
> >  			display_progress(progress_state, written);
> >  		}
> >  
> > +		bytes_written += hashfile_total(f) +
> > +			the_repository->hash_algo->rawsz;
> >  		if (pack_to_stdout) {
> >  			/*
> >  			 * We never fsync when writing to stdout since we may
> 
> I guess the addition here accounts for the trailing hash written by the
> hashfile. If so, shouldn't we also use the algortihm that the hashfile
> uses in the first place via `f->algop->rawsz`?

> On Thu, Aug 20, 2026 at 07:41:33AM +0200, Patrick Steinhardt wrote:
> > On Wed, Aug 19, 2026 at 04:28:10PM -0700, friel@openai.com wrote:
> > > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > > index 1ec5b6f206..252530172c 100644
> > > --- a/builtin/pack-objects.c
> > > +++ b/builtin/pack-objects.c
> > > @@ -1389,6 +1390,8 @@ static void write_pack_file(void)
> > >  			display_progress(progress_state, written);
> > >  		}
> > >  
> > > +		bytes_written += hashfile_total(f) +
> > > +			the_repository->hash_algo->rawsz;
> > >  		if (pack_to_stdout) {
> > >  			/*
> > >  			 * We never fsync when writing to stdout since we may
> > 
> > I guess the addition here accounts for the trailing hash written by the
> > hashfile. If so, shouldn't we also use the algortihm that the hashfile
> > uses in the first place via `f->algop->rawsz`?
> 
> Perhaps, though that is used to write the hash (via CSUM_HASH_IN_STREAM)
> only in two of the conditional blocks. In the third we finalize the
> hashfile and then use fixup_pack_header_footer(), passing the_hash_algo
> directly (not even the_repository->hash_algo, though of course they mean
> the same thing).
> 
> It all works out, of course, because we created the hashfile struct
> earlier using the_repository->hash_algo. So I think this is mostly
> academic in the first place, but your suggestion harmonizes two of the
> conditional blocks while creating disagreement with the third.
> 
> I think something like this would "fix" it by consistently using the
> hashfile's algo in all three blocks:
> 
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 4a5fcbe5f5..0fdff72f41 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1413,9 +1413,9 @@ static void write_pack_file(void)
>  			 * If we wrote the wrong number of entries in the
>  			 * header, rewrite it like in fast-import.
>  			 */
> -
> +			const struct git_hash_algo *algo = f->algop;
>  			int fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0);
> -			fixup_pack_header_footer(the_hash_algo, fd, hash,
> +			fixup_pack_header_footer(algo, fd, hash,
>  						 pack_tmp_name, nr_written,
>  						 hash, offset);
>  			close(fd);
> 
> 
> But there's a subtle yet interesting difference here! f->algop won't
> necessarily be the same pointer as the_hash_algo. If we compiled with an
> unsafe variant, that will be used for hashfiles. If we're just looking
> at rawsz that's OK; the two variants should be identical (other than
> performance and collision detection), so taking rawsz from either is
> fine.
> 
> But fixup_pack_header_footer() actually recomputes the hash (as it must
> if we tweak the header). Right now it does it using the "normal"
> variant, but we should be able to use the unsafe one (which my diff
> snippet above would start to do).

> Of course this whole thing is absurdly pessimal in the first place. If
> we are just going to throw out the hashfile's checksum, then why bother
> computing it in the first place? Because we don't trust a disk write at
> all, and actually verify the original hash computation as we read the
> bytes back in! So we'll actually sha1 the written packfile three times.
> Yikes. I wonder if it's really worth being so paranoid. But that is how
> it has always been.

> Anyway, that is a bit of a tangent from the patch in question. I think
> either spelling is OK for the purposes of this patch. If somebody wants
> to pursue harmonizing the paths (and maybe even doing some timings to
> see if switching to the unsafe variant is noticeable here, and what the
> total cost of this triple-write approach is), that can happen
> separately.

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 4a5fcbe5f5..0fdff72f41 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1413,9 +1413,9 @@ static void write_pack_file(void)
>  			 * If we wrote the wrong number of entries in the
>  			 * header, rewrite it like in fast-import.
>  			 */
> -
> +			const struct git_hash_algo *algo = f->algop;
>  			int fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0);
> -			fixup_pack_header_footer(the_hash_algo, fd, hash,
> +			fixup_pack_header_footer(algo, fd, hash,
>  						 pack_tmp_name, nr_written,
>  						 hash, offset);
>  			close(fd);
>
> ...
> But fixup_pack_header_footer() actually recomputes the hash (as it must
> if we tweak the header). Right now it does it using the "normal"
> variant, but we should be able to use the unsafe one (which my diff
> snippet above would start to do).

> > But there's a subtle yet interesting difference here! f->algop won't
> > necessarily be the same pointer as the_hash_algo. If we compiled with an
> > unsafe variant, that will be used for hashfiles. If we're just looking
> > at rawsz that's OK; the two variants should be identical (other than
> > performance and collision detection), so taking rawsz from either is
> > fine.
> > 
> > But fixup_pack_header_footer() actually recomputes the hash (as it must
> > if we tweak the header). Right now it does it using the "normal"
> > variant, but we should be able to use the unsafe one (which my diff
> > snippet above would start to do).
> 
> Yeah, I agree that switching over to the unsafe algortihm is sensible.
> Being able to speed up hashing of packfiles was one of the prime
> motivations of introducing the unsafe variants in the first place, so
> the fact that we still use the safe variant here feels like a plain
> oversight to me.

> > Of course this whole thing is absurdly pessimal in the first place. If
> > we are just going to throw out the hashfile's checksum, then why bother
> > computing it in the first place? Because we don't trust a disk write at
> > all, and actually verify the original hash computation as we read the
> > bytes back in! So we'll actually sha1 the written packfile three times.
> > Yikes. I wonder if it's really worth being so paranoid. But that is how
> > it has always been.
> 
> That's... awful. Honestly, if we cannot trust what we're writing to disk
> we're going to be kind of screwed anyway. We don't re-verify loose
> objects, refs or whatever other data structures we write to disk either.
> So doing this thrice here feels wrong.

> From: Friel <friel@openai.com>
> 
> 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 <friel@openai.com>
> ---
> Junio, you're right. Updating bytes_written before finalization is
> equivalent. I've dropped pack_bytes; everything else is unchanged.
> Thanks.

> Of course what I'd _really_ like to do is rip out --max-pack-size
> entirely. I don't think it's generally helpful, and it introduces all
> kinds of weird corner cases and complications like this. But obviously
> that's a much bigger change, and naturally if I seriously proposed it
> somebody would come out of the woodwork so with obscure case where it's
> useful.

> On Wed, Aug 19, 2026 at 04:28:10PM -0700, friel@openai.com wrote:
>
>> From: Friel <friel@openai.com>
>> 
>> 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 <friel@openai.com>
>> ---
>> Junio, you're right. Updating bytes_written before finalization is
>> equivalent. I've dropped pack_bytes; everything else is unchanged.
>> Thanks.
>
> The downthread discussion went pretty far off-topic, so for those who do
> not want to read it, the summary is: this patch looks good to me. ;)