Re: [PATCH v3 0/2] read-cache: call verify_hdr() in a background thread

From: Jeff King <peff@peff.net>
Date: Mar 30, 2017, 19:58 UTC
Message-ID: <20170330195820.kexvl44x7ncthlcx@sigill.intra.peff.net>
In-Reply-To: <xmqqtw6a35qc.fsf@gitster.mtv.corp.google.com>

On Thu, Mar 30, 2017 at 12:49:15PM -0700, Junio C Hamano wrote:

> Notable suggested changes I have in this one are:
> 
>  * I stole the numbers from the cover letter of v2 and added them at
>    the end of the log message.

Yeah, good.

Show 5 quoted lines

>  * As the checksum is not a useless relic, but is an integrity
>    check, I dropped the "ancient relic" from the proposed log
>    message.  It is just that the modern disks are reliable enough to
>    make it worthwhile to think about a trade-off this patch makes
>    between performance and integrity.

Yeah, I'd agree this is more of a tradeoff than a cleanup.

>  * As it is customary, the configuration variable starts as an opt
>    in feature.  In a few releases, perhaps we can flip the default,
>    but we do not do so from day one.

I think this is so user-invisible that it doesn't really matter much, but I'm OK with taking it slow.

>  * Updated the code around the call to config-get-bool to avoid
>    asking the same question twice.

A few comments on that below.

>  * Added minimum documentation.

Yep, looks good.

Show 5 quoted lines

> By the way, are we sure we have something that validates the
> checksum regardless of the configuration setting?  If not, we may
> want to tweak this further so that we can force the validation from
> "git fsck" or something.  I am not going to do that myself, but it
> may be necessary before this graduates to 'master'.

Yes, I'd agree this shouldn't graduate without the matching change to teach fsck to flip the switch back.

Show 15 quoted lines

> +	static int do_checksum = -1;
> [...]
> +	if (do_checksum < 0) {
> +		/*
> +		 * Since we run very early in command startup, git_config()
> +		 * may not have been called yet and the various "core_*"
> +		 * global variables haven't been set.  So look it up
> +		 * explicitly.
> +		 */
> +		git_config_get_bool("core.checksumindex", &do_checksum);
> +		if (do_checksum < 0)
> +			do_checksum = 0; /* default to false */
> +	}
> +	if (!do_checksum)
> +		return 0;

This is basically introducing a new caching layer, but there's no way to invalidate it (if, say, we looked at the config once and then we knew that the config changed). I think it's probably OK, because the main reason for the config to change is reading it before/after repository setup. But since this is index code, it shouldn't be called at all before repository setup.

Still, I'm not sure the extra layer of cache is all that valuable. It should be a single hash lookup in the config cache (in an operation that otherwise reads the entire index).

-Peff