From: Derrick Stolee <stolee@gmail.com>
Date: Thu, 24 Jul 2025 13:25:34 GMT
Subject: Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
Message-ID: <43aaec10-2696-44c9-8728-2045b83dc5d3@gmail.com>
In-Reply-To: <xmqqcy9qlfm8.fsf@gitster.g>

On 7/23/25 6:14 PM, Junio C Hamano wrote:
> Ayush Chandekar <ayu.chandekar@gmail.com> writes:
> 
>> This patch series aims to remove global variables related to
>> sparse-checkout from the global scope and to remove the definition
>> '#define USE_THE_REPOSITORY_VARIABLE' from a few files.

Sorry that I missed early versions of this thread. It's an
interesting topic to me, but I've been distracted.

>> Discussions since v5:
>>
>> * For 1/3 and 2/3, Junio told me that it was concerning to put so
>>    many calls to `prepare_repo_settings()` so I tried to minimize the
>>    calls and made sure that there's no useless calling.
> 
> I didn't mean that the number of places is the problem.  What I
> found troubling was that this is not done in any central place, so
> it is hard to notice even if some random cmd_foo() failed to call
> the function before doing its real work.  For example, shouldn't we
> be able to, at least for built-in commands that have RUN_SETUP bit
> set, centrally call prepare_repo_settings() somewhere late in
> git.c:run_builtin() after we figure out what should be in
> the_repository?  Now historically, setting up a repository may never
> have involved opening and parsing tons of configuration files, so
> such a change may be incurring extra overhead we did not have to
> pay, so it needs a lot more thought than just trying to minimize the
> number of calls, but some performance measurement.

I think that the core issue here (and probably causing the issues
that were seen in the user-facing issues) is that the repo settings
struct was intended as a place to fill config for some one-off
"feature flags" and not to replace core functionality for a repo.

There are two ways to change the approach here to fix the problem
of needing prepare_repo_settings() everyhwere:

  1. With the idea that these sparse-checkout variables are
     critical to the functionality of the repo, they should move
     into the repository struct itself and be initialized along
     with all other values there. This changes the patches (and my
     follow-up series) significantly, but mechanically.

  2. If we are going to change the intention of the repo settings
     struct to move from "optional one-off feature flags" to
     "important information about the core behavior of a repo"
     then we should prepare_repo_settings() when initializing the
     repository struct.

My preference is (1). The only argument for (2) that I can think
of is that it is sometimes helpful to share only the settings for
a repo without sharing the whole repo. But that seems like a weak
reason right now.

>> * For 3/3, Phillip told me that it broke user-facing as it will be
>>    parsed quite late in the callchain and might throw an error mid
>>    operation which we do not want.
> 
> So has the behaviour change caused by 3/3 been resolved?

>   * This throws everything in repo_settings, but these settings are
>     inherently per repository and they are meaningful only when you
>     are working with a repository.  What makes us choose to make them
>     new members in the repo_settings structure, not direct members in
>     the repository structure?

(This is the same thought I expressed earlier in this message.)

Thanks,
-Stolee