Re: [PATCH v3 0/4] Optionally skip hashing index on write

On Thu, Dec 15 2022, Derrick Stolee via GitGitGadget wrote:

Re the comments I had on an earlier version[1] I tried this series with
these changes squashed in:
	
	1:  b582d681581 = 1:  53d289cf82c hashfile: allow skipping the hash function
	2:  00738c81a12 ! 2:  db487f3e76d read-cache: add index.skipHash config option
	    @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf
	      
	      	f = hashfd(tempfile->fd, tempfile->filename.buf);
	      
	    -+	git_config_get_maybe_bool("index.skiphash", (int *)&f->skip_hash);
	    ++	repo_config_get_maybe_bool(istate->repo, "index.skiphash", (int *)&f->skip_hash);
	     +
	      	for (i = removed = extended = 0; i < entries; i++) {
	      		if (cache[i]->ce_flags & CE_REMOVE)
	3:  86370af1351 = 3:  35188bbcfb5 test-lib-functions: add helper for trailing hash
	4:  6490bd445eb ! 4:  4354328e8fc features: feature.manyFiles implies fast index writes
	    @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf
	      
	      	f = hashfd(tempfile->fd, tempfile->filename.buf);
	      
	    --	git_config_get_maybe_bool("index.skiphash", (int *)&f->skip_hash);
	    +-	repo_config_get_maybe_bool(istate->repo, "index.skiphash", (int *)&f->skip_hash);
	     +	if (istate->repo) {
	    ++		int ours, theirs;
	    ++
	     +		prepare_repo_settings(istate->repo);
	    -+		f->skip_hash = istate->repo->settings.index_skip_hash;
	    ++
	    ++		ours = istate->repo->settings.index_skip_hash;
	    ++		theirs = the_repository->settings.index_skip_hash;
	    ++
	    ++		if (ours != theirs)
	    ++			BUG("ours %d theirs %d", ours, theirs);
	    ++		f->skip_hash = ours;
	    ++	} else {
	    ++		f->skip_hash = the_repository->settings.index_skip_hash;
	    ++		/*BUG("no repo???");*/
	     +	}
	      
	      	for (i = removed = extended = 0; i < entries; i++) {

With those all of your tests still pass. Which leads to these outstanding questions:

a) If we're doing to use "istate->repo" in 4/4 why not do so in 2/4,
   neither patch discusses *that* part of the change.

   On an unrelated topic there's this[2] fix-up of yours, 2/2 still
   seems like it needs the same treatment.

b) In 2/4 we'd get the config if "istate->repo" was NULL, if you
   uncomment that BUG() in the squashed 4/4 we'll get test failures all
   over the place. Aren't these places where we'd get the config before,
   but istate->repo isn't set up properly, so with 4/4 we won't get it?

   Maybe that's desired, but again, neither commit discusses this
   change.

Now, I *think* it's a good idea to defer to the already set-up 'istate->repo', but I also know (and have some local patches to fix...) about the cases where we don't set it up (but should), so it can't be fully relied on.

So even if we can't produce a behavior difference, just doing e.g.:

	struct repository *r = istate->repo ? istate->repo : the_repository;

And then using:

	prepare_repo_settings(r);
        f->skip_hash = r->->settings.index_skip_hash;

Seems sensible. I just don't get why 4/4 has that seeming fix-up of 2/4 after-the-fact. Isn't it better to carve that bit out of 4/4, just do the config setup in repo-settings.c to begin with, and have 4/4 do what its $subject says, i.e. to have "feature.manyFiles" imply this new config?

The rest of this all looks sensible to me, sans some isolated things I'll comment on on individual patches.

1. https://lore.kernel.org/git/221212.86v8mg4gr2.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/857d1abec4cf124e011c7f84276ce105cb5b3a96.1670866407.git.gitgitgadget@gmail.com/