Re: [PATCH 4/4] features: feature.manyFiles implies fast index writes

["Æ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 fb4d6fb6387..1844953fba7 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2923,12 +2923,13 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>  	int ieot_entries = 1;
>  	struct index_entry_offset_table *ieot = NULL;
>  	int nr, nr_threads;
> -	int skip_hash;
>  
>  	f = hashfd(tempfile->fd, tempfile->filename.buf);
>  
> -	if (!git_config_get_maybe_bool("index.skiphash", &skip_hash))
> -		f->skip_hash = skip_hash;
> +	if (istate->repo) {
> +		prepare_repo_settings(istate->repo);
> +		f->skip_hash = istate->repo->settings.index_skip_hash;
> +	}

Urm, are we ever going to find ourselves in a situation where:

 * We have read the settings for the_repository
 * We have an index we're about to write out as our "main index", but
   the istate->repo *isn't* the_repository.
 * Even then, wouldn't the two copies of the repos have read the same
   repo settings?

But maybe there's a really obvious submodule / worktree / whatever edge
case I'm missing.

But if not, shouldn't we just always read/write this from
the_repository?

> +		rm -f .git/index &&
> +		git -c feature.manyFiles=true \
> +		    -c index.skipHash=false add a &&
> +		test_trailing_hash .git/index >hash &&
> +		! test_cmp expect hash

We had a parallel thread where we discussed "! test_cmp" being an
anti-pattern, i.e. you want them not to be the same, but you want it to
still show a diff, Maybe just "! cmp" ?

I.e. either the diff will be meaningless, or we really should be
asserting the actual value we want, not what it shouldn't be.

so in this case, shouldn't we assert that it's the 0000... value, or the
actual hash (depending on which way around we're testing this)?