Re: propagating repo corruption across clone

[Jeff King <peff@peff.net>]

On Tue, Mar 26, 2013 at 09:43:01AM -0400, Jeff Mitchell wrote:

> On Mon, Mar 25, 2013 at 4:07 PM, Jeff King <peff@peff.net> wrote:
> > On Mon, Mar 25, 2013 at 12:32:50PM -0400, Jeff Mitchell wrote:
> >> For commit corruptions, the --no-hardlinks, non --mirror case refused
> >> to create the new repository and exited with an error code of 128. The
> >> --no-hardlinks, --mirror case spewed errors to the console, yet
> >> *still* created the new clone *and* returned an error code of zero.
> >
> > I wasn't able to reproduce this; can you post a succint test case?
>
> [...link to tar.gz...]
> Once you extract that, you should be able to run a clone using paths
> (not file://) with --no-hardlinks --mirror and replicate the behavior
> I saw. FYI, I'm on Git 1.8.2.

Thanks for providing an example.

The difference is the same "--mirror implies --bare" issue; the non-bare
case dies during the checkout (even before my patches, as the corruption
is not in a blob, but rather in the HEAD commit object itself). You can
replace --mirror with --bare and see the same behavior.

The troubling part is that we see errors in the bare case, but do not
die. Those errors all come from upload-pack, the "sending" side of a
clone/fetch. Even though we do not transfer the objects via the git
protocol, we still invoke upload-pack to get the ref list (and then copy
the objects themselves out-of-band).

What happens is that upload-pack sees the errors while trying to see if
the object is a tag that can be peeled (the server advertises both tags
and the objects they point to). It does not distinguish between "errors
did not let me peel this object" and "this object is not a tag, and
therefore there is nothing to peel".

We could change that, but I'm not sure whether it is a good idea. I
think the intent is that upload-pack's ref advertisement would remain
resilient to corruption in the repository (e.g., even if that commit is
corrupt, you can still fetch the rest of the data). We should not worry
about advertising broken objects, because we will encounter the same
error when we actually do try to send the objects. Dying at the
advertisement phase would be premature, since we do not yet know what
the client will request.

The problem, of course, is that the --local optimization _skips_ the
part where we actually ask upload-pack for data, and instead blindly
copies it. So this is the same issue as usual, which is that the local
transport is not thorough enough to catch corruption. It seems like a
failing in this case, because upload-pack does notice the problem, but
that is only luck; if the corruption were in a non-tip object, it would
not notice it at all. So trying to die on errors in the ref
advertisement would just be a band-aid. Fundamentally the problem is
that the --local transport is not safe from propagating corruption, and
should not be used if that's a requirement.

-Peff