* Re: [GSOC PATCH] environment: move access to "core.sparsecheckout" into repo_settings
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-08 0:31 ` [GSOC PATCH v2] " Ayush Chandekar
` (4 subsequent siblings)
5 siblings, 1 reply; 50+ messages in thread
From: Patrick Steinhardt @ 2025-06-03 13:41 UTC (permalink / raw)
To: Ayush Chandekar; +Cc: git, christian.couder, shyamthakkar001
On Tue, Jun 03, 2025 at 06:48:06PM +0530, Ayush Chandekar wrote:
> diff --git a/repo-settings.c b/repo-settings.c
> index 4129f8fb2b..406c70601c 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -81,6 +81,7 @@ void prepare_repo_settings(struct repository *r)
> &r->settings.pack_use_bitmap_boundary_traversal,
> r->settings.pack_use_bitmap_boundary_traversal);
> repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
> + repo_cfg_bool(r, "core.sparsecheckout", &r->settings.core_apply_sparse_checkout, 0);
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`.
> @@ -227,3 +228,13 @@ void repo_settings_reset_shared_repository(struct repository *repo)
> {
> repo->settings.shared_repository_initialized = 0;
> }
> +
> +int repo_settings_get_apply_sparse_checkout(struct repository *repo)
Same remark here -- where does the "apply" part come from?
> +{
> + 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.
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()`.
Patrick
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH] environment: move access to "core.sparsecheckout" into repo_settings
2025-06-03 13:41 ` Patrick Steinhardt
@ 2025-06-03 16:20 ` Ayush Chandekar
2025-06-04 2:20 ` Ben Knoble
0 siblings, 1 reply; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-03 16:20 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, christian.couder, shyamthakkar001
> 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.
>
> > +{
> > + 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.
>
> 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.
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH] environment: move access to "core.sparsecheckout" into repo_settings
2025-06-03 16:20 ` Ayush Chandekar
@ 2025-06-04 2:20 ` Ben Knoble
2025-06-04 7:28 ` Patrick Steinhardt
2025-06-04 23:48 ` Ayush Chandekar
0 siblings, 2 replies; 50+ messages in thread
From: Ben Knoble @ 2025-06-04 2:20 UTC (permalink / raw)
To: Ayush Chandekar
Cc: Patrick Steinhardt, git, christian.couder, shyamthakkar001
> 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?
>
>>> +{
>>> + 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?
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH] environment: move access to "core.sparsecheckout" into repo_settings
2025-06-04 2:20 ` Ben Knoble
@ 2025-06-04 7:28 ` Patrick Steinhardt
2025-06-04 23:48 ` Ayush Chandekar
1 sibling, 0 replies; 50+ messages in thread
From: Patrick Steinhardt @ 2025-06-04 7:28 UTC (permalink / raw)
To: Ben Knoble; +Cc: Ayush Chandekar, git, christian.couder, shyamthakkar001
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
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH] environment: move access to "core.sparsecheckout" into repo_settings
2025-06-04 2:20 ` Ben Knoble
2025-06-04 7:28 ` Patrick Steinhardt
@ 2025-06-04 23:48 ` Ayush Chandekar
1 sibling, 0 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-04 23:48 UTC (permalink / raw)
To: Ben Knoble; +Cc: Patrick Steinhardt, git, christian.couder, shyamthakkar001
On Wed, Jun 4, 2025 at 7:50 AM Ben Knoble <ben.knoble@gmail.com> wrote:
>
>
> > 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?
>
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* [GSOC PATCH v2] environment: move access to "core.sparsecheckout" into repo_settings
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-08 0:31 ` Ayush Chandekar
2025-06-08 6:39 ` Christian Couder
2025-06-11 17:34 ` [PATCH v3] " Ayush Chandekar
` (3 subsequent siblings)
5 siblings, 1 reply; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-08 0:31 UTC (permalink / raw)
To: ayu.chandekar; +Cc: christian.couder, git, shyamthakkar001, ben.knoble, ps
The setting "core.sparsecheckout" is stored in the global
`core_apply_sparse_checkout` and is populated in config.c. Refactor the
code to store it in the variable `sparse_checkout` in the struct
`repo_settings`. Also, create functions to set and get the value of the
setting and update all the occurrences.
This also allows us to remove the definition `#define
USE_THE_REPOSITORY_VARIABLE` from the file 'builtin/backfill.c'.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
builtin/backfill.c | 5 +----
builtin/clone.c | 2 +-
builtin/grep.c | 2 +-
builtin/mv.c | 2 +-
builtin/sparse-checkout.c | 20 ++++++++++----------
builtin/worktree.c | 2 +-
config.c | 5 -----
dir.c | 2 +-
environment.c | 1 -
environment.h | 1 -
repo-settings.c | 13 +++++++++++++
repo-settings.h | 5 +++++
sparse-index.c | 4 ++--
unpack-trees.c | 2 +-
wt-status.c | 2 +-
15 files changed, 38 insertions(+), 30 deletions(-)
diff --git a/builtin/backfill.c b/builtin/backfill.c
index fa82ad2f6f..e16d116e98 100644
--- a/builtin/backfill.c
+++ b/builtin/backfill.c
@@ -1,6 +1,3 @@
-/* We need this macro to access core_apply_sparse_checkout */
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "builtin.h"
#include "git-compat-util.h"
#include "config.h"
@@ -139,7 +136,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
repo_config(repo, git_default_config, NULL);
if (ctx.sparse < 0)
- ctx.sparse = core_apply_sparse_checkout;
+ ctx.sparse = repo_settings_get_sparse_checkout(repo);
result = do_backfill(&ctx);
backfill_context_clear(&ctx);
diff --git a/builtin/clone.c b/builtin/clone.c
index 91b9cd0d16..ba25e58258 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -621,7 +621,7 @@ static int git_sparse_checkout_init(const char *repo)
* We must apply the setting in the current process
* for the later checkout to use the sparse-checkout file.
*/
- core_apply_sparse_checkout = 1;
+ repo_settings_set_sparse_checkout(the_repository, 1);
cmd.git_cmd = 1;
if (run_command(&cmd)) {
diff --git a/builtin/grep.c b/builtin/grep.c
index 3ce574a605..63342f5e36 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -481,7 +481,7 @@ static int grep_submodule(struct grep_opt *opt,
* "forget" the sparse-index feature switch. As a result, the index
* of these submodules are expanded unexpectedly.
*
- * 2. "core_apply_sparse_checkout"
+ * 2. "sparse_checkout"
* When running `grep` in the superproject, this setting is
* populated using the superproject's configs. However, once
* initialized, this config is globally accessible and is read by
diff --git a/builtin/mv.c b/builtin/mv.c
index 07548fe96a..95d24843d9 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -572,7 +572,7 @@ int cmd_mv(int argc,
rename_index_entry_at(the_repository->index, pos, dst);
if (ignore_sparse &&
- core_apply_sparse_checkout &&
+ repo_settings_get_sparse_checkout(the_repository) &&
core_sparse_checkout_cone) {
/*
* NEEDSWORK: we are *not* paying attention to
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 1bf01591b2..662858b0a8 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -62,7 +62,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
int res;
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!repo_settings_get_sparse_checkout(the_repository))
die(_("this worktree is not sparse"));
argc = parse_options(argc, argv, prefix,
@@ -397,11 +397,11 @@ static int set_config(enum sparse_checkout_mode mode)
static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {
/* If not specified, use previous definition of cone mode */
- if (*cone_mode == -1 && core_apply_sparse_checkout)
+ if (*cone_mode == -1 && repo_settings_get_sparse_checkout(the_repository))
*cone_mode = core_sparse_checkout_cone;
/* Set cone/non-cone mode appropriately */
- core_apply_sparse_checkout = 1;
+ repo_settings_set_sparse_checkout(the_repository, 1);
if (*cone_mode == 1 || *cone_mode == -1) {
core_sparse_checkout_cone = 1;
return MODE_CONE_PATTERNS;
@@ -415,7 +415,7 @@ static int update_modes(int *cone_mode, int *sparse_index)
int mode, record_mode;
/* Determine if we need to record the mode; ensure sparse checkout on */
- record_mode = (*cone_mode != -1) || !core_apply_sparse_checkout;
+ record_mode = (*cone_mode != -1) || !repo_settings_get_sparse_checkout(the_repository);
mode = update_cone_mode(cone_mode);
if (record_mode && set_config(mode))
@@ -695,9 +695,9 @@ static int modify_pattern_list(struct strvec *args, int use_stdin,
break;
}
- if (!core_apply_sparse_checkout) {
+ if (!repo_settings_get_sparse_checkout(the_repository)) {
set_config(MODE_ALL_PATTERNS);
- core_apply_sparse_checkout = 1;
+ repo_settings_set_sparse_checkout(the_repository, 1);
changed_config = 1;
}
@@ -793,7 +793,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix,
int ret;
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!repo_settings_get_sparse_checkout(the_repository))
die(_("no sparse-checkout to add to"));
repo_read_index(the_repository);
@@ -902,7 +902,7 @@ static int sparse_checkout_reapply(int argc, const char **argv,
};
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!repo_settings_get_sparse_checkout(the_repository))
die(_("must be in a sparse-checkout to reapply sparsity patterns"));
reapply_opts.cone_mode = -1;
@@ -935,7 +935,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
struct pattern_list pl;
/*
- * We do not exit early if !core_apply_sparse_checkout; due to the
+ * We do not exit early if !sparse_checkout; due to the
* ability for users to manually muck things up between
* direct editing of .git/info/sparse-checkout
* running read-tree -m u HEAD or update-index --skip-worktree
@@ -961,7 +961,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
pl.use_cone_patterns = 0;
- core_apply_sparse_checkout = 1;
+ repo_settings_set_sparse_checkout(the_repository, 1);
add_pattern("/*", empty_base, 0, &pl, 0);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 88a36ea9f8..590bec334f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -536,7 +536,7 @@ static int add_worktree(const char *path, const char *refname,
* If the current worktree has sparse-checkout enabled, then copy
* the sparse-checkout patterns from the current worktree.
*/
- if (core_apply_sparse_checkout)
+ if (repo_settings_get_sparse_checkout(the_repository))
copy_sparse_checkout(sb_repo.buf);
/*
diff --git a/config.c b/config.c
index b18b5617fc..8fd4dd8c81 100644
--- a/config.c
+++ b/config.c
@@ -1612,11 +1612,6 @@ static int git_default_core_config(const char *var, const char *value,
return 0;
}
- if (!strcmp(var, "core.sparsecheckout")) {
- core_apply_sparse_checkout = git_config_bool(var, value);
- return 0;
- }
-
if (!strcmp(var, "core.sparsecheckoutcone")) {
core_sparse_checkout_cone = git_config_bool(var, value);
return 0;
diff --git a/dir.c b/dir.c
index a374972b62..323017e7a5 100644
--- a/dir.c
+++ b/dir.c
@@ -1503,7 +1503,7 @@ enum pattern_match_result path_matches_pattern_list(
int init_sparse_checkout_patterns(struct index_state *istate)
{
- if (!core_apply_sparse_checkout)
+ if (!repo_settings_get_sparse_checkout(the_repository))
return 1;
if (istate->sparse_checkout_patterns)
return 0;
diff --git a/environment.c b/environment.c
index c61d773e7e..a379a9149e 100644
--- a/environment.c
+++ b/environment.c
@@ -64,7 +64,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#endif
enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
-int core_apply_sparse_checkout;
int core_sparse_checkout_cone;
int sparse_expect_files_outside_of_patterns;
int merge_log_config = -1;
diff --git a/environment.h b/environment.h
index 3d98461a06..6a30512f3c 100644
--- a/environment.h
+++ b/environment.h
@@ -160,7 +160,6 @@ extern int precomposed_unicode;
extern int protect_hfs;
extern int protect_ntfs;
-extern int core_apply_sparse_checkout;
extern int core_sparse_checkout_cone;
extern int sparse_expect_files_outside_of_patterns;
diff --git a/repo-settings.c b/repo-settings.c
index 4129f8fb2b..e11201e25b 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -81,6 +81,7 @@ void prepare_repo_settings(struct repository *r)
&r->settings.pack_use_bitmap_boundary_traversal,
r->settings.pack_use_bitmap_boundary_traversal);
repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
+ repo_cfg_bool(r, "core.sparsecheckout", &r->settings.sparse_checkout, 0);
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
@@ -227,3 +228,15 @@ void repo_settings_reset_shared_repository(struct repository *repo)
{
repo->settings.shared_repository_initialized = 0;
}
+
+int repo_settings_get_sparse_checkout(struct repository *repo)
+{
+ prepare_repo_settings(repo);
+ return repo->settings.sparse_checkout;
+}
+
+void repo_settings_set_sparse_checkout(struct repository *repo, int value)
+{
+ prepare_repo_settings(repo);
+ repo->settings.sparse_checkout = value;
+}
diff --git a/repo-settings.h b/repo-settings.h
index 2bf24b2597..0d181e3f21 100644
--- a/repo-settings.h
+++ b/repo-settings.h
@@ -67,6 +67,7 @@ struct repo_settings {
unsigned long big_file_threshold;
char *hooks_path;
+ int sparse_checkout;
};
#define REPO_SETTINGS_INIT { \
.shared_repository = -1, \
@@ -98,4 +99,8 @@ int repo_settings_get_shared_repository(struct repository *repo);
void repo_settings_set_shared_repository(struct repository *repo, int value);
void repo_settings_reset_shared_repository(struct repository *repo);
+/* Read or set the value for "core.sparseCheckout". */
+int repo_settings_get_sparse_checkout(struct repository *repo);
+void repo_settings_set_sparse_checkout(struct repository *repo, int value);
+
#endif /* REPO_SETTINGS_H */
diff --git a/sparse-index.c b/sparse-index.c
index 5634abafaa..1bedb35001 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -150,7 +150,7 @@ static int index_has_unmerged_entries(struct index_state *istate)
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
- if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
+ if (!repo_settings_get_sparse_checkout(istate->repo) || !core_sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
@@ -668,7 +668,7 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista
void clear_skip_worktree_from_present_files(struct index_state *istate)
{
- if (!core_apply_sparse_checkout ||
+ if (!repo_settings_get_sparse_checkout(istate->repo) ||
sparse_expect_files_outside_of_patterns)
return;
diff --git a/unpack-trees.c b/unpack-trees.c
index 471837f032..c8eba27e5f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1924,7 +1924,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
if (o->prefix)
update_sparsity_for_prefix(o->prefix, o->src_index);
- if (!core_apply_sparse_checkout || !o->update)
+ if (!repo_settings_get_sparse_checkout(repo) || !o->update)
o->skip_sparse_checkout = 1;
if (!o->skip_sparse_checkout) {
memset(&pl, 0, sizeof(pl));
diff --git a/wt-status.c b/wt-status.c
index 454601afa1..004f2945b7 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1773,7 +1773,7 @@ static void wt_status_check_sparse_checkout(struct repository *r,
int skip_worktree = 0;
int i;
- if (!core_apply_sparse_checkout || r->index->cache_nr == 0) {
+ if (!repo_settings_get_sparse_checkout(r) || r->index->cache_nr == 0) {
/*
* Don't compute percentage of checked out files if we
* aren't in a sparse checkout or would get division by 0.
--
2.49.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v2] environment: move access to "core.sparsecheckout" into repo_settings
2025-06-08 0:31 ` [GSOC PATCH v2] " Ayush Chandekar
@ 2025-06-08 6:39 ` Christian Couder
0 siblings, 0 replies; 50+ messages in thread
From: Christian Couder @ 2025-06-08 6:39 UTC (permalink / raw)
To: Ayush Chandekar; +Cc: git, shyamthakkar001, ben.knoble, ps
On Sun, Jun 8, 2025 at 2:32 AM Ayush Chandekar <ayu.chandekar@gmail.com> wrote:
>
> The setting "core.sparsecheckout" is stored in the global
> `core_apply_sparse_checkout` and is populated in config.c. Refactor the
> code to store it in the variable `sparse_checkout` in the struct
> `repo_settings`. Also, create functions to set and get the value of the
> setting and update all the occurrences.
>
> This also allows us to remove the definition `#define
> USE_THE_REPOSITORY_VARIABLE` from the file 'builtin/backfill.c'.
>
> This change is part of an ongoing effort to eliminate global variables,
> improve modularity and help libify the codebase.
>
> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> ---
When sending a v2 like this one, it's nice if you can describe what
changed since v1 here, after the line starting with 3 dashes "---".
Providing a range-diff here, when it makes sense, could be a good idea
and help reviewers too. Speaking of reviewers, thanking or just
mentioning them is nice too while at it.
> builtin/backfill.c | 5 +----
> builtin/clone.c | 2 +-
> builtin/grep.c | 2 +-
> builtin/mv.c | 2 +-
> builtin/sparse-checkout.c | 20 ++++++++++----------
> builtin/worktree.c | 2 +-
> config.c | 5 -----
> dir.c | 2 +-
> environment.c | 1 -
> environment.h | 1 -
> repo-settings.c | 13 +++++++++++++
> repo-settings.h | 5 +++++
> sparse-index.c | 4 ++--
> unpack-trees.c | 2 +-
> wt-status.c | 2 +-
> 15 files changed, 38 insertions(+), 30 deletions(-)
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v3] environment: move access to "core.sparsecheckout" into repo_settings
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-08 0:31 ` [GSOC PATCH v2] " Ayush Chandekar
@ 2025-06-11 17:34 ` Ayush Chandekar
2025-06-11 21:06 ` Junio C Hamano
2025-06-17 12:06 ` [GSOC PATCH v4 0/3] environment: remove sparse-checkout related global variables Ayush Chandekar
` (2 subsequent siblings)
5 siblings, 1 reply; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-11 17:34 UTC (permalink / raw)
To: ayu.chandekar; +Cc: christian.couder, git, shyamthakkar001, ps, ben.knoble
The setting "core.sparsecheckout" is stored in the global
`core_apply_sparse_checkout` and is populated in config.c. Refactor the
code to store it in the variable `sparse_checkout` in the struct
`repo_settings`. Also, create functions to set and get the value of the
setting and update all the occurrences.
This also allows us to remove the definition `#define
USE_THE_REPOSITORY_VARIABLE` from the file 'builtin/backfill.c'.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
Note that this patch is the same as v2, the v3 just contains the range-diff
of v2/v3 vs v1, and the 'Mentored-by' trailers.
I thank my mentors Christian and Ghanshyam, as well as Patrick and Ben,
for reviewing my patch and suggesting changes.
Summary of the range-diff:
* Change the variable name from 'core_apply_sparse_checkout' to 'sparse_checkout'.
* Update the getter and setter function names to use 'sparse_checkout' instead of 'apply_sparse_checkout'.
* Call the function 'prepare_repo_settings()' in the getter/setter functions of the variable.
Range-diff:
1: a385fc1dbe ! 1: c49dcde03b environment: move access to "core.sparsecheckout" into repo_settings
@@ Commit message
The setting "core.sparsecheckout" is stored in the global
`core_apply_sparse_checkout` and is populated in config.c. Refactor the
- code to store it inside the struct `repo_settings`. Also, create
- functions to set and get the value of the setting and update all the
- occurrences.
+ code to store it in the variable `sparse_checkout` in the struct
+ `repo_settings`. Also, create functions to set and get the value of the
+ setting and update all the occurrences.
This also allows us to remove the definition `#define
USE_THE_REPOSITORY_VARIABLE` from the file 'builtin/backfill.c'.
@@ Commit message
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
+ Mentored-by: Christian Couder <christian.couder@gmail.com>
+ Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
## builtin/backfill.c ##
@@ builtin/backfill.c: int cmd_backfill(int argc, const char **argv, const char *pr
if (ctx.sparse < 0)
- ctx.sparse = core_apply_sparse_checkout;
-+ ctx.sparse = repo_settings_get_apply_sparse_checkout(repo);
++ ctx.sparse = repo_settings_get_sparse_checkout(repo);
result = do_backfill(&ctx);
backfill_context_clear(&ctx);
@@ builtin/clone.c: static int git_sparse_checkout_init(const char *repo)
* for the later checkout to use the sparse-checkout file.
*/
- core_apply_sparse_checkout = 1;
-+ repo_settings_set_apply_sparse_checkout(the_repository, 1);
++ repo_settings_set_sparse_checkout(the_repository, 1);
cmd.git_cmd = 1;
if (run_command(&cmd)) {
+ ## builtin/grep.c ##
+@@ builtin/grep.c: static int grep_submodule(struct grep_opt *opt,
+ * "forget" the sparse-index feature switch. As a result, the index
+ * of these submodules are expanded unexpectedly.
+ *
+- * 2. "core_apply_sparse_checkout"
++ * 2. "sparse_checkout"
+ * When running `grep` in the superproject, this setting is
+ * populated using the superproject's configs. However, once
+ * initialized, this config is globally accessible and is read by
+
## builtin/mv.c ##
@@ builtin/mv.c: int cmd_mv(int argc,
rename_index_entry_at(the_repository->index, pos, dst);
if (ignore_sparse &&
- core_apply_sparse_checkout &&
-+ repo_settings_get_apply_sparse_checkout(the_repository) &&
++ repo_settings_get_sparse_checkout(the_repository) &&
core_sparse_checkout_cone) {
/*
* NEEDSWORK: we are *not* paying attention to
@@ builtin/sparse-checkout.c: static int sparse_checkout_list(int argc, const char
setup_work_tree();
- if (!core_apply_sparse_checkout)
-+ if (!repo_settings_get_apply_sparse_checkout(the_repository))
++ if (!repo_settings_get_sparse_checkout(the_repository))
die(_("this worktree is not sparse"));
argc = parse_options(argc, argv, prefix,
@@ builtin/sparse-checkout.c: static int set_config(enum sparse_checkout_mode mode)
static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {
/* If not specified, use previous definition of cone mode */
- if (*cone_mode == -1 && core_apply_sparse_checkout)
-+ if (*cone_mode == -1 && repo_settings_get_apply_sparse_checkout(the_repository))
++ if (*cone_mode == -1 && repo_settings_get_sparse_checkout(the_repository))
*cone_mode = core_sparse_checkout_cone;
/* Set cone/non-cone mode appropriately */
- core_apply_sparse_checkout = 1;
-+ repo_settings_set_apply_sparse_checkout(the_repository, 1);
++ repo_settings_set_sparse_checkout(the_repository, 1);
if (*cone_mode == 1 || *cone_mode == -1) {
core_sparse_checkout_cone = 1;
return MODE_CONE_PATTERNS;
@@ builtin/sparse-checkout.c: static int update_modes(int *cone_mode, int *sparse_i
/* Determine if we need to record the mode; ensure sparse checkout on */
- record_mode = (*cone_mode != -1) || !core_apply_sparse_checkout;
-+ record_mode = (*cone_mode != -1) || !repo_settings_get_apply_sparse_checkout(the_repository);
++ record_mode = (*cone_mode != -1) || !repo_settings_get_sparse_checkout(the_repository);
mode = update_cone_mode(cone_mode);
if (record_mode && set_config(mode))
@@ builtin/sparse-checkout.c: static int modify_pattern_list(struct strvec *args, i
}
- if (!core_apply_sparse_checkout) {
-+ if (!repo_settings_get_apply_sparse_checkout(the_repository)) {
++ if (!repo_settings_get_sparse_checkout(the_repository)) {
set_config(MODE_ALL_PATTERNS);
- core_apply_sparse_checkout = 1;
-+ repo_settings_set_apply_sparse_checkout(the_repository, 1);
++ repo_settings_set_sparse_checkout(the_repository, 1);
changed_config = 1;
}
@@ builtin/sparse-checkout.c: static int sparse_checkout_add(int argc, const char *
setup_work_tree();
- if (!core_apply_sparse_checkout)
-+ if (!repo_settings_get_apply_sparse_checkout(the_repository))
++ if (!repo_settings_get_sparse_checkout(the_repository))
die(_("no sparse-checkout to add to"));
repo_read_index(the_repository);
@@ builtin/sparse-checkout.c: static int sparse_checkout_reapply(int argc, const ch
setup_work_tree();
- if (!core_apply_sparse_checkout)
-+ if (!repo_settings_get_apply_sparse_checkout(the_repository))
++ if (!repo_settings_get_sparse_checkout(the_repository))
die(_("must be in a sparse-checkout to reapply sparsity patterns"));
reapply_opts.cone_mode = -1;
+@@ builtin/sparse-checkout.c: static int sparse_checkout_disable(int argc, const char **argv,
+ struct pattern_list pl;
+
+ /*
+- * We do not exit early if !core_apply_sparse_checkout; due to the
++ * We do not exit early if !sparse_checkout; due to the
+ * ability for users to manually muck things up between
+ * direct editing of .git/info/sparse-checkout
+ * running read-tree -m u HEAD or update-index --skip-worktree
@@ builtin/sparse-checkout.c: static int sparse_checkout_disable(int argc, const char **argv,
hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
pl.use_cone_patterns = 0;
- core_apply_sparse_checkout = 1;
-+ repo_settings_set_apply_sparse_checkout(the_repository, 1);
++ repo_settings_set_sparse_checkout(the_repository, 1);
add_pattern("/*", empty_base, 0, &pl, 0);
@@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
* the sparse-checkout patterns from the current worktree.
*/
- if (core_apply_sparse_checkout)
-+ if (repo_settings_get_apply_sparse_checkout(the_repository))
++ if (repo_settings_get_sparse_checkout(the_repository))
copy_sparse_checkout(sb_repo.buf);
/*
@@ dir.c: enum pattern_match_result path_matches_pattern_list(
int init_sparse_checkout_patterns(struct index_state *istate)
{
- if (!core_apply_sparse_checkout)
-+ if (!repo_settings_get_apply_sparse_checkout(the_repository))
++ if (!repo_settings_get_sparse_checkout(the_repository))
return 1;
if (istate->sparse_checkout_patterns)
return 0;
@@ repo-settings.c: void prepare_repo_settings(struct repository *r)
&r->settings.pack_use_bitmap_boundary_traversal,
r->settings.pack_use_bitmap_boundary_traversal);
repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
-+ repo_cfg_bool(r, "core.sparsecheckout", &r->settings.core_apply_sparse_checkout, 0);
++ repo_cfg_bool(r, "core.sparsecheckout", &r->settings.sparse_checkout, 0);
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
@@ repo-settings.c: void repo_settings_reset_shared_repository(struct repository *r
repo->settings.shared_repository_initialized = 0;
}
+
-+int repo_settings_get_apply_sparse_checkout(struct repository *repo)
++int repo_settings_get_sparse_checkout(struct repository *repo)
+{
-+ return repo->settings.core_apply_sparse_checkout;
++ prepare_repo_settings(repo);
++ return repo->settings.sparse_checkout;
+}
+
-+void repo_settings_set_apply_sparse_checkout(struct repository *repo, int value)
++void repo_settings_set_sparse_checkout(struct repository *repo, int value)
+{
-+ repo->settings.core_apply_sparse_checkout = value;
++ prepare_repo_settings(repo);
++ repo->settings.sparse_checkout = value;
+}
## repo-settings.h ##
@@ repo-settings.h: struct repo_settings {
unsigned long big_file_threshold;
char *hooks_path;
-+ int core_apply_sparse_checkout;
++ int sparse_checkout;
};
#define REPO_SETTINGS_INIT { \
.shared_repository = -1, \
@@ repo-settings.h: int repo_settings_get_shared_repository(struct repository *repo
void repo_settings_set_shared_repository(struct repository *repo, int value);
void repo_settings_reset_shared_repository(struct repository *repo);
-+/* Read, set or reset the value for "core.sparsecheckout". */
-+int repo_settings_get_apply_sparse_checkout(struct repository *repo);
-+void repo_settings_set_apply_sparse_checkout(struct repository *repo, int value);
++/* Read or set the value for "core.sparseCheckout". */
++int repo_settings_get_sparse_checkout(struct repository *repo);
++void repo_settings_set_sparse_checkout(struct repository *repo, int value);
+
#endif /* REPO_SETTINGS_H */
@@ sparse-index.c: static int index_has_unmerged_entries(struct index_state *istate
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
- if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
-+ if (!repo_settings_get_apply_sparse_checkout(istate->repo) || !core_sparse_checkout_cone)
++ if (!repo_settings_get_sparse_checkout(istate->repo) || !core_sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
@@ sparse-index.c: static void clear_skip_worktree_from_present_files_full(struct i
void clear_skip_worktree_from_present_files(struct index_state *istate)
{
- if (!core_apply_sparse_checkout ||
-+ if (!repo_settings_get_apply_sparse_checkout(istate->repo) ||
++ if (!repo_settings_get_sparse_checkout(istate->repo) ||
sparse_expect_files_outside_of_patterns)
return;
@@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpac
update_sparsity_for_prefix(o->prefix, o->src_index);
- if (!core_apply_sparse_checkout || !o->update)
-+ if (!repo_settings_get_apply_sparse_checkout(repo) || !o->update)
++ if (!repo_settings_get_sparse_checkout(repo) || !o->update)
o->skip_sparse_checkout = 1;
if (!o->skip_sparse_checkout) {
memset(&pl, 0, sizeof(pl));
@@ wt-status.c: static void wt_status_check_sparse_checkout(struct repository *r,
int i;
- if (!core_apply_sparse_checkout || r->index->cache_nr == 0) {
-+ if (!repo_settings_get_apply_sparse_checkout(r) || r->index->cache_nr == 0) {
++ if (!repo_settings_get_sparse_checkout(r) || r->index->cache_nr == 0) {
/*
* Don't compute percentage of checked out files if we
* aren't in a sparse checkout or would get division by 0.
builtin/backfill.c | 5 +----
builtin/clone.c | 2 +-
builtin/grep.c | 2 +-
builtin/mv.c | 2 +-
builtin/sparse-checkout.c | 20 ++++++++++----------
builtin/worktree.c | 2 +-
config.c | 5 -----
dir.c | 2 +-
environment.c | 1 -
environment.h | 1 -
repo-settings.c | 13 +++++++++++++
repo-settings.h | 5 +++++
sparse-index.c | 4 ++--
unpack-trees.c | 2 +-
wt-status.c | 2 +-
15 files changed, 38 insertions(+), 30 deletions(-)
diff --git a/builtin/backfill.c b/builtin/backfill.c
index fa82ad2f6f..e16d116e98 100644
--- a/builtin/backfill.c
+++ b/builtin/backfill.c
@@ -1,6 +1,3 @@
-/* We need this macro to access core_apply_sparse_checkout */
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "builtin.h"
#include "git-compat-util.h"
#include "config.h"
@@ -139,7 +136,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
repo_config(repo, git_default_config, NULL);
if (ctx.sparse < 0)
- ctx.sparse = core_apply_sparse_checkout;
+ ctx.sparse = repo_settings_get_sparse_checkout(repo);
result = do_backfill(&ctx);
backfill_context_clear(&ctx);
diff --git a/builtin/clone.c b/builtin/clone.c
index 91b9cd0d16..ba25e58258 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -621,7 +621,7 @@ static int git_sparse_checkout_init(const char *repo)
* We must apply the setting in the current process
* for the later checkout to use the sparse-checkout file.
*/
- core_apply_sparse_checkout = 1;
+ repo_settings_set_sparse_checkout(the_repository, 1);
cmd.git_cmd = 1;
if (run_command(&cmd)) {
diff --git a/builtin/grep.c b/builtin/grep.c
index 3ce574a605..63342f5e36 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -481,7 +481,7 @@ static int grep_submodule(struct grep_opt *opt,
* "forget" the sparse-index feature switch. As a result, the index
* of these submodules are expanded unexpectedly.
*
- * 2. "core_apply_sparse_checkout"
+ * 2. "sparse_checkout"
* When running `grep` in the superproject, this setting is
* populated using the superproject's configs. However, once
* initialized, this config is globally accessible and is read by
diff --git a/builtin/mv.c b/builtin/mv.c
index 07548fe96a..95d24843d9 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -572,7 +572,7 @@ int cmd_mv(int argc,
rename_index_entry_at(the_repository->index, pos, dst);
if (ignore_sparse &&
- core_apply_sparse_checkout &&
+ repo_settings_get_sparse_checkout(the_repository) &&
core_sparse_checkout_cone) {
/*
* NEEDSWORK: we are *not* paying attention to
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 1bf01591b2..662858b0a8 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -62,7 +62,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
int res;
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!repo_settings_get_sparse_checkout(the_repository))
die(_("this worktree is not sparse"));
argc = parse_options(argc, argv, prefix,
@@ -397,11 +397,11 @@ static int set_config(enum sparse_checkout_mode mode)
static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {
/* If not specified, use previous definition of cone mode */
- if (*cone_mode == -1 && core_apply_sparse_checkout)
+ if (*cone_mode == -1 && repo_settings_get_sparse_checkout(the_repository))
*cone_mode = core_sparse_checkout_cone;
/* Set cone/non-cone mode appropriately */
- core_apply_sparse_checkout = 1;
+ repo_settings_set_sparse_checkout(the_repository, 1);
if (*cone_mode == 1 || *cone_mode == -1) {
core_sparse_checkout_cone = 1;
return MODE_CONE_PATTERNS;
@@ -415,7 +415,7 @@ static int update_modes(int *cone_mode, int *sparse_index)
int mode, record_mode;
/* Determine if we need to record the mode; ensure sparse checkout on */
- record_mode = (*cone_mode != -1) || !core_apply_sparse_checkout;
+ record_mode = (*cone_mode != -1) || !repo_settings_get_sparse_checkout(the_repository);
mode = update_cone_mode(cone_mode);
if (record_mode && set_config(mode))
@@ -695,9 +695,9 @@ static int modify_pattern_list(struct strvec *args, int use_stdin,
break;
}
- if (!core_apply_sparse_checkout) {
+ if (!repo_settings_get_sparse_checkout(the_repository)) {
set_config(MODE_ALL_PATTERNS);
- core_apply_sparse_checkout = 1;
+ repo_settings_set_sparse_checkout(the_repository, 1);
changed_config = 1;
}
@@ -793,7 +793,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix,
int ret;
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!repo_settings_get_sparse_checkout(the_repository))
die(_("no sparse-checkout to add to"));
repo_read_index(the_repository);
@@ -902,7 +902,7 @@ static int sparse_checkout_reapply(int argc, const char **argv,
};
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!repo_settings_get_sparse_checkout(the_repository))
die(_("must be in a sparse-checkout to reapply sparsity patterns"));
reapply_opts.cone_mode = -1;
@@ -935,7 +935,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
struct pattern_list pl;
/*
- * We do not exit early if !core_apply_sparse_checkout; due to the
+ * We do not exit early if !sparse_checkout; due to the
* ability for users to manually muck things up between
* direct editing of .git/info/sparse-checkout
* running read-tree -m u HEAD or update-index --skip-worktree
@@ -961,7 +961,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
pl.use_cone_patterns = 0;
- core_apply_sparse_checkout = 1;
+ repo_settings_set_sparse_checkout(the_repository, 1);
add_pattern("/*", empty_base, 0, &pl, 0);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 88a36ea9f8..590bec334f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -536,7 +536,7 @@ static int add_worktree(const char *path, const char *refname,
* If the current worktree has sparse-checkout enabled, then copy
* the sparse-checkout patterns from the current worktree.
*/
- if (core_apply_sparse_checkout)
+ if (repo_settings_get_sparse_checkout(the_repository))
copy_sparse_checkout(sb_repo.buf);
/*
diff --git a/config.c b/config.c
index b18b5617fc..8fd4dd8c81 100644
--- a/config.c
+++ b/config.c
@@ -1612,11 +1612,6 @@ static int git_default_core_config(const char *var, const char *value,
return 0;
}
- if (!strcmp(var, "core.sparsecheckout")) {
- core_apply_sparse_checkout = git_config_bool(var, value);
- return 0;
- }
-
if (!strcmp(var, "core.sparsecheckoutcone")) {
core_sparse_checkout_cone = git_config_bool(var, value);
return 0;
diff --git a/dir.c b/dir.c
index a374972b62..323017e7a5 100644
--- a/dir.c
+++ b/dir.c
@@ -1503,7 +1503,7 @@ enum pattern_match_result path_matches_pattern_list(
int init_sparse_checkout_patterns(struct index_state *istate)
{
- if (!core_apply_sparse_checkout)
+ if (!repo_settings_get_sparse_checkout(the_repository))
return 1;
if (istate->sparse_checkout_patterns)
return 0;
diff --git a/environment.c b/environment.c
index c61d773e7e..a379a9149e 100644
--- a/environment.c
+++ b/environment.c
@@ -64,7 +64,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#endif
enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
-int core_apply_sparse_checkout;
int core_sparse_checkout_cone;
int sparse_expect_files_outside_of_patterns;
int merge_log_config = -1;
diff --git a/environment.h b/environment.h
index 3d98461a06..6a30512f3c 100644
--- a/environment.h
+++ b/environment.h
@@ -160,7 +160,6 @@ extern int precomposed_unicode;
extern int protect_hfs;
extern int protect_ntfs;
-extern int core_apply_sparse_checkout;
extern int core_sparse_checkout_cone;
extern int sparse_expect_files_outside_of_patterns;
diff --git a/repo-settings.c b/repo-settings.c
index 4129f8fb2b..e11201e25b 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -81,6 +81,7 @@ void prepare_repo_settings(struct repository *r)
&r->settings.pack_use_bitmap_boundary_traversal,
r->settings.pack_use_bitmap_boundary_traversal);
repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
+ repo_cfg_bool(r, "core.sparsecheckout", &r->settings.sparse_checkout, 0);
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
@@ -227,3 +228,15 @@ void repo_settings_reset_shared_repository(struct repository *repo)
{
repo->settings.shared_repository_initialized = 0;
}
+
+int repo_settings_get_sparse_checkout(struct repository *repo)
+{
+ prepare_repo_settings(repo);
+ return repo->settings.sparse_checkout;
+}
+
+void repo_settings_set_sparse_checkout(struct repository *repo, int value)
+{
+ prepare_repo_settings(repo);
+ repo->settings.sparse_checkout = value;
+}
diff --git a/repo-settings.h b/repo-settings.h
index 2bf24b2597..0d181e3f21 100644
--- a/repo-settings.h
+++ b/repo-settings.h
@@ -67,6 +67,7 @@ struct repo_settings {
unsigned long big_file_threshold;
char *hooks_path;
+ int sparse_checkout;
};
#define REPO_SETTINGS_INIT { \
.shared_repository = -1, \
@@ -98,4 +99,8 @@ int repo_settings_get_shared_repository(struct repository *repo);
void repo_settings_set_shared_repository(struct repository *repo, int value);
void repo_settings_reset_shared_repository(struct repository *repo);
+/* Read or set the value for "core.sparseCheckout". */
+int repo_settings_get_sparse_checkout(struct repository *repo);
+void repo_settings_set_sparse_checkout(struct repository *repo, int value);
+
#endif /* REPO_SETTINGS_H */
diff --git a/sparse-index.c b/sparse-index.c
index 5634abafaa..1bedb35001 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -150,7 +150,7 @@ static int index_has_unmerged_entries(struct index_state *istate)
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
- if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
+ if (!repo_settings_get_sparse_checkout(istate->repo) || !core_sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
@@ -668,7 +668,7 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista
void clear_skip_worktree_from_present_files(struct index_state *istate)
{
- if (!core_apply_sparse_checkout ||
+ if (!repo_settings_get_sparse_checkout(istate->repo) ||
sparse_expect_files_outside_of_patterns)
return;
diff --git a/unpack-trees.c b/unpack-trees.c
index 471837f032..c8eba27e5f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1924,7 +1924,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
if (o->prefix)
update_sparsity_for_prefix(o->prefix, o->src_index);
- if (!core_apply_sparse_checkout || !o->update)
+ if (!repo_settings_get_sparse_checkout(repo) || !o->update)
o->skip_sparse_checkout = 1;
if (!o->skip_sparse_checkout) {
memset(&pl, 0, sizeof(pl));
diff --git a/wt-status.c b/wt-status.c
index 454601afa1..004f2945b7 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1773,7 +1773,7 @@ static void wt_status_check_sparse_checkout(struct repository *r,
int skip_worktree = 0;
int i;
- if (!core_apply_sparse_checkout || r->index->cache_nr == 0) {
+ if (!repo_settings_get_sparse_checkout(r) || r->index->cache_nr == 0) {
/*
* Don't compute percentage of checked out files if we
* aren't in a sparse checkout or would get division by 0.
--
2.49.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v3] environment: move access to "core.sparsecheckout" into repo_settings
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
0 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2025-06-11 21:06 UTC (permalink / raw)
To: Ayush Chandekar; +Cc: christian.couder, git, shyamthakkar001, ps, ben.knoble
Ayush Chandekar <ayu.chandekar@gmail.com> writes:
> The setting "core.sparsecheckout" is stored in the global
> `core_apply_sparse_checkout` and is populated in config.c. Refactor the
> code to store it in the variable `sparse_checkout` in the struct
> `repo_settings`. Also, create functions to set and get the value of the
> setting and update all the occurrences.
>
> This also allows us to remove the definition `#define
> USE_THE_REPOSITORY_VARIABLE` from the file 'builtin/backfill.c'.
>
> This change is part of an ongoing effort to eliminate global variables,
> improve modularity and help libify the codebase.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> ---
It may make sense to move the sparse-checkout bit from the
process-wide global to per-repository settings. One advantage of
having it at the global level is that the code that does not require
to be in a repository can refer to it, but the nature of this variable
requires a repository to make sense anyway, so that advantage does
not apply to it.
But looking at members of repo.settings, among 25+ of them, there
are only a handful of variables with getter-setter pair, and this
patch seems to add yet another for a simple boolean. Among existing
setters and getters, prevailing pattern seem to be
/* getter */
if (!repo->settings.foo)
repo_cfg_ulong(repo, "core.foo", &repo->settings.foo, FOO_DEFAULT);
return repo->settings.foo;
/* setter */
repo->settings.foo = value;
but there are more involved ones.
This new one adopts the most expensive pattern from
get_warn_ambiguous_refs() where prepare_repo_settings() is used, as
if it fears to be fed a repo instance that hasn't been prepared yet,
and it even does so for its setter, which I think is probably a
mistake.
And there are tons of members in repo-settings that are initialized
in prepare_repo_settings() (perhaps reading from the configuration
file or assigned their default values) and without having an
explicit setter/getter pair. If the code paths that depends on the
value of these members are functioning correctly (and they should,
given how widely Git is used these days and not having heard about
bugs coming from these variables), would not it mean that in the
program start up sequence, prepare_repo_settings() is called early
enough so that all the code paths that care about these repository
settings values do not have to worry about having a getter setter
pair at all?
What is it so special about sparse-checkout bit that cannot be using
the same "initialize in prepare_repo_settings() and access it
directly thereafter" model, or "ah, do you need to know about this
bit? Let me see if I already have figured it out, and otherwise let
me read from the configuration, and give it back to you" model?
My gut feeling, if I have to choose between "lazy loading" and
"popluate in prepare_repo_settings() and then access the member
directly thereafter" for this variable, I may pick the latter for
this particular variable.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v3] environment: move access to "core.sparsecheckout" into repo_settings
2025-06-11 21:06 ` Junio C Hamano
@ 2025-06-11 21:33 ` Junio C Hamano
2025-06-13 6:57 ` Ayush Chandekar
0 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2025-06-11 21:33 UTC (permalink / raw)
To: Ayush Chandekar; +Cc: christian.couder, git, shyamthakkar001, ps, ben.knoble
Junio C Hamano <gitster@pobox.com> writes:
> My gut feeling, if I have to choose between "lazy loading" and
> "popluate in prepare_repo_settings() and then access the member
> directly thereafter" for this variable, I may pick the latter for
> this particular variable.
I left the reason behind this choice unsaid, but let's spell it out.
Originally, this was read in git_config(git_default_config) into a
global, and that is probably because almost everybody that touches
the working tree files needs to know about it. So populating it in
prepare_repo_settings() for everybody, even though the calling code
path does not even need it, would be OK---they were paying the cost
to read it when they read the default configuration variables.
It seems Patrick earlier made a confused comment on the two models
that may need a bit clarifying.
Here are the rules to follow.
- "lazy loading" is not wrong. Initialize the member to an
"uninitialized" state, never touch the member in
prepare_repo_settings(), and have its getter check for the
"uninitialized" state to lazily load it, or have its setter do
its thing. prepare_repo_settings() should not even be aware of
the member, if we are going to give the member a getter/setter
pair.
- "Without getter/setter" is not wrong, either. Load the member in
prepare_repo_settings(), which will turn into a no-op once it is
called to a repo instance. Use the member directly afterwards.
You cannot mix and match. If the variable is rarely used, you'd
want to catch the initial access and lazily load it, hence you are
required to have a getter/setter pair and lazily populate the member
in your getter. If your variable is very commonly used, load it
once in prepare_repo_settings(), and because you are not going to do
anything special upon the first access, there is no need to have a
getter/setter pair.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v3] environment: move access to "core.sparsecheckout" into repo_settings
2025-06-11 21:33 ` Junio C Hamano
@ 2025-06-13 6:57 ` Ayush Chandekar
0 siblings, 0 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-13 6:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: christian.couder, git, shyamthakkar001, ps, ben.knoble
On Thu, Jun 12, 2025 at 3:03 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > My gut feeling, if I have to choose between "lazy loading" and
> > "popluate in prepare_repo_settings() and then access the member
> > directly thereafter" for this variable, I may pick the latter for
> > this particular variable.
>
> I left the reason behind this choice unsaid, but let's spell it out.
>
> Originally, this was read in git_config(git_default_config) into a
> global, and that is probably because almost everybody that touches
> the working tree files needs to know about it. So populating it in
> prepare_repo_settings() for everybody, even though the calling code
> path does not even need it, would be OK---they were paying the cost
> to read it when they read the default configuration variables.
>
> It seems Patrick earlier made a confused comment on the two models
> that may need a bit clarifying.
>
> Here are the rules to follow.
>
> - "lazy loading" is not wrong. Initialize the member to an
> "uninitialized" state, never touch the member in
> prepare_repo_settings(), and have its getter check for the
> "uninitialized" state to lazily load it, or have its setter do
> its thing. prepare_repo_settings() should not even be aware of
> the member, if we are going to give the member a getter/setter
> pair.
>
> - "Without getter/setter" is not wrong, either. Load the member in
> prepare_repo_settings(), which will turn into a no-op once it is
> called to a repo instance. Use the member directly afterwards.
>
> You cannot mix and match. If the variable is rarely used, you'd
> want to catch the initial access and lazily load it, hence you are
> required to have a getter/setter pair and lazily populate the member
> in your getter. If your variable is very commonly used, load it
> once in prepare_repo_settings(), and because you are not going to do
> anything special upon the first access, there is no need to have a
> getter/setter pair.
>
>
Oh, now I understand it. I will keep this in mind when working on
these settings.
I will send a new version of this patch soon.
Thanks a lot, this helped clarify things.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [GSOC PATCH v4 0/3] environment: remove sparse-checkout related global variables
2025-06-03 13:18 [GSOC PATCH] environment: move access to "core.sparsecheckout" into repo_settings Ayush Chandekar
` (2 preceding siblings ...)
2025-06-11 17:34 ` [PATCH v3] " Ayush Chandekar
@ 2025-06-17 12:06 ` Ayush Chandekar
2025-06-17 12:06 ` [GSOC PATCH v4 1/3] environment: move access to "core.sparsecheckout" into repo_settings Ayush Chandekar
` (2 more replies)
2025-06-30 19:27 ` [GSOC PATCH v5 0/3] environment: remove sparse-checkout related global variables Ayush Chandekar
2025-07-19 0:11 ` [GSOC PATCH v6 " Ayush Chandekar
5 siblings, 3 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-17 12:06 UTC (permalink / raw)
To: ayu.chandekar; +Cc: christian.couder, git, shyamthakkar001, gitster
This patch series aims to remove global variables related to sparse-checkout from the global scope and to remove the definition '#define USE_THE_REPOSITORY_VARIABLE' from a few files.
It contains three patches:
1 - Remove the global variable 'core_apply_sparse_checkout' and move its setting to the 'struct repo_settings'. Also remove the definition '#define USE_THE_REPOSITORY_VARIABLE' from "builtin/backfill.c".
2 - Remove the global variable 'core_sparse_checkout_cone' and move its setting to the 'struct repo_settings'.
3 - Remove the global variable 'sparse_expect_files_outside_of_patterns` and localize it in the function which calls it. Also remove the definition '#define USE_THE_REPOSITORY_VARIABLE' from "sparse-index.c"
As Junio suggested while reviewing v3 patch, I removed the getter and setter functions for the 'core.sparsecheckout' settings. After making this change, I realized the other sparse-checkout-related global variables could be cleaned up as well, leading to the addition of two new patches in this series.
Ayush Chandekar (3):
environment: move access to "core.sparsecheckout" into
repo_settings
environment: move access to "core.sparsecheckoutcone" into
repo_settings
environment: remove the global variable
'sparse_expect_files_outside_of_patterns'
builtin/backfill.c | 7 ++----
builtin/clone.c | 2 +-
builtin/grep.c | 4 ++--
builtin/mv.c | 4 ++--
builtin/sparse-checkout.c | 48 +++++++++++++++++++--------------------
builtin/worktree.c | 2 +-
config.c | 24 --------------------
dir.c | 4 ++--
environment.c | 3 ---
environment.h | 4 ----
repo-settings.c | 2 ++
repo-settings.h | 3 +++
sparse-index.c | 8 ++++---
unpack-trees.c | 2 +-
wt-status.c | 2 +-
15 files changed, 46 insertions(+), 73 deletions(-)
--
Summary of the range-diff:
* Removed the getter and setter functions for the core.sparseCheckout setting. Instead, it is now initialized once in 'prepare_repo_settings()' and accessed directly, since it is commonly used.
* Added two new commits to eliminate the global variables 'core_sparse_checkout_cone' and 'sparse_expect_files_outside_of_patterns'.
Range-diff vs v3:
1: c49dcde03b ! 30: e221c68ab5 environment: move access to "core.sparsecheckout" into repo_settings
@@ Commit message
The setting "core.sparsecheckout" is stored in the global
`core_apply_sparse_checkout` and is populated in config.c. Refactor the
code to store it in the variable `sparse_checkout` in the struct
- `repo_settings`. Also, create functions to set and get the value of the
- setting and update all the occurrences.
+ `repo_settings`.
+ It's fine not to lazily load it from the config, as the variable
+ is used quite commonly.
This also allows us to remove the definition `#define
USE_THE_REPOSITORY_VARIABLE` from the file 'builtin/backfill.c'.
@@ builtin/backfill.c
#include "git-compat-util.h"
#include "config.h"
@@ builtin/backfill.c: int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
- repo_config(repo, git_default_config, NULL);
+ 0);
+ repo_config(repo, git_default_config, NULL);
+-
++ prepare_repo_settings(repo);
if (ctx.sparse < 0)
- ctx.sparse = core_apply_sparse_checkout;
-+ ctx.sparse = repo_settings_get_sparse_checkout(repo);
++ ctx.sparse = repo->settings.sparse_checkout;
result = do_backfill(&ctx);
backfill_context_clear(&ctx);
@@ builtin/clone.c: static int git_sparse_checkout_init(const char *repo)
* for the later checkout to use the sparse-checkout file.
*/
- core_apply_sparse_checkout = 1;
-+ repo_settings_set_sparse_checkout(the_repository, 1);
++ the_repository->settings.sparse_checkout = 1;
cmd.git_cmd = 1;
if (run_command(&cmd)) {
@@ builtin/mv.c: int cmd_mv(int argc,
if (ignore_sparse &&
- core_apply_sparse_checkout &&
-+ repo_settings_get_sparse_checkout(the_repository) &&
++ the_repository->settings.sparse_checkout &&
core_sparse_checkout_cone) {
/*
* NEEDSWORK: we are *not* paying attention to
@@ builtin/sparse-checkout.c: static int sparse_checkout_list(int argc, const char
setup_work_tree();
- if (!core_apply_sparse_checkout)
-+ if (!repo_settings_get_sparse_checkout(the_repository))
++ if (!the_repository->settings.sparse_checkout)
die(_("this worktree is not sparse"));
argc = parse_options(argc, argv, prefix,
@@ builtin/sparse-checkout.c: static int set_config(enum sparse_checkout_mode mode)
static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {
/* If not specified, use previous definition of cone mode */
- if (*cone_mode == -1 && core_apply_sparse_checkout)
-+ if (*cone_mode == -1 && repo_settings_get_sparse_checkout(the_repository))
++ if (*cone_mode == -1 && the_repository->settings.sparse_checkout)
*cone_mode = core_sparse_checkout_cone;
/* Set cone/non-cone mode appropriately */
- core_apply_sparse_checkout = 1;
-+ repo_settings_set_sparse_checkout(the_repository, 1);
++ the_repository->settings.sparse_checkout = 1;
if (*cone_mode == 1 || *cone_mode == -1) {
core_sparse_checkout_cone = 1;
return MODE_CONE_PATTERNS;
@@ builtin/sparse-checkout.c: static int update_modes(int *cone_mode, int *sparse_i
/* Determine if we need to record the mode; ensure sparse checkout on */
- record_mode = (*cone_mode != -1) || !core_apply_sparse_checkout;
-+ record_mode = (*cone_mode != -1) || !repo_settings_get_sparse_checkout(the_repository);
++ record_mode = (*cone_mode != -1) || !the_repository->settings.sparse_checkout;
mode = update_cone_mode(cone_mode);
if (record_mode && set_config(mode))
@@ builtin/sparse-checkout.c: static int modify_pattern_list(struct strvec *args, i
}
- if (!core_apply_sparse_checkout) {
-+ if (!repo_settings_get_sparse_checkout(the_repository)) {
++ if (!the_repository->settings.sparse_checkout) {
set_config(MODE_ALL_PATTERNS);
- core_apply_sparse_checkout = 1;
-+ repo_settings_set_sparse_checkout(the_repository, 1);
++ the_repository->settings.sparse_checkout = 1;
changed_config = 1;
}
@@ builtin/sparse-checkout.c: static int sparse_checkout_add(int argc, const char *
setup_work_tree();
- if (!core_apply_sparse_checkout)
-+ if (!repo_settings_get_sparse_checkout(the_repository))
++ if (!the_repository->settings.sparse_checkout)
die(_("no sparse-checkout to add to"));
repo_read_index(the_repository);
@@ builtin/sparse-checkout.c: static int sparse_checkout_reapply(int argc, const ch
setup_work_tree();
- if (!core_apply_sparse_checkout)
-+ if (!repo_settings_get_sparse_checkout(the_repository))
++ if (!the_repository->settings.sparse_checkout)
die(_("must be in a sparse-checkout to reapply sparsity patterns"));
reapply_opts.cone_mode = -1;
@@ builtin/sparse-checkout.c: static int sparse_checkout_disable(int argc, const ch
hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
pl.use_cone_patterns = 0;
- core_apply_sparse_checkout = 1;
-+ repo_settings_set_sparse_checkout(the_repository, 1);
++ the_repository->settings.sparse_checkout = 1;
add_pattern("/*", empty_base, 0, &pl, 0);
@@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
* the sparse-checkout patterns from the current worktree.
*/
- if (core_apply_sparse_checkout)
-+ if (repo_settings_get_sparse_checkout(the_repository))
++ if (the_repository->settings.sparse_checkout)
copy_sparse_checkout(sb_repo.buf);
/*
@@ dir.c: enum pattern_match_result path_matches_pattern_list(
int init_sparse_checkout_patterns(struct index_state *istate)
{
- if (!core_apply_sparse_checkout)
-+ if (!repo_settings_get_sparse_checkout(the_repository))
++ if (!istate->repo->settings.sparse_checkout)
return 1;
if (istate->sparse_checkout_patterns)
return 0;
@@ repo-settings.c: void prepare_repo_settings(struct repository *r)
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
-@@ repo-settings.c: void repo_settings_reset_shared_repository(struct repository *repo)
- {
- repo->settings.shared_repository_initialized = 0;
- }
-+
-+int repo_settings_get_sparse_checkout(struct repository *repo)
-+{
-+ prepare_repo_settings(repo);
-+ return repo->settings.sparse_checkout;
-+}
-+
-+void repo_settings_set_sparse_checkout(struct repository *repo, int value)
-+{
-+ prepare_repo_settings(repo);
-+ repo->settings.sparse_checkout = value;
-+}
## repo-settings.h ##
@@ repo-settings.h: struct repo_settings {
@@ repo-settings.h: struct repo_settings {
};
#define REPO_SETTINGS_INIT { \
.shared_repository = -1, \
-@@ repo-settings.h: int repo_settings_get_shared_repository(struct repository *repo);
- void repo_settings_set_shared_repository(struct repository *repo, int value);
- void repo_settings_reset_shared_repository(struct repository *repo);
-
-+/* Read or set the value for "core.sparseCheckout". */
-+int repo_settings_get_sparse_checkout(struct repository *repo);
-+void repo_settings_set_sparse_checkout(struct repository *repo, int value);
-+
- #endif /* REPO_SETTINGS_H */
## sparse-index.c ##
@@ sparse-index.c: static int index_has_unmerged_entries(struct index_state *istate)
@@ sparse-index.c: static int index_has_unmerged_entries(struct index_state *istate
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
- if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
-+ if (!repo_settings_get_sparse_checkout(istate->repo) || !core_sparse_checkout_cone)
++ if (!istate->repo->settings.sparse_checkout || !core_sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
@@ sparse-index.c: static void clear_skip_worktree_from_present_files_full(struct i
void clear_skip_worktree_from_present_files(struct index_state *istate)
{
- if (!core_apply_sparse_checkout ||
-+ if (!repo_settings_get_sparse_checkout(istate->repo) ||
++ if (!istate->repo->settings.sparse_checkout ||
sparse_expect_files_outside_of_patterns)
return;
@@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpac
update_sparsity_for_prefix(o->prefix, o->src_index);
- if (!core_apply_sparse_checkout || !o->update)
-+ if (!repo_settings_get_sparse_checkout(repo) || !o->update)
++ if (!repo->settings.sparse_checkout || !o->update)
o->skip_sparse_checkout = 1;
if (!o->skip_sparse_checkout) {
memset(&pl, 0, sizeof(pl));
@@ wt-status.c: static void wt_status_check_sparse_checkout(struct repository *r,
int i;
- if (!core_apply_sparse_checkout || r->index->cache_nr == 0) {
-+ if (!repo_settings_get_sparse_checkout(r) || r->index->cache_nr == 0) {
++ if (!r->settings.sparse_checkout || r->index->cache_nr == 0) {
/*
* Don't compute percentage of checked out files if we
* aren't in a sparse checkout or would get division by 0.
-: ---------- > 31: 9a63884341 environment: move access to "core.sparsecheckoutcone" into repo_settings
-: ---------- > 32: a9e1e23685 environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
2.49.0
^ permalink raw reply [flat|nested] 50+ messages in thread* [GSOC PATCH v4 1/3] environment: move access to "core.sparsecheckout" into repo_settings
2025-06-17 12:06 ` [GSOC PATCH v4 0/3] environment: remove sparse-checkout related global variables Ayush Chandekar
@ 2025-06-17 12:06 ` 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 12:06 ` [GSOC PATCH v4 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns' Ayush Chandekar
2 siblings, 1 reply; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-17 12:06 UTC (permalink / raw)
To: ayu.chandekar; +Cc: christian.couder, git, shyamthakkar001, gitster
The setting "core.sparsecheckout" is stored in the global
`core_apply_sparse_checkout` and is populated in config.c. Refactor the
code to store it in the variable `sparse_checkout` in the struct
`repo_settings`.
It's fine not to lazily load it from the config, as the variable
is used quite commonly.
This also allows us to remove the definition `#define
USE_THE_REPOSITORY_VARIABLE` from the file 'builtin/backfill.c'.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
builtin/backfill.c | 7 ++-----
builtin/clone.c | 2 +-
builtin/grep.c | 2 +-
builtin/mv.c | 2 +-
builtin/sparse-checkout.c | 20 ++++++++++----------
builtin/worktree.c | 2 +-
config.c | 5 -----
dir.c | 2 +-
environment.c | 1 -
environment.h | 1 -
repo-settings.c | 1 +
repo-settings.h | 1 +
sparse-index.c | 4 ++--
unpack-trees.c | 2 +-
wt-status.c | 2 +-
15 files changed, 23 insertions(+), 31 deletions(-)
diff --git a/builtin/backfill.c b/builtin/backfill.c
index fa82ad2f6f..bf9e56bff3 100644
--- a/builtin/backfill.c
+++ b/builtin/backfill.c
@@ -1,6 +1,3 @@
-/* We need this macro to access core_apply_sparse_checkout */
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "builtin.h"
#include "git-compat-util.h"
#include "config.h"
@@ -137,9 +134,9 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
0);
repo_config(repo, git_default_config, NULL);
-
+ prepare_repo_settings(repo);
if (ctx.sparse < 0)
- ctx.sparse = core_apply_sparse_checkout;
+ ctx.sparse = repo->settings.sparse_checkout;
result = do_backfill(&ctx);
backfill_context_clear(&ctx);
diff --git a/builtin/clone.c b/builtin/clone.c
index 91b9cd0d16..1bc9c1bada 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -621,7 +621,7 @@ static int git_sparse_checkout_init(const char *repo)
* We must apply the setting in the current process
* for the later checkout to use the sparse-checkout file.
*/
- core_apply_sparse_checkout = 1;
+ the_repository->settings.sparse_checkout = 1;
cmd.git_cmd = 1;
if (run_command(&cmd)) {
diff --git a/builtin/grep.c b/builtin/grep.c
index 3ce574a605..63342f5e36 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -481,7 +481,7 @@ static int grep_submodule(struct grep_opt *opt,
* "forget" the sparse-index feature switch. As a result, the index
* of these submodules are expanded unexpectedly.
*
- * 2. "core_apply_sparse_checkout"
+ * 2. "sparse_checkout"
* When running `grep` in the superproject, this setting is
* populated using the superproject's configs. However, once
* initialized, this config is globally accessible and is read by
diff --git a/builtin/mv.c b/builtin/mv.c
index 07548fe96a..1e9f4d3eba 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -572,7 +572,7 @@ int cmd_mv(int argc,
rename_index_entry_at(the_repository->index, pos, dst);
if (ignore_sparse &&
- core_apply_sparse_checkout &&
+ the_repository->settings.sparse_checkout &&
core_sparse_checkout_cone) {
/*
* NEEDSWORK: we are *not* paying attention to
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 1bf01591b2..869d574a03 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -62,7 +62,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
int res;
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!the_repository->settings.sparse_checkout)
die(_("this worktree is not sparse"));
argc = parse_options(argc, argv, prefix,
@@ -397,11 +397,11 @@ static int set_config(enum sparse_checkout_mode mode)
static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {
/* If not specified, use previous definition of cone mode */
- if (*cone_mode == -1 && core_apply_sparse_checkout)
+ if (*cone_mode == -1 && the_repository->settings.sparse_checkout)
*cone_mode = core_sparse_checkout_cone;
/* Set cone/non-cone mode appropriately */
- core_apply_sparse_checkout = 1;
+ the_repository->settings.sparse_checkout = 1;
if (*cone_mode == 1 || *cone_mode == -1) {
core_sparse_checkout_cone = 1;
return MODE_CONE_PATTERNS;
@@ -415,7 +415,7 @@ static int update_modes(int *cone_mode, int *sparse_index)
int mode, record_mode;
/* Determine if we need to record the mode; ensure sparse checkout on */
- record_mode = (*cone_mode != -1) || !core_apply_sparse_checkout;
+ record_mode = (*cone_mode != -1) || !the_repository->settings.sparse_checkout;
mode = update_cone_mode(cone_mode);
if (record_mode && set_config(mode))
@@ -695,9 +695,9 @@ static int modify_pattern_list(struct strvec *args, int use_stdin,
break;
}
- if (!core_apply_sparse_checkout) {
+ if (!the_repository->settings.sparse_checkout) {
set_config(MODE_ALL_PATTERNS);
- core_apply_sparse_checkout = 1;
+ the_repository->settings.sparse_checkout = 1;
changed_config = 1;
}
@@ -793,7 +793,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix,
int ret;
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!the_repository->settings.sparse_checkout)
die(_("no sparse-checkout to add to"));
repo_read_index(the_repository);
@@ -902,7 +902,7 @@ static int sparse_checkout_reapply(int argc, const char **argv,
};
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!the_repository->settings.sparse_checkout)
die(_("must be in a sparse-checkout to reapply sparsity patterns"));
reapply_opts.cone_mode = -1;
@@ -935,7 +935,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
struct pattern_list pl;
/*
- * We do not exit early if !core_apply_sparse_checkout; due to the
+ * We do not exit early if !sparse_checkout; due to the
* ability for users to manually muck things up between
* direct editing of .git/info/sparse-checkout
* running read-tree -m u HEAD or update-index --skip-worktree
@@ -961,7 +961,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
pl.use_cone_patterns = 0;
- core_apply_sparse_checkout = 1;
+ the_repository->settings.sparse_checkout = 1;
add_pattern("/*", empty_base, 0, &pl, 0);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 88a36ea9f8..92e1c92afc 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -536,7 +536,7 @@ static int add_worktree(const char *path, const char *refname,
* If the current worktree has sparse-checkout enabled, then copy
* the sparse-checkout patterns from the current worktree.
*/
- if (core_apply_sparse_checkout)
+ if (the_repository->settings.sparse_checkout)
copy_sparse_checkout(sb_repo.buf);
/*
diff --git a/config.c b/config.c
index b18b5617fc..8fd4dd8c81 100644
--- a/config.c
+++ b/config.c
@@ -1612,11 +1612,6 @@ static int git_default_core_config(const char *var, const char *value,
return 0;
}
- if (!strcmp(var, "core.sparsecheckout")) {
- core_apply_sparse_checkout = git_config_bool(var, value);
- return 0;
- }
-
if (!strcmp(var, "core.sparsecheckoutcone")) {
core_sparse_checkout_cone = git_config_bool(var, value);
return 0;
diff --git a/dir.c b/dir.c
index a374972b62..8f0f7ca8a4 100644
--- a/dir.c
+++ b/dir.c
@@ -1503,7 +1503,7 @@ enum pattern_match_result path_matches_pattern_list(
int init_sparse_checkout_patterns(struct index_state *istate)
{
- if (!core_apply_sparse_checkout)
+ if (!istate->repo->settings.sparse_checkout)
return 1;
if (istate->sparse_checkout_patterns)
return 0;
diff --git a/environment.c b/environment.c
index c61d773e7e..a379a9149e 100644
--- a/environment.c
+++ b/environment.c
@@ -64,7 +64,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#endif
enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
-int core_apply_sparse_checkout;
int core_sparse_checkout_cone;
int sparse_expect_files_outside_of_patterns;
int merge_log_config = -1;
diff --git a/environment.h b/environment.h
index 3d98461a06..6a30512f3c 100644
--- a/environment.h
+++ b/environment.h
@@ -160,7 +160,6 @@ extern int precomposed_unicode;
extern int protect_hfs;
extern int protect_ntfs;
-extern int core_apply_sparse_checkout;
extern int core_sparse_checkout_cone;
extern int sparse_expect_files_outside_of_patterns;
diff --git a/repo-settings.c b/repo-settings.c
index 4129f8fb2b..9270cca561 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -81,6 +81,7 @@ void prepare_repo_settings(struct repository *r)
&r->settings.pack_use_bitmap_boundary_traversal,
r->settings.pack_use_bitmap_boundary_traversal);
repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
+ repo_cfg_bool(r, "core.sparsecheckout", &r->settings.sparse_checkout, 0);
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
diff --git a/repo-settings.h b/repo-settings.h
index 2bf24b2597..9caa7c57a3 100644
--- a/repo-settings.h
+++ b/repo-settings.h
@@ -67,6 +67,7 @@ struct repo_settings {
unsigned long big_file_threshold;
char *hooks_path;
+ int sparse_checkout;
};
#define REPO_SETTINGS_INIT { \
.shared_repository = -1, \
diff --git a/sparse-index.c b/sparse-index.c
index 5634abafaa..2428b20634 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -150,7 +150,7 @@ static int index_has_unmerged_entries(struct index_state *istate)
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
- if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
+ if (!istate->repo->settings.sparse_checkout || !core_sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
@@ -668,7 +668,7 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista
void clear_skip_worktree_from_present_files(struct index_state *istate)
{
- if (!core_apply_sparse_checkout ||
+ if (!istate->repo->settings.sparse_checkout ||
sparse_expect_files_outside_of_patterns)
return;
diff --git a/unpack-trees.c b/unpack-trees.c
index 471837f032..02e32c4ba1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1924,7 +1924,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
if (o->prefix)
update_sparsity_for_prefix(o->prefix, o->src_index);
- if (!core_apply_sparse_checkout || !o->update)
+ if (!repo->settings.sparse_checkout || !o->update)
o->skip_sparse_checkout = 1;
if (!o->skip_sparse_checkout) {
memset(&pl, 0, sizeof(pl));
diff --git a/wt-status.c b/wt-status.c
index 454601afa1..ec2be98194 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1773,7 +1773,7 @@ static void wt_status_check_sparse_checkout(struct repository *r,
int skip_worktree = 0;
int i;
- if (!core_apply_sparse_checkout || r->index->cache_nr == 0) {
+ if (!r->settings.sparse_checkout || r->index->cache_nr == 0) {
/*
* Don't compute percentage of checked out files if we
* aren't in a sparse checkout or would get division by 0.
--
2.49.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v4 1/3] environment: move access to "core.sparsecheckout" into repo_settings
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
0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2025-06-17 16:15 UTC (permalink / raw)
To: Ayush Chandekar; +Cc: christian.couder, git, shyamthakkar001
Ayush Chandekar <ayu.chandekar@gmail.com> writes:
> diff --git a/builtin/backfill.c b/builtin/backfill.c
> index fa82ad2f6f..bf9e56bff3 100644
> --- a/builtin/backfill.c
> +++ b/builtin/backfill.c
> @@ -1,6 +1,3 @@
> -/* We need this macro to access core_apply_sparse_checkout */
> -#define USE_THE_REPOSITORY_VARIABLE
> -
> #include "builtin.h"
> #include "git-compat-util.h"
> #include "config.h"
> @@ -137,9 +134,9 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
> 0);
>
> repo_config(repo, git_default_config, NULL);
> -
> + prepare_repo_settings(repo);
At this point, because show_usage_with_options_if_asked() has
already been called and returned, we know repo is not NULL, since
the only time git.c:run_builtin() calls us with repo==NULL is when
there is "-h" with nothing else on the command line and that causes
show_usage_with_options_if_asked() to emit usage and exit.
OK.
> if (ctx.sparse < 0)
> - ctx.sparse = core_apply_sparse_checkout;
> + ctx.sparse = repo->settings.sparse_checkout;
This is safe for the same reason.
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 91b9cd0d16..1bc9c1bada 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -621,7 +621,7 @@ static int git_sparse_checkout_init(const char *repo)
> * We must apply the setting in the current process
> * for the later checkout to use the sparse-checkout file.
> */
> - core_apply_sparse_checkout = 1;
> + the_repository->settings.sparse_checkout = 1;
Have anybody called prepare_repo_settings() on the repository yet?
What will prevent another call to the function from overwriting this
value later?
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 07548fe96a..1e9f4d3eba 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -572,7 +572,7 @@ int cmd_mv(int argc,
> rename_index_entry_at(the_repository->index, pos, dst);
>
> if (ignore_sparse &&
> - core_apply_sparse_checkout &&
> + the_repository->settings.sparse_checkout &&
> core_sparse_checkout_cone) {
Have anybody called prepare_repo_settings() on the repository yet
before the control comes here?
The guarantee of the original code being correct relied on the fact
that git_default_core_config() was called way before these places so
the global variable has been already initialized correctly.
The .sparse_checkout member is read in prepare_repo_settings() in
your new code; in order to give the correctness guarantee, there
needs to be some way to make sure prepare_repo_settings() has
already been called on the_repository before these places.
The same comment applies to all the code paths that access
the_repository->settings.sparse_checkout member instead of the
global. As the source of their correctness guarantee is quite
different, a mechanical replacement from global to a struct member
is not sufficient.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [GSOC PATCH v4 2/3] environment: move access to "core.sparsecheckoutcone" into repo_settings
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 12:06 ` 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
2 siblings, 1 reply; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-17 12:06 UTC (permalink / raw)
To: ayu.chandekar; +Cc: christian.couder, git, shyamthakkar001, gitster
The setting "core.sparsecheckoutcone" is stored in the global
`core_sparse_checkout_cone` and is populated in config.c. Refactor the
code to store it in the variable `sparse_checkout_cone` in the struct
`repo_settings`.
It's fine not to lazily load it from the config, as the variable
is used quite commonly.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
builtin/grep.c | 2 +-
builtin/mv.c | 2 +-
builtin/sparse-checkout.c | 28 ++++++++++++++--------------
config.c | 5 -----
dir.c | 2 +-
environment.c | 1 -
environment.h | 1 -
repo-settings.c | 1 +
repo-settings.h | 2 ++
sparse-index.c | 2 +-
10 files changed, 21 insertions(+), 25 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 63342f5e36..94d6245b85 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -491,7 +491,7 @@ static int grep_submodule(struct grep_opt *opt,
* dictate the behavior for the submodule, making it "forget" its
* sparse-checkout state.
*
- * 3. "core_sparse_checkout_cone"
+ * 3. "sparse_checkout_cone"
* ditto.
*
* Note that this list is not exhaustive.
diff --git a/builtin/mv.c b/builtin/mv.c
index 1e9f4d3eba..833fa761dd 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -573,7 +573,7 @@ int cmd_mv(int argc,
if (ignore_sparse &&
the_repository->settings.sparse_checkout &&
- core_sparse_checkout_cone) {
+ the_repository->settings.sparse_checkout_cone) {
/*
* NEEDSWORK: we are *not* paying attention to
* "out-to-out" move (<source> is out-of-cone and
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 869d574a03..e65a62f250 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -71,7 +71,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
memset(&pl, 0, sizeof(pl));
- pl.use_cone_patterns = core_sparse_checkout_cone;
+ pl.use_cone_patterns = the_repository->settings.sparse_checkout_cone;
sparse_filename = get_sparse_checkout_filename();
res = add_patterns_from_file_to_list(sparse_filename, "", 0, &pl, NULL, 0);
@@ -352,7 +352,7 @@ static int write_patterns_and_update(struct pattern_list *pl)
if (!fp)
die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk));
- if (core_sparse_checkout_cone)
+ if (the_repository->settings.sparse_checkout_cone)
write_cone_to_file(fp, pl);
else
write_patterns_to_file(fp, pl);
@@ -398,15 +398,15 @@ static int set_config(enum sparse_checkout_mode mode)
static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {
/* If not specified, use previous definition of cone mode */
if (*cone_mode == -1 && the_repository->settings.sparse_checkout)
- *cone_mode = core_sparse_checkout_cone;
+ *cone_mode = the_repository->settings.sparse_checkout_cone;
/* Set cone/non-cone mode appropriately */
the_repository->settings.sparse_checkout = 1;
if (*cone_mode == 1 || *cone_mode == -1) {
- core_sparse_checkout_cone = 1;
+ the_repository->settings.sparse_checkout_cone = 1;
return MODE_CONE_PATTERNS;
}
- core_sparse_checkout_cone = 0;
+ the_repository->settings.sparse_checkout_cone = 0;
return MODE_ALL_PATTERNS;
}
@@ -572,7 +572,7 @@ static void add_patterns_from_input(struct pattern_list *pl,
FILE *file)
{
int i;
- if (core_sparse_checkout_cone) {
+ if (the_repository->settings.sparse_checkout_cone) {
struct strbuf line = STRBUF_INIT;
hashmap_init(&pl->recursive_hashmap, pl_hashmap_cmp, NULL, 0);
@@ -637,7 +637,7 @@ static void add_patterns_cone_mode(int argc, const char **argv,
use_stdin ? stdin : NULL);
memset(&existing, 0, sizeof(existing));
- existing.use_cone_patterns = core_sparse_checkout_cone;
+ existing.use_cone_patterns = the_repository->settings.sparse_checkout_cone;
if (add_patterns_from_file_to_list(sparse_filename, "", 0,
&existing, NULL, 0))
@@ -683,7 +683,7 @@ static int modify_pattern_list(struct strvec *args, int use_stdin,
switch (m) {
case ADD:
- if (core_sparse_checkout_cone)
+ if (the_repository->settings.sparse_checkout_cone)
add_patterns_cone_mode(args->nr, args->v, pl, use_stdin);
else
add_patterns_literal(args->nr, args->v, pl, use_stdin);
@@ -719,7 +719,7 @@ static void sanitize_paths(struct strvec *args,
if (!args->nr)
return;
- if (prefix && *prefix && core_sparse_checkout_cone) {
+ if (prefix && *prefix && the_repository->settings.sparse_checkout_cone) {
/*
* The args are not pathspecs, so unfortunately we
* cannot imitate how cmd_add() uses parse_pathspec().
@@ -736,10 +736,10 @@ static void sanitize_paths(struct strvec *args,
if (skip_checks)
return;
- if (prefix && *prefix && !core_sparse_checkout_cone)
+ if (prefix && *prefix && !the_repository->settings.sparse_checkout_cone)
die(_("please run from the toplevel directory in non-cone mode"));
- if (core_sparse_checkout_cone) {
+ if (the_repository->settings.sparse_checkout_cone) {
for (i = 0; i < args->nr; i++) {
if (args->v[i][0] == '/')
die(_("specify directories rather than patterns (no leading slash)"));
@@ -761,7 +761,7 @@ static void sanitize_paths(struct strvec *args,
if (S_ISSPARSEDIR(ce->ce_mode))
continue;
- if (core_sparse_checkout_cone)
+ if (the_repository->settings.sparse_checkout_cone)
die(_("'%s' is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), args->v[i]);
else
warning(_("pass a leading slash before paths such as '%s' if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), args->v[i]);
@@ -864,7 +864,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix,
* non-cone mode, if nothing is specified, manually select just the
* top-level directory (much as 'init' would do).
*/
- if (!core_sparse_checkout_cone && !set_opts.use_stdin && argc == 0) {
+ if (!the_repository->settings.sparse_checkout_cone && !set_opts.use_stdin && argc == 0) {
for (int i = 0; i < default_patterns_nr; i++)
strvec_push(&patterns, default_patterns[i]);
} else {
@@ -1042,7 +1042,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char *
check_rules_opts.cone_mode = 1;
update_cone_mode(&check_rules_opts.cone_mode);
- pl.use_cone_patterns = core_sparse_checkout_cone;
+ pl.use_cone_patterns = the_repository->settings.sparse_checkout_cone;
if (check_rules_opts.rules_file) {
fp = xfopen(check_rules_opts.rules_file, "r");
add_patterns_from_input(&pl, argc, argv, fp);
diff --git a/config.c b/config.c
index 8fd4dd8c81..707fe0707a 100644
--- a/config.c
+++ b/config.c
@@ -1612,11 +1612,6 @@ static int git_default_core_config(const char *var, const char *value,
return 0;
}
- if (!strcmp(var, "core.sparsecheckoutcone")) {
- core_sparse_checkout_cone = git_config_bool(var, value);
- return 0;
- }
-
if (!strcmp(var, "core.precomposeunicode")) {
precomposed_unicode = git_config_bool(var, value);
return 0;
diff --git a/dir.c b/dir.c
index 8f0f7ca8a4..8378996b72 100644
--- a/dir.c
+++ b/dir.c
@@ -3459,7 +3459,7 @@ int get_sparse_checkout_patterns(struct pattern_list *pl)
int res;
char *sparse_filename = get_sparse_checkout_filename();
- pl->use_cone_patterns = core_sparse_checkout_cone;
+ pl->use_cone_patterns = the_repository->settings.sparse_checkout_cone;
res = add_patterns_from_file_to_list(sparse_filename, "", 0, pl, NULL, 0);
free(sparse_filename);
diff --git a/environment.c b/environment.c
index a379a9149e..7d46b80711 100644
--- a/environment.c
+++ b/environment.c
@@ -64,7 +64,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#endif
enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
-int core_sparse_checkout_cone;
int sparse_expect_files_outside_of_patterns;
int merge_log_config = -1;
int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
diff --git a/environment.h b/environment.h
index 6a30512f3c..00a5b332a0 100644
--- a/environment.h
+++ b/environment.h
@@ -160,7 +160,6 @@ extern int precomposed_unicode;
extern int protect_hfs;
extern int protect_ntfs;
-extern int core_sparse_checkout_cone;
extern int sparse_expect_files_outside_of_patterns;
enum rebase_setup_type {
diff --git a/repo-settings.c b/repo-settings.c
index 9270cca561..eebc1f941d 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -82,6 +82,7 @@ void prepare_repo_settings(struct repository *r)
r->settings.pack_use_bitmap_boundary_traversal);
repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
repo_cfg_bool(r, "core.sparsecheckout", &r->settings.sparse_checkout, 0);
+ repo_cfg_bool(r, "core.sparsecheckoutcone", &r->settings.sparse_checkout_cone, 0);
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
diff --git a/repo-settings.h b/repo-settings.h
index 9caa7c57a3..443e1399da 100644
--- a/repo-settings.h
+++ b/repo-settings.h
@@ -67,7 +67,9 @@ struct repo_settings {
unsigned long big_file_threshold;
char *hooks_path;
+
int sparse_checkout;
+ int sparse_checkout_cone;
};
#define REPO_SETTINGS_INIT { \
.shared_repository = -1, \
diff --git a/sparse-index.c b/sparse-index.c
index 2428b20634..444da8a753 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -150,7 +150,7 @@ static int index_has_unmerged_entries(struct index_state *istate)
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
- if (!istate->repo->settings.sparse_checkout || !core_sparse_checkout_cone)
+ if (!istate->repo->settings.sparse_checkout || !istate->repo->settings.sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
--
2.49.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v4 2/3] environment: move access to "core.sparsecheckoutcone" into repo_settings
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
0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2025-06-17 16:29 UTC (permalink / raw)
To: Ayush Chandekar; +Cc: christian.couder, git, shyamthakkar001
Ayush Chandekar <ayu.chandekar@gmail.com> writes:
> The setting "core.sparsecheckoutcone" is stored in the global
> `core_sparse_checkout_cone` and is populated in config.c. Refactor the
> code to store it in the variable `sparse_checkout_cone` in the struct
> `repo_settings`.
> It's fine not to lazily load it from the config, as the variable
> is used quite commonly.
>
> This change is part of an ongoing effort to eliminate global variables,
> improve modularity and help libify the codebase.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> ---
I think "the correctness guarantee comes from a different place now.
How are we making sure that these accesses are correct?" comment
applies equally here.
> builtin/grep.c | 2 +-
> builtin/mv.c | 2 +-
Looking at the output from
$ git grep -n -e prepare_repo_settings \*.c
there are many builtin/*.c that makes a call to the function on
the_repository fairly early in its start-up sequence. Unlike many
others these two do not seem to have any.
> builtin/sparse-checkout.c | 28 ++++++++++++++--------------
This one does, immediately after calling git_config(), so it should
be fairly safe.
> dir.c | 2 +-
> sparse-index.c | 2 +-
These two also need correctness guarantee.
You'd need to make sure any potential caller of the helper functions
have called prepare_repo_settings(). Those who wrote an access to
the global variable in the original would already have made sure
that the callers would have already read the configuration file, but
with the new code, that is no longer a guarantee for correctness.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [GSOC PATCH v4 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
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 12:06 ` [GSOC PATCH v4 2/3] environment: move access to "core.sparsecheckoutcone" " Ayush Chandekar
@ 2025-06-17 12:06 ` Ayush Chandekar
2025-06-17 16:30 ` Junio C Hamano
2 siblings, 1 reply; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-17 12:06 UTC (permalink / raw)
To: ayu.chandekar; +Cc: christian.couder, git, shyamthakkar001, gitster
The global variable 'sparse_expect_files_outside_of_patterns' is used in
a single function named 'clear_skip_worktree_from_present_files()' in
sparse-index.c. Move its declaration inside that function, removing
unnecessary global state.
This also allows us to remove the definition '#define
USE_THE_REPOSITORY_VARIABLE' from the file 'sparse-index.c'.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
config.c | 14 --------------
environment.c | 1 -
environment.h | 2 --
sparse-index.c | 4 +++-
4 files changed, 3 insertions(+), 18 deletions(-)
diff --git a/config.c b/config.c
index 707fe0707a..d212329799 100644
--- a/config.c
+++ b/config.c
@@ -1636,17 +1636,6 @@ static int git_default_core_config(const char *var, const char *value,
return platform_core_config(var, value, ctx, cb);
}
-static int git_default_sparse_config(const char *var, const char *value)
-{
- if (!strcmp(var, "sparse.expectfilesoutsideofpatterns")) {
- sparse_expect_files_outside_of_patterns = git_config_bool(var, value);
- return 0;
- }
-
- /* Add other config variables here and to Documentation/config/sparse.adoc. */
- return 0;
-}
-
static int git_default_i18n_config(const char *var, const char *value)
{
if (!strcmp(var, "i18n.commitencoding")) {
@@ -1808,9 +1797,6 @@ int git_default_config(const char *var, const char *value,
return 0;
}
- if (starts_with(var, "sparse."))
- return git_default_sparse_config(var, value);
-
/* Add other config variables here and to Documentation/config.adoc. */
return 0;
}
diff --git a/environment.c b/environment.c
index 7d46b80711..d51e0a14aa 100644
--- a/environment.c
+++ b/environment.c
@@ -64,7 +64,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#endif
enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
-int sparse_expect_files_outside_of_patterns;
int merge_log_config = -1;
int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
unsigned long pack_size_limit_cfg;
diff --git a/environment.h b/environment.h
index 00a5b332a0..5121a28d3f 100644
--- a/environment.h
+++ b/environment.h
@@ -160,8 +160,6 @@ extern int precomposed_unicode;
extern int protect_hfs;
extern int protect_ntfs;
-extern int sparse_expect_files_outside_of_patterns;
-
enum rebase_setup_type {
AUTOREBASE_NEVER = 0,
AUTOREBASE_LOCAL,
diff --git a/sparse-index.c b/sparse-index.c
index 444da8a753..5d87fc65c0 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -1,4 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
#define DISABLE_SIGN_COMPARE_WARNINGS
#include "git-compat-util.h"
@@ -668,6 +667,9 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista
void clear_skip_worktree_from_present_files(struct index_state *istate)
{
+ int sparse_expect_files_outside_of_patterns = 0;
+ repo_config_get_bool(istate->repo, "sparse.expectfilesoutsideofpatterns",
+ &sparse_expect_files_outside_of_patterns);
if (!istate->repo->settings.sparse_checkout ||
sparse_expect_files_outside_of_patterns)
return;
--
2.49.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v4 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
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
0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2025-06-17 16:30 UTC (permalink / raw)
To: Ayush Chandekar; +Cc: christian.couder, git, shyamthakkar001
Ayush Chandekar <ayu.chandekar@gmail.com> writes:
> diff --git a/sparse-index.c b/sparse-index.c
> index 444da8a753..5d87fc65c0 100644
> --- a/sparse-index.c
> +++ b/sparse-index.c
> @@ -1,4 +1,3 @@
> -#define USE_THE_REPOSITORY_VARIABLE
> #define DISABLE_SIGN_COMPARE_WARNINGS
>
> #include "git-compat-util.h"
> @@ -668,6 +667,9 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista
>
> void clear_skip_worktree_from_present_files(struct index_state *istate)
> {
> + int sparse_expect_files_outside_of_patterns = 0;
> + repo_config_get_bool(istate->repo, "sparse.expectfilesoutsideofpatterns",
There is a trailing whitespace here.
> + &sparse_expect_files_outside_of_patterns);
^ permalink raw reply [flat|nested] 50+ messages in thread
* [GSOC PATCH v5 0/3] environment: remove sparse-checkout related global variables
2025-06-03 13:18 [GSOC PATCH] environment: move access to "core.sparsecheckout" into repo_settings Ayush Chandekar
` (3 preceding siblings ...)
2025-06-17 12:06 ` [GSOC PATCH v4 0/3] environment: remove sparse-checkout related global variables Ayush Chandekar
@ 2025-06-30 19:27 ` Ayush Chandekar
2025-06-30 19:27 ` [GSOC PATCH v5 1/3] environment: move access to "core.sparsecheckout" into repo_settings Ayush Chandekar
` (4 more replies)
2025-07-19 0:11 ` [GSOC PATCH v6 " Ayush Chandekar
5 siblings, 5 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-30 19:27 UTC (permalink / raw)
To: ayu.chandekar
Cc: christian.couder, git, shyamthakkar001, gitster, ps, ben.knoble
This patch series aims to remove global variables related to sparse-checkout from the global scope and to remove the definition '#define USE_THE_REPOSITORY_VARIABLE' from a few files.
It contains three patches:
1 - Remove the global variable 'core_apply_sparse_checkout' and move its setting to the 'struct repo_settings'. Also remove the definition '#define USE_THE_REPOSITORY_VARIABLE' from "builtin/backfill.c".
2 - Remove the global variable 'core_sparse_checkout_cone' and move its setting to the 'struct repo_settings'.
3 - Remove the global variable 'sparse_expect_files_outside_of_patterns` and localize it in the function which calls it. Also remove the definition '#define USE_THE_REPOSITORY_VARIABLE' from "sparse-index.c"
Thanks a lot to Christian for mentoring, and to Junio, Patrick and Ben for reviewing.
Ayush Chandekar (2):
environment: move access to "core.sparsecheckoutcone" into
repo_settings
environment: remove the global variable
'sparse_expect_files_outside_of_patterns'
environment: move access to "core.sparsecheckout" into repo_settings
builtin/backfill.c | 7 ++----
builtin/clone.c | 3 ++-
builtin/grep.c | 4 ++--
builtin/mv.c | 6 ++---
builtin/sparse-checkout.c | 49 +++++++++++++++++++--------------------
builtin/worktree.c | 2 +-
config.c | 24 -------------------
dir.c | 5 ++--
environment.c | 3 ---
environment.h | 4 ----
repo-settings.c | 2 ++
repo-settings.h | 3 +++
sparse-index.c | 10 ++++----
unpack-trees.c | 3 ++-
wt-status.c | 3 ++-
15 files changed, 52 insertions(+), 76 deletions(-)
--
Summary of range-diff:
* Ensure that `prepare_repo_settings()` is called in all code paths before accessing `settings.sparse_checkout` and `settings.sparse_checkout_cone`.
Range-diff with v4:
1: e221c68ab5 ! 1: ba9929d128 environment: move access to "core.sparsecheckout" into repo_settings
@@ Commit message
`core_apply_sparse_checkout` and is populated in config.c. Refactor the
code to store it in the variable `sparse_checkout` in the struct
`repo_settings`.
- It's fine not to lazily load it from the config, as the variable
- is used quite commonly.
+
+ Call `prepare_repo_settings()` where necessary to ensure the `struct
+ repo_settings` is initialized before use:
+ - In "builtin/backfill.c", "builtin/mv.c" and "builtin/clone.c" call
+ `prepare_repo_settings()` since their respective `cmd_*()` functions
+ did not call it earlier.
+ - In "dir.c", the function using 'settings.sparse_checkout' is invoked
+ in multiple files that do not call `prepare_repo_settings()`, hence
+ add a call directly to that function.
+ - In "sparse-checkout.c", add a call to `prepare_repo_settings()` inside
+ `is_sparse_index_allowed()`, as it is used widely and relies on the
+ setting.
+ - In "wt-status.c", call `prepare_repo_settings()` before accessing
+ the setting because the function using it is commonly used.
+
+ Avoid reduntant calls to `prepare_repo_settings()` where it is already
+ present:
+ - In "builtin/worktree.c", it is already invoked in `cmd_worktree()`
+ before the setting is accessed.
+ - In "unpack-tress.c", the function accessing the setting already calls
+ it.
This also allows us to remove the definition `#define
USE_THE_REPOSITORY_VARIABLE` from the file 'builtin/backfill.c'.
@@ builtin/backfill.c: int cmd_backfill(int argc, const char **argv, const char *pr
## builtin/clone.c ##
@@ builtin/clone.c: static int git_sparse_checkout_init(const char *repo)
+ int result = 0;
+ strvec_pushl(&cmd.args, "-C", repo, "sparse-checkout", "set", NULL);
+
++ prepare_repo_settings(the_repository);
+ /*
* We must apply the setting in the current process
* for the later checkout to use the sparse-checkout file.
*/
@@ builtin/grep.c: static int grep_submodule(struct grep_opt *opt,
* of these submodules are expanded unexpectedly.
*
- * 2. "core_apply_sparse_checkout"
-+ * 2. "sparse_checkout"
++ * 2. "settings.sparse_checkout"
* When running `grep` in the superproject, this setting is
* populated using the superproject's configs. However, once
* initialized, this config is globally accessible and is read by
## builtin/mv.c ##
@@ builtin/mv.c: int cmd_mv(int argc,
+ &st,
+ 0);
rename_index_entry_at(the_repository->index, pos, dst);
-
+-
++ prepare_repo_settings(the_repository);
if (ignore_sparse &&
- core_apply_sparse_checkout &&
+ the_repository->settings.sparse_checkout &&
@@ builtin/sparse-checkout.c: static int sparse_checkout_disable(int argc, const ch
add_pattern("/*", empty_base, 0, &pl, 0);
+- prepare_repo_settings(the_repository);
+ the_repository->settings.sparse_index = 0;
+
+ if (update_working_directory(&pl))
## builtin/worktree.c ##
@@ builtin/worktree.c: static int add_worktree(const char *path, const char *refname,
@@ dir.c: enum pattern_match_result path_matches_pattern_list(
int init_sparse_checkout_patterns(struct index_state *istate)
{
- if (!core_apply_sparse_checkout)
++ prepare_repo_settings(istate->repo);
+ if (!istate->repo->settings.sparse_checkout)
return 1;
if (istate->sparse_checkout_patterns)
@@ sparse-index.c: static int index_has_unmerged_entries(struct index_state *istate
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
- if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
++ prepare_repo_settings(istate->repo);
+ if (!istate->repo->settings.sparse_checkout || !core_sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
+@@ sparse-index.c: int is_sparse_index_allowed(struct index_state *istate, int flags)
+ /*
+ * Only convert to sparse if index.sparse is set.
+ */
+- prepare_repo_settings(istate->repo);
+ if (!istate->repo->settings.sparse_index)
+ return 0;
+ }
@@ sparse-index.c: static void clear_skip_worktree_from_present_files_full(struct index_state *ista
void clear_skip_worktree_from_present_files(struct index_state *istate)
@@ wt-status.c: static void wt_status_check_sparse_checkout(struct repository *r,
int i;
- if (!core_apply_sparse_checkout || r->index->cache_nr == 0) {
++ prepare_repo_settings(r);
+ if (!r->settings.sparse_checkout || r->index->cache_nr == 0) {
/*
* Don't compute percentage of checked out files if we
2: 9a63884341 ! 2: 5a2f61443b environment: move access to "core.sparsecheckoutcone" into repo_settings
@@ Commit message
`core_sparse_checkout_cone` and is populated in config.c. Refactor the
code to store it in the variable `sparse_checkout_cone` in the struct
`repo_settings`.
- It's fine not to lazily load it from the config, as the variable
- is used quite commonly.
+
+ Call `prepare_repo_settings()` where necessary to ensure the `struct
+ repo_settings` is initialized before use:
+ - In "dir.c", the function accessing the setting is usually called after
+ `prepare_repo_settings()`, except for one code path in
+ "unpack-trees.c", so add a call there.
+
+ Avoid redundant calls to `prepare_repo_settings()` where it is already
+ present:
+ - In "builtin/mv.c" and "builtin/sparse-checkout.c", it is already
+ invoked in their respective `cmd_*()` functions.
+ - In "sparse-index.c", `prepare_repo_settings` is already called before
+ the setting is accessed.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
@@ builtin/grep.c: static int grep_submodule(struct grep_opt *opt,
* sparse-checkout state.
*
- * 3. "core_sparse_checkout_cone"
-+ * 3. "sparse_checkout_cone"
++ * 3. "settings.sparse_checkout_cone"
* ditto.
*
* Note that this list is not exhaustive.
## builtin/mv.c ##
@@ builtin/mv.c: int cmd_mv(int argc,
-
+ prepare_repo_settings(the_repository);
if (ignore_sparse &&
the_repository->settings.sparse_checkout &&
- core_sparse_checkout_cone) {
@@ repo-settings.h: struct repo_settings {
## sparse-index.c ##
@@ sparse-index.c: static int index_has_unmerged_entries(struct index_state *istate)
-
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
+ prepare_repo_settings(istate->repo);
- if (!istate->repo->settings.sparse_checkout || !core_sparse_checkout_cone)
+ if (!istate->repo->settings.sparse_checkout || !istate->repo->settings.sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
+
+ ## unpack-trees.c ##
+@@ unpack-trees.c: enum update_sparsity_result update_sparsity(struct unpack_trees_options *o,
+ BUG("update_sparsity() called wrong");
+
+ trace_performance_enter();
++ prepare_repo_settings(the_repository);
+
+ /* If we weren't given patterns, use the recorded ones */
+ if (!pl) {
3: 578c087188 = 3: 45c84a6615 environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
2.49.0
^ permalink raw reply [flat|nested] 50+ messages in thread* [GSOC PATCH v5 1/3] environment: move access to "core.sparsecheckout" into repo_settings
2025-06-30 19:27 ` [GSOC PATCH v5 0/3] environment: remove sparse-checkout related global variables Ayush Chandekar
@ 2025-06-30 19:27 ` Ayush Chandekar
2025-06-30 19:27 ` [GSOC PATCH v5 2/3] environment: move access to "core.sparsecheckoutcone" " Ayush Chandekar
` (3 subsequent siblings)
4 siblings, 0 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-30 19:27 UTC (permalink / raw)
To: ayu.chandekar
Cc: christian.couder, git, shyamthakkar001, gitster, ps, ben.knoble
The setting "core.sparsecheckout" is stored in the global
`core_apply_sparse_checkout` and is populated in config.c. Refactor the
code to store it in the variable `sparse_checkout` in the struct
`repo_settings`.
Call `prepare_repo_settings()` where necessary to ensure the `struct
repo_settings` is initialized before use:
- In "builtin/backfill.c", "builtin/mv.c" and "builtin/clone.c" call
`prepare_repo_settings()` since their respective `cmd_*()` functions
did not call it earlier.
- In "dir.c", the function using 'settings.sparse_checkout' is invoked
in multiple files that do not call `prepare_repo_settings()`, hence
add a call directly to that function.
- In "sparse-checkout.c", add a call to `prepare_repo_settings()` inside
`is_sparse_index_allowed()`, as it is used widely and relies on the
setting.
- In "wt-status.c", call `prepare_repo_settings()` before accessing
the setting because the function using it is commonly used.
Avoid reduntant calls to `prepare_repo_settings()` where it is already
present:
- In "builtin/worktree.c", it is already invoked in `cmd_worktree()`
before the setting is accessed.
- In "unpack-tress.c", the function accessing the setting already calls
it.
This also allows us to remove the definition `#define
USE_THE_REPOSITORY_VARIABLE` from the file 'builtin/backfill.c'.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
builtin/backfill.c | 7 ++-----
builtin/clone.c | 3 ++-
builtin/grep.c | 2 +-
builtin/mv.c | 4 ++--
builtin/sparse-checkout.c | 21 ++++++++++-----------
builtin/worktree.c | 2 +-
config.c | 5 -----
dir.c | 3 ++-
environment.c | 1 -
environment.h | 1 -
repo-settings.c | 1 +
repo-settings.h | 1 +
sparse-index.c | 6 +++---
unpack-trees.c | 2 +-
wt-status.c | 3 ++-
15 files changed, 28 insertions(+), 34 deletions(-)
diff --git a/builtin/backfill.c b/builtin/backfill.c
index fa82ad2f6f..bf9e56bff3 100644
--- a/builtin/backfill.c
+++ b/builtin/backfill.c
@@ -1,6 +1,3 @@
-/* We need this macro to access core_apply_sparse_checkout */
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "builtin.h"
#include "git-compat-util.h"
#include "config.h"
@@ -137,9 +134,9 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
0);
repo_config(repo, git_default_config, NULL);
-
+ prepare_repo_settings(repo);
if (ctx.sparse < 0)
- ctx.sparse = core_apply_sparse_checkout;
+ ctx.sparse = repo->settings.sparse_checkout;
result = do_backfill(&ctx);
backfill_context_clear(&ctx);
diff --git a/builtin/clone.c b/builtin/clone.c
index 91b9cd0d16..6d70986f3e 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -617,11 +617,12 @@ static int git_sparse_checkout_init(const char *repo)
int result = 0;
strvec_pushl(&cmd.args, "-C", repo, "sparse-checkout", "set", NULL);
+ prepare_repo_settings(the_repository);
/*
* We must apply the setting in the current process
* for the later checkout to use the sparse-checkout file.
*/
- core_apply_sparse_checkout = 1;
+ the_repository->settings.sparse_checkout = 1;
cmd.git_cmd = 1;
if (run_command(&cmd)) {
diff --git a/builtin/grep.c b/builtin/grep.c
index 3ce574a605..6111fee60b 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -481,7 +481,7 @@ static int grep_submodule(struct grep_opt *opt,
* "forget" the sparse-index feature switch. As a result, the index
* of these submodules are expanded unexpectedly.
*
- * 2. "core_apply_sparse_checkout"
+ * 2. "settings.sparse_checkout"
* When running `grep` in the superproject, this setting is
* populated using the superproject's configs. However, once
* initialized, this config is globally accessible and is read by
diff --git a/builtin/mv.c b/builtin/mv.c
index 07548fe96a..43ed2e3d0a 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -570,9 +570,9 @@ int cmd_mv(int argc,
&st,
0);
rename_index_entry_at(the_repository->index, pos, dst);
-
+ prepare_repo_settings(the_repository);
if (ignore_sparse &&
- core_apply_sparse_checkout &&
+ the_repository->settings.sparse_checkout &&
core_sparse_checkout_cone) {
/*
* NEEDSWORK: we are *not* paying attention to
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 1bf01591b2..8329d29a27 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -62,7 +62,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
int res;
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!the_repository->settings.sparse_checkout)
die(_("this worktree is not sparse"));
argc = parse_options(argc, argv, prefix,
@@ -397,11 +397,11 @@ static int set_config(enum sparse_checkout_mode mode)
static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {
/* If not specified, use previous definition of cone mode */
- if (*cone_mode == -1 && core_apply_sparse_checkout)
+ if (*cone_mode == -1 && the_repository->settings.sparse_checkout)
*cone_mode = core_sparse_checkout_cone;
/* Set cone/non-cone mode appropriately */
- core_apply_sparse_checkout = 1;
+ the_repository->settings.sparse_checkout = 1;
if (*cone_mode == 1 || *cone_mode == -1) {
core_sparse_checkout_cone = 1;
return MODE_CONE_PATTERNS;
@@ -415,7 +415,7 @@ static int update_modes(int *cone_mode, int *sparse_index)
int mode, record_mode;
/* Determine if we need to record the mode; ensure sparse checkout on */
- record_mode = (*cone_mode != -1) || !core_apply_sparse_checkout;
+ record_mode = (*cone_mode != -1) || !the_repository->settings.sparse_checkout;
mode = update_cone_mode(cone_mode);
if (record_mode && set_config(mode))
@@ -695,9 +695,9 @@ static int modify_pattern_list(struct strvec *args, int use_stdin,
break;
}
- if (!core_apply_sparse_checkout) {
+ if (!the_repository->settings.sparse_checkout) {
set_config(MODE_ALL_PATTERNS);
- core_apply_sparse_checkout = 1;
+ the_repository->settings.sparse_checkout = 1;
changed_config = 1;
}
@@ -793,7 +793,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix,
int ret;
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!the_repository->settings.sparse_checkout)
die(_("no sparse-checkout to add to"));
repo_read_index(the_repository);
@@ -902,7 +902,7 @@ static int sparse_checkout_reapply(int argc, const char **argv,
};
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!the_repository->settings.sparse_checkout)
die(_("must be in a sparse-checkout to reapply sparsity patterns"));
reapply_opts.cone_mode = -1;
@@ -935,7 +935,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
struct pattern_list pl;
/*
- * We do not exit early if !core_apply_sparse_checkout; due to the
+ * We do not exit early if !sparse_checkout; due to the
* ability for users to manually muck things up between
* direct editing of .git/info/sparse-checkout
* running read-tree -m u HEAD or update-index --skip-worktree
@@ -961,11 +961,10 @@ static int sparse_checkout_disable(int argc, const char **argv,
hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
pl.use_cone_patterns = 0;
- core_apply_sparse_checkout = 1;
+ the_repository->settings.sparse_checkout = 1;
add_pattern("/*", empty_base, 0, &pl, 0);
- prepare_repo_settings(the_repository);
the_repository->settings.sparse_index = 0;
if (update_working_directory(&pl))
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 88a36ea9f8..92e1c92afc 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -536,7 +536,7 @@ static int add_worktree(const char *path, const char *refname,
* If the current worktree has sparse-checkout enabled, then copy
* the sparse-checkout patterns from the current worktree.
*/
- if (core_apply_sparse_checkout)
+ if (the_repository->settings.sparse_checkout)
copy_sparse_checkout(sb_repo.buf);
/*
diff --git a/config.c b/config.c
index b18b5617fc..8fd4dd8c81 100644
--- a/config.c
+++ b/config.c
@@ -1612,11 +1612,6 @@ static int git_default_core_config(const char *var, const char *value,
return 0;
}
- if (!strcmp(var, "core.sparsecheckout")) {
- core_apply_sparse_checkout = git_config_bool(var, value);
- return 0;
- }
-
if (!strcmp(var, "core.sparsecheckoutcone")) {
core_sparse_checkout_cone = git_config_bool(var, value);
return 0;
diff --git a/dir.c b/dir.c
index a374972b62..5d1cb7a067 100644
--- a/dir.c
+++ b/dir.c
@@ -1503,7 +1503,8 @@ enum pattern_match_result path_matches_pattern_list(
int init_sparse_checkout_patterns(struct index_state *istate)
{
- if (!core_apply_sparse_checkout)
+ prepare_repo_settings(istate->repo);
+ if (!istate->repo->settings.sparse_checkout)
return 1;
if (istate->sparse_checkout_patterns)
return 0;
diff --git a/environment.c b/environment.c
index c61d773e7e..a379a9149e 100644
--- a/environment.c
+++ b/environment.c
@@ -64,7 +64,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#endif
enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
-int core_apply_sparse_checkout;
int core_sparse_checkout_cone;
int sparse_expect_files_outside_of_patterns;
int merge_log_config = -1;
diff --git a/environment.h b/environment.h
index 3d98461a06..6a30512f3c 100644
--- a/environment.h
+++ b/environment.h
@@ -160,7 +160,6 @@ extern int precomposed_unicode;
extern int protect_hfs;
extern int protect_ntfs;
-extern int core_apply_sparse_checkout;
extern int core_sparse_checkout_cone;
extern int sparse_expect_files_outside_of_patterns;
diff --git a/repo-settings.c b/repo-settings.c
index 4129f8fb2b..9270cca561 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -81,6 +81,7 @@ void prepare_repo_settings(struct repository *r)
&r->settings.pack_use_bitmap_boundary_traversal,
r->settings.pack_use_bitmap_boundary_traversal);
repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
+ repo_cfg_bool(r, "core.sparsecheckout", &r->settings.sparse_checkout, 0);
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
diff --git a/repo-settings.h b/repo-settings.h
index 2bf24b2597..9caa7c57a3 100644
--- a/repo-settings.h
+++ b/repo-settings.h
@@ -67,6 +67,7 @@ struct repo_settings {
unsigned long big_file_threshold;
char *hooks_path;
+ int sparse_checkout;
};
#define REPO_SETTINGS_INIT { \
.shared_repository = -1, \
diff --git a/sparse-index.c b/sparse-index.c
index 5634abafaa..8132c0f2fb 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -150,7 +150,8 @@ static int index_has_unmerged_entries(struct index_state *istate)
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
- if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
+ prepare_repo_settings(istate->repo);
+ if (!istate->repo->settings.sparse_checkout || !core_sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
@@ -172,7 +173,6 @@ int is_sparse_index_allowed(struct index_state *istate, int flags)
/*
* Only convert to sparse if index.sparse is set.
*/
- prepare_repo_settings(istate->repo);
if (!istate->repo->settings.sparse_index)
return 0;
}
@@ -668,7 +668,7 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista
void clear_skip_worktree_from_present_files(struct index_state *istate)
{
- if (!core_apply_sparse_checkout ||
+ if (!istate->repo->settings.sparse_checkout ||
sparse_expect_files_outside_of_patterns)
return;
diff --git a/unpack-trees.c b/unpack-trees.c
index 471837f032..02e32c4ba1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1924,7 +1924,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
if (o->prefix)
update_sparsity_for_prefix(o->prefix, o->src_index);
- if (!core_apply_sparse_checkout || !o->update)
+ if (!repo->settings.sparse_checkout || !o->update)
o->skip_sparse_checkout = 1;
if (!o->skip_sparse_checkout) {
memset(&pl, 0, sizeof(pl));
diff --git a/wt-status.c b/wt-status.c
index 454601afa1..8651134bcb 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1773,7 +1773,8 @@ static void wt_status_check_sparse_checkout(struct repository *r,
int skip_worktree = 0;
int i;
- if (!core_apply_sparse_checkout || r->index->cache_nr == 0) {
+ prepare_repo_settings(r);
+ if (!r->settings.sparse_checkout || r->index->cache_nr == 0) {
/*
* Don't compute percentage of checked out files if we
* aren't in a sparse checkout or would get division by 0.
--
2.49.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* [GSOC PATCH v5 2/3] environment: move access to "core.sparsecheckoutcone" into repo_settings
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 ` 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
` (2 subsequent siblings)
4 siblings, 0 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-30 19:27 UTC (permalink / raw)
To: ayu.chandekar
Cc: christian.couder, git, shyamthakkar001, gitster, ps, ben.knoble
The setting "core.sparsecheckoutcone" is stored in the global
`core_sparse_checkout_cone` and is populated in config.c. Refactor the
code to store it in the variable `sparse_checkout_cone` in the struct
`repo_settings`.
Call `prepare_repo_settings()` where necessary to ensure the `struct
repo_settings` is initialized before use:
- In "dir.c", the function accessing the setting is usually called after
`prepare_repo_settings()`, except for one code path in
"unpack-trees.c", so add a call there.
Avoid redundant calls to `prepare_repo_settings()` where it is already
present:
- In "builtin/mv.c" and "builtin/sparse-checkout.c", it is already
invoked in their respective `cmd_*()` functions.
- In "sparse-index.c", `prepare_repo_settings` is already called before
the setting is accessed.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
builtin/grep.c | 2 +-
builtin/mv.c | 2 +-
builtin/sparse-checkout.c | 28 ++++++++++++++--------------
config.c | 5 -----
dir.c | 2 +-
environment.c | 1 -
environment.h | 1 -
repo-settings.c | 1 +
repo-settings.h | 2 ++
sparse-index.c | 2 +-
unpack-trees.c | 1 +
11 files changed, 22 insertions(+), 25 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 6111fee60b..8971a15ef4 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -491,7 +491,7 @@ static int grep_submodule(struct grep_opt *opt,
* dictate the behavior for the submodule, making it "forget" its
* sparse-checkout state.
*
- * 3. "core_sparse_checkout_cone"
+ * 3. "settings.sparse_checkout_cone"
* ditto.
*
* Note that this list is not exhaustive.
diff --git a/builtin/mv.c b/builtin/mv.c
index 43ed2e3d0a..2d1326a18f 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -573,7 +573,7 @@ int cmd_mv(int argc,
prepare_repo_settings(the_repository);
if (ignore_sparse &&
the_repository->settings.sparse_checkout &&
- core_sparse_checkout_cone) {
+ the_repository->settings.sparse_checkout_cone) {
/*
* NEEDSWORK: we are *not* paying attention to
* "out-to-out" move (<source> is out-of-cone and
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 8329d29a27..8a0ffba9d4 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -71,7 +71,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
memset(&pl, 0, sizeof(pl));
- pl.use_cone_patterns = core_sparse_checkout_cone;
+ pl.use_cone_patterns = the_repository->settings.sparse_checkout_cone;
sparse_filename = get_sparse_checkout_filename();
res = add_patterns_from_file_to_list(sparse_filename, "", 0, &pl, NULL, 0);
@@ -352,7 +352,7 @@ static int write_patterns_and_update(struct pattern_list *pl)
if (!fp)
die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk));
- if (core_sparse_checkout_cone)
+ if (the_repository->settings.sparse_checkout_cone)
write_cone_to_file(fp, pl);
else
write_patterns_to_file(fp, pl);
@@ -398,15 +398,15 @@ static int set_config(enum sparse_checkout_mode mode)
static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {
/* If not specified, use previous definition of cone mode */
if (*cone_mode == -1 && the_repository->settings.sparse_checkout)
- *cone_mode = core_sparse_checkout_cone;
+ *cone_mode = the_repository->settings.sparse_checkout_cone;
/* Set cone/non-cone mode appropriately */
the_repository->settings.sparse_checkout = 1;
if (*cone_mode == 1 || *cone_mode == -1) {
- core_sparse_checkout_cone = 1;
+ the_repository->settings.sparse_checkout_cone = 1;
return MODE_CONE_PATTERNS;
}
- core_sparse_checkout_cone = 0;
+ the_repository->settings.sparse_checkout_cone = 0;
return MODE_ALL_PATTERNS;
}
@@ -572,7 +572,7 @@ static void add_patterns_from_input(struct pattern_list *pl,
FILE *file)
{
int i;
- if (core_sparse_checkout_cone) {
+ if (the_repository->settings.sparse_checkout_cone) {
struct strbuf line = STRBUF_INIT;
hashmap_init(&pl->recursive_hashmap, pl_hashmap_cmp, NULL, 0);
@@ -637,7 +637,7 @@ static void add_patterns_cone_mode(int argc, const char **argv,
use_stdin ? stdin : NULL);
memset(&existing, 0, sizeof(existing));
- existing.use_cone_patterns = core_sparse_checkout_cone;
+ existing.use_cone_patterns = the_repository->settings.sparse_checkout_cone;
if (add_patterns_from_file_to_list(sparse_filename, "", 0,
&existing, NULL, 0))
@@ -683,7 +683,7 @@ static int modify_pattern_list(struct strvec *args, int use_stdin,
switch (m) {
case ADD:
- if (core_sparse_checkout_cone)
+ if (the_repository->settings.sparse_checkout_cone)
add_patterns_cone_mode(args->nr, args->v, pl, use_stdin);
else
add_patterns_literal(args->nr, args->v, pl, use_stdin);
@@ -719,7 +719,7 @@ static void sanitize_paths(struct strvec *args,
if (!args->nr)
return;
- if (prefix && *prefix && core_sparse_checkout_cone) {
+ if (prefix && *prefix && the_repository->settings.sparse_checkout_cone) {
/*
* The args are not pathspecs, so unfortunately we
* cannot imitate how cmd_add() uses parse_pathspec().
@@ -736,10 +736,10 @@ static void sanitize_paths(struct strvec *args,
if (skip_checks)
return;
- if (prefix && *prefix && !core_sparse_checkout_cone)
+ if (prefix && *prefix && !the_repository->settings.sparse_checkout_cone)
die(_("please run from the toplevel directory in non-cone mode"));
- if (core_sparse_checkout_cone) {
+ if (the_repository->settings.sparse_checkout_cone) {
for (i = 0; i < args->nr; i++) {
if (args->v[i][0] == '/')
die(_("specify directories rather than patterns (no leading slash)"));
@@ -761,7 +761,7 @@ static void sanitize_paths(struct strvec *args,
if (S_ISSPARSEDIR(ce->ce_mode))
continue;
- if (core_sparse_checkout_cone)
+ if (the_repository->settings.sparse_checkout_cone)
die(_("'%s' is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), args->v[i]);
else
warning(_("pass a leading slash before paths such as '%s' if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), args->v[i]);
@@ -864,7 +864,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix,
* non-cone mode, if nothing is specified, manually select just the
* top-level directory (much as 'init' would do).
*/
- if (!core_sparse_checkout_cone && !set_opts.use_stdin && argc == 0) {
+ if (!the_repository->settings.sparse_checkout_cone && !set_opts.use_stdin && argc == 0) {
for (int i = 0; i < default_patterns_nr; i++)
strvec_push(&patterns, default_patterns[i]);
} else {
@@ -1041,7 +1041,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char *
check_rules_opts.cone_mode = 1;
update_cone_mode(&check_rules_opts.cone_mode);
- pl.use_cone_patterns = core_sparse_checkout_cone;
+ pl.use_cone_patterns = the_repository->settings.sparse_checkout_cone;
if (check_rules_opts.rules_file) {
fp = xfopen(check_rules_opts.rules_file, "r");
add_patterns_from_input(&pl, argc, argv, fp);
diff --git a/config.c b/config.c
index 8fd4dd8c81..707fe0707a 100644
--- a/config.c
+++ b/config.c
@@ -1612,11 +1612,6 @@ static int git_default_core_config(const char *var, const char *value,
return 0;
}
- if (!strcmp(var, "core.sparsecheckoutcone")) {
- core_sparse_checkout_cone = git_config_bool(var, value);
- return 0;
- }
-
if (!strcmp(var, "core.precomposeunicode")) {
precomposed_unicode = git_config_bool(var, value);
return 0;
diff --git a/dir.c b/dir.c
index 5d1cb7a067..d2b0a5aef6 100644
--- a/dir.c
+++ b/dir.c
@@ -3460,7 +3460,7 @@ int get_sparse_checkout_patterns(struct pattern_list *pl)
int res;
char *sparse_filename = get_sparse_checkout_filename();
- pl->use_cone_patterns = core_sparse_checkout_cone;
+ pl->use_cone_patterns = the_repository->settings.sparse_checkout_cone;
res = add_patterns_from_file_to_list(sparse_filename, "", 0, pl, NULL, 0);
free(sparse_filename);
diff --git a/environment.c b/environment.c
index a379a9149e..7d46b80711 100644
--- a/environment.c
+++ b/environment.c
@@ -64,7 +64,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#endif
enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
-int core_sparse_checkout_cone;
int sparse_expect_files_outside_of_patterns;
int merge_log_config = -1;
int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
diff --git a/environment.h b/environment.h
index 6a30512f3c..00a5b332a0 100644
--- a/environment.h
+++ b/environment.h
@@ -160,7 +160,6 @@ extern int precomposed_unicode;
extern int protect_hfs;
extern int protect_ntfs;
-extern int core_sparse_checkout_cone;
extern int sparse_expect_files_outside_of_patterns;
enum rebase_setup_type {
diff --git a/repo-settings.c b/repo-settings.c
index 9270cca561..eebc1f941d 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -82,6 +82,7 @@ void prepare_repo_settings(struct repository *r)
r->settings.pack_use_bitmap_boundary_traversal);
repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
repo_cfg_bool(r, "core.sparsecheckout", &r->settings.sparse_checkout, 0);
+ repo_cfg_bool(r, "core.sparsecheckoutcone", &r->settings.sparse_checkout_cone, 0);
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
diff --git a/repo-settings.h b/repo-settings.h
index 9caa7c57a3..443e1399da 100644
--- a/repo-settings.h
+++ b/repo-settings.h
@@ -67,7 +67,9 @@ struct repo_settings {
unsigned long big_file_threshold;
char *hooks_path;
+
int sparse_checkout;
+ int sparse_checkout_cone;
};
#define REPO_SETTINGS_INIT { \
.shared_repository = -1, \
diff --git a/sparse-index.c b/sparse-index.c
index 8132c0f2fb..6fd782a8fc 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -151,7 +151,7 @@ static int index_has_unmerged_entries(struct index_state *istate)
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
prepare_repo_settings(istate->repo);
- if (!istate->repo->settings.sparse_checkout || !core_sparse_checkout_cone)
+ if (!istate->repo->settings.sparse_checkout || !istate->repo->settings.sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
diff --git a/unpack-trees.c b/unpack-trees.c
index 02e32c4ba1..0e9813bddf 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2144,6 +2144,7 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o,
BUG("update_sparsity() called wrong");
trace_performance_enter();
+ prepare_repo_settings(the_repository);
/* If we weren't given patterns, use the recorded ones */
if (!pl) {
--
2.49.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* [GSOC PATCH v5 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
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 ` Ayush Chandekar
2025-07-01 13:18 ` Phillip Wood
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
4 siblings, 1 reply; 50+ messages in thread
From: Ayush Chandekar @ 2025-06-30 19:27 UTC (permalink / raw)
To: ayu.chandekar
Cc: christian.couder, git, shyamthakkar001, gitster, ps, ben.knoble
The global variable 'sparse_expect_files_outside_of_patterns' is used in
a single function named 'clear_skip_worktree_from_present_files()' in
sparse-index.c. Move its declaration inside that function, removing
unnecessary global state.
This also allows us to remove the definition '#define
USE_THE_REPOSITORY_VARIABLE' from the file 'sparse-index.c'.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
config.c | 14 --------------
environment.c | 1 -
environment.h | 2 --
sparse-index.c | 4 +++-
4 files changed, 3 insertions(+), 18 deletions(-)
diff --git a/config.c b/config.c
index 707fe0707a..d212329799 100644
--- a/config.c
+++ b/config.c
@@ -1636,17 +1636,6 @@ static int git_default_core_config(const char *var, const char *value,
return platform_core_config(var, value, ctx, cb);
}
-static int git_default_sparse_config(const char *var, const char *value)
-{
- if (!strcmp(var, "sparse.expectfilesoutsideofpatterns")) {
- sparse_expect_files_outside_of_patterns = git_config_bool(var, value);
- return 0;
- }
-
- /* Add other config variables here and to Documentation/config/sparse.adoc. */
- return 0;
-}
-
static int git_default_i18n_config(const char *var, const char *value)
{
if (!strcmp(var, "i18n.commitencoding")) {
@@ -1808,9 +1797,6 @@ int git_default_config(const char *var, const char *value,
return 0;
}
- if (starts_with(var, "sparse."))
- return git_default_sparse_config(var, value);
-
/* Add other config variables here and to Documentation/config.adoc. */
return 0;
}
diff --git a/environment.c b/environment.c
index 7d46b80711..d51e0a14aa 100644
--- a/environment.c
+++ b/environment.c
@@ -64,7 +64,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#endif
enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
-int sparse_expect_files_outside_of_patterns;
int merge_log_config = -1;
int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
unsigned long pack_size_limit_cfg;
diff --git a/environment.h b/environment.h
index 00a5b332a0..5121a28d3f 100644
--- a/environment.h
+++ b/environment.h
@@ -160,8 +160,6 @@ extern int precomposed_unicode;
extern int protect_hfs;
extern int protect_ntfs;
-extern int sparse_expect_files_outside_of_patterns;
-
enum rebase_setup_type {
AUTOREBASE_NEVER = 0,
AUTOREBASE_LOCAL,
diff --git a/sparse-index.c b/sparse-index.c
index 6fd782a8fc..ff33b8516b 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -1,4 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
#define DISABLE_SIGN_COMPARE_WARNINGS
#include "git-compat-util.h"
@@ -668,6 +667,9 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista
void clear_skip_worktree_from_present_files(struct index_state *istate)
{
+ int sparse_expect_files_outside_of_patterns = 0;
+ repo_config_get_bool(istate->repo, "sparse.expectfilesoutsideofpatterns",
+ &sparse_expect_files_outside_of_patterns);
if (!istate->repo->settings.sparse_checkout ||
sparse_expect_files_outside_of_patterns)
return;
--
2.49.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v5 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
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
0 siblings, 1 reply; 50+ messages in thread
From: Phillip Wood @ 2025-07-01 13:18 UTC (permalink / raw)
To: Ayush Chandekar
Cc: christian.couder, git, shyamthakkar001, gitster, ps, ben.knoble
Hi Ayush
On 30/06/2025 20:27, Ayush Chandekar wrote:
>
> void clear_skip_worktree_from_present_files(struct index_state *istate)
> {
> + int sparse_expect_files_outside_of_patterns = 0;
> + repo_config_get_bool(istate->repo, "sparse.expectfilesoutsideofpatterns",
> + &sparse_expect_files_outside_of_patterns);
This changes the user facing behavior if
sparse.expectfilesoutsideofpatterns is not a valid boolean value.
Currently git will error out when it first starts because that config
value is parsed by git_default_config() which is called by almost all
git commands. This means that if someone sets an invalid value they get
timely feedback that the value is invalid and git dies before doing
anything. Now, if the value is invalid, git will only die if this
function is called and it is likely to die in the middle of a command.
Thanks
Phillip
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v5 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
2025-07-01 13:18 ` Phillip Wood
@ 2025-07-01 23:53 ` Ayush Chandekar
2025-07-02 9:01 ` Phillip Wood
2025-07-02 9:21 ` Junio C Hamano
0 siblings, 2 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-07-01 23:53 UTC (permalink / raw)
To: phillip.wood
Cc: christian.couder, git, shyamthakkar001, gitster, ps, ben.knoble
On Tue, Jul 1, 2025 at 6:48 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Ayush
Hi Phillip,
>
> On 30/06/2025 20:27, Ayush Chandekar wrote:
> >
> > void clear_skip_worktree_from_present_files(struct index_state *istate)
> > {
> > + int sparse_expect_files_outside_of_patterns = 0;
> > + repo_config_get_bool(istate->repo, "sparse.expectfilesoutsideofpatterns",
> > + &sparse_expect_files_outside_of_patterns);
>
> This changes the user facing behavior if
> sparse.expectfilesoutsideofpatterns is not a valid boolean value.
> Currently git will error out when it first starts because that config
> value is parsed by git_default_config() which is called by almost all
> git commands. This means that if someone sets an invalid value they get
> timely feedback that the value is invalid and git dies before doing
> anything. Now, if the value is invalid, git will only die if this
> function is called and it is likely to die in the middle of a command.
>
> Thanks
>
> Phillip
>
Yes, I get your point. However, if we look at settings which are
shifted to `struct repo_settings`, the behaviour is to set a
fallback/default value in case of an invalid input, instead of
throwing an error. This is done inside the `prepare_repo_settings()`
function, which is often called in the middle of a process.
Thanks
Ayush:)
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v5 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
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
1 sibling, 1 reply; 50+ messages in thread
From: Phillip Wood @ 2025-07-02 9:01 UTC (permalink / raw)
To: Ayush Chandekar, phillip.wood
Cc: christian.couder, git, shyamthakkar001, gitster, ps, ben.knoble
Hi Ayush
On 02/07/2025 00:53, Ayush Chandekar wrote:
> On Tue, Jul 1, 2025 at 6:48 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 30/06/2025 20:27, Ayush Chandekar wrote:
>>>
>>> void clear_skip_worktree_from_present_files(struct index_state *istate)
>>> {
>>> + int sparse_expect_files_outside_of_patterns = 0;
>>> + repo_config_get_bool(istate->repo, "sparse.expectfilesoutsideofpatterns",
>>> + &sparse_expect_files_outside_of_patterns);
>>
>> This changes the user facing behavior if
>> sparse.expectfilesoutsideofpatterns is not a valid boolean value.
>> Currently git will error out when it first starts because that config
>> value is parsed by git_default_config() which is called by almost all
>> git commands. This means that if someone sets an invalid value they get
>> timely feedback that the value is invalid and git dies before doing
>> anything. Now, if the value is invalid, git will only die if this
>> function is called and it is likely to die in the middle of a command.
>
> Yes, I get your point. However, if we look at settings which are
> shifted to `struct repo_settings`, the behaviour is to set a
> fallback/default value in case of an invalid input, instead of
> throwing an error. This is done inside the `prepare_repo_settings()`
> function, which is often called in the middle of a process.
I'm a bit confused by this and I'm not quite sure what you're saying for
a couple of reasons. Firstly this patch is not adding a new member to
struct repo_settings, it is parsing the config directly and will die()
in git_config_bool() if the config value is invalid. Secondly
prepare_repo_settings() ends up calling git_config_bool() and so will
also die if the config value is invalid rather than setting a default
value. In the case of prepare_repo_settings() commands that do not want
to die in the middle of an operation can call that function early on
before they start doing any real work. Looking at the output of "git
grep prepare_repo_settings()" many do exactly that. Here there is no
option for a command to die() early on invalid config values if it wants to.
Thanks
Phillip
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v5 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
2025-07-02 9:01 ` Phillip Wood
@ 2025-07-11 19:24 ` Ayush Chandekar
0 siblings, 0 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-07-11 19:24 UTC (permalink / raw)
To: phillip.wood
Cc: christian.couder, git, shyamthakkar001, gitster, ps, ben.knoble
Hi Phillip,
On Wed, Jul 2, 2025 at 2:31 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Ayush
>
> On 02/07/2025 00:53, Ayush Chandekar wrote:
> > On Tue, Jul 1, 2025 at 6:48 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >> On 30/06/2025 20:27, Ayush Chandekar wrote:
> >>>
> >>> void clear_skip_worktree_from_present_files(struct index_state *istate)
> >>> {
> >>> + int sparse_expect_files_outside_of_patterns = 0;
> >>> + repo_config_get_bool(istate->repo, "sparse.expectfilesoutsideofpatterns",
> >>> + &sparse_expect_files_outside_of_patterns);
> >>
> >> This changes the user facing behavior if
> >> sparse.expectfilesoutsideofpatterns is not a valid boolean value.
> >> Currently git will error out when it first starts because that config
> >> value is parsed by git_default_config() which is called by almost all
> >> git commands. This means that if someone sets an invalid value they get
> >> timely feedback that the value is invalid and git dies before doing
> >> anything. Now, if the value is invalid, git will only die if this
> >> function is called and it is likely to die in the middle of a command.
> >
> > Yes, I get your point. However, if we look at settings which are
> > shifted to `struct repo_settings`, the behaviour is to set a
> > fallback/default value in case of an invalid input, instead of
> > throwing an error. This is done inside the `prepare_repo_settings()`
> > function, which is often called in the middle of a process.
>
> I'm a bit confused by this and I'm not quite sure what you're saying for
> a couple of reasons. Firstly this patch is not adding a new member to
> struct repo_settings, it is parsing the config directly and will die()
> in git_config_bool() if the config value is invalid. Secondly
> prepare_repo_settings() ends up calling git_config_bool() and so will
> also die if the config value is invalid rather than setting a default
> value. In the case of prepare_repo_settings() commands that do not want
> to die in the middle of an operation can call that function early on
> before they start doing any real work. Looking at the output of "git
> grep prepare_repo_settings()" many do exactly that. Here there is no
> option for a command to die() early on invalid config values if it wants to.
>
> Thanks
>
> Phillip
>
Yeah, you're right about the `prepare_repo_settings()` which throws
error and since they're called early on, we are notified about it
before any heavy operation takes place. I had it confused as it also
stores a default value if there isn't a config setting set. I can move
this setting into `struct repo_settings`, and since
`prepare_repo_settings()` is already called just before the function
that uses this variable, I don't have to add any extra call to it.
Thanks!
Ayush
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [GSOC PATCH v5 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
2025-07-01 23:53 ` Ayush Chandekar
2025-07-02 9:01 ` Phillip Wood
@ 2025-07-02 9:21 ` Junio C Hamano
2025-07-11 19:35 ` Ayush Chandekar
1 sibling, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2025-07-02 9:21 UTC (permalink / raw)
To: Ayush Chandekar
Cc: phillip.wood, christian.couder, git, shyamthakkar001, ps,
ben.knoble
Ayush Chandekar <ayu.chandekar@gmail.com> writes:
> Yes, I get your point. However, if we look at settings which are
> shifted to `struct repo_settings`, the behaviour is to set a
> fallback/default value in case of an invalid input, instead of
> throwing an error.
So the user will not be told about misconfiguration like they used
to? Is that an acceptable way forward, I have to wonder...
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [GSOC PATCH v5 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
2025-07-02 9:21 ` Junio C Hamano
@ 2025-07-11 19:35 ` Ayush Chandekar
0 siblings, 0 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-07-11 19:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: phillip.wood, christian.couder, git, shyamthakkar001, ps,
ben.knoble
On Wed, Jul 2, 2025 at 2:51 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Ayush Chandekar <ayu.chandekar@gmail.com> writes:
>
> > Yes, I get your point. However, if we look at settings which are
> > shifted to `struct repo_settings`, the behaviour is to set a
> > fallback/default value in case of an invalid input, instead of
> > throwing an error.
>
> So the user will not be told about misconfiguration like they used
> to? Is that an acceptable way forward, I have to wonder...
>
I was actually mistaken that `prepare_repo_settings()` does not throw
any error due to the fact that it sets default value when a config
setting is not set. So I thought, if there's invalid value in the
config, it would just set the default value, but I was wrong.
Thanks,
Ayush
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [GSOC PATCH v5 0/3] environment: remove sparse-checkout related global variables
2025-06-30 19:27 ` [GSOC PATCH v5 0/3] environment: remove sparse-checkout related global variables Ayush Chandekar
` (2 preceding siblings ...)
2025-06-30 19:27 ` [GSOC PATCH v5 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns' Ayush Chandekar
@ 2025-06-30 21:08 ` Junio C Hamano
2025-07-09 0:18 ` Junio C Hamano
4 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2025-06-30 21:08 UTC (permalink / raw)
To: Ayush Chandekar; +Cc: christian.couder, git, shyamthakkar001, ps, ben.knoble
Ayush Chandekar <ayu.chandekar@gmail.com> writes:
> This patch series aims to remove global variables related to
> sparse-checkout from the global scope and to remove the definition
> '#define USE_THE_REPOSITORY_VARIABLE' from a few files.
>
> It contains three patches:
>
> 1 - Remove the global variable 'core_apply_sparse_checkout' and
> move its setting to the 'struct repo_settings'. Also remove the
> definition '#define USE_THE_REPOSITORY_VARIABLE' from
> "builtin/backfill.c".
>
> 2 - Remove the global variable 'core_sparse_checkout_cone' and
> move its setting to the 'struct repo_settings'.
>
> 3 - Remove the global variable
> 'sparse_expect_files_outside_of_patterns` and localize it in the
> function which calls it. Also remove the definition '#define
> USE_THE_REPOSITORY_VARIABLE' from "sparse-index.c"
A call to prepare_repo_settings() function is not free even though
the .settings.initialized member makes second and subsequent calls
to it as cheap as possible. It makes me a bit worried to see
patches that add new calls to it to places that are fairly deep in
the callchain (as opposed to in cmd_foo() for various built-in
commands). As long as the control passes these places only once
before we do the heavy lifting and then after the heavy lifting,
the only thing left for us is to exit, we would be fine, but I do
not know if all new calls added in these patches are that kind.
Thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v5 0/3] environment: remove sparse-checkout related global variables
2025-06-30 19:27 ` [GSOC PATCH v5 0/3] environment: remove sparse-checkout related global variables Ayush Chandekar
` (3 preceding siblings ...)
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
4 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2025-07-09 0:18 UTC (permalink / raw)
To: Ayush Chandekar; +Cc: christian.couder, git, shyamthakkar001, ps, ben.knoble
Ayush Chandekar <ayu.chandekar@gmail.com> writes:
> This patch series aims to remove global variables related to
> sparse-checkout from the global scope and to remove the definition
> '#define USE_THE_REPOSITORY_VARIABLE' from a few files.
For this topic, it seems that the ball is in the author's court
after a few review comments. What's the status of it?
If it is back-burnered for now, then we may eject the topic out of
'seen' to make room for a new topic that touches the same area with
overlapping changes.
Thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v5 0/3] environment: remove sparse-checkout related global variables
2025-07-09 0:18 ` Junio C Hamano
@ 2025-07-09 1:39 ` Ayush Chandekar
0 siblings, 0 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-07-09 1:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: christian.couder, git, shyamthakkar001, ps, ben.knoble
Hi Junio,
On Wed, Jul 9, 2025 at 5:48 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Ayush Chandekar <ayu.chandekar@gmail.com> writes:
>
> > This patch series aims to remove global variables related to
> > sparse-checkout from the global scope and to remove the definition
> > '#define USE_THE_REPOSITORY_VARIABLE' from a few files.
>
> For this topic, it seems that the ball is in the author's court
> after a few review comments. What's the status of it?
>
> If it is back-burnered for now, then we may eject the topic out of
> 'seen' to make room for a new topic that touches the same area with
> overlapping changes.
>
> Thanks.
>
Apologies for not keeping you updated. I had actually drafted a
response explaining that in 1/3, I only added two new calls to
`prepare_repo_settings()`. Some other calls were made in cmd_foo()
functions, and a few others were rearranged and removed as well. (I
should've made it clearer in the commit message). In fact, the commit
message mistaknely suggests that one extra call was added which wasn't
the case. For 2/3, only one new call was added. As for 3/3, I can move
the setting into the repo_settings struct, since
`prepare_repo_settings()` is already being called before the variable
is accessed.
At the time, I felt my draft wasn't convincinv enough, and then got
caught up with responses on other patch series and also was working on
another one.
You can eject this topic if you think another one is more important or
if this one still needs work.
Again, Sorry for not updating earlied, will do better next time.
Thanks,
Ayush
^ permalink raw reply [flat|nested] 50+ messages in thread
* [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
2025-06-03 13:18 [GSOC PATCH] environment: move access to "core.sparsecheckout" into repo_settings Ayush Chandekar
` (4 preceding siblings ...)
2025-06-30 19:27 ` [GSOC PATCH v5 0/3] environment: remove sparse-checkout related global variables Ayush Chandekar
@ 2025-07-19 0:11 ` Ayush Chandekar
2025-07-19 0:11 ` [GSOC PATCH v6 1/3] environment: move access to "core.sparsecheckout" into repo_settings Ayush Chandekar
` (4 more replies)
5 siblings, 5 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-07-19 0:11 UTC (permalink / raw)
To: ayu.chandekar
Cc: christian.couder, git, shyamthakkar001, phillip.wood123, ps,
gitster, ben.knoble
This patch series aims to remove global variables related to sparse-checkout from the global scope and to remove the definition '#define USE_THE_REPOSITORY_VARIABLE' from a few files.
It contains three patches:
1 - Remove the global variable 'core_apply_sparse_checkout' and move its setting to the 'struct repo_settings'. Also remove the definition '#define USE_THE_REPOSITORY_VARIABLE' from "builtin/backfill.c".
2 - Remove the global variable 'core_sparse_checkout_cone' and move its setting to the 'struct repo_settings'.
3 - Remove the global variable 'sparse_expect_files_outside_of_patterns` and move its setting to 'struct repo_settings'. Also remove the definition '#define USE_THE_REPOSITORY_VARIABLE' from "sparse-index.c"
Thanks a lot to Christian for mentoring, and to Junio, Patrick, Phillip and Ben for reviewing
Ayush Chandekar (3):
environment: move access to "core.sparsecheckout" into repo_settings
environment: move access to "core.sparsecheckoutcone" into
repo_settings
environment: remove the global variable
'sparse_expect_files_outside_of_patterns'
builtin/backfill.c | 7 ++----
builtin/clone.c | 3 ++-
builtin/grep.c | 4 ++--
builtin/mv.c | 6 ++---
builtin/sparse-checkout.c | 49 +++++++++++++++++++--------------------
builtin/worktree.c | 2 +-
config.c | 24 -------------------
dir.c | 5 ++--
environment.c | 3 ---
environment.h | 4 ----
repo-settings.c | 3 +++
repo-settings.h | 4 ++++
sparse-index.c | 9 ++++---
unpack-trees.c | 2 +-
wt-status.c | 3 ++-
15 files changed, 51 insertions(+), 77 deletions(-)
--
Discussions since v5:
* For 1/3 and 2/3, Junio told me that it was concerning to put so many calls to `prepare_repo_settings()` so I tried to minimize the calls and made sure that there's no useless calling.
* 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.
Main problem I was dealing with was `prepare_repo_settings()`. I think we can try to get useless calls to `prepare_repo_settings()`. Other than that, I agree with the approach that came up in the discussion here:
https://lore.kernel.org/git/32fceddc-c867-4a47-bde8-c873279edbc1@gmail.com/
which was adding a call to `prepare_repo_settings()` in `repo_config()`.
Summary of range-diff:
* Changed some calls to `prepare_repo_settings()` to make sure that it is added in code paths only where it has no been called before.
Also changed a mistake in commit message: s/sparse-checkout.c/sparse-index.c and have explained it more.
* Shifted the global variable to `struct repo_settings`(the previous version just localized it leading to user experience issues) and changed the commit message
Range-diff with v5:
1: 54ea376768 ! 1: d0e2042b30 environment: move access to "core.sparsecheckout" into repo_settings
@@ Commit message
- In "dir.c", the function using 'settings.sparse_checkout' is invoked
in multiple files that do not call `prepare_repo_settings()`, hence
add a call directly to that function.
- - In "sparse-checkout.c", add a call to `prepare_repo_settings()` inside
- `is_sparse_index_allowed()`, as it is used widely and relies on the
- setting.
+ - In "sparse-index.c", remove a call to `prepare_repo_settings()`
+ from the function `is_sparse_index_allowed()` as it is called
+ everytime before the function is called, and add a call to
+ `prepare_repo_settings()` inside `convert_to_sparse()`, as it is
+ used widely without having a call to `prepare_repo_settings()`
+ before and relies on the setting.
- In "wt-status.c", call `prepare_repo_settings()` before accessing
the setting because the function using it is commonly used.
@@ dir.c: enum pattern_match_result path_matches_pattern_list(
int init_sparse_checkout_patterns(struct index_state *istate)
{
- if (!core_apply_sparse_checkout)
-+ prepare_repo_settings(istate->repo);
+ if (!istate->repo->settings.sparse_checkout)
return 1;
if (istate->sparse_checkout_patterns)
return 0;
+@@ dir.c: static int path_in_sparse_checkout_1(const char *path,
+ enum pattern_match_result match = UNDECIDED;
+ const char *end, *slash;
+
++ prepare_repo_settings(istate->repo);
+ /*
+ * We default to accepting a path if the path is empty, there are no
+ * patterns, or the patterns are of the wrong type.
## environment.c ##
@@ environment.c: enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
@@ sparse-index.c: static int index_has_unmerged_entries(struct index_state *istate
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
- if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
-+ prepare_repo_settings(istate->repo);
+ if (!istate->repo->settings.sparse_checkout || !core_sparse_checkout_cone)
return 0;
@@ sparse-index.c: int is_sparse_index_allowed(struct index_state *istate, int flag
if (!istate->repo->settings.sparse_index)
return 0;
}
+@@ sparse-index.c: int is_sparse_index_allowed(struct index_state *istate, int flags)
+
+ int convert_to_sparse(struct index_state *istate, int flags)
+ {
++ prepare_repo_settings(istate->repo);
+ /*
+ * If the index is already sparse, empty, or otherwise
+ * cannot be converted to sparse, do not convert.
@@ sparse-index.c: static void clear_skip_worktree_from_present_files_full(struct index_state *ista
void clear_skip_worktree_from_present_files(struct index_state *istate)
2: c4d37dc7c5 ! 2: d799faca3f environment: move access to "core.sparsecheckoutcone" into repo_settings
@@ Commit message
code to store it in the variable `sparse_checkout_cone` in the struct
`repo_settings`.
- Call `prepare_repo_settings()` where necessary to ensure the `struct
- repo_settings` is initialized before use:
- - In "dir.c", the function accessing the setting is usually called after
- `prepare_repo_settings()`, except for one code path in
- "unpack-trees.c", so add a call there.
-
Avoid redundant calls to `prepare_repo_settings()` where it is already
present:
- In "builtin/mv.c" and "builtin/sparse-checkout.c", it is already
invoked in their respective `cmd_*()` functions.
- - In "sparse-index.c", `prepare_repo_settings` is already called before
- the setting is accessed.
+ - In "sparse-index.c", `prepare_repo_settings()` is already called
+ before the setting is accessed.
+ - In "dir.c", `prepare_repo_settings()` is already called in all code
+ paths before the setting is accessed.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
@@ repo-settings.h: struct repo_settings {
## sparse-index.c ##
@@ sparse-index.c: static int index_has_unmerged_entries(struct index_state *istate)
+
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
- prepare_repo_settings(istate->repo);
- if (!istate->repo->settings.sparse_checkout || !core_sparse_checkout_cone)
+ if (!istate->repo->settings.sparse_checkout || !istate->repo->settings.sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
-
- ## unpack-trees.c ##
-@@ unpack-trees.c: enum update_sparsity_result update_sparsity(struct unpack_trees_options *o,
- BUG("update_sparsity() called wrong");
-
- trace_performance_enter();
-+ prepare_repo_settings(the_repository);
-
- /* If we weren't given patterns, use the recorded ones */
- if (!pl) {
3: 84cb67469e ! 3: 35137b6814 environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
@@ Metadata
## Commit message ##
environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
- The global variable 'sparse_expect_files_outside_of_patterns' is used in
- a single function named 'clear_skip_worktree_from_present_files()' in
- sparse-index.c. Move its declaration inside that function, removing
- unnecessary global state.
+ The setting "sparse.expectFilesOutsideOfPatterns" is stored in the
+ global variable 'sparse_expect_files_outside_of_patterns' and allows
+ files marked with the `SKIP_WORKTREE` bit to be present in the worktree.
+
+ As this setting is closely related to repository, remove the global
+ variable and store the setting in the `struct repo_settings` along
+ with other sparse checkout related settings.
This also allows us to remove the definition '#define
USE_THE_REPOSITORY_VARIABLE' from the file 'sparse-index.c'.
@@ environment.h: extern int precomposed_unicode;
AUTOREBASE_NEVER = 0,
AUTOREBASE_LOCAL,
+ ## repo-settings.c ##
+@@ repo-settings.c: void prepare_repo_settings(struct repository *r)
+ repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
+ repo_cfg_bool(r, "core.sparsecheckout", &r->settings.sparse_checkout, 0);
+ repo_cfg_bool(r, "core.sparsecheckoutcone", &r->settings.sparse_checkout_cone, 0);
++ repo_cfg_bool(r, "sparse.expectfilesoutsideofpatterns", &r->settings.sparse_expect_files_outside_of_patterns, 0);
+
+ /*
+ * The GIT_TEST_MULTI_PACK_INDEX variable is special in that
+
+ ## repo-settings.h ##
+@@ repo-settings.h: struct repo_settings {
+
+ int sparse_checkout;
+ int sparse_checkout_cone;
++ int sparse_expect_files_outside_of_patterns;
+ };
+ #define REPO_SETTINGS_INIT { \
+ .shared_repository = -1, \
+
## sparse-index.c ##
@@
-#define USE_THE_REPOSITORY_VARIABLE
@@ sparse-index.c
#include "git-compat-util.h"
@@ sparse-index.c: static void clear_skip_worktree_from_present_files_full(struct index_state *ista
-
void clear_skip_worktree_from_present_files(struct index_state *istate)
{
-+ int sparse_expect_files_outside_of_patterns = 0;
-+ repo_config_get_bool(istate->repo, "sparse.expectfilesoutsideofpatterns",
-+ &sparse_expect_files_outside_of_patterns);
if (!istate->repo->settings.sparse_checkout ||
- sparse_expect_files_outside_of_patterns)
+- sparse_expect_files_outside_of_patterns)
++ istate->repo->settings.sparse_expect_files_outside_of_patterns)
return;
+
+ if (clear_skip_worktree_from_present_files_sparse(istate)) {
2.49.0
^ permalink raw reply [flat|nested] 50+ messages in thread* [GSOC PATCH v6 1/3] environment: move access to "core.sparsecheckout" into repo_settings
2025-07-19 0:11 ` [GSOC PATCH v6 " Ayush Chandekar
@ 2025-07-19 0:11 ` Ayush Chandekar
2025-07-19 0:11 ` [GSOC PATCH v6 2/3] environment: move access to "core.sparsecheckoutcone" " Ayush Chandekar
` (3 subsequent siblings)
4 siblings, 0 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-07-19 0:11 UTC (permalink / raw)
To: ayu.chandekar
Cc: christian.couder, git, shyamthakkar001, phillip.wood123, ps,
gitster, ben.knoble
The setting "core.sparsecheckout" is stored in the global
`core_apply_sparse_checkout` and is populated in config.c. Refactor the
code to store it in the variable `sparse_checkout` in the struct
`repo_settings`.
Call `prepare_repo_settings()` where necessary to ensure the `struct
repo_settings` is initialized before use:
- In "builtin/backfill.c", "builtin/mv.c" and "builtin/clone.c" call
`prepare_repo_settings()` since their respective `cmd_*()` functions
did not call it earlier.
- In "dir.c", the function using 'settings.sparse_checkout' is invoked
in multiple files that do not call `prepare_repo_settings()`, hence
add a call directly to that function.
- In "sparse-index.c", remove a call to `prepare_repo_settings()`
from the function `is_sparse_index_allowed()` as it is called
everytime before the function is called, and add a call to
`prepare_repo_settings()` inside `convert_to_sparse()`, as it is
used widely without having a call to `prepare_repo_settings()`
before and relies on the setting.
- In "wt-status.c", call `prepare_repo_settings()` before accessing
the setting because the function using it is commonly used.
Avoid reduntant calls to `prepare_repo_settings()` where it is already
present:
- In "builtin/worktree.c", it is already invoked in `cmd_worktree()`
before the setting is accessed.
- In "unpack-tress.c", the function accessing the setting already calls
it.
This also allows us to remove the definition `#define
USE_THE_REPOSITORY_VARIABLE` from the file 'builtin/backfill.c'.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
builtin/backfill.c | 7 ++-----
builtin/clone.c | 3 ++-
builtin/grep.c | 2 +-
builtin/mv.c | 4 ++--
builtin/sparse-checkout.c | 21 ++++++++++-----------
builtin/worktree.c | 2 +-
config.c | 5 -----
dir.c | 3 ++-
environment.c | 1 -
environment.h | 1 -
repo-settings.c | 1 +
repo-settings.h | 1 +
sparse-index.c | 6 +++---
unpack-trees.c | 2 +-
wt-status.c | 3 ++-
15 files changed, 28 insertions(+), 34 deletions(-)
diff --git a/builtin/backfill.c b/builtin/backfill.c
index 80056abe47..48b2518743 100644
--- a/builtin/backfill.c
+++ b/builtin/backfill.c
@@ -1,6 +1,3 @@
-/* We need this macro to access core_apply_sparse_checkout */
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "builtin.h"
#include "git-compat-util.h"
#include "config.h"
@@ -137,9 +134,9 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
0);
repo_config(repo, git_default_config, NULL);
-
+ prepare_repo_settings(repo);
if (ctx.sparse < 0)
- ctx.sparse = core_apply_sparse_checkout;
+ ctx.sparse = repo->settings.sparse_checkout;
result = do_backfill(&ctx);
backfill_context_clear(&ctx);
diff --git a/builtin/clone.c b/builtin/clone.c
index 6d08abed37..6b6d429fd8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -619,11 +619,12 @@ static int git_sparse_checkout_init(const char *repo)
int result = 0;
strvec_pushl(&cmd.args, "-C", repo, "sparse-checkout", "set", NULL);
+ prepare_repo_settings(the_repository);
/*
* We must apply the setting in the current process
* for the later checkout to use the sparse-checkout file.
*/
- core_apply_sparse_checkout = 1;
+ the_repository->settings.sparse_checkout = 1;
cmd.git_cmd = 1;
if (run_command(&cmd)) {
diff --git a/builtin/grep.c b/builtin/grep.c
index 39273d9c0f..fbad1a72a2 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -481,7 +481,7 @@ static int grep_submodule(struct grep_opt *opt,
* "forget" the sparse-index feature switch. As a result, the index
* of these submodules are expanded unexpectedly.
*
- * 2. "core_apply_sparse_checkout"
+ * 2. "settings.sparse_checkout"
* When running `grep` in the superproject, this setting is
* populated using the superproject's configs. However, once
* initialized, this config is globally accessible and is read by
diff --git a/builtin/mv.c b/builtin/mv.c
index 07548fe96a..43ed2e3d0a 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -570,9 +570,9 @@ int cmd_mv(int argc,
&st,
0);
rename_index_entry_at(the_repository->index, pos, dst);
-
+ prepare_repo_settings(the_repository);
if (ignore_sparse &&
- core_apply_sparse_checkout &&
+ the_repository->settings.sparse_checkout &&
core_sparse_checkout_cone) {
/*
* NEEDSWORK: we are *not* paying attention to
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 1bf01591b2..8329d29a27 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -62,7 +62,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
int res;
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!the_repository->settings.sparse_checkout)
die(_("this worktree is not sparse"));
argc = parse_options(argc, argv, prefix,
@@ -397,11 +397,11 @@ static int set_config(enum sparse_checkout_mode mode)
static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {
/* If not specified, use previous definition of cone mode */
- if (*cone_mode == -1 && core_apply_sparse_checkout)
+ if (*cone_mode == -1 && the_repository->settings.sparse_checkout)
*cone_mode = core_sparse_checkout_cone;
/* Set cone/non-cone mode appropriately */
- core_apply_sparse_checkout = 1;
+ the_repository->settings.sparse_checkout = 1;
if (*cone_mode == 1 || *cone_mode == -1) {
core_sparse_checkout_cone = 1;
return MODE_CONE_PATTERNS;
@@ -415,7 +415,7 @@ static int update_modes(int *cone_mode, int *sparse_index)
int mode, record_mode;
/* Determine if we need to record the mode; ensure sparse checkout on */
- record_mode = (*cone_mode != -1) || !core_apply_sparse_checkout;
+ record_mode = (*cone_mode != -1) || !the_repository->settings.sparse_checkout;
mode = update_cone_mode(cone_mode);
if (record_mode && set_config(mode))
@@ -695,9 +695,9 @@ static int modify_pattern_list(struct strvec *args, int use_stdin,
break;
}
- if (!core_apply_sparse_checkout) {
+ if (!the_repository->settings.sparse_checkout) {
set_config(MODE_ALL_PATTERNS);
- core_apply_sparse_checkout = 1;
+ the_repository->settings.sparse_checkout = 1;
changed_config = 1;
}
@@ -793,7 +793,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix,
int ret;
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!the_repository->settings.sparse_checkout)
die(_("no sparse-checkout to add to"));
repo_read_index(the_repository);
@@ -902,7 +902,7 @@ static int sparse_checkout_reapply(int argc, const char **argv,
};
setup_work_tree();
- if (!core_apply_sparse_checkout)
+ if (!the_repository->settings.sparse_checkout)
die(_("must be in a sparse-checkout to reapply sparsity patterns"));
reapply_opts.cone_mode = -1;
@@ -935,7 +935,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
struct pattern_list pl;
/*
- * We do not exit early if !core_apply_sparse_checkout; due to the
+ * We do not exit early if !sparse_checkout; due to the
* ability for users to manually muck things up between
* direct editing of .git/info/sparse-checkout
* running read-tree -m u HEAD or update-index --skip-worktree
@@ -961,11 +961,10 @@ static int sparse_checkout_disable(int argc, const char **argv,
hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
pl.use_cone_patterns = 0;
- core_apply_sparse_checkout = 1;
+ the_repository->settings.sparse_checkout = 1;
add_pattern("/*", empty_base, 0, &pl, 0);
- prepare_repo_settings(the_repository);
the_repository->settings.sparse_index = 0;
if (update_working_directory(&pl))
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 2dceeeed8b..a3a1bb00e3 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -536,7 +536,7 @@ static int add_worktree(const char *path, const char *refname,
* If the current worktree has sparse-checkout enabled, then copy
* the sparse-checkout patterns from the current worktree.
*/
- if (core_apply_sparse_checkout)
+ if (the_repository->settings.sparse_checkout)
copy_sparse_checkout(sb_repo.buf);
/*
diff --git a/config.c b/config.c
index 095a17bd42..da76bf4fde 100644
--- a/config.c
+++ b/config.c
@@ -1607,11 +1607,6 @@ static int git_default_core_config(const char *var, const char *value,
return 0;
}
- if (!strcmp(var, "core.sparsecheckout")) {
- core_apply_sparse_checkout = git_config_bool(var, value);
- return 0;
- }
-
if (!strcmp(var, "core.sparsecheckoutcone")) {
core_sparse_checkout_cone = git_config_bool(var, value);
return 0;
diff --git a/dir.c b/dir.c
index 02873f59ea..01d7574c09 100644
--- a/dir.c
+++ b/dir.c
@@ -1516,7 +1516,7 @@ enum pattern_match_result path_matches_pattern_list(
int init_sparse_checkout_patterns(struct index_state *istate)
{
- if (!core_apply_sparse_checkout)
+ if (!istate->repo->settings.sparse_checkout)
return 1;
if (istate->sparse_checkout_patterns)
return 0;
@@ -1539,6 +1539,7 @@ static int path_in_sparse_checkout_1(const char *path,
enum pattern_match_result match = UNDECIDED;
const char *end, *slash;
+ prepare_repo_settings(istate->repo);
/*
* We default to accepting a path if the path is empty, there are no
* patterns, or the patterns are of the wrong type.
diff --git a/environment.c b/environment.c
index 7c2480b22e..3a21629f86 100644
--- a/environment.c
+++ b/environment.c
@@ -63,7 +63,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#endif
enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
-int core_apply_sparse_checkout;
int core_sparse_checkout_cone;
int sparse_expect_files_outside_of_patterns;
int merge_log_config = -1;
diff --git a/environment.h b/environment.h
index 3d806ced6e..1e1e83fff1 100644
--- a/environment.h
+++ b/environment.h
@@ -159,7 +159,6 @@ extern int precomposed_unicode;
extern int protect_hfs;
extern int protect_ntfs;
-extern int core_apply_sparse_checkout;
extern int core_sparse_checkout_cone;
extern int sparse_expect_files_outside_of_patterns;
diff --git a/repo-settings.c b/repo-settings.c
index 195c24e9c0..c3aa92c065 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -84,6 +84,7 @@ void prepare_repo_settings(struct repository *r)
&r->settings.pack_use_bitmap_boundary_traversal,
r->settings.pack_use_bitmap_boundary_traversal);
repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
+ repo_cfg_bool(r, "core.sparsecheckout", &r->settings.sparse_checkout, 0);
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
diff --git a/repo-settings.h b/repo-settings.h
index d477885561..95900784f1 100644
--- a/repo-settings.h
+++ b/repo-settings.h
@@ -68,6 +68,7 @@ struct repo_settings {
unsigned long big_file_threshold;
char *hooks_path;
+ int sparse_checkout;
};
#define REPO_SETTINGS_INIT { \
.shared_repository = -1, \
diff --git a/sparse-index.c b/sparse-index.c
index 5634abafaa..c9e5a5efe1 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -150,7 +150,7 @@ static int index_has_unmerged_entries(struct index_state *istate)
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
- if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
+ if (!istate->repo->settings.sparse_checkout || !core_sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
@@ -172,7 +172,6 @@ int is_sparse_index_allowed(struct index_state *istate, int flags)
/*
* Only convert to sparse if index.sparse is set.
*/
- prepare_repo_settings(istate->repo);
if (!istate->repo->settings.sparse_index)
return 0;
}
@@ -196,6 +195,7 @@ int is_sparse_index_allowed(struct index_state *istate, int flags)
int convert_to_sparse(struct index_state *istate, int flags)
{
+ prepare_repo_settings(istate->repo);
/*
* If the index is already sparse, empty, or otherwise
* cannot be converted to sparse, do not convert.
@@ -668,7 +668,7 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista
void clear_skip_worktree_from_present_files(struct index_state *istate)
{
- if (!core_apply_sparse_checkout ||
+ if (!istate->repo->settings.sparse_checkout ||
sparse_expect_files_outside_of_patterns)
return;
diff --git a/unpack-trees.c b/unpack-trees.c
index f38c761ab9..2e6d3f98f7 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1924,7 +1924,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
if (o->prefix)
update_sparsity_for_prefix(o->prefix, o->src_index);
- if (!core_apply_sparse_checkout || !o->update)
+ if (!repo->settings.sparse_checkout || !o->update)
o->skip_sparse_checkout = 1;
if (!o->skip_sparse_checkout) {
memset(&pl, 0, sizeof(pl));
diff --git a/wt-status.c b/wt-status.c
index 454601afa1..8651134bcb 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1773,7 +1773,8 @@ static void wt_status_check_sparse_checkout(struct repository *r,
int skip_worktree = 0;
int i;
- if (!core_apply_sparse_checkout || r->index->cache_nr == 0) {
+ prepare_repo_settings(r);
+ if (!r->settings.sparse_checkout || r->index->cache_nr == 0) {
/*
* Don't compute percentage of checked out files if we
* aren't in a sparse checkout or would get division by 0.
--
2.49.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* [GSOC PATCH v6 2/3] environment: move access to "core.sparsecheckoutcone" into repo_settings
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 ` 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
` (2 subsequent siblings)
4 siblings, 0 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-07-19 0:11 UTC (permalink / raw)
To: ayu.chandekar
Cc: christian.couder, git, shyamthakkar001, phillip.wood123, ps,
gitster, ben.knoble
The setting "core.sparsecheckoutcone" is stored in the global
`core_sparse_checkout_cone` and is populated in config.c. Refactor the
code to store it in the variable `sparse_checkout_cone` in the struct
`repo_settings`.
Avoid redundant calls to `prepare_repo_settings()` where it is already
present:
- In "builtin/mv.c" and "builtin/sparse-checkout.c", it is already
invoked in their respective `cmd_*()` functions.
- In "sparse-index.c", `prepare_repo_settings()` is already called
before the setting is accessed.
- In "dir.c", `prepare_repo_settings()` is already called in all code
paths before the setting is accessed.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
builtin/grep.c | 2 +-
builtin/mv.c | 2 +-
builtin/sparse-checkout.c | 28 ++++++++++++++--------------
config.c | 5 -----
dir.c | 2 +-
environment.c | 1 -
environment.h | 1 -
repo-settings.c | 1 +
repo-settings.h | 2 ++
sparse-index.c | 2 +-
10 files changed, 21 insertions(+), 25 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index fbad1a72a2..bd5481cc44 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -491,7 +491,7 @@ static int grep_submodule(struct grep_opt *opt,
* dictate the behavior for the submodule, making it "forget" its
* sparse-checkout state.
*
- * 3. "core_sparse_checkout_cone"
+ * 3. "settings.sparse_checkout_cone"
* ditto.
*
* Note that this list is not exhaustive.
diff --git a/builtin/mv.c b/builtin/mv.c
index 43ed2e3d0a..2d1326a18f 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -573,7 +573,7 @@ int cmd_mv(int argc,
prepare_repo_settings(the_repository);
if (ignore_sparse &&
the_repository->settings.sparse_checkout &&
- core_sparse_checkout_cone) {
+ the_repository->settings.sparse_checkout_cone) {
/*
* NEEDSWORK: we are *not* paying attention to
* "out-to-out" move (<source> is out-of-cone and
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 8329d29a27..8a0ffba9d4 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -71,7 +71,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
memset(&pl, 0, sizeof(pl));
- pl.use_cone_patterns = core_sparse_checkout_cone;
+ pl.use_cone_patterns = the_repository->settings.sparse_checkout_cone;
sparse_filename = get_sparse_checkout_filename();
res = add_patterns_from_file_to_list(sparse_filename, "", 0, &pl, NULL, 0);
@@ -352,7 +352,7 @@ static int write_patterns_and_update(struct pattern_list *pl)
if (!fp)
die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk));
- if (core_sparse_checkout_cone)
+ if (the_repository->settings.sparse_checkout_cone)
write_cone_to_file(fp, pl);
else
write_patterns_to_file(fp, pl);
@@ -398,15 +398,15 @@ static int set_config(enum sparse_checkout_mode mode)
static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {
/* If not specified, use previous definition of cone mode */
if (*cone_mode == -1 && the_repository->settings.sparse_checkout)
- *cone_mode = core_sparse_checkout_cone;
+ *cone_mode = the_repository->settings.sparse_checkout_cone;
/* Set cone/non-cone mode appropriately */
the_repository->settings.sparse_checkout = 1;
if (*cone_mode == 1 || *cone_mode == -1) {
- core_sparse_checkout_cone = 1;
+ the_repository->settings.sparse_checkout_cone = 1;
return MODE_CONE_PATTERNS;
}
- core_sparse_checkout_cone = 0;
+ the_repository->settings.sparse_checkout_cone = 0;
return MODE_ALL_PATTERNS;
}
@@ -572,7 +572,7 @@ static void add_patterns_from_input(struct pattern_list *pl,
FILE *file)
{
int i;
- if (core_sparse_checkout_cone) {
+ if (the_repository->settings.sparse_checkout_cone) {
struct strbuf line = STRBUF_INIT;
hashmap_init(&pl->recursive_hashmap, pl_hashmap_cmp, NULL, 0);
@@ -637,7 +637,7 @@ static void add_patterns_cone_mode(int argc, const char **argv,
use_stdin ? stdin : NULL);
memset(&existing, 0, sizeof(existing));
- existing.use_cone_patterns = core_sparse_checkout_cone;
+ existing.use_cone_patterns = the_repository->settings.sparse_checkout_cone;
if (add_patterns_from_file_to_list(sparse_filename, "", 0,
&existing, NULL, 0))
@@ -683,7 +683,7 @@ static int modify_pattern_list(struct strvec *args, int use_stdin,
switch (m) {
case ADD:
- if (core_sparse_checkout_cone)
+ if (the_repository->settings.sparse_checkout_cone)
add_patterns_cone_mode(args->nr, args->v, pl, use_stdin);
else
add_patterns_literal(args->nr, args->v, pl, use_stdin);
@@ -719,7 +719,7 @@ static void sanitize_paths(struct strvec *args,
if (!args->nr)
return;
- if (prefix && *prefix && core_sparse_checkout_cone) {
+ if (prefix && *prefix && the_repository->settings.sparse_checkout_cone) {
/*
* The args are not pathspecs, so unfortunately we
* cannot imitate how cmd_add() uses parse_pathspec().
@@ -736,10 +736,10 @@ static void sanitize_paths(struct strvec *args,
if (skip_checks)
return;
- if (prefix && *prefix && !core_sparse_checkout_cone)
+ if (prefix && *prefix && !the_repository->settings.sparse_checkout_cone)
die(_("please run from the toplevel directory in non-cone mode"));
- if (core_sparse_checkout_cone) {
+ if (the_repository->settings.sparse_checkout_cone) {
for (i = 0; i < args->nr; i++) {
if (args->v[i][0] == '/')
die(_("specify directories rather than patterns (no leading slash)"));
@@ -761,7 +761,7 @@ static void sanitize_paths(struct strvec *args,
if (S_ISSPARSEDIR(ce->ce_mode))
continue;
- if (core_sparse_checkout_cone)
+ if (the_repository->settings.sparse_checkout_cone)
die(_("'%s' is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), args->v[i]);
else
warning(_("pass a leading slash before paths such as '%s' if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), args->v[i]);
@@ -864,7 +864,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix,
* non-cone mode, if nothing is specified, manually select just the
* top-level directory (much as 'init' would do).
*/
- if (!core_sparse_checkout_cone && !set_opts.use_stdin && argc == 0) {
+ if (!the_repository->settings.sparse_checkout_cone && !set_opts.use_stdin && argc == 0) {
for (int i = 0; i < default_patterns_nr; i++)
strvec_push(&patterns, default_patterns[i]);
} else {
@@ -1041,7 +1041,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char *
check_rules_opts.cone_mode = 1;
update_cone_mode(&check_rules_opts.cone_mode);
- pl.use_cone_patterns = core_sparse_checkout_cone;
+ pl.use_cone_patterns = the_repository->settings.sparse_checkout_cone;
if (check_rules_opts.rules_file) {
fp = xfopen(check_rules_opts.rules_file, "r");
add_patterns_from_input(&pl, argc, argv, fp);
diff --git a/config.c b/config.c
index da76bf4fde..e13f7d8fe8 100644
--- a/config.c
+++ b/config.c
@@ -1607,11 +1607,6 @@ static int git_default_core_config(const char *var, const char *value,
return 0;
}
- if (!strcmp(var, "core.sparsecheckoutcone")) {
- core_sparse_checkout_cone = git_config_bool(var, value);
- return 0;
- }
-
if (!strcmp(var, "core.precomposeunicode")) {
precomposed_unicode = git_config_bool(var, value);
return 0;
diff --git a/dir.c b/dir.c
index 01d7574c09..5bed22a479 100644
--- a/dir.c
+++ b/dir.c
@@ -3473,7 +3473,7 @@ int get_sparse_checkout_patterns(struct pattern_list *pl)
int res;
char *sparse_filename = get_sparse_checkout_filename();
- pl->use_cone_patterns = core_sparse_checkout_cone;
+ pl->use_cone_patterns = the_repository->settings.sparse_checkout_cone;
res = add_patterns_from_file_to_list(sparse_filename, "", 0, pl, NULL, 0);
free(sparse_filename);
diff --git a/environment.c b/environment.c
index 3a21629f86..cd5ec5c736 100644
--- a/environment.c
+++ b/environment.c
@@ -63,7 +63,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#endif
enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
-int core_sparse_checkout_cone;
int sparse_expect_files_outside_of_patterns;
int merge_log_config = -1;
int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
diff --git a/environment.h b/environment.h
index 1e1e83fff1..5642156d3d 100644
--- a/environment.h
+++ b/environment.h
@@ -159,7 +159,6 @@ extern int precomposed_unicode;
extern int protect_hfs;
extern int protect_ntfs;
-extern int core_sparse_checkout_cone;
extern int sparse_expect_files_outside_of_patterns;
enum rebase_setup_type {
diff --git a/repo-settings.c b/repo-settings.c
index c3aa92c065..505e402276 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -85,6 +85,7 @@ void prepare_repo_settings(struct repository *r)
r->settings.pack_use_bitmap_boundary_traversal);
repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
repo_cfg_bool(r, "core.sparsecheckout", &r->settings.sparse_checkout, 0);
+ repo_cfg_bool(r, "core.sparsecheckoutcone", &r->settings.sparse_checkout_cone, 0);
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
diff --git a/repo-settings.h b/repo-settings.h
index 95900784f1..1b43c4029c 100644
--- a/repo-settings.h
+++ b/repo-settings.h
@@ -68,7 +68,9 @@ struct repo_settings {
unsigned long big_file_threshold;
char *hooks_path;
+
int sparse_checkout;
+ int sparse_checkout_cone;
};
#define REPO_SETTINGS_INIT { \
.shared_repository = -1, \
diff --git a/sparse-index.c b/sparse-index.c
index c9e5a5efe1..3b51ea46e3 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -150,7 +150,7 @@ static int index_has_unmerged_entries(struct index_state *istate)
int is_sparse_index_allowed(struct index_state *istate, int flags)
{
- if (!istate->repo->settings.sparse_checkout || !core_sparse_checkout_cone)
+ if (!istate->repo->settings.sparse_checkout || !istate->repo->settings.sparse_checkout_cone)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
--
2.49.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* [GSOC PATCH v6 3/3] environment: remove the global variable 'sparse_expect_files_outside_of_patterns'
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 ` Ayush Chandekar
2025-07-23 22:14 ` [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables Junio C Hamano
2025-08-10 15:36 ` Ayush Chandekar
4 siblings, 0 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-07-19 0:11 UTC (permalink / raw)
To: ayu.chandekar
Cc: christian.couder, git, shyamthakkar001, phillip.wood123, ps,
gitster, ben.knoble
The setting "sparse.expectFilesOutsideOfPatterns" is stored in the
global variable 'sparse_expect_files_outside_of_patterns' and allows
files marked with the `SKIP_WORKTREE` bit to be present in the worktree.
As this setting is closely related to repository, remove the global
variable and store the setting in the `struct repo_settings` along
with other sparse checkout related settings.
This also allows us to remove the definition '#define
USE_THE_REPOSITORY_VARIABLE' from the file 'sparse-index.c'.
This change is part of an ongoing effort to eliminate global variables,
improve modularity and help libify the codebase.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
config.c | 14 --------------
environment.c | 1 -
environment.h | 2 --
repo-settings.c | 1 +
repo-settings.h | 1 +
sparse-index.c | 3 +--
6 files changed, 3 insertions(+), 19 deletions(-)
diff --git a/config.c b/config.c
index e13f7d8fe8..ea6de843f7 100644
--- a/config.c
+++ b/config.c
@@ -1631,17 +1631,6 @@ static int git_default_core_config(const char *var, const char *value,
return platform_core_config(var, value, ctx, cb);
}
-static int git_default_sparse_config(const char *var, const char *value)
-{
- if (!strcmp(var, "sparse.expectfilesoutsideofpatterns")) {
- sparse_expect_files_outside_of_patterns = git_config_bool(var, value);
- return 0;
- }
-
- /* Add other config variables here and to Documentation/config/sparse.adoc. */
- return 0;
-}
-
static int git_default_i18n_config(const char *var, const char *value)
{
if (!strcmp(var, "i18n.commitencoding")) {
@@ -1803,9 +1792,6 @@ int git_default_config(const char *var, const char *value,
return 0;
}
- if (starts_with(var, "sparse."))
- return git_default_sparse_config(var, value);
-
/* Add other config variables here and to Documentation/config.adoc. */
return 0;
}
diff --git a/environment.c b/environment.c
index cd5ec5c736..cf52fa9617 100644
--- a/environment.c
+++ b/environment.c
@@ -63,7 +63,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#endif
enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
-int sparse_expect_files_outside_of_patterns;
int merge_log_config = -1;
int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
unsigned long pack_size_limit_cfg;
diff --git a/environment.h b/environment.h
index 5642156d3d..aaa3025df3 100644
--- a/environment.h
+++ b/environment.h
@@ -159,8 +159,6 @@ extern int precomposed_unicode;
extern int protect_hfs;
extern int protect_ntfs;
-extern int sparse_expect_files_outside_of_patterns;
-
enum rebase_setup_type {
AUTOREBASE_NEVER = 0,
AUTOREBASE_LOCAL,
diff --git a/repo-settings.c b/repo-settings.c
index 505e402276..5e0ba4ae23 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -86,6 +86,7 @@ void prepare_repo_settings(struct repository *r)
repo_cfg_bool(r, "core.usereplacerefs", &r->settings.read_replace_refs, 1);
repo_cfg_bool(r, "core.sparsecheckout", &r->settings.sparse_checkout, 0);
repo_cfg_bool(r, "core.sparsecheckoutcone", &r->settings.sparse_checkout_cone, 0);
+ repo_cfg_bool(r, "sparse.expectfilesoutsideofpatterns", &r->settings.sparse_expect_files_outside_of_patterns, 0);
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
diff --git a/repo-settings.h b/repo-settings.h
index 1b43c4029c..695c0fd0ce 100644
--- a/repo-settings.h
+++ b/repo-settings.h
@@ -71,6 +71,7 @@ struct repo_settings {
int sparse_checkout;
int sparse_checkout_cone;
+ int sparse_expect_files_outside_of_patterns;
};
#define REPO_SETTINGS_INIT { \
.shared_repository = -1, \
diff --git a/sparse-index.c b/sparse-index.c
index 3b51ea46e3..552d26adc1 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -1,4 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
#define DISABLE_SIGN_COMPARE_WARNINGS
#include "git-compat-util.h"
@@ -669,7 +668,7 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista
void clear_skip_worktree_from_present_files(struct index_state *istate)
{
if (!istate->repo->settings.sparse_checkout ||
- sparse_expect_files_outside_of_patterns)
+ istate->repo->settings.sparse_expect_files_outside_of_patterns)
return;
if (clear_skip_worktree_from_present_files_sparse(istate)) {
--
2.49.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
2025-07-19 0:11 ` [GSOC PATCH v6 " Ayush Chandekar
` (2 preceding siblings ...)
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 ` Junio C Hamano
2025-07-24 13:25 ` Derrick Stolee
2025-07-26 23:55 ` Ayush Chandekar
2025-08-10 15:36 ` Ayush Chandekar
4 siblings, 2 replies; 50+ messages in thread
From: Junio C Hamano @ 2025-07-23 22:14 UTC (permalink / raw)
To: Ayush Chandekar
Cc: christian.couder, git, shyamthakkar001, phillip.wood123, ps,
ben.knoble
Ayush Chandekar <ayu.chandekar@gmail.com> writes:
> This patch series aims to remove global variables related to
> sparse-checkout from the global scope and to remove the definition
> '#define USE_THE_REPOSITORY_VARIABLE' from a few files.
>
> It contains three patches:
>
> 1 - Remove the global variable 'core_apply_sparse_checkout' and
> move its setting to the 'struct repo_settings'. Also remove the
> definition '#define USE_THE_REPOSITORY_VARIABLE' from
> "builtin/backfill.c".
>
> 2 - Remove the global variable 'core_sparse_checkout_cone' and
> move its setting to the 'struct repo_settings'.
>
> 3 - Remove the global variable
> 'sparse_expect_files_outside_of_patterns` and move its setting to
> 'struct repo_settings'. Also remove the definition '#define
> USE_THE_REPOSITORY_VARIABLE' from "sparse-index.c"
> --
> Discussions since v5:
>
> * For 1/3 and 2/3, Junio told me that it was concerning to put so
> many calls to `prepare_repo_settings()` so I tried to minimize the
> calls and made sure that there's no useless calling.
I didn't mean that the number of places is the problem. What I
found troubling was that this is not done in any central place, so
it is hard to notice even if some random cmd_foo() failed to call
the function before doing its real work. For example, shouldn't we
be able to, at least for built-in commands that have RUN_SETUP bit
set, centrally call prepare_repo_settings() somewhere late in
git.c:run_builtin() after we figure out what should be in
the_repository? Now historically, setting up a repository may never
have involved opening and parsing tons of configuration files, so
such a change may be incurring extra overhead we did not have to
pay, so it needs a lot more thought than just trying to minimize the
number of calls, but some performance measurement.
> * 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?
A meta-level comment and a half.
* Please do not use "-- " (that is a line that has dash dash and a
single space and nothing else on it) lightly. It is called
signature line and often MUA pays attention to it when responding
to a message with such a line by omitting everything after it
(which is supposed to be your "who I am" advertisement) when
quoting the original. Since you had one before the "discussions
since v5" section and the range-diff, I had to manually resurrect
the part after the signature line while composing this message.
* 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?
Not an objection and not a suggestion to move them out of the
repo_settings and to the repository proper. Just wanted to hear
the reasoning behind it (and have the rationale clearly
documented, preferrably in the proposed log messages).
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
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
` (2 more replies)
2025-07-26 23:55 ` Ayush Chandekar
1 sibling, 3 replies; 50+ messages in thread
From: Derrick Stolee @ 2025-07-24 13:25 UTC (permalink / raw)
To: Junio C Hamano, Ayush Chandekar
Cc: christian.couder, git, shyamthakkar001, phillip.wood123, ps,
ben.knoble
On 7/23/25 6:14 PM, Junio C Hamano wrote:
> Ayush Chandekar <ayu.chandekar@gmail.com> writes:
>
>> This patch series aims to remove global variables related to
>> sparse-checkout from the global scope and to remove the definition
>> '#define USE_THE_REPOSITORY_VARIABLE' from a few files.
Sorry that I missed early versions of this thread. It's an
interesting topic to me, but I've been distracted.
>> Discussions since v5:
>>
>> * For 1/3 and 2/3, Junio told me that it was concerning to put so
>> many calls to `prepare_repo_settings()` so I tried to minimize the
>> calls and made sure that there's no useless calling.
>
> I didn't mean that the number of places is the problem. What I
> found troubling was that this is not done in any central place, so
> it is hard to notice even if some random cmd_foo() failed to call
> the function before doing its real work. For example, shouldn't we
> be able to, at least for built-in commands that have RUN_SETUP bit
> set, centrally call prepare_repo_settings() somewhere late in
> git.c:run_builtin() after we figure out what should be in
> the_repository? Now historically, setting up a repository may never
> have involved opening and parsing tons of configuration files, so
> such a change may be incurring extra overhead we did not have to
> pay, so it needs a lot more thought than just trying to minimize the
> number of calls, but some performance measurement.
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.
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
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
2025-07-24 13:25 ` Derrick Stolee
@ 2025-07-24 18:44 ` Junio C Hamano
2025-07-29 11:36 ` Ayush Chandekar
2025-07-30 8:53 ` Phillip Wood
2 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2025-07-24 18:44 UTC (permalink / raw)
To: Derrick Stolee
Cc: Ayush Chandekar, christian.couder, git, shyamthakkar001,
phillip.wood123, ps, ben.knoble
Derrick Stolee <stolee@gmail.com> writes:
> 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.
>
> 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.
I do agree with the sentiment that being able to pass &repo.settings
to helper function makes us feel safer, but I agree that it is a
weak argument. If we reexamine other things in repo_settings, it
may turn out that the same reasoning applies to them and we may be
better off to roll repo_settings into the repository itself (after
all, it is an embedded structure, not even a pointer in the main
structure that points at an indenendent repo_settings structure),
but that is totally outside the scope of this discussion.
Thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
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
2 siblings, 2 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-07-29 11:36 UTC (permalink / raw)
To: Derrick Stolee
Cc: Junio C Hamano, christian.couder, git, shyamthakkar001,
phillip.wood123, ps, ben.knoble
Hey Derrick,
On Thu, Jul 24, 2025 at 6:55 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 7/23/25 6:14 PM, Junio C Hamano wrote:
> > Ayush Chandekar <ayu.chandekar@gmail.com> writes:
> >
> >> This patch series aims to remove global variables related to
> >> sparse-checkout from the global scope and to remove the definition
> >> '#define USE_THE_REPOSITORY_VARIABLE' from a few files.
>
> Sorry that I missed early versions of this thread. It's an
> interesting topic to me, but I've been distracted.
>
Thanks for joining the discussion!
> >> Discussions since v5:
> >>
> >> * For 1/3 and 2/3, Junio told me that it was concerning to put so
> >> many calls to `prepare_repo_settings()` so I tried to minimize the
> >> calls and made sure that there's no useless calling.
> >
> > I didn't mean that the number of places is the problem. What I
> > found troubling was that this is not done in any central place, so
> > it is hard to notice even if some random cmd_foo() failed to call
> > the function before doing its real work. For example, shouldn't we
> > be able to, at least for built-in commands that have RUN_SETUP bit
> > set, centrally call prepare_repo_settings() somewhere late in
> > git.c:run_builtin() after we figure out what should be in
> > the_repository? Now historically, setting up a repository may never
> > have involved opening and parsing tons of configuration files, so
> > such a change may be incurring extra overhead we did not have to
> > pay, so it needs a lot more thought than just trying to minimize the
> > number of calls, but some performance measurement.
>
> 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.
>
Oh, that is the complete opposite of what I had understood. I assumed
that repo_settings is used to hold some core repository-related config
settings, especially since there are already quite a few stored there,
and shifting these to the struct repository would probably clutter it.
Given that the existing configs in the struct repository are mostly
'repository_format_*' and having Patrick address that we embed the
repository_format in the repository as they were increasing[1], it let
me to think that we should try not to use the repository to store
these configs.
> 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.
>
> 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.
>
Okay, I agree with your points. I can maybe send a new version to address this.
Do we also shift settings like index.sparse to the repository then?
> >> * 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
>
Thanks
Ayush
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
2025-07-29 11:36 ` Ayush Chandekar
@ 2025-07-29 12:19 ` Derrick Stolee
2025-07-29 12:53 ` Ayush Chandekar
1 sibling, 0 replies; 50+ messages in thread
From: Derrick Stolee @ 2025-07-29 12:19 UTC (permalink / raw)
To: Ayush Chandekar
Cc: Junio C Hamano, christian.couder, git, shyamthakkar001,
phillip.wood123, ps, ben.knoble
On 7/29/25 7:36 AM, Ayush Chandekar wrote:
> Hey Derrick,
>
> On Thu, Jul 24, 2025 at 6:55 PM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> 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.
>>
>> 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.
>>
>
> Okay, I agree with your points. I can maybe send a new version to address this.
>
> Do we also shift settings like index.sparse to the repository then?
For now, it's important to focus this series on the globals being
converted. We can come back around to the ideas around removing
the settings struct and the prepare_repo_settings() method as a
separate series.
The index.sparse setting is something that is colocated in the
settings partly because of its interaction with feature.experimental
being handled in prepare_repo_settings() but also its very isolated
use. The sparse checkout globals are much more spread out across the
codebase.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
2025-07-29 11:36 ` Ayush Chandekar
2025-07-29 12:19 ` Derrick Stolee
@ 2025-07-29 12:53 ` Ayush Chandekar
1 sibling, 0 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-07-29 12:53 UTC (permalink / raw)
To: Derrick Stolee
Cc: Junio C Hamano, christian.couder, git, shyamthakkar001,
phillip.wood123, ps, ben.knoble
On Tue, Jul 29, 2025 at 5:06 PM Ayush Chandekar <ayu.chandekar@gmail.com> wrote:
>
[snip]
>
> Oh, that is the complete opposite of what I had understood. I assumed
> that repo_settings is used to hold some core repository-related config
> settings, especially since there are already quite a few stored there,
> and shifting these to the struct repository would probably clutter it.
> Given that the existing configs in the struct repository are mostly
> 'repository_format_*' and having Patrick address that we embed the
> repository_format in the repository as they were increasing[1], it let
> me to think that we should try not to use the repository to store
> these configs.
>
It seems that I missed out on adding the reference for [1]
[1]: https://lore.kernel.org/git/aGPcJMfBCJuQLdtu@pks.im/
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
2025-07-24 13:25 ` Derrick Stolee
2025-07-24 18:44 ` Junio C Hamano
2025-07-29 11:36 ` Ayush Chandekar
@ 2025-07-30 8:53 ` Phillip Wood
2025-07-30 15:52 ` Junio C Hamano
2 siblings, 1 reply; 50+ messages in thread
From: Phillip Wood @ 2025-07-30 8:53 UTC (permalink / raw)
To: Derrick Stolee, Junio C Hamano, Ayush Chandekar
Cc: christian.couder, git, shyamthakkar001, ps, ben.knoble
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@gmail.com/
> 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
>
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
2025-07-30 8:53 ` Phillip Wood
@ 2025-07-30 15:52 ` Junio C Hamano
0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2025-07-30 15:52 UTC (permalink / raw)
To: Phillip Wood
Cc: Derrick Stolee, Ayush Chandekar, christian.couder, git,
shyamthakkar001, ps, ben.knoble
Phillip Wood <phillip.wood123@gmail.com> writes:
> ... 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().
Sounds very painful, but it does sound like something worthwhile to
do.
Thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
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-26 23:55 ` Ayush Chandekar
1 sibling, 0 replies; 50+ messages in thread
From: Ayush Chandekar @ 2025-07-26 23:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: christian.couder, git, shyamthakkar001, phillip.wood123, ps,
ben.knoble
On Thu, Jul 24, 2025 at 3:44 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Ayush Chandekar <ayu.chandekar@gmail.com> writes:
>
> >
> > * For 1/3 and 2/3, Junio told me that it was concerning to put so
> > many calls to `prepare_repo_settings()` so I tried to minimize the
> > calls and made sure that there's no useless calling.
>
> I didn't mean that the number of places is the problem. What I
> found troubling was that this is not done in any central place, so
> it is hard to notice even if some random cmd_foo() failed to call
> the function before doing its real work. For example, shouldn't we
> be able to, at least for built-in commands that have RUN_SETUP bit
> set, centrally call prepare_repo_settings() somewhere late in
> git.c:run_builtin() after we figure out what should be in
> the_repository? Now historically, setting up a repository may never
> have involved opening and parsing tons of configuration files, so
> such a change may be incurring extra overhead we did not have to
> pay, so it needs a lot more thought than just trying to minimize the
> number of calls, but some performance measurement.
>
I was quite stumped as I don't know what the perfect solution for this
would be. I get your point that we have calls to the function all over
the place and would take some toll on the performance as well. As you
said that we can probably call the function in git.c:run_builtin() or
we can have a call to it in config.c:repo_config() so that just as the
other settings, we will have our repo_settings parsed, which were once
parsed through the same function(repo_config() or git_config()) and
since all the cmd_*() functions have a call to this, we will also be
able to call prepare_repo_settings() there itself.
> > * 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?
Well, I am not parsing it at that place. But, I am relying on an
already existing call to prepre_repo_settings() before the function
using the setting is called repository.c:repo_read_index(). I tried to
narrow down to a cmd_foo() function so that I can shift a call to the
prepare_repo_settings() from repo_read_index() to it, but this
function is widely called and cannot be narrowed down so I had to
settle with it. I'm afraid the issue still isn't completely resolved
>
> A meta-level comment and a half.
>
> * Please do not use "-- " (that is a line that has dash dash and a
> single space and nothing else on it) lightly. It is called
> signature line and often MUA pays attention to it when responding
> to a message with such a line by omitting everything after it
> (which is supposed to be your "who I am" advertisement) when
> quoting the original. Since you had one before the "discussions
> since v5" section and the range-diff, I had to manually resurrect
> the part after the signature line while composing this message.
>
I am sorry for that. I will keep that in mind from next time.
> * 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?
>
> Not an objection and not a suggestion to move them out of the
> repo_settings and to the repository proper. Just wanted to hear
> the reasoning behind it (and have the rationale clearly
> documented, preferrably in the proposed log messages).
Yeah, so what I thought was that if it is a "core.foo" setting, I
would club it with other core.* settings in the struct repo_settings.
But other config settings like in the previous patch series,
"extensions.preciousObjects" or also in this series, the
"sparse.expectfilesoutsideofpatterns", I would put them in some local
context or if they're tied to a repository, I would store them in the
repository struct itself. But, as other "core.sparse_*" variables are
stored in the repo_settings, I thought it was better to store the
"sparse.expectfilesoutsideofpatterns" along with them rather than
storing it in the repository.
Thanks
Ayush
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
2025-07-19 0:11 ` [GSOC PATCH v6 " Ayush Chandekar
` (3 preceding siblings ...)
2025-07-23 22:14 ` [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables Junio C Hamano
@ 2025-08-10 15:36 ` Ayush Chandekar
2025-08-26 12:20 ` Derrick Stolee
4 siblings, 1 reply; 50+ messages in thread
From: Ayush Chandekar @ 2025-08-10 15:36 UTC (permalink / raw)
To: ayu.chandekar
Cc: christian.couder, git, shyamthakkar001, phillip.wood123, ps,
gitster, ben.knoble
Just an update, I'm still working on this patch series.
Thanks,
Ayush
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
2025-08-10 15:36 ` Ayush Chandekar
@ 2025-08-26 12:20 ` Derrick Stolee
2025-08-27 21:31 ` Ayush Chandekar
0 siblings, 1 reply; 50+ messages in thread
From: Derrick Stolee @ 2025-08-26 12:20 UTC (permalink / raw)
To: Ayush Chandekar
Cc: christian.couder, git, shyamthakkar001, phillip.wood123, ps,
gitster, ben.knoble
On 8/10/25 11:36 AM, Ayush Chandekar wrote:
> Just an update, I'm still working on this patch series.
Hi Ayush. Do you have an update on your progress? Perhaps there
is something you're stuck on and could use some help?
A few weeks ago, I played around with the ideas around updating
the location of these globals into the repository struct and
made this critical observation:
It's "easy" to move the global into the_repository, but it
becomes harder (and changes behavior) if we start referring
to the data in each repository struct.
It may be good to separate the two things into different steps:
1. Move the globals into the repository struct, but only set
or read from the_repository->sparse_checkout[_cone].
2. Replace the use of the_repository and instead refer to a
specific repo. This may change behavior of the feature in
the presence of submodules with different config than the
root repo (tests before and after will be necessary).
We'll also need to update the_repository during the very
early config parsing but then update other repos during
repo initialization.
Does this make sense based on your progress in this space?
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
2025-08-26 12:20 ` Derrick Stolee
@ 2025-08-27 21:31 ` Ayush Chandekar
2025-09-05 14:15 ` Junio C Hamano
0 siblings, 1 reply; 50+ messages in thread
From: Ayush Chandekar @ 2025-08-27 21:31 UTC (permalink / raw)
To: Derrick Stolee
Cc: christian.couder, git, shyamthakkar001, phillip.wood123, ps,
gitster, ben.knoble
On Tue, Aug 26, 2025 at 5:50 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 8/10/25 11:36 AM, Ayush Chandekar wrote:
> > Just an update, I'm still working on this patch series.
>
> Hi Ayush. Do you have an update on your progress? Perhaps there
> is something you're stuck on and could use some help?
>
Hi Derrick, thanks for checking in!
I have made a few branches by trying out different ways: [1],[2] & [3]
I am quite close to sending the patch series now.
> A few weeks ago, I played around with the ideas around updating
> the location of these globals into the repository struct and
> made this critical observation:
>
> It's "easy" to move the global into the_repository, but it
> becomes harder (and changes behavior) if we start referring
> to the data in each repository struct.
>
> It may be good to separate the two things into different steps:
>
> 1. Move the globals into the repository struct, but only set
> or read from the_repository->sparse_checkout[_cone].
>
> 2. Replace the use of the_repository and instead refer to a
> specific repo. This may change behavior of the feature in
> the presence of submodules with different config than the
> root repo (tests before and after will be necessary).
> We'll also need to update the_repository during the very
> early config parsing but then update other repos during
> repo initialization.
>
> Does this make sense based on your progress in this space?
>
Yes, I was able to do the first step, the second step means that I
have to pass the repo struct to quite a few functions.
> Thanks,
> -Stolee
>
Thanks!
Ayush:)
[1]: https://github.com/ayu-ch/git/commits/sparse-checkout-6 (Stored
the variables in the 'struct repository' as int, and made a function
to read their values from the config)
[2]: https://github.com/ayu-ch/git/commits/sparse-checkout-7
(Christian suggested me that I can also store them as booleans so I
tried it.)
[3]: https://github.com/ayu-ch/git/commits/sparse-checkout-8 (Similar
to [1] but set their value as -1 when uninitialized. Tests seem to
fail though when I make this change.)
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
2025-08-27 21:31 ` Ayush Chandekar
@ 2025-09-05 14:15 ` Junio C Hamano
2025-09-05 17:10 ` Junio C Hamano
0 siblings, 1 reply; 50+ messages in thread
From: Junio C Hamano @ 2025-09-05 14:15 UTC (permalink / raw)
To: Ayush Chandekar
Cc: Derrick Stolee, christian.couder, git, shyamthakkar001,
phillip.wood123, ps, ben.knoble
Ayush Chandekar <ayu.chandekar@gmail.com> writes:
>> It may be good to separate the two things into different steps:
>>
>> 1. Move the globals into the repository struct, but only set
>> or read from the_repository->sparse_checkout[_cone].
>>
>> 2. Replace the use of the_repository and instead refer to a
>> specific repo. This may change behavior of the feature in
>> the presence of submodules with different config than the
>> root repo (tests before and after will be necessary).
>> We'll also need to update the_repository during the very
>> early config parsing but then update other repos during
>> repo initialization.
>>
>> Does this make sense based on your progress in this space?
>
> Yes, I was able to do the first step, the second step means that I
> have to pass the repo struct to quite a few functions.
Ayush, what's the status of this effort?
Currently a topic by Derrick is built on top of this one, which
means it is stuck waiting for this topic to stabilize. Should we
ask Derrick to rebuild his topic independent from this topic and let
it graduate sooner, and when you reroll this series, you'd base
yours on top of whatever the Git codebase looks like when it
happens?
Thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables
2025-09-05 14:15 ` Junio C Hamano
@ 2025-09-05 17:10 ` Junio C Hamano
0 siblings, 0 replies; 50+ messages in thread
From: Junio C Hamano @ 2025-09-05 17:10 UTC (permalink / raw)
To: Derrick Stolee, Ayush Chandekar
Cc: christian.couder, git, shyamthakkar001, phillip.wood123, ps,
ben.knoble
Junio C Hamano <gitster@pobox.com> writes:
> Ayush, what's the status of this effort?
>
> Currently a topic by Derrick is built on top of this one, which
> means it is stuck waiting for this topic to stabilize. Should we
> ask Derrick to rebuild his topic independent from this topic and let
> it graduate sooner, and when you reroll this series, you'd base
> yours on top of whatever the Git codebase looks like when it
> happens?
I tried to (re)adjust Derrick's ds/sparse-checkout-clean topic to
build directly on top of 'master', which involved removing its first
step to make it depend on the global core_apply_sparse_checkout and
core_sparse_checkout_cone variables, which was not too bad.
The result is queued on 'seen' I just pushed out. Derrick, could
you take a look to see if I screwed up any? At least it seems to
pass the tests locally.
I've ejected the ac/deglobal-sparse-variables topic from 'seen' for
now, but perhaps it may want to be rebased on Derrick's series when
it is rerolled. We'll see how fast the sparse-checkout-clean topic
can enter 'next' and graduate, hopefully soon enough.
Thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread