On Tue, Jun 03, 2025 at 10:20:38PM -0400, Ben Knoble wrote:
> > Le 3 juin 2025 à 12:21, Ayush Chandekar <ayu.chandekar@xxxxxxxxx> a écrit :
> >>> +{
> >>> + 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?
Yes, it is a code convention. We have two patterns though:
- Those that access the repo settings fields directly _always_ call
`prepare_repo_settings()` manually beforehand.
- Those that use a getter/setter rely on those to call
`prepare_repo_settings()`.
So if you add the call to `prepare_repo_settings()` the getter and
setter do provide additional value. So in that case it may be sensible
to retain them indeed.
Patrick
