Re: verify_pack ignores return value of verify_fn

From: Jeff King <peff@peff.net>
Date: Nov 24, 2015, 22:11 UTC
Message-ID: <20151124221134.GD29185@sigill.intra.peff.net>
In-Reply-To: <1447806715.5082.11.camel@twopensource.com>

On Tue, Nov 17, 2015 at 07:31:55PM -0500, David Turner wrote:

Show 8 quoted lines

> In pack-check.c, line 129, a caller-supplied verification function is
> called.  The function returns an int, but that return value is ignored.
> 
> The only caller of verify_pack is in builtin/fsck.c, whose verify_fn
> *does* return a meaningful error code (which is then ignored).  If it
> were not ignored, fsck might return a different error code (in the
> unlikely event that a weird object gets into a pack and is somehow not 
> totally corrupt enough to fail an earlier check).

Hrm. I think what is supposed to happen is that the callback sets a bit in errors_found, and ultimately we use that to determine fsck's exit code. But it seems like there are cases where we do not do so (specifically, if it's valid git object, but doesn't conform to the correct tree or commit format).

> I think we should probably have verify_pack return a non-zero result if
> any call to verify_fn returns a non-zero result.  Any objections to
> this?

That makes sense. Usually a callback returning non-zero would also prematurely end the traversal, but I think we explicitly _don't_ want that here. We want fsck to continue through all objects.

As an aside, the reason this is the only caller of verify_pack is due to 3de89c9 (verify-pack: use index-pack --verify, 2011-06-03). Using index-pack is much faster, as it hits the objects topologically by delta base, so we never have to access a base twice. It might be nice if fsck learned to use it, too, and then we could just get rid of verify_pack() entirely.

-Peff