Re: [PATCH v6 17/19] fsck: Introduce `git fsck --quick`

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 19, 2015, 20:32 UTC
Message-ID: <xmqq1th77829.fsf@gitster.dls.corp.google.com>
In-Reply-To: <5f1c4c16027b00ef80490d67bec5e948481153ec.1434720655.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> This option avoids unpacking each and all objects, and just verifies the
> connectivity.

That sounds like marketing ;-)

"Wow this does not unpack unnecessarily, wait, it needs to unpack and parse 3 out of 4 kinds of objects?"

Jokes aside, given that you should regularly repack your repository anyway, I do not think it is such a big downside that this mode misses a corrupt objects, and the 1 out of 4 kinds of objects, i.e. blobs, occupy major part of the repository storage, so this new mode probably makes sense.

Show 31 quoted lines

> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index 922c346..2863a8a 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -431,4 +431,26 @@ test_expect_success 'fsck notices ref pointing to missing tag' '
>  	test_must_fail git -C missing fsck
>  '
>  
> +test_expect_success 'fsck --quick' '
> +	rm -rf quick &&
> +	git init quick &&
> +	(
> +		cd quick &&
> +		touch empty &&
> +		git add empty &&
> +		test_commit empty &&
> +		empty=.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 &&
> +		rm -f $empty &&
> +		echo invalid >$empty &&
> +		test_must_fail git fsck --strict &&
> +		git fsck --strict --quick &&
> +		tree=$(git rev-parse HEAD:) &&
> +		suffix=${tree#??} &&
> +		tree=.git/objects/${tree%$suffix}/$suffix &&
> +		rm -f $tree &&
> +		echo invalid >$tree &&
> +		test_must_fail git fsck --strict --quick
> +	)
> +'
> +
>  test_done

I see a few impedance mismatch here. For --quick, I would have expected that the addition would be in t/perf/, not here.

Also the fact that quickness comes by cheating on blobs is an implementation detail; in the future, perhaps somebody may come up with a way to do a quick fsck while making sure blob corruption is also detected. The new test that expects "--quick" to ignore a corrupt blob forbids such a progress.

If the option name was "--ignore-corrupt-blob", then the above change is 100% justified, though.