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@xxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html