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

On 6/28/26 04:47, Junio C Hamano wrote:

Tian Yuchen [off-list ref] writes:

quoted

Hi all,

Apologies again for the duplicate...

On 6/28/26 00:08, Tian Yuchen wrote:

quoted

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

quoted

quoted

+	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