Re: [PATCH 5/5] treewide: always have a valid "index_state.repo" member

From: Derrick Stolee <derrickstolee@github.com>
Date: Jan 10, 2023, 15:08 UTC
Message-ID: <6b92fad2-6b74-fddb-679c-8c8735e7103d@github.com>
In-Reply-To: <patch-5.5-b3b0e592101-20230110T060340Z-avarab@gmail.com>

On 1/10/2023 1:17 AM, Ævar Arnfjörð Bjarmason wrote:

Show 7 quoted lines

> When the "repo" member was added to "the_index" in [1] the
> repo_read_index() was made to populate it, but the unpopulated
> "the_index" variable didn't get the same treatment.
> 
> Let's do that in initialize_the_repository() when we set it up, and
> likewise for all of the current callers initialized an empty "struct
> index_state".

> +	struct index_state result = { .repo = state->repo };

> +	struct index_state wtindex = { .repo = the_repository };

> +	o.result.repo = r;

> +	struct index_state istate = { .repo = the_repository };

I think these initialization updates (along with the others I didn't include) are satisfactory for now. What worries me is that future consumers that create an index_state will need to remember to manually initialize like this or risk hitting the BUG() statements in read-cache.c.

The only alternative I can think about is to create an initialization method, say "init_index_state(struct index_state *, struct repository *)", that should be called before doing anything with an index_state. This includes running a memset() to clear the struct, making these inline initializers unnecessary.

However, I can't decide if that's actually an improvement. I think things tip in favor of the init_index_state() method if there ever becomes another member of struct index_state that _needs_ to be set before the struct is "valid". I doubt that we would add such a thing in the near future, so I recommend sticking with this patch as-is.

Thanks, -Stolee