From: Patrick Steinhardt <ps@pks•im>
To: Ben Knoble <ben.knoble@gmail•com>
Cc: Ayush Chandekar <ayu.chandekar@gmail•com>,
git@vger•kernel.org, christian.couder@gmail•com,
shyamthakkar001@gmail•com
Subject: Re: [GSOC PATCH] environment: move access to "core.sparsecheckout" into repo_settings
Date: Wed, 4 Jun 2025 09:28:52 +0200 [thread overview]
Message-ID: <aD_1tD-H74SOh1Xx@pks.im> (raw)
In-Reply-To: <4F074544-4E25-472D-A42C-C50A1E1CAC69@gmail.com>
On Tue, Jun 03, 2025 at 10:20:38PM -0400, Ben Knoble wrote:
> > Le 3 juin 2025 à 12:21, Ayush Chandekar <ayu.chandekar@gmail•com> 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
next prev parent reply other threads:[~2025-06-04 7:28 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-03 13:18 [GSOC PATCH] environment: move access to "core.sparsecheckout" into repo_settings Ayush Chandekar
2025-06-03 13:41 ` Patrick Steinhardt
2025-06-03 16:20 ` Ayush Chandekar
2025-06-04 2:20 ` Ben Knoble
2025-06-04 7:28 ` Patrick Steinhardt [this message]
2025-06-04 23:48 ` Ayush Chandekar
2025-06-08 0:31 ` [GSOC PATCH v2] " Ayush Chandekar
2025-06-08 6:39 ` Christian Couder
2025-06-11 17:34 ` [PATCH v3] " Ayush Chandekar
2025-06-11 21:06 ` Junio C Hamano
2025-06-11 21:33 ` Junio C Hamano
2025-06-13 6:57 ` Ayush Chandekar
2025-06-17 12:06 ` [GSOC PATCH v4 0/3] environment: remove sparse-checkout related global variables Ayush Chandekar
2025-06-17 12:06 ` [GSOC PATCH v4 1/3] environment: move access to "core.sparsecheckout" into repo_settings Ayush Chandekar
2025-06-17 16:15 ` Junio C Hamano
2025-06-17 12:06 ` [GSOC PATCH v4 2/3] environment: move access to "core.sparsecheckoutcone" " Ayush Chandekar
2025-06-17 16:29 ` Junio C Hamano
2025-06-17 12:06 ` [GSOC PATCH v4 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns' Ayush Chandekar
2025-06-17 16:30 ` Junio C Hamano
2025-06-30 19:27 ` [GSOC PATCH v5 0/3] environment: remove sparse-checkout related global variables Ayush Chandekar
2025-06-30 19:27 ` [GSOC PATCH v5 1/3] environment: move access to "core.sparsecheckout" into repo_settings Ayush Chandekar
2025-06-30 19:27 ` [GSOC PATCH v5 2/3] environment: move access to "core.sparsecheckoutcone" " Ayush Chandekar
2025-06-30 19:27 ` [GSOC PATCH v5 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns' Ayush Chandekar
2025-07-01 13:18 ` Phillip Wood
2025-07-01 23:53 ` Ayush Chandekar
2025-07-02 9:01 ` Phillip Wood
2025-07-11 19:24 ` Ayush Chandekar
2025-07-02 9:21 ` Junio C Hamano
2025-07-11 19:35 ` Ayush Chandekar
2025-06-30 21:08 ` [GSOC PATCH v5 0/3] environment: remove sparse-checkout related global variables Junio C Hamano
2025-07-09 0:18 ` Junio C Hamano
2025-07-09 1:39 ` Ayush Chandekar
2025-07-19 0:11 ` [GSOC PATCH v6 " Ayush Chandekar
2025-07-19 0:11 ` [GSOC PATCH v6 1/3] environment: move access to "core.sparsecheckout" into repo_settings Ayush Chandekar
2025-07-19 0:11 ` [GSOC PATCH v6 2/3] environment: move access to "core.sparsecheckoutcone" " Ayush Chandekar
2025-07-19 0:11 ` [GSOC PATCH v6 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns' Ayush Chandekar
2025-07-23 22:14 ` [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables Junio C Hamano
2025-07-24 13:25 ` Derrick Stolee
2025-07-24 18:44 ` Junio C Hamano
2025-07-29 11:36 ` Ayush Chandekar
2025-07-29 12:19 ` Derrick Stolee
2025-07-29 12:53 ` Ayush Chandekar
2025-07-30 8:53 ` Phillip Wood
2025-07-30 15:52 ` Junio C Hamano
2025-07-26 23:55 ` Ayush Chandekar
2025-08-10 15:36 ` Ayush Chandekar
2025-08-26 12:20 ` Derrick Stolee
2025-08-27 21:31 ` Ayush Chandekar
2025-09-05 14:15 ` Junio C Hamano
2025-09-05 17:10 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aD_1tD-H74SOh1Xx@pks.im \
--to=ps@pks$(echo .)im \
--cc=ayu.chandekar@gmail$(echo .)com \
--cc=ben.knoble@gmail$(echo .)com \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=shyamthakkar001@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox