# [PATCH v7] read-cache: call verify_hdr() in a background thread 13 messages from 2017-04-14 to 2017-05-09. Participants: git@jeffhostetler.com, Johannes Sixt, Jeff Hostetler, Junio C Hamano, Johannes Schindelin, Christian Couder. Thread: https://gitlist.dev/t/45696 ## git@jeffhostetler.com, 2017-04-14 20:32 Subject: [PATCH v7] read-cache: call verify_hdr() in a background thread Message-ID: <20170414203221.43015-1-git@jeffhostetler.com> URL: https://gitlist.dev/e/20170414203221.43015-1-git%40jeffhostetler.com ``` From: Jeff Hostetler Version 7 of this patch cleans up the fsck test using test_when_finished and eliminates unnecessary mv/rm's. Jeff Hostetler (1): read-cache: force_verify_index_checksum builtin/fsck.c | 1 + cache.h | 2 ++ read-cache.c | 7 +++++++ t/t1450-fsck.sh | 13 +++++++++++++ 4 files changed, 23 insertions(+) -- 2.9.3 ``` ## git@jeffhostetler.com, 2017-04-14 20:32 Subject: [PATCH v7] read-cache: force_verify_index_checksum Message-ID: <20170414203221.43015-2-git@jeffhostetler.com> URL: https://gitlist.dev/e/20170414203221.43015-2-git%40jeffhostetler.com In-Reply-To: <20170414203221.43015-1-git@jeffhostetler.com> ``` From: Jeff Hostetler Teach git to skip verification of the SHA1-1 checksum at the end of the index file in verify_hdr() which is called from read_index() unless the "force_verify_index_checksum" global variable is set. Teach fsck to force this verification. The checksum verification is for detecting disk corruption, and for small projects, the time it takes to compute SHA-1 is not that significant, but for gigantic repositories this calculation adds significant time to every command. These effect can be seen using t/perf/p0002-read-cache.sh: Test HEAD~1 HEAD -------------------------------------------------------------------------------------- 0002.1: read_cache/discard_cache 1000 times 0.66(0.44+0.20) 0.30(0.27+0.02) -54.5% Signed-off-by: Jeff Hostetler --- builtin/fsck.c | 1 + cache.h | 2 ++ read-cache.c | 7 +++++++ t/t1450-fsck.sh | 13 +++++++++++++ 4 files changed, 23 insertions(+) diff --git a/builtin/fsck.c b/builtin/fsck.c index 1a5cacc..5512d06 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -771,6 +771,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) } if (keep_cache_objects) { + verify_index_checksum = 1; read_cache(); for (i = 0; i < active_nr; i++) { unsigned int mode; diff --git a/cache.h b/cache.h index 80b6372..87f13bf 100644 --- a/cache.h +++ b/cache.h @@ -685,6 +685,8 @@ extern void update_index_if_able(struct index_state *, struct lock_file *); extern int hold_locked_index(struct lock_file *, int); extern void set_alternate_index_output(const char *); +extern int verify_index_checksum; + /* Environment bits from configuration mechanism */ extern int trust_executable_bit; extern int trust_ctime; diff --git a/read-cache.c b/read-cache.c index 9054369..c4205aa 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1371,6 +1371,9 @@ struct ondisk_cache_entry_extended { ondisk_cache_entry_extended_size(ce_namelen(ce)) : \ ondisk_cache_entry_size(ce_namelen(ce))) +/* Allow fsck to force verification of the index checksum. */ +int verify_index_checksum; + static int verify_hdr(struct cache_header *hdr, unsigned long size) { git_SHA_CTX c; @@ -1382,6 +1385,10 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size) hdr_version = ntohl(hdr->hdr_version); if (hdr_version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < hdr_version) return error("bad index version %d", hdr_version); + + if (!verify_index_checksum) + return 0; + git_SHA1_Init(&c); git_SHA1_Update(&c, hdr, size - 20); git_SHA1_Final(sha1, &c); diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index 33a51c9..677e15a 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' ' ! grep $blob out ' +test_expect_success 'detect corrupt index file in fsck' ' + cp .git/index .git/index.backup && + test_when_finished "mv .git/index.backup .git/index" && + echo zzzzzzzz >zzzzzzzz && + git add zzzzzzzz && + sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy && + mv .git/index.yyy .git/index && + # Confirm that fsck detects invalid checksum + test_must_fail git fsck --cache && + # Confirm that status no longer complains about invalid checksum + git status +' + test_done -- 2.9.3 ``` ## Johannes Sixt, 2017-04-24 17:26 Subject: Re: [PATCH v7] read-cache: force_verify_index_checksum Message-ID: <870a8a36-fc6c-6b07-d09e-eec8a9f46a5c@kdbg.org> URL: https://gitlist.dev/e/870a8a36-fc6c-6b07-d09e-eec8a9f46a5c%40kdbg.org In-Reply-To: <20170414203221.43015-2-git@jeffhostetler.com> ``` Am 14.04.2017 um 22:32 schrieb git@jeffhostetler.com: > diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh > index 33a51c9..677e15a 100755 > --- a/t/t1450-fsck.sh > +++ b/t/t1450-fsck.sh > @@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' ' > ! grep $blob out > ' > > +test_expect_success 'detect corrupt index file in fsck' ' > + cp .git/index .git/index.backup && > + test_when_finished "mv .git/index.backup .git/index" && > + echo zzzzzzzz >zzzzzzzz && > + git add zzzzzzzz && > + sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy && sed on a binary file? Sooner or later we are going to run into portability issues. > + mv .git/index.yyy .git/index && > + # Confirm that fsck detects invalid checksum > + test_must_fail git fsck --cache && You should ensure that this failure is really because of an invalid checksum. The failure could also be due to an extra LF at the end that sed inserted, no? > + # Confirm that status no longer complains about invalid checksum > + git status > +' > + > test_done -- Hannes ``` ## Jeff Hostetler, 2017-04-24 18:39 Subject: Re: [PATCH v7] read-cache: force_verify_index_checksum Message-ID: <8bde0b54-c0f6-364d-1f08-ce5207eb6c86@jeffhostetler.com> URL: https://gitlist.dev/e/8bde0b54-c0f6-364d-1f08-ce5207eb6c86%40jeffhostetler.com In-Reply-To: <870a8a36-fc6c-6b07-d09e-eec8a9f46a5c@kdbg.org> ``` On 4/24/2017 1:26 PM, Johannes Sixt wrote: > Am 14.04.2017 um 22:32 schrieb git@jeffhostetler.com: >> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh >> index 33a51c9..677e15a 100755 >> --- a/t/t1450-fsck.sh >> +++ b/t/t1450-fsck.sh >> @@ -689,4 +689,17 @@ test_expect_success 'bogus head does not >> fallback to all heads' ' >> ! grep $blob out >> ' >> >> +test_expect_success 'detect corrupt index file in fsck' ' >> + cp .git/index .git/index.backup && >> + test_when_finished "mv .git/index.backup .git/index" && >> + echo zzzzzzzz >zzzzzzzz && >> + git add zzzzzzzz && >> + sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy && > > sed on a binary file? Sooner or later we are going to run into > portability issues. In v5 of this patch series I used "perl" and it was suggested that I use "sed" instead. It doesn't matter to me which we use. My testing showed that it was safe, but that was only Linux. Does the mailing list have a preference for this ? > > >> + mv .git/index.yyy .git/index && >> + # Confirm that fsck detects invalid checksum >> + test_must_fail git fsck --cache && > > You should ensure that this failure is really because of an invalid > checksum. The failure could also be due to an extra LF at the end that > sed inserted, no? I suppose we could, but I'm tempted to wait on that for now. Jeff ``` ## Junio C Hamano, 2017-04-25 02:10 Subject: Re: [PATCH v7] read-cache: force_verify_index_checksum Message-ID: URL: https://gitlist.dev/e/xmqqk269xmmq.fsf%40gitster.mtv.corp.google.com In-Reply-To: <8bde0b54-c0f6-364d-1f08-ce5207eb6c86@jeffhostetler.com> ``` Jeff Hostetler writes: >>> +test_expect_success 'detect corrupt index file in fsck' ' >>> + cp .git/index .git/index.backup && >>> + test_when_finished "mv .git/index.backup .git/index" && >>> + echo zzzzzzzz >zzzzzzzz && >>> + git add zzzzzzzz && >>> + sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy && >> >> sed on a binary file? Sooner or later we are going to run into >> portability issues. > > In v5 of this patch series I used "perl" and it was suggested that I > use "sed" instead. > It doesn't matter to me which we use. My testing showed that it was > safe, but that > was only Linux. > > Does the mailing list have a preference for this ? Instead of munging pathnames z* to y*, I'd prefer to see the actual checksum bytes at the end replaced in the index file. After all that is what this test really cares about, and it ensures that the failure detected is due to checksum mismatch. >>> + mv .git/index.yyy .git/index && >>> + # Confirm that fsck detects invalid checksum >>> + test_must_fail git fsck --cache && >> >> You should ensure that this failure is really because of an invalid >> checksum. The failure could also be due to an extra LF at the end >> that sed inserted, no? > > I suppose we could, but I'm tempted to wait on that for now. > > Jeff ``` ## Johannes Schindelin, 2017-04-25 21:04 Subject: Re: [PATCH v7] read-cache: force_verify_index_checksum Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1704252225450.3480%40virtualbox In-Reply-To: ``` Hi, On Mon, 24 Apr 2017, Junio C Hamano wrote: > Jeff Hostetler writes: > > >>> +test_expect_success 'detect corrupt index file in fsck' ' > >>> + cp .git/index .git/index.backup && > >>> + test_when_finished "mv .git/index.backup .git/index" && > >>> + echo zzzzzzzz >zzzzzzzz && > >>> + git add zzzzzzzz && > >>> + sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy && > >> > >> sed on a binary file? Sooner or later we are going to run into > >> portability issues. > > > > In v5 of this patch series I used "perl" and it was suggested that I > > use "sed" instead. It doesn't matter to me which we use. My testing > > showed that it was safe, but that was only Linux. I am sorry to hear that the Git mailing list's review gives you whiplash. The problem with sed is that BSD sed behaves a bit differently than GNU sed, and we quietly expect every contributor to be an expert in the portability aspects of sed. TBH I am quite surprised that anybody would have suggested to use sed rather than Perl to edit binary files in the first place. In my opinion, that was bad advice. > > Does the mailing list have a preference for this ? > > Instead of munging pathnames z* to y*, I'd prefer to see the actual > checksum bytes at the end replaced in the index file. After all > that is what this test really cares about, and it ensures that the > failure detected is due to checksum mismatch. I see that v8 uses a Perl script again, and it is well written and obvious. Just in case that certain reviewers favor length over readability, let me offer this snippet: size=$(perl -e "print -s \".git/index\"") && dd if=/dev/zero of=.git/index bs=1 seek=$(($size-20) count=20 Since whatever hash will be used in the future is most likely larger than 20 bytes, this should still work fine (and even if somebody sane replaces the SHA-1 of the index with a CRC-32 for the same benefit we have now, the test will fail quickly and it is easy to replace the 20 by 4). Ciao, Dscho ``` ## Junio C Hamano, 2017-04-26 03:25 Subject: Re: [PATCH v7] read-cache: force_verify_index_checksum Message-ID: URL: https://gitlist.dev/e/xmqq60hru9xg.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: > Just in case that certain reviewers favor length over readability, let me > offer this snippet: > > size=$(perl -e "print -s \".git/index\"") && > dd if=/dev/zero of=.git/index bs=1 seek=$(($size-20) count=20 Yup, this does sound like a good direction to go (I think you have some unbalanced parens but it is sufficient to convey the idea). > Since whatever hash will be used in the future is most likely larger than > 20 bytes, this should still work fine (and even if somebody sane replaces > the SHA-1 of the index with a CRC-32 for the same benefit we have now, the > test will fail quickly and it is easy to replace the 20 by 4). True that, too. ``` ## Christian Couder, 2017-05-08 09:45 Subject: Re: [PATCH v7] read-cache: force_verify_index_checksum Message-ID: URL: https://gitlist.dev/e/CAP8UFD2v8R8bSjUi8%2B1271fJ%3DjR5JbbGfgO_fdeYpzE%3DEGx_Pw%40mail.gmail.com In-Reply-To: <20170414203221.43015-2-git@jeffhostetler.com> ``` On Fri, Apr 14, 2017 at 10:32 PM, wrote: > diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh > index 33a51c9..677e15a 100755 > --- a/t/t1450-fsck.sh > +++ b/t/t1450-fsck.sh > @@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' ' > ! grep $blob out > ' > > +test_expect_success 'detect corrupt index file in fsck' ' > + cp .git/index .git/index.backup && > + test_when_finished "mv .git/index.backup .git/index" && > + echo zzzzzzzz >zzzzzzzz && > + git add zzzzzzzz && > + sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy && > + mv .git/index.yyy .git/index && > + # Confirm that fsck detects invalid checksum > + test_must_fail git fsck --cache && > + # Confirm that status no longer complains about invalid checksum > + git status > +' This test does not pass when the GIT_TEST_SPLIT_INDEX env variable is set on my Linux machine. Also it looks like you sent a v8 of this patch series with a different test, but what is in master looks like the above test instead of the test in your v8. ``` ## Jeff Hostetler, 2017-05-08 16:50 Subject: Re: [PATCH v7] read-cache: force_verify_index_checksum Message-ID: <962eefec-2ea6-6b43-7bd5-51da4150bb6a@jeffhostetler.com> URL: https://gitlist.dev/e/962eefec-2ea6-6b43-7bd5-51da4150bb6a%40jeffhostetler.com In-Reply-To: ``` On 5/8/2017 5:45 AM, Christian Couder wrote: > On Fri, Apr 14, 2017 at 10:32 PM, wrote: >> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh >> index 33a51c9..677e15a 100755 >> --- a/t/t1450-fsck.sh >> +++ b/t/t1450-fsck.sh >> @@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' ' >> ! grep $blob out >> ' >> >> +test_expect_success 'detect corrupt index file in fsck' ' >> + cp .git/index .git/index.backup && >> + test_when_finished "mv .git/index.backup .git/index" && >> + echo zzzzzzzz >zzzzzzzz && >> + git add zzzzzzzz && >> + sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy && >> + mv .git/index.yyy .git/index && >> + # Confirm that fsck detects invalid checksum >> + test_must_fail git fsck --cache && >> + # Confirm that status no longer complains about invalid checksum >> + git status >> +' > > This test does not pass when the GIT_TEST_SPLIT_INDEX env variable is > set on my Linux machine. > > Also it looks like you sent a v8 of this patch series with a different > test, but what is in master looks like the above test instead of the > test in your v8. > There was concern about using sed on a binary file not being portable and a request to change the test to just corrupt the checksum rather than an index-entry, so I changed it in v8. Does the v8 version of the test also fail on your machine ? Thanks Jeff ``` ## Christian Couder, 2017-05-08 20:03 Subject: Re: [PATCH v7] read-cache: force_verify_index_checksum Message-ID: URL: https://gitlist.dev/e/CAP8UFD1dR1QY81tCAcbB6RP61vBxXy-u8zFAhLpk9uP71Jna-Q%40mail.gmail.com In-Reply-To: <962eefec-2ea6-6b43-7bd5-51da4150bb6a@jeffhostetler.com> ``` On Mon, May 8, 2017 at 6:50 PM, Jeff Hostetler wrote: > > > On 5/8/2017 5:45 AM, Christian Couder wrote: >> >> This test does not pass when the GIT_TEST_SPLIT_INDEX env variable is >> set on my Linux machine. >> >> Also it looks like you sent a v8 of this patch series with a different >> test, but what is in master looks like the above test instead of the >> test in your v8. > > There was concern about using sed on a binary file not being portable > and a request to change the test to just corrupt the checksum rather > than an index-entry, so I changed it in v8. > > Does the v8 version of the test also fail on your machine ? The v8 version of the test succeeds on my machine. ``` ## Jeff Hostetler, 2017-05-08 20:18 Subject: Re: [PATCH v7] read-cache: force_verify_index_checksum Message-ID: URL: https://gitlist.dev/e/c9f77c02-c498-1b0d-26c2-27d6f576ee71%40jeffhostetler.com In-Reply-To: ``` On 5/8/2017 4:03 PM, Christian Couder wrote: > On Mon, May 8, 2017 at 6:50 PM, Jeff Hostetler wrote: >> >> >> On 5/8/2017 5:45 AM, Christian Couder wrote: >>> >>> This test does not pass when the GIT_TEST_SPLIT_INDEX env variable is >>> set on my Linux machine. >>> >>> Also it looks like you sent a v8 of this patch series with a different >>> test, but what is in master looks like the above test instead of the >>> test in your v8. >> >> There was concern about using sed on a binary file not being portable >> and a request to change the test to just corrupt the checksum rather >> than an index-entry, so I changed it in v8. >> >> Does the v8 version of the test also fail on your machine ? > > The v8 version of the test succeeds on my machine. OK, thanks. It worked on mine too. Since the v8 version is a better test, I'm content to stick with it and not try to address the problem with the previous version. Jeff ``` ## Junio C Hamano, 2017-05-09 02:11 Subject: Re: [PATCH v7] read-cache: force_verify_index_checksum Message-ID: URL: https://gitlist.dev/e/xmqqk25q6aph.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Jeff Hostetler writes: > On 5/8/2017 4:03 PM, Christian Couder wrote: >> On Mon, May 8, 2017 at 6:50 PM, Jeff Hostetler wrote: >>> >>> >>> On 5/8/2017 5:45 AM, Christian Couder wrote: >>>> >>>> This test does not pass when the GIT_TEST_SPLIT_INDEX env variable is >>>> set on my Linux machine. >>>> >>>> Also it looks like you sent a v8 of this patch series with a different >>>> test, but what is in master looks like the above test instead of the >>>> test in your v8. >>> >>> There was concern about using sed on a binary file not being portable >>> and a request to change the test to just corrupt the checksum rather >>> than an index-entry, so I changed it in v8. >>> >>> Does the v8 version of the test also fail on your machine ? >> >> The v8 version of the test succeeds on my machine. > > OK, thanks. It worked on mine too. Since the v8 version is a > better test, I'm content to stick with it and not try to address > the problem with the previous version. Thanks. Let's merge that in. ``` ## Junio C Hamano, 2017-05-09 05:27 Subject: Re: [PATCH v7] read-cache: force_verify_index_checksum Message-ID: URL: https://gitlist.dev/e/xmqqtw4u38h2.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Christian Couder writes: > On Fri, Apr 14, 2017 at 10:32 PM, wrote: >> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh >> index 33a51c9..677e15a 100755 >> --- a/t/t1450-fsck.sh >> +++ b/t/t1450-fsck.sh >> @@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' ' >> ! grep $blob out >> ' >> >> +test_expect_success 'detect corrupt index file in fsck' ' >> + cp .git/index .git/index.backup && >> + test_when_finished "mv .git/index.backup .git/index" && >> + echo zzzzzzzz >zzzzzzzz && >> + git add zzzzzzzz && >> + sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy && >> + mv .git/index.yyy .git/index && >> + # Confirm that fsck detects invalid checksum >> + test_must_fail git fsck --cache && >> + # Confirm that status no longer complains about invalid checksum >> + git status >> +' > > This test does not pass when the GIT_TEST_SPLIT_INDEX env variable is > set on my Linux machine. > > Also it looks like you sent a v8 of this patch series with a different > test, but what is in master looks like the above test instead of the > test in your v8. Thanks for raising this. I almost forgot that the final finishing touch to update the test was needed in 'master', and this exchange caught it before it is too late. ```