Re: [PATCH] environment: move access to "core.attributesfile" into repo settings

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> 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.