Re: [PATCH 0/5] global: drop external `the_index` variable

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 15, 2024, 14:15 UTC
Message-ID: <Zh02ZpS8x9H7FpHJ@tanuki>
In-Reply-To: <28541f00-9054-4aa0-8e47-348043b5f863@gmail.com>

On Mon, Apr 15, 2024 at 02:55:07PM +0100, Phillip Wood wrote:

Show 16 quoted lines

> Hi Patrick
> 
> On 15/04/2024 12:42, Patrick Steinhardt wrote:
> > The motivation of these patches comes from the introduction of the ref
> > format extension. I found it really hard to reason about the state of
> > `the_repository` and would like to continue its deprecation in favor of
> > explicitly passing down a `struct repository *`. Getting rid of
> > `the_index` is a first easy step into that direction to make things more
> > self-contained.
> 
> Maybe I've got the wrong end of the stick but my impression is that it is
> the use of "the_repository" in library code (i.e. the files outside
> builtin/) that causes most of the pain. With that in mind would be we better
> focusing contributor and reviewer effort on eliminating "the_repository"
> from those files instead? It would need to be done in carefully in stages
> but would bring real benefits.

I see these two things as orthogonal. Overall, the target is to have as few global state as possible. This requires us to over time remove both `the_repository`, but also `the_index`. Removing the latter is a ton easier to achieve than removing the former, and so I took the easier step first.

Whatever we do, we have to do things gradually anyway and thus we have to start somewhere. I've already got more things in the pipeline, where the next step for me is to touch up "refs.h" so that we always pass in the `struct ref_store *`. And here we will be introducing more uses of `the_repository`, as well. But I think this is okay, and by working from the bottom up we will over time get to a state where the lower levels of Git don't require globals anymore, whereas higher levels like e.g. the builtins still do use them.

Eventually, we can then introduce a macro `USE_THE_REPOSITORY_VARIABLE`, similar to `USE_THE_INDEX_VARIABLE` that I have just removed in this patch series.

Patrick