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

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 16, 2024, 05:27 UTC
Message-ID: <Zh4MOkyOrM8kYeiK@tanuki>
In-Reply-To: <xmqq8r1eo6pz.fsf@gitster.g>

On Mon, Apr 15, 2024 at 10:50:48AM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > 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 am afraid that it would take a much larger effort.
> 
> I have a suspicion that many of the users of the_index do not have
> to even need that the index_state they work on is connected to any
> instance of a repository object (in other words, I tend to think
> that the value of having a pointer to an index_state in an instance
> of a repository structure is dubious), so in a sense, this rewrite
> of code that use the_index to use the_repository may be going in a
> wrong direction.

I wouldn't say it's going in a wrong direction. It simply makes explicit what already is implicit and thus simplifies our ability to reason about the code.

At least it does that for me -- if others disagree then I will rethink my approach. But the third patch demonstrates that the current approach also causes weird inconsistencies in how we track the state of both `the_index` and `the_repository` in tandem.

Show 6 quoted lines

> In other words, these functions may eventually want to take a pointer
> to an index_state structure as their parameters, without having to
> deal with the whole repository structure, but this rewrite assumes
> they would eventually want to all work with a repository structure
> when the_repository dependency is lifted.  I'll need to see the
> codepaths involved more carefully and think about it.

Yes, that's something that we will have to do eventually. I simply think that the easiest way to approach this is bottom-up by moving the level at which we inject the index one level up at a time. And with every level that we move up we should figure out the intent of the code so that we can decide whether it should use a local copy, the index of the repository or something else.

Patrick