Re: [PATCH v2 2/3] config: let sequence require a successful file
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 26, 2026, 18:20 UTC
- Message-ID
- <xmqqy0dsg2vt.fsf@gitster.g>
- In-Reply-To
- <20260823-fix-config-list-global-home-and-xdg-v2-2-b29cc63f017b@microsoft.com>
Delilah Ashley Wu <delilahwu@linux.microsoft.com> writes:
Show 11 quoted lines
> From: Delilah Ashley Wu <delilahwu@microsoft.com> > > Teach `do_git_config_sequence()` to optionally report an error if no > configuration files in the sequence were successfully processed. Gate > this new behaviour with a flag and keep it disabled for now. > > Add tests to record existing behaviour and prevent regressions in the > next patch, "config: read global scope via config_sequence", which adds > a code path that enables the flag. When no global configuration file > exists, `git config list` succeeds whereas `git config list --global` > fails. The command output is irrelevant, so only check the exit code.
It is not exactly 'irrelevant' as that is how the user learns what caused the command to fail, e.g. "fatal: unable to read config file <path>".
What you meant was that you are not interested in the exact message, you only want to make sure it fails because of the missing file, and you thought that it is a good way to do so to check the exit code.
Show 13 quoted lines
> Signed-off-by: Delilah Ashley Wu <delilahwu@microsoft.com>
> ---
> config.c | 57 ++++++++++++++++++++++++++++++++++++++-----------------
> t/t1300-config.sh | 12 ++++++++++++
> 2 files changed, 52 insertions(+), 17 deletions(-)
>
> diff --git a/config.c b/config.c
> index 1bdd702e7a..4c958f46bf 100644
> --- a/config.c
> +++ b/config.c
> @@ -1544,11 +1544,27 @@ int git_config_system(void)
> return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
> }Perhaps "attempt" -> "try" or something more clever can be used to make sure we won't have to type so many characters. "try_config()" should be decriptive enough for the purpose, for example.
File scope static helper functions do not have to be and should not be named with so many words. Shorter names would also help to keep your lines under ~70 column limit.
Show 37 quoted lines
> +static void attempt_git_config_from_file_with_options(config_fn_t fn,
> + const char *filename,
> + void *data,
> + enum config_scope scope,
> + const struct config_options *opts,
> + int *success_count,
> + int *cumulative_ret)
> +{
> + int ret = git_config_from_file_with_options(fn, filename, data,
> + scope, opts);
> + if (!ret)
> + (*success_count)++;
> + *cumulative_ret += ret;
> +}
> +
> static int do_git_config_sequence(const struct config_options *opts,
> - const struct repository *repo,
> - config_fn_t fn, void *data)
> + const struct repository *repo, config_fn_t fn,
> + void *data, int require_successful_config)
> {
> int ret = 0;
> + int success_count = 0;
> char *system_config = git_system_config();
> char *xdg_config = NULL;
> char *user_config = NULL;
> @@ -1574,32 +1590,35 @@ static int do_git_config_sequence(const struct config_options *opts,
> if (git_config_system() && system_config &&
> !access_or_die(system_config, R_OK,
> opts->system_gently ? ACCESS_EACCES_OK : 0))
> - ret += git_config_from_file_with_options(fn, system_config,
> - data, CONFIG_SCOPE_SYSTEM,
> - NULL);
> + attempt_git_config_from_file_with_options(fn, system_config, data,
> + CONFIG_SCOPE_SYSTEM, NULL,
> + &success_count, &ret);
> If we are allowed to use system config, system_config is defined, and we can read the system config, we try to grab values from it, and record the fact that we did so successfully.
> git_global_config_paths(&user_config, &xdg_config);
We grab paths to two files, as before.
Show 7 quoted lines
> if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) > - ret += git_config_from_file_with_options(fn, xdg_config, data, > - CONFIG_SCOPE_GLOBAL, NULL); > + attempt_git_config_from_file_with_options(fn, xdg_config, > + data, > + CONFIG_SCOPE_GLOBAL, > + NULL, &success_count, &ret);
If xdg config is to be used (note: GIT_CONFIG_GLOBAL environment can disable the use of it) and xdg file is available, we read and record just like we saw is done for the system config above.
Show 7 quoted lines
> if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) > - ret += git_config_from_file_with_options(fn, user_config, data, > - CONFIG_SCOPE_GLOBAL, NULL); > + attempt_git_config_from_file_with_options(fn, user_config, > + data, > + CONFIG_SCOPE_GLOBAL, > + NULL, &success_count, &ret);
Ditto fo user config.
Show 6 quoted lines
> if (!opts->ignore_repo && repo_config && > !access_or_die(repo_config, R_OK, 0)) > - ret += git_config_from_file_with_options(fn, repo_config, data, > - CONFIG_SCOPE_LOCAL, NULL); > + attempt_git_config_from_file_with_options(fn, repo_config, data, > + CONFIG_SCOPE_LOCAL, NULL, &success_count, &ret);
And the local one.
Show 11 quoted lines
> if (!opts->ignore_worktree && worktree_config &&
> repo && repo->repository_format_worktree_config &&
> - !access_or_die(worktree_config, R_OK, 0)) {
> - ret += git_config_from_file_with_options(fn, worktree_config, data,
> - CONFIG_SCOPE_WORKTREE,
> - NULL);
> - }
> + !access_or_die(worktree_config, R_OK, 0))
> + attempt_git_config_from_file_with_options(fn, worktree_config, data,
> + CONFIG_SCOPE_WORKTREE,
> + NULL, &success_count, &ret);And the per-worktree one.
Show 9 quoted lines
> if (!opts->ignore_cmdline && git_config_from_parameters(fn, data) < 0)
> die(_("unable to parse command-line config"));
> @@ -1609,6 +1628,10 @@ static int do_git_config_sequence(const struct config_options *opts,
> free(user_config);
> free(repo_config);
> free(worktree_config);
> +
> + if (require_successful_config && !success_count && !ret)
> + ret = -1;If we are asked to ensure that we successfully read at least one place and we didn't, we assign -1 to ret but we do so ONLY when we haven't seen any other errors (i.e., existing non-zero ret is preserved, which may not be -1). OK.
> return ret; > }
I am not convinced 100% that we need "success_count", either, until we see how it is used in the later steps. But from the way the try_config() thing is used, I find it dubious that it now returns void. It should just keep returning the error code as before, and the caller should just keep accumulcating as the original code used to. I.e.,
ret += try_config(fn, frotz_config, data,
CONFIG_SCOPE_FROTZ, NULL,
&success);