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

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

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.