Re: [PATCH v2] pack-objects: trace pack bytes written

On Wed, Aug 19, 2026 at 04:28:10PM -0700, [email protected] 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`?

> @@ -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)

The "write_pack_file/wrote" event is quite awkwardly named, if you ask
me, as it's not immediately obvious what exactly it's counting, and the
second metric may make this even more confusing. In retrospect it
would've been preferable to call this "wrote_objects" to clarify.

I don't really think we guarantee any kind of stability around those
traces, so we could in theory change it here, too. But I don't feel like
my argument is strong enough to really warrant such a change, so maybe
we should just leave it as-is.

Thanks!

Patrick