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?
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