Re: [PATCH] {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 15, 2012, 21:09 UTC
Message-ID: <7vy5r1inax.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1331736055-21019-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 7 quoted lines

> When we fetch or push, usually "git rev-list --verify-objects --not
> --all --stdin" is used to make sure that all objects between existing
> refs and new refs are good. This means no gaps in between, all objects
> are well-formed, object content agrees with its sha-1 signature.
>
> For the last one, --verify-objects calls check_sha1_signature() via
> parse_object(). check_sha1_signature() is an expensive operation,

After thinking about this a bit more, I am beginning to think that the validation of object contents is unnecessary in _all_ cases that involve "git fetch". Unpack-objects and index-pack already validate individual objects, and the only thing we would want to catch are objects that we already happened to have had in our repository but were unreferenced from our refs. But the codepaths that write out loose objects or packfiles that must have left these objects during the earlier run in our repository should already have done the validation.

So the only thing that we would be catching with this extra check is a loose object or a packfile that was deposited outside the control of git in your repository.

But that is not something we should be trying to catch every time we run fetch---if your repository is open to be written by a random person from sideways bypassing git, you have a much bigger problem. And we have the tool to catch that kind of breakage: "git fsck".

So the right solution is probably use --objects for connectivity checks as before; we could add a fetch.paranoid configuration to allow people to still use it (with this patch to remove the over-pessimism from the code) but only if they want to.

Cc'ing Shawn, as I took inspiration from him in the first place for the update to --verify-objects (no, the overly-pessimistic implementation and excess overhead you solved partially with this patch is not his fault but is mine).

I said "partially" above in the parentheses because we would still end up revalidating all the objects in quickfetch() check when you are fetching from the repository that is your alternate, even with this patch.