Re: [PATCH v2 5/6] cache API: add a "INDEX_STATE_INIT" macro/function, add release_index()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 12, 2023, 16:19 UTC
Message-ID: <230112.86zganzpwk.gmgdl@evledraar.gmail.com>
In-Reply-To: <270ef93c-cfdc-d119-5740-f704d7f2a029@github.com>

On Thu, Jan 12 2023, Derrick Stolee wrote:

Show 18 quoted lines

> On 1/12/2023 7:55 AM, Ævar Arnfjörð Bjarmason wrote:
>> As we'll see in a subsequent commit we'll get advantages from always
>> initializing the "repo" member of the "struct index_state". To make
>> that easier let's introduce an initialization macro & function.
>> 
>> The various ad-hoc initialization of the structure can then be changed
>> over to it, and we can remove the various "0" assignments in
>> discard_index() in favor of calling index_state_init() at the end.
>
>> -	memset(&o->result, 0, sizeof(o->result));
>> +	index_state_init(&o->result);
>>  	o->result.initialized = 1;
>
> It's interesting that 'struct index_state' has an 'initialized'
> member that we aren't setting in index_state_init(). Perhaps it's
> only being used in special cases like this, and means something
> more specific than "index_state_init() was run"? Or maybe we
> could add it to INDEX_STATE_INIT and drop this line?

It's unrelated, and doing that would be a bug. It's a bit unfortunately named, a better name might be "read_index_data" or something.

It was added in 913e0e99b6a (unpack_trees(): protect the handcrafted in-core index from read_cache(), 2008-08-23), which shows the use-case, i.e. it's for avoiding re-reading the index file itself (or in that case, to trust our hand-crafted faked-up version of it).

I opted not to mention it in the commit message, after being sufficiently convinced that it was unrelated, which was probably a mistake :)

Just as a sanity check, we do have really good test coverage of the difference, at least 1/2 of the tests I bothered to wait for failed when I tried this on top:

diff --git a/cache.h b/cache.h
index 4bf14e0bd94..1f8e5f4e823 100644
--- a/cache.h
+++ b/cache.h
@@ -371,6 +371,7 @@ struct index_state {
  * "r" argument to index_state_init() in that case.
  */
 #define INDEX_STATE_INIT(r) { \
+	.initialized = 1, \
 	.repo = (r), \
 }
 void index_state_init(struct index_state *istate, struct repository *r);