Re: [PATCH 4/4] features: feature.manyFiles implies fast index writes
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 7, 2022, 22:30 UTC
- Message-ID
- <221207.86v8mm972z.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <77bf5d5ff27729a39ac00d52af3c09610d733b14.1670433958.git.gitgitgadget@gmail.com>
On Wed, Dec 07 2022, Derrick Stolee via GitGitGadget wrote:
Show 20 quoted lines
> 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?
Show 5 quoted lines
> + 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)?