verify_pack ignores return value of verify_fn

verify_pack ignores return value of verify_fn

[David Turner]

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).

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?

Re: verify_pack ignores return value of verify_fn

[Jeff King]

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

> 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