Re: [PATCH v3 1/3] environment: drop redundant NULL checks in config getters

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 11, 2026, 06:22 UTC
Message-ID: <aqOeHlPWer60LcoO@pks.im>
In-Reply-To: <20260807085932.3958759-2-cat@malon.dev>

On Fri, Aug 07, 2026 at 04:59:30PM +0800, Tian Yuchen wrote:

Show 5 quoted lines

> These repository config getters require a valid repository pointer.
> While an uninitialized repository is a valid state and is handled by
> returning default values, passing NULL is a programming error.
> 
> Drop the NULL checks so that invalid callers are not silently accepted.

I'm not quite convinced that having these checks in the first place is a good idea. The single biggest problem is that we silently ignore the settings in case the repository just happens to be uninitialized, and we wouldn't ever notice.

On top of that, we even fall back to the wrong value: if we don't have a repository, we shouldn't fall back to the default values. Instead, shouldn't we fall back to the global- or system-level configuration?

I'm not convinced that this design is correct. What I think we should be doing is:

  - Have the functions accept an optional repository.

  - If a repository is passed, then we verify that it is initialized.
    If not, we BUG.

  - If we haven't yet read the configuration for that repository, then
    we automatically do it so that we can also pass a repository other
    than `the_repository`.

  - If no repository is passed, then we populate a global variable that
    contains the system- and global-level configuration and return that
    value instead.

That'd work both in the context where we have a repository and where we don't have one, and we'd detect the edge case where we have a repository that is uninitialized.

Show 15 quoted lines

> diff --git a/environment.c b/environment.c
> index 76ee65e62b..f5628b6758 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -119,23 +119,23 @@ int is_bare_repository(struct repository *repo)
>  
>  int repo_protect_ntfs(struct repository *repo)
>  {
> -	return (repo && repo->initialized) ?
> -		repo_config_values(repo)->protect_ntfs :
> -		PROTECT_NTFS_DEFAULT;
> +	return repo->initialized
> +		? repo_config_values(repo)->protect_ntfs
> +		: PROTECT_NTFS_DEFAULT;
>  }

So I think if we want to lose these checks, we should lose both of them and require the repository to be initialized. But I feel like this whole subsystem needs a bit of a redesign before we can continue iterating on it.

Patrick