{"thread":{"id":"45606","subject":"[PATCH v5] read-cache: call verify_hdr() in a background thread","startedAt":"2017-04-05T14:58:22Z","lastAt":"2017-04-05T18:10:25Z","messageCount":4,"participants":["git@jeffhostetler.com","Jonathan Nieder","Jeff Hostetler"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"316208","messageId":"20170405145510.2324-1-git@jeffhostetler.com","threadId":"45606","inReplyTo":null,"subject":"[PATCH v5] read-cache: call verify_hdr() in a background thread","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-05T14:55:09Z","receivedAt":"2017-04-05T14:58:22Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhost@microsoft.com>\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\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 | 11 +++++++++++\n 4 files changed, 21 insertions(+)\n\n-- \n2.9.3\n\n"},{"id":"316209","messageId":"20170405145510.2324-2-git@jeffhostetler.com","threadId":"45606","inReplyTo":"20170405145510.2324-1-git@jeffhostetler.com","subject":"[PATCH v5] read-cache: force_verify_index_checksum","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-05T14:55:10Z","receivedAt":"2017-04-05T14:58:32Z","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 | 11 +++++++++++\n 4 files changed, 21 insertions(+)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 1a5cacc..04c5e50 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\tforce_verify_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..1cea517 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 force_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..5637fa3 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 force_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 (!force_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..86757ff 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -689,4 +689,15 @@ test_expect_success 'bogus head does not fallback to all heads' '\n \t! grep $blob out\n '\n \n+test_expect_success PERL 'detect corrupt index file in fsck' '\n+\tcp .git/index .git/index.backup &&\n+\techo zzzzzzzz >zzzzzzzz &&\n+\tgit add zzzzzzzz &&\n+\tperl -pi -e \"s/zzzzzzzz/yyyyyyyy/\" .git/index &&\n+\ttest_must_fail git fsck --cache &&\n+\trm .git/index &&\n+\tmv .git/index.backup .git/index &&\n+\trm zzzzzzzz\n+'\n+\n test_done\n-- \n2.9.3\n\n"},{"id":"316212","messageId":"20170405170336.GE8741@aiede.mtv.corp.google.com","threadId":"45606","inReplyTo":"20170405145510.2324-2-git@jeffhostetler.com","subject":"Re: [PATCH v5] read-cache: force_verify_index_checksum","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-05T17:03:36Z","receivedAt":"2017-04-05T17:03:58Z","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> Signed-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 | 11 +++++++++++\n>  4 files changed, 21 insertions(+)\n\nYay!  I love this version.\n\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\tforce_verify_index_checksum = 1;\n\nnit: now that there isn't a config this is overriding, this isn't\n\"force_verify\" so much as \"verify\".\n\n[...]\n> +++ b/t/t1450-fsck.sh\n> @@ -689,4 +689,15 @@ test_expect_success 'bogus head does not fallback to all heads' '\n>  \t! grep $blob out\n>  '\n>  \n> +test_expect_success PERL 'detect corrupt index file in fsck' '\n> +\tcp .git/index .git/index.backup &&\n> +\techo zzzzzzzz >zzzzzzzz &&\n> +\tgit add zzzzzzzz &&\n> +\tperl -pi -e \"s/zzzzzzzz/yyyyyyyy/\" .git/index &&\n> +\ttest_must_fail git fsck --cache &&\n> +\trm .git/index &&\n> +\tmv .git/index.backup .git/index &&\n> +\trm zzzzzzzz\n> +'\n\nThis is great.\n\noptional: you can do the cleanup commands in test_when_finished to\nmake sure they happen even if the test fails.\n\nTests don't seem to use \"perl -pi\" anywhere else.  This instance could\nbe simplified by using sed.\n\nWith whatever subset of the changes below look good,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\ndiff --git i/builtin/fsck.c w/builtin/fsck.c\nindex 92324e130c..b5e13a4556 100644\n--- i/builtin/fsck.c\n+++ w/builtin/fsck.c\n@@ -771,7 +771,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (keep_cache_objects) {\n-\t\tforce_verify_index_checksum = 1;\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 i/cache.h w/cache.h\nindex 48b47083d3..9834d71f28 100644\n--- i/cache.h\n+++ w/cache.h\n@@ -706,7 +706,7 @@ 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 force_verify_index_checksum;\n+extern int verify_index_checksum;\n \n /* Environment bits from configuration mechanism */\n extern int trust_executable_bit;\ndiff --git i/read-cache.c w/read-cache.c\nindex 13326464d4..008b335844 100644\n--- i/read-cache.c\n+++ w/read-cache.c\n@@ -1372,7 +1372,7 @@ struct ondisk_cache_entry_extended {\n \t\t\t    ondisk_cache_entry_size(ce_namelen(ce)))\n \n /* Allow fsck to force verification of the index checksum. */\n-int force_verify_index_checksum;\n+int verify_index_checksum;\n \n static int verify_hdr(struct cache_header *hdr, unsigned long size)\n {\n@@ -1386,7 +1386,7 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)\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 (!force_verify_index_checksum)\n+\tif (!verify_index_checksum)\n \t\treturn 0;\n \n \tgit_SHA1_Init(&c);\ndiff --git i/t/t1450-fsck.sh w/t/t1450-fsck.sh\nindex 86757ffa52..e69b32f219 100755\n--- i/t/t1450-fsck.sh\n+++ w/t/t1450-fsck.sh\n@@ -689,15 +689,15 @@ test_expect_success 'bogus head does not fallback to all heads' '\n \t! grep $blob out\n '\n \n-test_expect_success PERL 'detect corrupt index file in fsck' '\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-\tperl -pi -e \"s/zzzzzzzz/yyyyyyyy/\" .git/index &&\n-\ttest_must_fail git fsck --cache &&\n-\trm .git/index &&\n-\tmv .git/index.backup .git/index &&\n-\trm zzzzzzzz\n+\tsed -e \"s/zzzzzzzz/yyyyyyyy/\" .git/index >.git/index+ &&\n+\tmv .git/index+ .git/index &&\n+\ttest_must_fail git fsck --cache\n '\n \n test_done\n"},{"id":"316226","messageId":"18953c00-0eb0-c1e0-0d29-070e0ec60280@jeffhostetler.com","threadId":"45606","inReplyTo":"20170405170336.GE8741@aiede.mtv.corp.google.com","subject":"Re: [PATCH v5] read-cache: force_verify_index_checksum","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-05T18:08:42Z","receivedAt":"2017-04-05T18:10:25Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 4/5/2017 1:03 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> git@jeffhostetler.com wrote:\n>\n>> Signed-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 | 11 +++++++++++\n>>  4 files changed, 21 insertions(+)\n>\n> Yay!  I love this version.\n>\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\tforce_verify_index_checksum = 1;\n>\n> nit: now that there isn't a config this is overriding, this isn't\n> \"force_verify\" so much as \"verify\".\n\ngood point.\n\n>\n> [...]\n>> +++ b/t/t1450-fsck.sh\n>> @@ -689,4 +689,15 @@ test_expect_success 'bogus head does not fallback to all heads' '\n>>  \t! grep $blob out\n>>  '\n>>\n>> +test_expect_success PERL 'detect corrupt index file in fsck' '\n>> +\tcp .git/index .git/index.backup &&\n>> +\techo zzzzzzzz >zzzzzzzz &&\n>> +\tgit add zzzzzzzz &&\n>> +\tperl -pi -e \"s/zzzzzzzz/yyyyyyyy/\" .git/index &&\n>> +\ttest_must_fail git fsck --cache &&\n>> +\trm .git/index &&\n>> +\tmv .git/index.backup .git/index &&\n>> +\trm zzzzzzzz\n>> +'\n>\n> This is great.\n>\n> optional: you can do the cleanup commands in test_when_finished to\n> make sure they happen even if the test fails.\n>\n> Tests don't seem to use \"perl -pi\" anywhere else.  This instance could\n> be simplified by using sed.\n>\n> With whatever subset of the changes below look good,\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n> [...]\n\ngood point.\n\nI just pushed another version.\nThanks,\nJeff\n\n\n"}]}