Re: [PATCH v2 2/2] environment: move excludes_file into repo_config_values

[Tian Yuchen <cat@malon.dev>]

On 6/27/26 03:12, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
> 
>> Continue the libification effort by moving the 'excludes_file' global
>> variable into 'struct repo_config_values'.
>>
>> Since 'excludes_file' is a dynamically allocated string (char *), it
>> requires proper memory management. Introduce repo_config_values_clear()
>> to safely free the heap memory when repository instance is destroyed.
>>
>> Note:
>>
>>   - 'if (repo != the_repository)' fallback logic is temporarily added
>> in both the getter and the clear function. This prevents calling
>> repo_config_values() on uninitialized submodules, which triggers BUG().
> 
> Would it be possible for the function to be called on the_repository
> before it gets initialized?
> 
>> +void repo_config_values_clear(struct repository *repo)
>> +{
>> +	struct repo_config_values *cfg;
> 
> What I am wondering is if this check
> 
>> +	if (repo != the_repository)
>> +		return;
> 
> wants to be more like
> 
> 	if (!repo->initialized)
> 		return;
> 
> or even
> 
> 	if (!repo->initialized) {
> 		BUG("clearing uninitialised repo config");
> 		return;
> 	}
> 
> Or perhaps not doing anything special there.
> 
> For that matter,
> 
>> +
>> +	cfg = repo_config_values(repo);
>> +	if (!cfg)
>> +		return;
> 
> Wouldn't it be a bug to see NULL returned from the above function?
> Why is it healthy to pretend as if nothing bad happened?
> 
>> +	FREE_AND_NULL(cfg->attributes_file);
>> +	FREE_AND_NULL(cfg->excludes_file);
>> +}
> 
> 
> What do we want to happen when somebody does want to access (not
> _clear(), but repo_config_values() itself) repo_config_values() in
> today's code?  Don't we want to catch such a code as buggy?  Isn't
> it the reason why repository.c:repo_config_values() check these
> conditions and calls BUG() in the first place?  And if that is the
> case, I find that a caller that "works around" by pretending nothing
> bad happened and not calling repo_config_values(), like the above
> code does, highly questionable, as it smells like sweeping problems
> that you designed other parts of the code to detect with BUG() under
> the rug.
> 
> In the longer run, we would want to have separate settings, which
> used to be global variables but now are stored in per repository
> config, available and usable in the context of each repository that
> they are configured within.  If a caller wants to clear per repo
> config for a repository instance by calling this function, this
> function is in no place to tweak the intention of the caller by
> short-circuiting the request and pretending it did what it was asked
> to do.  In other words, the rest of the code not quite prepared to
> deal with these global variables that turned into per repository
> configuration values is *not* a problem this function should
> address.  Let it be noticed by repo_config_values() function to
> catch offending callers for now, and once the codebase becomes ready
> to use one repo_config_values per repository, this function does not
> have to change.

Yes, indeed. I initially thought these were two solutions leading to the 
same goal, but now that you mention it, I think your point is more valid.

First, 'repo_config_values_clear()' shouldn't know whether "only 
'the_repository' is currently supported" (even if this logic is 
necessary, this function is clearly not the optimal place), so we 
shouldn't use 'repo != the_repository' to determine this.

Second, what's wrong with '!cfg' is that what it's supposed to avoid 
deviates from what it tries to avoid. 'repo_config_values(repo)' 
shouldn't validly return NULL at all. I admit this code is nonsensical.

Using 'repo->initialized' to determine this is consistent with our 
previous approach. I'll try it. Thanks for the review!

Regards, yuchen