Re: [PATCH v3] cache-tree.c: remove implicit dependency on the_repository

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 5, 2021, 17:48 UTC
Message-ID: <xmqqv99062sr.fsf@gitster.g>
In-Reply-To: <be9793f0-f437-8e42-d463-21a48d2ee948@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 9 quoted lines

> With your additional comments, I think it is clear that the "fourth
> option" I mentioned earlier [1] is the way to go:
>
>   Finally, there is yet a fourth option: use istate->repo instead. In
>   1fd9ae51 (repository: add repo reference to index_state), I added a
>   'repo' member to struct index_state. This is intended for methods to
>   access a repository directly from the index.
>
> [1] https://lore.kernel.org/git/f187df01-8e59-ac74-01e1-586a7a63fd4e@gmail.com/

Thanks. I wasn't following the earlier discussion closely, as the topic seemed to be in the hands of good reviewers ;-)

Show 9 quoted lines

> So in this sense, we should always use istate->repo, but we might
> still need the following guard in some places:
>
> 	if (!istate->repo)
> 		istate->repo = the_repository;
>
> in case there are situations where the index is loaded before
> the_repository is loaded. I have hit this in testing, but don't fully
> understand the cases where this can happen.

As a longer term goal, it may not be a bad idea to make sure that anybody who passes an istate should not have to pass a repo. I do not think of a reason why, other than historical inertia, to do so in post 1fd9ae51 world.

> The way it would change this patch is to drop the 'struct repository *r'
> pointers and changes to the method signatures. Instead, keep the
> methods only taking a 'struct index_state *istate' and use istate->repo
> everywhere.

Yes, and that would result in a patch that touches very limited small parts of cache-tree.c without needing to touch any caller, I would presume.

Thanks.