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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 19, 2015, 20:42 UTC
Message-ID: <95e42f21de69ab5299c03ce6ad107037@www.dscho.org>
In-Reply-To: <xmqq1th77829.fsf@gitster.dls.corp.google.com>

Hi Junio,

On 2015-06-19 22:32, Junio C Hamano wrote:

Show 9 quoted lines

> 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?"

Hah, you caught me there. I wanted to say "blob objects".

Show 5 quoted lines

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

It actually makes a ton of sense as a kind of light-weight check ;-) Try it, it is really much, much faster than a full fsck.

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

The meaning of "quick" that I was thinking of was not the same as "fast", but more like "just a quick check". As in "quick & dirty" ;-)

The point is not to ignore corrupt blobs, by the way, it is to check the connectivity only, and save substantial amounts of time doing so.

Can you think of a name for the option that is as short as `--quick` but means the same as `--connectivity-only`?

Ciao, Dscho