Re: Funnies with "git fetch"

Jeff King [off-list ref] writes:

If I understand correctly, your series is just about checking that we
have newly-referenced blobs. We were already checking commits and trees,
and we should already be hashing individual objects when we index the
pack. Right?

You may be slightly misunderstanding the series.

We let unpack-objects or index-pack consume the pack stream, either by
exploding them into loose objects, or computing the object name for each
object to create the mapping from object name to the offset. During this
process, we deflate to read the contents and resolve the delta to come up
with the object name for individual objects, so we would notice corruption
at the individual object level. As pack stream does not say what name each
object is (the recipient is expected to compute it), there is no "stream
says it is object X but the data is actually for object Y" problem. The
recipient does not even see "X"---all it sees is Y.

The current code does not try to make sure we really have the objects
necessary to connect the updated tips to our original refs at all.  Not
just blobs but neither commits nor trees are traversed. The new check in
store_updated_refs() is about that. So in that sense, the series is not
about "just blobs".

The "rev-list --verify-objects" patch is about "blob vs everything else".
It is used in the existing quickfetch() check, and also the additional
check in store_updated_refs(). The existing check we run with "--objects"
is capable of detecting corruptions of commits and trees (as we had to be
able to read them to discover objects they refer to), but that is not a
sufficient check if we worry about missing blobs.

On Thu, Sep 01, 2011 at 08:09:49PM -0700, Junio C Hamano wrote:

You may be slightly misunderstanding the series.
[...]
The current code does not try to make sure we really have the objects
necessary to connect the updated tips to our original refs at all.  Not
just blobs but neither commits nor trees are traversed. The new check in
store_updated_refs() is about that. So in that sense, the series is not
about "just blobs".

Ah, OK, I see. I was too focused on pulling the bits out of quickfetch
into check_everything_connected, and missed the important new call in
store_updated_refs.

So what you are doing makes sense to me. I am curious, though, what the
performance impact is like. In particular, it seems that we will pull
each blob into memory via parse_object. Until now, we were mostly
streaming the blobs straight into packs. That makes me a little nervous
given the discussions recently about large blobs, and not accessing them
unnecessarily. But maybe that is a silly concern, as we will have just
reconstructed and hashed such an object anyway to get its name.

-Peff