On 7/23/25 6:14 PM, Junio C Hamano wrote:
Ayush Chandekar <ayu.chandekar@xxxxxxxxx> 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
