Re: [GSOC PATCH] environment: move access to "core.sparsecheckout" into repo_settings
Show 11 quoted lines
> Le 3 juin 2025 à 12:21, Ayush Chandekar <ayu.chandekar@gmail.com> a écrit :
>
>
>> The config is called "core.sparseCheckout", so why is the variable
>> called `core_apply_sparse_checkout`? `core_sparse_checkout` I would've
>> understood, but where does "apply" come from? Also, for brevity I think
>> we could just call it `settings.sparse_checkout`.
> Yes, I had this thought as well that adding "apply" doesn't make a lot of sense.
> But I thought since the global variable has this name for a long time, there
> must have been some reason. I can change the name if the "apply" doesn't hold
> any value.
Perhaps "git log -S core_apply_sparse_checkout config.c" or similar will reveal a reason? Or point us at a patch series that has some discussion?
Show 12 quoted lines
>
>>> +{
>>> + return repo->settings.core_apply_sparse_checkout;
>>> +}
>>> +
>>> +void repo_settings_set_apply_sparse_checkout(struct repository *repo, int value)
>>> +{
>>> + repo->settings.core_apply_sparse_checkout = value;
>>> +}
>> Getters and setters only really help in the case where they actually
>> provide a benefit. These don't though, so it's dubious whether we should
>> have them.My thoughts exactly; see below.
Show 9 quoted lines
>> Also, shouldn't these functions call `prepare_repo_settings()`?
>> Otherwise we cannot guarantee that those settings have already been
>> parsed at all. And for the setter it could happen that the settings get
>> overwritten by the next caller of `prepare_repo_settings()`.
>
> Oh, yeah, you're right. So, if we use `prepare_repo_settings()` in
> them, wouldn't
> it be better to use getter and setter functions? Otherwise, I'd have to call
> `prepare_repo_settings()` everywhere I'm using the setting.
Aren’t most of the consumers builtins? And from a recent look, don’t they (all?) initialize the repo settings? I agree it is relatively painful to require developers to make sure that prepare_repo_settings has been called on each (new) code path that reads this variable, but OTOH I would expect that to be a straightforward audit during this change and then (see following) relatively easy to catch going forward. Is already a code convention that reading things in repo->settings depends on having prepared them?
Ben Knoble <ben.knoble@gmail.com>