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

[Tian Yuchen <cat@malon.dev>]

On 6/28/26 04:47, Junio C Hamano wrote:
> 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?
> 

That makes sense, but from my point of view...

'repo_config_values()' already has a check for 'repo->initialized'. If 
we're absolutely certain that the 'repo' is initialized, wouldn't it be 
better to simply remove all the checks inside the getter and leave the 
judgment to 'repo_config_values()'?

This also aligns to some extent with the previous flag getters: since an 
uninitialized 'repo' will trigger a BUG() in 'repo_config_values()', but 
"reading and writing these flags when the 'repo' is uninitialized" is 
sometimes a valid operation that's why we choose to intercept 
'repo->initialized' _before_ 'repo_config_values()' and fallback to the 
hard-coded values.

This gives the impression that _'repo_config_values()' is the function 
responsible for checking_, and the way flags are handled is an exception 
to this approach, which I think is more consistent and self-explanatory.

What do you think?


> 
>>> +	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.
> 

Thanks.

regards, yuchen