Re: [PATCH 1/1] config: allow user to know scope of config options

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

"Matthew Rogers via GitGitGadget" <gitgitgadget@gmail.com> writes:

> -static int end_null;
> +static int end_nul;
> -	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
> +	OPT_BOOL('z', "null", &end_nul, N_("terminate values with NUL byte")),
> -	const char term = end_null ? '\0' : '\t';
> +	const char term = end_nul ? '\0' : '\t';
> -	if (end_null)
> +	if (end_nul)

These are correct changes, but is unrelated noise in the context of
the theme of the patch, no?

> +static const char *scope_to_string(enum config_scope scope) {
> +	/*
> +	 * --local, --global, and --system work the same as --file so there's
> +	 * no easy way for the parser to tell the difference when it is
> +	 * setting the scope, so we use our information about which options
> +	 * were passed
> +	 */
> +	if (use_local_config || scope == CONFIG_SCOPE_REPO) {
> +		return "local";
> +	} else if (use_global_config || scope == CONFIG_SCOPE_GLOBAL) {
> +		return "global";
> +	} else if (use_system_config || scope == CONFIG_SCOPE_SYSTEM) {
> +		return "system";

The above is slightly tricky; --global/--system/--local are made
mutually exclusive in the higher level, so if any of them is set,
we do not even need to look at the "scope" to tell what kind of
source we are reading from.

> +	} else if (given_config_source.use_stdin ||
> +		given_config_source.blob ||
> +		given_config_source.file ||
> +		scope == CONFIG_SCOPE_CMDLINE) {
> +		return "command line";

I am not sure what the implication of saying "they came from the
command line" when we read from the standard input or from a blob.

> +	} else {
> +		return "unknown";
> +	}
> +}

In any case, the need for such logic that says "scope might not say
it is REPO, but when use_local_config is true, we are doing a local
config" implies that "scope" parameter the caller of this function
has is not set correctly when these options are used---would that be
the real bug that needs fixing, rather than getting "worked around"
with a code like this?

It almost makes me point fingers at config.c::config_with_options()
where config_source is inspected and git_config_from_*() helpers are
called without setting the current_parsing_scope.  Unlike these
cases, when do_git_config_sequence() is called from that function,
the scope is recorded in the variable before each standard config
source file is opened and read.  What would we be breaking if we
taught the function to set the current_parsing_scope variable
correctly even when reading from the config_source?  That would
certainly simplify this function quite a lot, but if the other parts
of the codebase relies on the current behaviour, we cannot make such
a change lightly.

> +static void show_config_scope(struct strbuf *buf)
> +{
> +	const char term = end_nul ? '\0' : '\t';
> +	const char *scope = scope_to_string(current_config_scope());
> +
> +	strbuf_addch(buf, '(');
> +	if (end_nul)
> +		strbuf_addstr(buf, N_(scope));
> +	else
> +		quote_c_style(scope, buf, NULL, 0);

Isn't this overkill?  I think this code was copied-and-pasted from
the other function that needs to show an arbitrary end-user supplied
data which is a pathname, so it makes perfect sense to use c-style
quoting, but the token scope_to_string() returns is taken from a
bounded set that doesn't require such quoting, no?

> +	strbuf_addch(buf, ')');
> +	strbuf_addch(buf, term);
> +}
> +