Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 30, 2025, 08:53 UTC
Message-ID: <d61c966b-61ae-4ba9-b983-c8dab6e2c292@gmail.com>
In-Reply-To: <43aaec10-2696-44c9-8728-2045b83dc5d3@gmail.com>

On 24/07/2025 14:25, Derrick Stolee wrote:

Show 14 quoted lines

> 
> 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.

Patrick and I had a discussion about calling prepare_repo_settings() from repo_read_config() recently [1]. It turned out that does not work but I wonder if instead we could change git_default_config() to expect a repository pointer as the callback data and use that to initialize things. That would mean that we would not need to move code out of git_default_config() to remove global variables and we would retain the "last one wins" behavior when two or more config keys such are "merge.log" and "merge.summary" set the same variable. It would be fairly invasive though as we'd need to pass the repository pointer down through all the other callbacks that end up calling git_default_config().

Thanks

Phillip

[1] https://lore.kernel.org/git/f6479d6a-32a4-4a49-a75c-589978cb9a57@gmail.com/

Show 29 quoted lines

>   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
> 
>