Re: [PATCH v2 0/6] cache API: always have a "istate->repo"

[Derrick Stolee <derrickstolee@github.com>]

On 1/12/2023 7:55 AM, Ævar Arnfjörð Bjarmason wrote:

>  * Derrick suggested in
>    https://lore.kernel.org/git/6b92fad2-6b74-fddb-679c-8c8735e7103d@github.com/
>    that things might be nicer if we had an explicit initializer, which
>    was also the subject of the previous discussion at
>    https://lore.kernel.org/git/xmqqmt6vqo2w.fsf@gitster.g/; but
>    concluded that it was probably best to leave it for now.
> 
>    I tried it out, and I think it's worth just doing that now, which
>    is why there's a new 5/6 here: We start by adding an
>    INDEX_STATE_INIT macro, and corresponding function.
> 
>    There's a bit of churn in 6/6 as all of those now will take a
>    "repo" argument, but I think the end result is worth it, because
>    even if "repo" remains the only thing that we need to initialize
>    we're now able to use ALLOC_ARRAY() instead of CALLOC_ARRAY().
> 
>    We'll thus be helped by analysis tools (which would show access to
>    un-init'd memory) if we miss properly init-ing not just the "repo"
>    field, but anything in the structure, so our test coverage will be
>    better.
> 
>    It also makes the code easier to follow and change, as it's now
>    more obvious where we initialize this, and it'll be easier to
>    change it in the future if e.g. we add a new member that has
>    mandatory initialization (e.g. a "struct strbuf").

I wasn't expecting the initializer idea to work as well as it has.
Now, Patch 5 does all the heavy lifting and Patch 6 is an easy read.

Outside of one question about the istate->initialized member (which
might not need any change) I'm happy with this version.

Thanks,
-Stolee