Re: [PATCH v2] config.c: NULL check when reading protected config

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

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/config.c b/config.c
> index 015bec360f5..e8ebef77d5c 100644
> --- a/config.c
> +++ b/config.c
> @@ -2645,9 +2647,12 @@ static void read_protected_config(void)
>  	system_config = git_system_config();
>  	git_global_config(&user_config, &xdg_config);
>  
> -	git_configset_add_file(&protected_config, system_config);
> -	git_configset_add_file(&protected_config, xdg_config);
> -	git_configset_add_file(&protected_config, user_config);
> +	if (system_config)
> +		git_configset_add_file(&protected_config, system_config);
> +	if (xdg_config)
> +		git_configset_add_file(&protected_config, xdg_config);
> +	if (user_config)
> +		git_configset_add_file(&protected_config, user_config);
>  	git_configset_add_parameters(&protected_config);

This does make it similar to the way how the primary "config
sequence" reader calls git_config_from_file(), so I do prefer the
patch as posted as a short-term "oops, we merged a buggy code that
segfaults and here is a fix" patch.

It however makes me wonder if it is simpler to allow passing NULL to
git_config_from_file_with_options() and make it silently turn into a
no-op.  I.e. instead of ...

> @@ -1979,6 +1979,8 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,
>  	int ret = -1;
>  	FILE *f;
>  
> +	if (!filename)
> +		BUG("filename cannot be NULL");

... we could do

	if (!filename)
		return 0; /* successful no-op */

Even if there are codepaths that feed arbitrary pathnames given by
the end user, they wouldn't be passing NULL (they may pass an empty
string, or a filename that causes fopen() to fail), would they?

But that is something we should leave to a follow-up series, not
"oops, we need to fix it now" fix.

Thanks, will queue.

>  	f = fopen_or_warn(filename, "r");
>  	if (f) {
>  		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,