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

From: Junio C Hamano <[email-masked]>
Date: Fri, 19 Jun 2015 13:32:46 -0700

Johannes Schindelin <[email-masked]> 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.

> 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.