Re: Mozilla .git tree

Nicolas Pitre [off-list ref] writes:

And the zlib header contains a CRC which we're about to use for 
validating the data when doing delta data reuse in order to prevent pack 
corruption propagation like the one recently posted on the list.  

Ah, never thought of using the CRC directly.  I was thinking
about inflating into void and see if it succeeds, which as you
say is perhaps quite expensive.

This brings me back to my pet-peeve, though.  I do not think
zlib stream seeks back and leaves some clue at the beginning to
tell me the deflated length, so it is quite hard to find where
each deflated stream ends in a packfile cheaply.  Loose objects
(with new or legacy style header) are easy (st.st_size is
available), but I do not think of a way short of building a
reverse index of pack .idx file, which means I am already
talking about not so cheap way X-<.

It might be a reason to define a new .idx format.  We could lift
32-bit offset limitation while we are at it.  Each entry could
have 20-byte hash, 64-bit offset into the corresponding .pack,
and 32-bit deflated length (heh, why not make it 64-bit while we
are at it).  Luckily, .idx _is_ a local matter so we can even
have a flag day and tell people to run the updated index-pack on
existing packfiles to regenerate .idx.

Using an offset instead of a sha1 to reference a delta base object is 
certainly a good idea though.  But I'd use the same variable encoding as 
the object size to avoid the 32-bit limit issue.  When generating a thin 
pack the real sha1 of the delta object could be substituted for the 
offset quite easily if the base object is not sent a part of the same 
pack.

That sounds quite a reasonable suggestion.  I love this kind of
moment when I find us very fortunate to have bright people on
the list ;-).

Junio C Hamano wrote:

Nicolas Pitre [off-list ref] writes:

quoted

And the zlib header contains a CRC which we're about to use for 
validating the data when doing delta data reuse in order to prevent pack 
corruption propagation like the one recently posted on the list.  

Ah, never thought of using the CRC directly.  I was thinking
about inflating into void and see if it succeeds, which as you
say is perhaps quite expensive.

Unfortunately, the zlib CRC is of the _uncompressed_ data [1], so
inflating the stream is still necessary to check for corruption.

[1] RFC 1950, "ZLIB Compressed Data Format Specification", May 1996.
Pages 4-6.

On Fri, 1 Sep 2006, A Large Angry SCM wrote:

Unfortunately, the zlib CRC is of the _uncompressed_ data [1], so
inflating the stream is still necessary to check for corruption.

I don't think that is unfortunate.

We really should inflate the stream anyway, since not only inflating it, 
but also applying any deltas to the base object is really the only way to 
verify its correctness for a delta thing. Otherwise, the SHA1 of the base 
could be totally corrupt.

And once you inflate it and apply all deltas, you obviously also get the 
full SHA1 check, so you're _really_ safe.

So let's do the really safe thing first, and see if it actually results in 
any problems.

NOTE NOTE NOTE! We might well choose to do this checking _only_ when we 
write the index file (ie we have "!pack_to_stdout" set). Why? Because if 
we pack to stdout, and don't generate an index file, then by _definition_ 
the other end has to do the index generation, which means that the other 
end will be doing all the SHA1 re-calculation and sanity checking (and 
thus inflation and CRC checking).

So this means that if we only do this for the "!pack_to_stdout" case, we 
won't be adding any overhead to the git network protocol server, only to 
"git repack". Which is exactly what we want to do.

			Linus