| Message ID | 20250310151048.69825-3-ayu.chandekar@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Stop depending on `the_repository` for core.attributesfile | expand |
Ayush Chandekar <ayu.chandekar@gmail.com> writes: > Update attribute-related functions to retrieve the "core.attributesfile" > configuration via the new repository-scoped accessor > `repo_settings_get_attributesfile_path()`. This improves behaviour in > multi-repository contexts and aligns with the goal of minimizing > reliance on global state. > We should also talk about the modifications made to pass around the repository struct. > Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com> > --- > attr.c | 28 ++++++++++------------------ > attr.h | 7 +++---- > builtin/check-attr.c | 2 +- > builtin/var.c | 2 +- > 4 files changed, 15 insertions(+), 24 deletions(-) > > diff --git a/attr.c b/attr.c > index 0bd2750528..8f28463e8c 100644 > --- a/attr.c > +++ b/attr.c > @@ -879,14 +879,6 @@ const char *git_attr_system_file(void) > return system_wide; > } > > -const char *git_attr_global_file(void) > -{ > - if (!git_attributes_file) > - git_attributes_file = xdg_config_home("attributes"); > - > - return git_attributes_file; > -} > - > int git_attr_system_is_enabled(void) > { > return !git_env_bool("GIT_ATTR_NOSYSTEM", 0); > @@ -906,7 +898,7 @@ static void push_stack(struct attr_stack **attr_stack_p, > } > } > > -static void bootstrap_attr_stack(struct index_state *istate, > +static void bootstrap_attr_stack(struct repository *repo, struct index_state *istate, Nit: here and other places, can this be a 'const'? > const struct object_id *tree_oid, > struct attr_stack **stack) > { > @@ -927,8 +919,8 @@ static void bootstrap_attr_stack(struct index_state *istate, > } > > /* home directory */ > - if (git_attr_global_file()) { > - e = read_attr_from_file(git_attr_global_file(), flags); > + if (repo_settings_get_attributesfile_path(repo)) { > + e = read_attr_from_file(repo_settings_get_attributesfile_path(repo), flags); > push_stack(stack, e, NULL, 0); > } > > @@ -946,7 +938,7 @@ static void bootstrap_attr_stack(struct index_state *istate, > push_stack(stack, e, NULL, 0); > } > > -static void prepare_attr_stack(struct index_state *istate, > +static void prepare_attr_stack(struct repository *repo, struct index_state *istate, > const struct object_id *tree_oid, > const char *path, int dirlen, > struct attr_stack **stack) > @@ -969,7 +961,7 @@ static void prepare_attr_stack(struct index_state *istate, > * .gitattributes in deeper directories to shallower ones, > * and finally use the built-in set as the default. > */ > - bootstrap_attr_stack(istate, tree_oid, stack); > + bootstrap_attr_stack(repo, istate, tree_oid, stack); > > /* > * Pop the "info" one that is always at the top of the stack. > @@ -1143,7 +1135,7 @@ static void determine_macros(struct all_attrs_item *all_attrs, > * If check->check_nr is non-zero, only attributes in check[] are collected. > * Otherwise all attributes are collected. > */ > -static void collect_some_attrs(struct index_state *istate, > +static void collect_some_attrs(struct repository *repo, struct index_state *istate, > const struct object_id *tree_oid, > const char *path, struct attr_check *check) > { > @@ -1164,7 +1156,7 @@ static void collect_some_attrs(struct index_state *istate, > dirlen = 0; > } > > - prepare_attr_stack(istate, tree_oid, path, dirlen, &check->stack); > + prepare_attr_stack(repo, istate, tree_oid, path, dirlen, &check->stack); > all_attrs_init(&g_attr_hashmap, check); > determine_macros(check->all_attrs, check->stack); > > @@ -1310,7 +1302,7 @@ void git_check_attr(struct index_state *istate, > int i; > const struct object_id *tree_oid = default_attr_source(); > > - collect_some_attrs(istate, tree_oid, path, check); > + collect_some_attrs(the_repository, istate, tree_oid, path, check); > The other places in the same file we pass around the 'repository' struct, but here we use 'the_repository'. Even below, we modify an external function's signature to avail the 'repository' struct. Can't we modify 'git_check_attr()' to also receive a 'repository'? If not, perhaps it would be much simpler to simply pass 'the_repository' everywhere and cleanup this file in another follow up series? > for (i = 0; i < check->nr; i++) { > unsigned int n = check->items[i].attr->attr_nr; > @@ -1321,14 +1313,14 @@ void git_check_attr(struct index_state *istate, > } > } > > -void git_all_attrs(struct index_state *istate, > +void git_all_attrs(struct repository *repo, struct index_state *istate, > const char *path, struct attr_check *check) > { > int i; > const struct object_id *tree_oid = default_attr_source(); > > attr_check_reset(check); > - collect_some_attrs(istate, tree_oid, path, check); > + collect_some_attrs(repo, istate, tree_oid, path, check); > > for (i = 0; i < check->all_attrs_nr; i++) { > const char *name = check->all_attrs[i].attr->name; > diff --git a/attr.h b/attr.h > index a04a521092..1ff058bef7 100644 > --- a/attr.h > +++ b/attr.h > @@ -213,11 +213,13 @@ void git_check_attr(struct index_state *istate, > const char *path, > struct attr_check *check); > > +struct repository; > + > /* > * Retrieve all attributes that apply to the specified path. > * check holds the attributes and their values. > */ > -void git_all_attrs(struct index_state *istate, > +void git_all_attrs(struct repository *repo, struct index_state *istate, > const char *path, struct attr_check *check); > > enum git_attr_direction { > @@ -232,9 +234,6 @@ void attr_start(void); > /* Return the system gitattributes file. */ > const char *git_attr_system_file(void); > > -/* Return the global gitattributes file, if any. */ > -const char *git_attr_global_file(void); > - > /* Return whether the system gitattributes file is enabled and should be used. */ > int git_attr_system_is_enabled(void); > > diff --git a/builtin/check-attr.c b/builtin/check-attr.c > index 7cf275b893..1b8a89dfb2 100644 > --- a/builtin/check-attr.c > +++ b/builtin/check-attr.c > @@ -70,7 +70,7 @@ static void check_attr(const char *prefix, struct attr_check *check, > prefix_path(prefix, prefix ? strlen(prefix) : 0, file); > > if (collect_all) { > - git_all_attrs(the_repository->index, full_path, check); > + git_all_attrs(the_repository, the_repository->index, full_path, check); > } else { > git_check_attr(the_repository->index, full_path, check); > } > diff --git a/builtin/var.c b/builtin/var.c > index ada642a9fe..8fbf5430a4 100644 > --- a/builtin/var.c > +++ b/builtin/var.c > @@ -71,7 +71,7 @@ static char *git_attr_val_system(int ident_flag UNUSED) > > static char *git_attr_val_global(int ident_flag UNUSED) > { > - char *file = xstrdup_or_null(git_attr_global_file()); > + char *file = xstrdup_or_null(repo_settings_get_attributesfile_path(the_repository)); > if (file) { > normalize_path_copy(file, file); > return file; > -- > 2.48.GIT
Karthik Nayak <karthik.188@gmail.com> writes: > Ayush Chandekar <ayu.chandekar@gmail.com> writes: > >> Update attribute-related functions to retrieve the "core.attributesfile" >> configuration via the new repository-scoped accessor >> `repo_settings_get_attributesfile_path()`. This improves behaviour in >> multi-repository contexts and aligns with the goal of minimizing >> reliance on global state. >> > > We should also talk about the modifications made to pass around the > repository struct. Yes. We first should justify if it makes sense to cram attribute set into the repository object and pass it around in the first place. Many index-state related functions do not pass repository around because they work on index-state, so index-state is passed around instead. Perhaps the attribute subsystem should be the same way, in that their globals should belong to its own abstraction that is smaller scale than a repository object (it is permissible to have such an attribute-set object know about which repository instance it is related to, though).
On Mon, Mar 10, 2025 at 08:40:48PM +0530, Ayush Chandekar wrote: > Update attribute-related functions to retrieve the "core.attributesfile" > configuration via the new repository-scoped accessor > `repo_settings_get_attributesfile_path()`. This improves behaviour in > multi-repository contexts and aligns with the goal of minimizing > reliance on global state. > > Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com> > --- > attr.c | 28 ++++++++++------------------ > attr.h | 7 +++---- > builtin/check-attr.c | 2 +- > builtin/var.c | 2 +- > 4 files changed, 15 insertions(+), 24 deletions(-) > > diff --git a/attr.c b/attr.c > index 0bd2750528..8f28463e8c 100644 > --- a/attr.c > +++ b/attr.c > @@ -879,14 +879,6 @@ const char *git_attr_system_file(void) > return system_wide; > } > > -const char *git_attr_global_file(void) > -{ > - if (!git_attributes_file) > - git_attributes_file = xdg_config_home("attributes"); > - > - return git_attributes_file; > -} > - > int git_attr_system_is_enabled(void) > { > return !git_env_bool("GIT_ATTR_NOSYSTEM", 0); > @@ -906,7 +898,7 @@ static void push_stack(struct attr_stack **attr_stack_p, > } > } > > -static void bootstrap_attr_stack(struct index_state *istate, > +static void bootstrap_attr_stack(struct repository *repo, struct index_state *istate, I have scanned the definition of the "struct index_state", there is a "struct repository *repo" member in this data structure. This makes me think why do we need to pass the "struct repository *repo" in the first place. A design question, should we just use `istate->repo` directly? > const struct object_id *tree_oid, > struct attr_stack **stack) > { > @@ -927,8 +919,8 @@ static void bootstrap_attr_stack(struct index_state *istate, > } > > /* home directory */ > - if (git_attr_global_file()) { > - e = read_attr_from_file(git_attr_global_file(), flags); > + if (repo_settings_get_attributesfile_path(repo)) { > + e = read_attr_from_file(repo_settings_get_attributesfile_path(repo), flags); > push_stack(stack, e, NULL, 0); > } > > @@ -946,7 +938,7 @@ static void bootstrap_attr_stack(struct index_state *istate, > push_stack(stack, e, NULL, 0); > } > > -static void prepare_attr_stack(struct index_state *istate, > +static void prepare_attr_stack(struct repository *repo, struct index_state *istate, > const struct object_id *tree_oid, > const char *path, int dirlen, > struct attr_stack **stack) If we use "istate->repo", we don't even need to change this function. > @@ -969,7 +961,7 @@ static void prepare_attr_stack(struct index_state *istate, > * .gitattributes in deeper directories to shallower ones, > * and finally use the built-in set as the default. > */ > - bootstrap_attr_stack(istate, tree_oid, stack); > + bootstrap_attr_stack(repo, istate, tree_oid, stack); > > /* > * Pop the "info" one that is always at the top of the stack. [snip] Thanks, Jialuo
shejialuo <shejialuo@gmail.com> writes: >> -static void bootstrap_attr_stack(struct index_state *istate, >> +static void bootstrap_attr_stack(struct repository *repo, struct index_state *istate, > > I have scanned the definition of the "struct index_state", there is a > "struct repository *repo" member in this data structure. This makes me > think why do we need to pass the "struct repository *repo" in the first > place. A design question, should we just use `istate->repo` directly? Good thing to notice. As the attribute system is all about giving extra information on the paths that appear in the index and in the working tree, it may make sense for the API to go from the index state which is about the index and the working tree to access the attributes, rather than from the repository structure, which controls a lot wider concept and moving anything and everything there will easily and quickly make it a messy kitchen sink.
> If we use "istate->repo", we don't even need to change this function.
Oh you're absolutely right about that. I actually just got to learn more
about `index_state` from another thread where Junio brought it up.
But now with his suggestion that attributes may not belong in the
repository struct at all, I'm a bit unsure how to move the patch forward.
One thing I did take away from this is that we shouldn't be cramming
environment variables into the repository struct. This discussion has
definitely helped me think more clearly about the design, and I think
it'll guide me take better decisions going forward.
I’d really appreciate your thoughts on how you think we should
approach this from here.
Thanks,
Ayush:)
> Can't we modify 'git_check_attr()' to also receive a 'repository'? If > not, perhaps it would be much simpler to simply pass 'the_repository' > everywhere and cleanup this file in another follow up series? > Right, that was one of the things I considered too. But since `git_check_attr()` is used in a lot of widely used code paths that don't currently pass a struct repository, it felt like threading repo through all of them would create a much larger change that I intended for this patch series. That's why I decided to stick with `the_repository` for now, and perhaps revisit the cleanup in a follow-up series once the proposed changes are accepted by the community.
diff --git a/attr.c b/attr.c index 0bd2750528..8f28463e8c 100644 --- a/attr.c +++ b/attr.c @@ -879,14 +879,6 @@ const char *git_attr_system_file(void) return system_wide; } -const char *git_attr_global_file(void) -{ - if (!git_attributes_file) - git_attributes_file = xdg_config_home("attributes"); - - return git_attributes_file; -} - int git_attr_system_is_enabled(void) { return !git_env_bool("GIT_ATTR_NOSYSTEM", 0); @@ -906,7 +898,7 @@ static void push_stack(struct attr_stack **attr_stack_p, } } -static void bootstrap_attr_stack(struct index_state *istate, +static void bootstrap_attr_stack(struct repository *repo, struct index_state *istate, const struct object_id *tree_oid, struct attr_stack **stack) { @@ -927,8 +919,8 @@ static void bootstrap_attr_stack(struct index_state *istate, } /* home directory */ - if (git_attr_global_file()) { - e = read_attr_from_file(git_attr_global_file(), flags); + if (repo_settings_get_attributesfile_path(repo)) { + e = read_attr_from_file(repo_settings_get_attributesfile_path(repo), flags); push_stack(stack, e, NULL, 0); } @@ -946,7 +938,7 @@ static void bootstrap_attr_stack(struct index_state *istate, push_stack(stack, e, NULL, 0); } -static void prepare_attr_stack(struct index_state *istate, +static void prepare_attr_stack(struct repository *repo, struct index_state *istate, const struct object_id *tree_oid, const char *path, int dirlen, struct attr_stack **stack) @@ -969,7 +961,7 @@ static void prepare_attr_stack(struct index_state *istate, * .gitattributes in deeper directories to shallower ones, * and finally use the built-in set as the default. */ - bootstrap_attr_stack(istate, tree_oid, stack); + bootstrap_attr_stack(repo, istate, tree_oid, stack); /* * Pop the "info" one that is always at the top of the stack. @@ -1143,7 +1135,7 @@ static void determine_macros(struct all_attrs_item *all_attrs, * If check->check_nr is non-zero, only attributes in check[] are collected. * Otherwise all attributes are collected. */ -static void collect_some_attrs(struct index_state *istate, +static void collect_some_attrs(struct repository *repo, struct index_state *istate, const struct object_id *tree_oid, const char *path, struct attr_check *check) { @@ -1164,7 +1156,7 @@ static void collect_some_attrs(struct index_state *istate, dirlen = 0; } - prepare_attr_stack(istate, tree_oid, path, dirlen, &check->stack); + prepare_attr_stack(repo, istate, tree_oid, path, dirlen, &check->stack); all_attrs_init(&g_attr_hashmap, check); determine_macros(check->all_attrs, check->stack); @@ -1310,7 +1302,7 @@ void git_check_attr(struct index_state *istate, int i; const struct object_id *tree_oid = default_attr_source(); - collect_some_attrs(istate, tree_oid, path, check); + collect_some_attrs(the_repository, istate, tree_oid, path, check); for (i = 0; i < check->nr; i++) { unsigned int n = check->items[i].attr->attr_nr; @@ -1321,14 +1313,14 @@ void git_check_attr(struct index_state *istate, } } -void git_all_attrs(struct index_state *istate, +void git_all_attrs(struct repository *repo, struct index_state *istate, const char *path, struct attr_check *check) { int i; const struct object_id *tree_oid = default_attr_source(); attr_check_reset(check); - collect_some_attrs(istate, tree_oid, path, check); + collect_some_attrs(repo, istate, tree_oid, path, check); for (i = 0; i < check->all_attrs_nr; i++) { const char *name = check->all_attrs[i].attr->name; diff --git a/attr.h b/attr.h index a04a521092..1ff058bef7 100644 --- a/attr.h +++ b/attr.h @@ -213,11 +213,13 @@ void git_check_attr(struct index_state *istate, const char *path, struct attr_check *check); +struct repository; + /* * Retrieve all attributes that apply to the specified path. * check holds the attributes and their values. */ -void git_all_attrs(struct index_state *istate, +void git_all_attrs(struct repository *repo, struct index_state *istate, const char *path, struct attr_check *check); enum git_attr_direction { @@ -232,9 +234,6 @@ void attr_start(void); /* Return the system gitattributes file. */ const char *git_attr_system_file(void); -/* Return the global gitattributes file, if any. */ -const char *git_attr_global_file(void); - /* Return whether the system gitattributes file is enabled and should be used. */ int git_attr_system_is_enabled(void); diff --git a/builtin/check-attr.c b/builtin/check-attr.c index 7cf275b893..1b8a89dfb2 100644 --- a/builtin/check-attr.c +++ b/builtin/check-attr.c @@ -70,7 +70,7 @@ static void check_attr(const char *prefix, struct attr_check *check, prefix_path(prefix, prefix ? strlen(prefix) : 0, file); if (collect_all) { - git_all_attrs(the_repository->index, full_path, check); + git_all_attrs(the_repository, the_repository->index, full_path, check); } else { git_check_attr(the_repository->index, full_path, check); } diff --git a/builtin/var.c b/builtin/var.c index ada642a9fe..8fbf5430a4 100644 --- a/builtin/var.c +++ b/builtin/var.c @@ -71,7 +71,7 @@ static char *git_attr_val_system(int ident_flag UNUSED) static char *git_attr_val_global(int ident_flag UNUSED) { - char *file = xstrdup_or_null(git_attr_global_file()); + char *file = xstrdup_or_null(repo_settings_get_attributesfile_path(the_repository)); if (file) { normalize_path_copy(file, file); return file;
Update attribute-related functions to retrieve the "core.attributesfile" configuration via the new repository-scoped accessor `repo_settings_get_attributesfile_path()`. This improves behaviour in multi-repository contexts and aligns with the goal of minimizing reliance on global state. Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com> --- attr.c | 28 ++++++++++------------------ attr.h | 7 +++---- builtin/check-attr.c | 2 +- builtin/var.c | 2 +- 4 files changed, 15 insertions(+), 24 deletions(-)