On Wed, Jun 4, 2025 at 7:50 AM Ben Knoble <ben.knoble@xxxxxxxxx> wrote:
>
>
> > Le 3 juin 2025 à 12:21, Ayush Chandekar <ayu.chandekar@xxxxxxxxx> 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?
>
I ran the command to see previous commits and also went through the
patch that introduced that variable, but there was no reasoning for
why it was named that way.
There was no comment as well for the patch that introduced it. I think
we can also get rid of the "core" (so it just becomes
"sparse_checkout") as there exist other core settings which don't have
"core" in their variable names and since we are changing the name
anyways.
> >
> >>> +{
> >>> + 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.
>
> >> 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?
Yeah, as Patrick said, we have to call the `prepare_repo_settings()`
before we access these settings.
So, either we manually call the function or use it inside a
getter/setter function.
Thanks
