From: Derrick Stolee <derrickstolee@github.com>
Date: Mon, 09 Jan 2023 18:00:22 GMT
Subject: Re: [PATCH v4 0/4] Optionally skip hashing index on write
Message-ID: <8bb310ff-5dd1-1470-18ac-68b81d2f488c@github.com>
In-Reply-To: <230109.86mt6r1v6d.gmgdl@evledraar.gmail.com>

On 1/9/2023 12:15 PM, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Jan 06 2023, Derrick Stolee wrote:
>> On 1/6/2023 5:45 PM, Junio C Hamano wrote:

>>> We probably should start paying down such technical debts.  We've
>>> punted on them too many times, saying "yes this is klunky but what
>>> we have is good enough for adding this feature", I suspect?
>>
>> Yes, I'll make note to prioritize this soon.
> 
> I noted in passing in [1] that I had those patches locally, if I'm
> understanding you correctly and you're planning to work on changes
> that'll make "istate->repo" always non-NULL.
> 
> I've rebased those on top of your "ds/omit-trailing-hash-in-index". I'm
> CI-ing those now & hoping to submit them soon (I've had it working for a
> while, but there was some interaction with your patches). Preview at
> [2].
> 
> 1. https://lore.kernel.org/git/221215.865yec3b1j.gmgdl@evledraar.gmail.com/
> 2. https://github.com/avar/git/compare/avar-ds/omit-trailing-hash-in-index...avar/do-not-lazy-populate-istate-repo

Thanks for doing this so I don't need to.

Some quick pre-submission feedback: your "treewide" patch [1] is far
too big and doing too much all at once. It's difficult to know why
you're doing things when you're doing them, especially the choices
for the validate_cache_repo() calls.

In particular, I noticed that you used "the_repository" in some cases
where a local "struct repository *" would be better (even if it is
currently pointing to the_repository as in builtin/sparse-checkout.c
in update_working_directory()). These would be easier to catch in
smaller diffs.

[1] https://github.com/avar/git/commit/7732a41800dfc8f5dbf909560615d6048d583ed9

Looking forward to your series.

-Stolee