[PATCH v7] read-cache: call verify_hdr() in a background thread

[PATCH v7] read-cache: call verify_hdr() in a background thread

[git]

From: Jeff Hostetler <jeffhost@microsoft.com>

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

[PATCH v7] read-cache: force_verify_index_checksum

[git]

From: Jeff Hostetler <jeffhost@microsoft.com>

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 <jeffhost@microsoft.com>
---
 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

Re: [PATCH v7] read-cache: force_verify_index_checksum

[Johannes Sixt]

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

Re: [PATCH v7] read-cache: force_verify_index_checksum

[Jeff Hostetler]

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

Re: [PATCH v7] read-cache: force_verify_index_checksum

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> 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

Re: [PATCH v7] read-cache: force_verify_index_checksum

[Johannes Schindelin]

Hi,

On Mon, 24 Apr 2017, Junio C Hamano wrote:

> Jeff Hostetler <git@jeffhostetler.com> 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

Re: [PATCH v7] read-cache: force_verify_index_checksum

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

Re: [PATCH v7] read-cache: force_verify_index_checksum

[Christian Couder]

On Fri, Apr 14, 2017 at 10:32 PM,  <git@jeffhostetler.com> 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.

Re: [PATCH v7] read-cache: force_verify_index_checksum

[Jeff Hostetler]

On 5/8/2017 5:45 AM, Christian Couder wrote:
> On Fri, Apr 14, 2017 at 10:32 PM,  <git@jeffhostetler.com> 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

Re: [PATCH v7] read-cache: force_verify_index_checksum

[Christian Couder]

On Mon, May 8, 2017 at 6:50 PM, Jeff Hostetler <git@jeffhostetler.com> 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.

Re: [PATCH v7] read-cache: force_verify_index_checksum

[Jeff Hostetler]

On 5/8/2017 4:03 PM, Christian Couder wrote:
> On Mon, May 8, 2017 at 6:50 PM, Jeff Hostetler <git@jeffhostetler.com> 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

Re: [PATCH v7] read-cache: force_verify_index_checksum

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> On 5/8/2017 4:03 PM, Christian Couder wrote:
>> On Mon, May 8, 2017 at 6:50 PM, Jeff Hostetler <git@jeffhostetler.com> 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.

Re: [PATCH v7] read-cache: force_verify_index_checksum

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Fri, Apr 14, 2017 at 10:32 PM,  <git@jeffhostetler.com> 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.