Re: [PATCH 2/4] read-cache: add index.skipHash config option

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <derrickstolee@github.com>
> [...]
> diff --git a/read-cache.c b/read-cache.c
> index 46f5e497b14..fb4d6fb6387 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1817,6 +1817,8 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)
>  	git_hash_ctx c;
>  	unsigned char hash[GIT_MAX_RAWSZ];
>  	int hdr_version;
> +	unsigned char *start, *end;
> +	struct object_id oid;
>  
>  	if (hdr->hdr_signature != htonl(CACHE_SIGNATURE))
>  		return error(_("bad signature 0x%08x"), hdr->hdr_signature);
> @@ -1827,10 +1829,16 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)
>  	if (!verify_index_checksum)
>  		return 0;
>  
> +	end = (unsigned char *)hdr + size;
> +	start = end - the_hash_algo->rawsz;
> +	oidread(&oid, start);
> +	if (oideq(&oid, null_oid()))
> +		return 0;

It's good to see this use the existing hashing support, as I suggested
in the RFC comments. Glad it helped.

>  	int ieot_entries = 1;
>  	struct index_entry_offset_table *ieot = NULL;
>  	int nr, nr_threads;
> +	int skip_hash;

You don't need this variable.
>  
>  	f = hashfd(tempfile->fd, tempfile->filename.buf);
>  
> +	if (!git_config_get_maybe_bool("index.skiphash", &skip_hash))
> +		f->skip_hash = skip_hash;

Because this can just be:

	git_config_get_maybe_bool("index.skiphash", &f->skip_hash);

I.e. the config API guarantees that it won't touch the variable if the
key doesn't exist, so no need for the intermediate variable.

In a later commit you convert this to that very API use when moving this
to the repo-settings.

Can we maybe skip straight to that step?

> +test_expect_success 'index.skipHash config option' '
> +	(
> +		rm -f .git/index &&
> +		git -c index.skipHash=true add a &&
> +		git fsck
> +	)

Why the subshell?