On 24/07/2025 14:25, Derrick Stolee wrote:
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@xxxxxxxxx/
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
