{"thread":{"id":"45609","subject":"[PATCH v6] read-cache: call verify_hdr() in a background thread","startedAt":"2017-04-05T18:07:54Z","lastAt":"2017-04-05T19:06:26Z","messageCount":5,"participants":["git@jeffhostetler.com","Jeff King","Jonathan Nieder","Jeff Hostetler"],"isPatch":true,"patchVersion":6,"patchTotal":null},"messages":[{"id":"316224","messageId":"20170405180625.12439-1-git@jeffhostetler.com","threadId":"45609","inReplyTo":null,"subject":"[PATCH v6] read-cache: call verify_hdr() in a background thread","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-05T18:06:24Z","receivedAt":"2017-04-05T18:07:54Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhost@microsoft.com>\n\nVersion 6 of this patch series cleans up a little further\nand uses test_when_finished in the unit test.\n\n================\nVersion 5 of this patch series further simplifies the change to a\nsingle global variable for use by fsck.  It eliminates the core\nconfig setting.  Further discussion on the mailing list indicated\nthat the config setting only added confusion and probably would not\nbe used by anyone.\n\n\nJeff Hostetler (1):\n  read-cache: force_verify_index_checksum\n\n builtin/fsck.c  |  1 +\n cache.h         |  2 ++\n read-cache.c    |  7 +++++++\n t/t1450-fsck.sh | 13 +++++++++++++\n 4 files changed, 23 insertions(+)\n\n-- \n2.9.3\n\n"},{"id":"316225","messageId":"20170405180625.12439-2-git@jeffhostetler.com","threadId":"45609","inReplyTo":"20170405180625.12439-1-git@jeffhostetler.com","subject":"[PATCH v6] read-cache: force_verify_index_checksum","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-05T18:06:25Z","receivedAt":"2017-04-05T18:08:15Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhost@microsoft.com>\n\nTeach git to skip verification of the SHA1-1 checksum at the end of\nthe index file in verify_hdr() which is called from read_index()\nunless the \"force_verify_index_checksum\" global variable is set.\n\nTeach fsck to force this verification.\n\nThe checksum verification is for detecting disk corruption, and for\nsmall projects, the time it takes to compute SHA-1 is not that\nsignificant, but for gigantic repositories this calculation adds\nsignificant time to every command.\n\nThese effect can be seen using t/perf/p0002-read-cache.sh:\n\n================\nBEFORE:\n$ ./p0002-read-cache.sh\n0002.1: read_cache/discard_cache 1000 times   0.66(0.58+0.08)\n\nAFTER:\n$ ./p0002-read-cache.sh\n0002.1: read_cache/discard_cache 1000 times   0.30(0.28+0.01)\n================\n\nAlso, using the t/perf/p0004-read-tree.sh test that I created\nin a parallel patch series.\n\nhttps://public-inbox.org/git/20170404210847.50860-1-git@jeffhostetler.com/\n\nSigned-off-by: Jeff Hostetler <jeffhost@microsoft.com>\n---\n builtin/fsck.c  |  1 +\n cache.h         |  2 ++\n read-cache.c    |  7 +++++++\n t/t1450-fsck.sh | 13 +++++++++++++\n 4 files changed, 23 insertions(+)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 1a5cacc..5512d06 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -771,6 +771,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (keep_cache_objects) {\n+\t\tverify_index_checksum = 1;\n \t\tread_cache();\n \t\tfor (i = 0; i < active_nr; i++) {\n \t\t\tunsigned int mode;\ndiff --git a/cache.h b/cache.h\nindex 80b6372..87f13bf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -685,6 +685,8 @@ extern void update_index_if_able(struct index_state *, struct lock_file *);\n extern int hold_locked_index(struct lock_file *, int);\n extern void set_alternate_index_output(const char *);\n \n+extern int verify_index_checksum;\n+\n /* Environment bits from configuration mechanism */\n extern int trust_executable_bit;\n extern int trust_ctime;\ndiff --git a/read-cache.c b/read-cache.c\nindex 9054369..c4205aa 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1371,6 +1371,9 @@ struct ondisk_cache_entry_extended {\n \t\t\t    ondisk_cache_entry_extended_size(ce_namelen(ce)) : \\\n \t\t\t    ondisk_cache_entry_size(ce_namelen(ce)))\n \n+/* Allow fsck to force verification of the index checksum. */\n+int verify_index_checksum;\n+\n static int verify_hdr(struct cache_header *hdr, unsigned long size)\n {\n \tgit_SHA_CTX c;\n@@ -1382,6 +1385,10 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)\n \thdr_version = ntohl(hdr->hdr_version);\n \tif (hdr_version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < hdr_version)\n \t\treturn error(\"bad index version %d\", hdr_version);\n+\n+\tif (!verify_index_checksum)\n+\t\treturn 0;\n+\n \tgit_SHA1_Init(&c);\n \tgit_SHA1_Update(&c, hdr, size - 20);\n \tgit_SHA1_Final(sha1, &c);\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 33a51c9..fc1acb5 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' '\n \t! grep $blob out\n '\n \n+test_expect_success 'detect corrupt index file in fsck' '\n+\tcp .git/index .git/index.backup &&\n+\ttest_when_finished \"mv .git/index.backup .git/index\" &&\n+\techo zzzzzzzz >zzzzzzzz &&\n+\ttest_when_finished \"rm zzzzzzzz\" &&\n+\tgit add zzzzzzzz &&\n+\tsed -e \"s/zzzzzzzz/yyyyyyyy/\" .git/index >.git/index.yyy &&\n+\trm .git/index &&\n+\tmv .git/index.yyy .git/index &&\n+\ttest_when_finished \"rm .git/index\" &&\n+\ttest_must_fail git fsck --cache\n+'\n+\n test_done\n-- \n2.9.3\n\n"},{"id":"316227","messageId":"20170405182533.a5divj63zfkfejui@sigill.intra.peff.net","threadId":"45609","inReplyTo":"20170405180625.12439-2-git@jeffhostetler.com","subject":"Re: [PATCH v6] read-cache: force_verify_index_checksum","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-05T18:25:33Z","receivedAt":"2017-04-05T18:27:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 05, 2017 at 06:06:25PM +0000, git@jeffhostetler.com wrote:\n\n> ---\n>  builtin/fsck.c  |  1 +\n>  cache.h         |  2 ++\n>  read-cache.c    |  7 +++++++\n>  t/t1450-fsck.sh | 13 +++++++++++++\n>  4 files changed, 23 insertions(+)\n\nThis version is delightfully simple. Thanks for sticking with it.\n\n> +test_expect_success 'detect corrupt index file in fsck' '\n> +\tcp .git/index .git/index.backup &&\n> +\ttest_when_finished \"mv .git/index.backup .git/index\" &&\n> +\techo zzzzzzzz >zzzzzzzz &&\n> +\ttest_when_finished \"rm zzzzzzzz\" &&\n> +\tgit add zzzzzzzz &&\n> +\tsed -e \"s/zzzzzzzz/yyyyyyyy/\" .git/index >.git/index.yyy &&\n> +\trm .git/index &&\n> +\tmv .git/index.yyy .git/index &&\n> +\ttest_when_finished \"rm .git/index\" &&\n> +\ttest_must_fail git fsck --cache\n> +'\n\nI was surprised by the second when-finished \"rm\". The earlier \"mv\"\nshould be enough to restore everything. I guess you're trying to\novercome the tendency of \"mv\" to complain about overwriting on some\nplatforms.\n\nBut wouldn't that make the first when-finished \"mv\" potentially a\nproblem, if it happens before the \"rm .git/index\" line?\n\nPerhaps \"mv -f\" would make it all simpler (ordinarily I'd suggest just\n\"git reset --hard\", but since we're corrupting the index, it may be\nbetter not to rely on git for the restore). So maybe just:\n\n  cp .git/index .git/index.backup &&\n  test_when_finished \"mv -f .git/index.backup .git/index\" &&\n  ...\n\nI think the \"rm zzzzzzzz\" can go, too. We don't typically worry too much\nabout accumulating random files in the working tree. The important thing\nis that the index is restored to a sane state.\n\n-Peff\n"},{"id":"316228","messageId":"20170405184119.GF8741@aiede.mtv.corp.google.com","threadId":"45609","inReplyTo":"20170405180625.12439-2-git@jeffhostetler.com","subject":"Re: [PATCH v6] read-cache: force_verify_index_checksum","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-05T18:41:19Z","receivedAt":"2017-04-05T18:41:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\ngit@jeffhostetler.com wrote:\n\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' '\n>  \t! grep $blob out\n>  '\n>  \n> +test_expect_success 'detect corrupt index file in fsck' '\n> +\tcp .git/index .git/index.backup &&\n> +\ttest_when_finished \"mv .git/index.backup .git/index\" &&\n> +\techo zzzzzzzz >zzzzzzzz &&\n> +\ttest_when_finished \"rm zzzzzzzz\" &&\n> +\tgit add zzzzzzzz &&\n> +\tsed -e \"s/zzzzzzzz/yyyyyyyy/\" .git/index >.git/index.yyy &&\n> +\trm .git/index &&\n> +\tmv .git/index.yyy .git/index &&\n\nWhy is the \"rm\" before \"mv\" needed?  I would have expected \"mv\" to\nreplace the file.\n\nIf that doesn't work, I expect there are a large number of other tests\nthat would need fixing...\n\n> +\ttest_when_finished \"rm .git/index\" &&\n\nLikewise --- this seems redundant next to the \"mv\" enqueued previously.\n\n> +\ttest_must_fail git fsck --cache\n\nThat said,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"316229","messageId":"91d35c27-f0e3-0dca-dc87-5ab086e18e59@jeffhostetler.com","threadId":"45609","inReplyTo":"20170405182533.a5divj63zfkfejui@sigill.intra.peff.net","subject":"Re: [PATCH v6] read-cache: force_verify_index_checksum","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-05T19:06:14Z","receivedAt":"2017-04-05T19:06:26Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 4/5/2017 2:25 PM, Jeff King wrote:\n> On Wed, Apr 05, 2017 at 06:06:25PM +0000, git@jeffhostetler.com wrote:\n>\n>> ---\n>>  builtin/fsck.c  |  1 +\n>>  cache.h         |  2 ++\n>>  read-cache.c    |  7 +++++++\n>>  t/t1450-fsck.sh | 13 +++++++++++++\n>>  4 files changed, 23 insertions(+)\n>\n> This version is delightfully simple. Thanks for sticking with it.\n>\n>> +test_expect_success 'detect corrupt index file in fsck' '\n>> +\tcp .git/index .git/index.backup &&\n>> +\ttest_when_finished \"mv .git/index.backup .git/index\" &&\n>> +\techo zzzzzzzz >zzzzzzzz &&\n>> +\ttest_when_finished \"rm zzzzzzzz\" &&\n>> +\tgit add zzzzzzzz &&\n>> +\tsed -e \"s/zzzzzzzz/yyyyyyyy/\" .git/index >.git/index.yyy &&\n>> +\trm .git/index &&\n>> +\tmv .git/index.yyy .git/index &&\n>> +\ttest_when_finished \"rm .git/index\" &&\n>> +\ttest_must_fail git fsck --cache\n>> +'\n>\n> I was surprised by the second when-finished \"rm\". The earlier \"mv\"\n> should be enough to restore everything. I guess you're trying to\n> overcome the tendency of \"mv\" to complain about overwriting on some\n> platforms.\n\nYeah, just being cautious. (I spend a lot of time on Windows.)\n\n>\n> But wouldn't that make the first when-finished \"mv\" potentially a\n> problem, if it happens before the \"rm .git/index\" line?\n>\n> Perhaps \"mv -f\" would make it all simpler (ordinarily I'd suggest just\n> \"git reset --hard\", but since we're corrupting the index, it may be\n> better not to rely on git for the restore). So maybe just:\n>\n>   cp .git/index .git/index.backup &&\n>   test_when_finished \"mv -f .git/index.backup .git/index\" &&\n>   ...\n>\n> I think the \"rm zzzzzzzz\" can go, too. We don't typically worry too much\n> about accumulating random files in the working tree. The important thing\n> is that the index is restored to a sane state.\n>\n> -Peff\n>\n\nGood point.  Let me take another stab at it.\nThanks\nJeff\n"}]}