Patrick Steinhardt <ps@xxxxxx> writes: > On Sun, Mar 09, 2025 at 09:03:21PM +0530, Ayush Chandekar wrote: >> Stop relying on global state to access the "core.attributesfile" >> configuration. Instead, store the value in `struct repo_settings` and >> retrieve it via `repo_settings_get_attributes_file_path()`. >> >> This prevents incorrect values from being used when a user or tool is >> handling multiple repositories in the same process, each with different >> attribute configurations. It also improves repository isolation and helps >> progress towards libification by avoiding unnecessary global state. > > We typically switch the order around a bit in our commit messages: we > first explain what the actual problem is, and then we say how we fix it. A good suggestion. >> int git_attr_system_is_enabled(void) > > Hm. I wonder what the actual merit of this function is after the > refactoring. Right now there isn't really any as it is a direct wrapper > of `repo_settings_get_attributesfile_path()`. Another thing that felt awkward about this change is actually larger. The current attributes globals are built around the notion that the functions involved work on a single set of attributes at a time. Even in a single repository, when you are checking new contents into the object database and when you are checking objects out of the object database, you'd need to switch the direction manually, which means you always have two sets of attributes active that you can switch between (one is from the working tree and the other one is from the index, if I am recalling correctly). But step back and think. What does it mean to make them belong to a repository instance? Whose index and working tree does the attribute set that belongs to a repository that is not the_repository come from? So it seems to me that removing the reliance on globals is a good thing, and introducing some abstraction is a good step forward, but it is dubious if the abstracted "set of attributes" should be part of a repository instance. This is a bit similar to the_index situation, where we can have more than one in-core index instance for a given repository we are working with, an index_state knows its repository, but a repository instance only has a pointer to its primary index_state instance and does not know other index_state objects. The right way to deal with in-core index is to pass index_state, not repository, through the call chain for this reason, and I suspect that we are better off to make sure that we do the same for the attributes, i.e. pass pointer to an attribute set instance through the callchain, not a repository.
