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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, vdye@github.com, newren@gmail.com,
	Jacob Keller <jacob.keller@gmail.com>
Subject: Re: [PATCH v4 0/4] Optionally skip hashing index on write
Date: Mon, 09 Jan 2023 20:22:11 +0100	[thread overview]
Message-ID: <230109.86a62r1p38.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <8bb310ff-5dd1-1470-18ac-68b81d2f488c@github.com>

On Mon, Jan 09 2023, Derrick Stolee wrote:

> 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.

Yeah, I don't like it either :)

I'm mulling over whether to keep it at all. I.e. the fix to pass the
data is relatively small, but most of that patch is validation paranoia.

It was helpful to write that to validate it for my own sanity, but maybe
it's not worth keeping it.

Ultimately it's just making things BUG(...)  during testing that would
otherwise be a NULL-pointer dereference, so I'm currently leaning
towards (especially with this early feedback) just skipping it.

Now that we have a SANITIZE=address CI job the error feedback if/when
that would happen is arguably worse than the BUG(...) output.

> 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.

Yes, definitely. I was careful to use an existing "struct repository *"
in favor of "the_repository", but clearly not careful enough!

I'll fix that, and look carefully over the rest before submission in
case I've missed other similar cases.

> [1] https://github.com/avar/git/commit/7732a41800dfc8f5dbf909560615d6048d583ed9
>
> Looking forward to your series.

Thanks, since I sent the above CI passed, so it'll be sooner than later,
but I'll massage it a bit per the above before submission.