Re: ds/omit-trailing-hash-in-index (was: What's cooking in git.git (Dec 2022, #06; Sun, 18))

[Derrick Stolee <derrickstolee@github.com>]

On 12/19/22 5:49 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Dec 18 2022, Junio C Hamano wrote:
> 
>> * ds/omit-trailing-hash-in-index (2022-12-17) 4 commits
>>  - features: feature.manyFiles implies fast index writes
>>  - test-lib-functions: add helper for trailing hash
>>  - read-cache: add index.skipHash config option
>>  - hashfile: allow skipping the hash function
>>
>>  Introduce an optional configuration to allow the trailing hash that
>>  protects the index file from bit flipping.
>>
>>  Will merge to 'next'?
>>  source: <pull.1439.v4.git.1671204678.gitgitgadget@gmail.com>
> 
> I've been following this closely & reviewing it. I think the end-state
> is probably good, but noted in [1] that the intermediate progression
> equates bad config with "true", so:
> 
> 	git -c index.skipHash=blahblah status
> 
> Enables it, fixing that is trivial, and probably worth a re-roll.

Fixing it is trivial, but as you say it is correct in the final
version so I don't think this triviality is worth a re-roll.

> The "probably" above is then because the patches seemingly try to make
> this compatible with different config for submodules, but there's no
> tests for submodule interaction, so that may or may not work.
> 
> Normally we could just trust the "struct repository *" parameter we get,
> but in this case it's "istate->repo", which (as I showed in the v3
> feedback[2]) is sometimes NULL.

This, and other feedback you've given around 'struct repository *' values
makes me think you are not aware of the current state of the submodule
work. I'm also a bit out-of-date, but my understanding is that the
conversion to stop using the_repository everywhere is only halfway
complete, which makes it difficult to properly test things with multiple
'struct repository *' pointers. However, the best thing we can do is to
use whatever local repository we have, so we don't contribute further to
the problem.

The fact that istate->repo is sometimes NULL is a separate issue, but is
generally unrelated to the subject at hand and should be fixed by another
topic, not used to block this one.

Thanks,
-Stolee