Re: [PATCH v4 1/1] environment: move excludes_file into repo_config_values

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

Tian Yuchen <cat@malon.dev> writes:

> Hi all,
>
> Apologies again for the duplicate...
>
> On 6/28/26 00:08, Tian Yuchen wrote:
>
>> +const char *repo_excludes_file(struct repository *repo)
>> +{
>> +	if (!repo || !repo->initialized)
>> +		return NULL;

I might already have said this, but I am not sure why want to be as
loose as this code.  It is not limited to this line, but I think we
saw plenty of other "We know we must get an already initialized
thing here, and the subsequent operation we perform on that thing
will cause us to die() later, so let's return silently and early
to avoid hitting die()" attempts to sweep problems under the rug.

Wouldn't we rather want to try to be more strict and say

	if (!repo || !repo->initialized)
		BUG("repo must be an initialied repository");

here?  Aren't all the callers of this function supposed to be
dealing with an already initialized repository?


>> +	if (!repo_config_values(repo)->excludes_file)
>> +		repo_config_values(repo)->excludes_file = xdg_config_home("ignore");
>> +
>> +	return repo_config_values(repo)->excludes_file;
>> +}
>
> One more thing:
>
> I deliberately didn't write a comment for the getter because it will 
> probably be merged with comments from the previous several patches in 
> some form in the near future... I'm not sure if it would be more 
> appropriate to write a separate patch to add the corresponding comments 
> then.

That's very sensible.