Re: [PATCH v2 2/3] config: let sequence require a successful file

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

Delilah Ashley Wu <delilahwu@linux.microsoft.com> writes:

> 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.

> 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.

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

>  	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.

>  	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.

>  	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.

>  	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.

>  	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);