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

On Thu, Aug 20, 2026 at 11:13:16AM +0200, Patrick Steinhardt wrote:

> > 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.

Yes, though I think the oversight can be forgiven here. The unsafe
variants are purely for performance, so we started by converting a few
hot code paths, knowing that it was OK to leave other spots using the
collision-detecting implementation. The main one we cared about is
"pack-objects --stdout" to serve fetches.

But this particular case is almost never exercised! It triggers only
when --max-pack-size causes us to split the result into multiple packs
(we can't write the header up front in that case, because we don't know
how many objects we'll fit into the output). So I doubt anybody would
have noticed or cared about the performance difference.

But it also means that cleaning up the triple-hash is tricky. The three
hashes in this code path are:

  a. we hash as we write, via struct hashfile

  b. we hash as we read back the data to verify it

  c. we re-hash the data on top of the fixed-up header

We can obviously drop (b) if we choose. We can't drop (c); it's the
final value that goes into the on-disk packfile. So we'd like to drop
(a), which is pointless (except for cross-checking step b).

But we don't know if we're in this code path until we've finished
writing the file! If the output is smaller than --max-pack-size, then we
just write the hash from (a) directly, and neither (b) nor (c) happens
at all. This is the "nr_written == nr_remaining" conditional, the second
in the chain.

We could pessimistically assume that we'll need to do (c), and skip the
hash for (a). But that is worse for the usual case that we don't split
the packfiles. Instead of hashing as the data is written, we have to
re-read it (passing all those bytes through memory again).

So realistically the best we can do is drop (b).

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.

> > 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.

There was an attitude in the early days of Git that we should be
checking hashes and checksums all the time. I.e., that the validity of
the data was the most precious thing, and we should notice an on-disk
corruption as quickly and reliably as possible, similar to filesystems
that checksum the data.

But over time we've relaxed that quite a bit because of the quite
noticeable costs. For example, we used to re-hash every object we
loaded, but these days we have PARSE_OBJECT_SKIP_HASH_CHECK, and
features like the commit graph.

I think this is a case where we could similarly relax. Especially
because this is just the pack checksum. The actual object contents are
still protected by their respective hashes.

-Peff