Re: propagating repo corruption across clone

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> We _do_ see a problem during the checkout phase, but we don't propagate
> a checkout failure to the exit code from clone.  That is bad in general,
> and should probably be fixed. Though it would never find corruption of
> older objects in the history, anyway, so checkout should not be relied
> on for robustness.

It is obvious that we should exit with non-zero status when we see a
failure from the checkout, but do we want to nuke the resulting
repository as in the case of normal transport failure?  A checkout
failure might be due to being under quota for object store but
running out of quota upon populating the working tree, in which case
we probably do not want to.

> We do not notice the sha1 mis-match on the sending side (which we could,
> if we checked the sha1 as we were sending). We do not notice the broken
> object graph during the receive process either. I would have expected
> check_everything_connected to handle this, but we don't actually call it
> during clone! If you do this:
>
>   $ git init non-local && cd non-local && git fetch ..
>   remote: Counting objects: 3, done.
>   remote: Total 3 (delta 0), reused 3 (delta 0)
>   Unpacking objects: 100% (3/3), done.
>   fatal: missing blob object 'd95f3ad14dee633a758d2e331151e950dd13e4ed'
>   error: .. did not send all necessary objects
>
> we do notice.

Yes, it is OK to add connectedness check to "git clone".

> And one final check:
>
>   $ git -c transfer.fsckobjects=1 clone --no-local . fsck
>   Cloning into 'fsck'...
>   remote: Counting objects: 3, done.
>   remote: Total 3 (delta 0), reused 3 (delta 0)
>   Receiving objects: 100% (3/3), done.
>   error: unable to find d95f3ad14dee633a758d2e331151e950dd13e4ed
>   fatal: object of unexpected type
>   fatal: index-pack failed
>
> Fscking the incoming objects does work, but of course it comes at a cost
> in the normal case (for linux-2.6, I measured an increase in CPU time
> with "index-pack --strict" from ~2.5 minutes to ~4 minutes). And I think
> it is probably overkill for finding corruption; index-pack already
> recognizes bit corruption inside an object, and
> check_everything_connected can detect object graph problems much more
> cheaply.

> One thing I didn't check is bit corruption inside a packed object that
> still correctly zlib inflates. check_everything_connected will end up
> reading all of the commits and trees (to walk them), but not the blobs.

Correct.

> So I think at the very least we should:
>
>   1. Make sure clone propagates errors from checkout to the final exit
>      code.
>
>   2. Teach clone to run check_everything_connected.

I agree with both but with a slight reservation on the former one
(see above).

Thanks.