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

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

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.