"Glen Choo via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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,
