Re: [PATCH v6] read-cache: force_verify_index_checksum

From: Jeff King <peff@peff.net>
Date: Apr 5, 2017, 18:25 UTC
Message-ID: <20170405182533.a5divj63zfkfejui@sigill.intra.peff.net>
In-Reply-To: <20170405180625.12439-2-git@jeffhostetler.com>

On Wed, Apr 05, 2017 at 06:06:25PM +0000, git@jeffhostetler.com wrote:

Show 6 quoted lines

> ---
>  builtin/fsck.c  |  1 +
>  cache.h         |  2 ++
>  read-cache.c    |  7 +++++++
>  t/t1450-fsck.sh | 13 +++++++++++++
>  4 files changed, 23 insertions(+)

This version is delightfully simple. Thanks for sticking with it.

Show 12 quoted lines

> +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 &&
> +	test_when_finished "rm zzzzzzzz" &&
> +	git add zzzzzzzz &&
> +	sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy &&
> +	rm .git/index &&
> +	mv .git/index.yyy .git/index &&
> +	test_when_finished "rm .git/index" &&
> +	test_must_fail git fsck --cache
> +'

I was surprised by the second when-finished "rm". The earlier "mv" should be enough to restore everything. I guess you're trying to overcome the tendency of "mv" to complain about overwriting on some platforms.

But wouldn't that make the first when-finished "mv" potentially a problem, if it happens before the "rm .git/index" line?

Perhaps "mv -f" would make it all simpler (ordinarily I'd suggest just "git reset --hard", but since we're corrupting the index, it may be better not to rely on git for the restore). So maybe just:

  cp .git/index .git/index.backup &&
  test_when_finished "mv -f .git/index.backup .git/index" &&
  ...

I think the "rm zzzzzzzz" can go, too. We don't typically worry too much about accumulating random files in the working tree. The important thing is that the index is restored to a sane state.

-Peff