Re: [PATCH 3/4] fsck: check rev-index position values

From: Derrick Stolee <derrickstolee@github.com>
Date: Apr 18, 2023, 14:32 UTC
Message-ID: <e29e6584-709f-d172-e73a-88a2027a9283@github.com>
In-Reply-To: <xmqq1qkijhho.fsf@gitster.g>

On 4/17/2023 6:01 PM, Junio C Hamano wrote:

Show 19 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>>  	if (!hashfile_checksum_valid((const unsigned char *)p->revindex_map, p->revindex_size)) {
>>  		error(_("invalid checksum"));
>> -		return -1;
>> +		res = -1;
>>  	}
>>  
>> -	return 0;
>> +	/* This may fail due to a broken .idx. */
>> +	if (create_pack_revindex_in_memory(p))
>> +		return res;
> 
> Here, if the revindex file itself is self consistent, res would
> still be 0, so we will end up silently returning.  Is the idea that
> while whatever causes in-memory revindex fail to be computed is a
> concern from the point of view of the repository's health, it would
> be caught elsewhere as a problem for the <pack,idx> pair we are
> seeing here?

This is something I noticed during a test suite run, where the .idx was corrupt, so the .rev file could not be checked. In this situation, we are not using the .rev file for anything since Git processes would fail to lookup objects in the associated packfile.

In this case, we have no way to validate the rest of the .rev file's contents, but at least we checked its header and checksum, which is the best we can hope to do in this case.

Thanks, -Stolee