{"thread":{"id":"40818","subject":"verify_pack ignores return value of verify_fn","startedAt":"2015-11-18T00:31:55Z","lastAt":"2015-11-24T22:11:35Z","messageCount":2,"participants":["David Turner","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"273459","messageId":"1447806715.5082.11.camel@twopensource.com","threadId":"40818","inReplyTo":null,"subject":"verify_pack ignores return value of verify_fn","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-11-18T00:31:55Z","receivedAt":"2015-11-18T00:31:55Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"In pack-check.c, line 129, a caller-supplied verification function is\ncalled.  The function returns an int, but that return value is ignored.\n\nThe only caller of verify_pack is in builtin/fsck.c, whose verify_fn\n*does* return a meaningful error code (which is then ignored).  If it\nwere not ignored, fsck might return a different error code (in the\nunlikely event that a weird object gets into a pack and is somehow not \ntotally corrupt enough to fail an earlier check).\n\nI think we should probably have verify_pack return a non-zero result if\nany call to verify_fn returns a non-zero result.  Any objections to\nthis?\n"},{"id":"273661","messageId":"20151124221134.GD29185@sigill.intra.peff.net","threadId":"40818","inReplyTo":"1447806715.5082.11.camel@twopensource.com","subject":"Re: verify_pack ignores return value of verify_fn","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-24T22:11:35Z","receivedAt":"2015-11-24T22:11:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 17, 2015 at 07:31:55PM -0500, David Turner wrote:\n\n> In pack-check.c, line 129, a caller-supplied verification function is\n> called.  The function returns an int, but that return value is ignored.\n> \n> The only caller of verify_pack is in builtin/fsck.c, whose verify_fn\n> *does* return a meaningful error code (which is then ignored).  If it\n> were not ignored, fsck might return a different error code (in the\n> unlikely event that a weird object gets into a pack and is somehow not \n> totally corrupt enough to fail an earlier check).\n\nHrm. I think what is supposed to happen is that the callback sets a bit\nin errors_found, and ultimately we use that to determine fsck's exit\ncode. But it seems like there are cases where we do not do so\n(specifically, if it's valid git object, but doesn't conform to the\ncorrect tree or commit format).\n\n> I think we should probably have verify_pack return a non-zero result if\n> any call to verify_fn returns a non-zero result.  Any objections to\n> this?\n\nThat makes sense. Usually a callback returning non-zero would also\nprematurely end the traversal, but I think we explicitly _don't_ want\nthat here. We want fsck to continue through all objects.\n\nAs an aside, the reason this is the only caller of verify_pack is due to\n3de89c9 (verify-pack: use index-pack --verify, 2011-06-03). Using\nindex-pack is much faster, as it hits the objects topologically by delta\nbase, so we never have to access a base twice. It might be nice if fsck\nlearned to use it, too, and then we could just get rid of verify_pack()\nentirely.\n\n-Peff\n"}]}