Re: Minor bug with help.autocorrect.

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

Jeff King <peff@peff.net> writes:

> I think the plan is:
>
>   1. squelch the warning message from the config code; even if we change
>      the config format to pager.*.command, we will have to support
>      pager.* for historical reasons.
>
>   2. introduce pager.*.command so that "git foo_bar" can use
>      pager.foo_bar.command.
>
> We should do (1) in the near-term. We do not have to do (2) at all (and
> people with funny command names are simply out of luck), but it would be
> nice in the long run.

That sounds sensible.

> The patch from Tanay in $gmane/263888 accomplishes (1), but there was a
> minor cleanup needed (checking the individual bit in "flags", rather
> than the whole variable). Here it is with that fix:

Thanks; let's take a look.  I have a suspicion that it "accomplishes"
a lot more than (1) and may be discarding useful errors.

> diff --git a/config.c b/config.c
> index 9fd275f..dd0cb52 100644
> --- a/config.c
> +++ b/config.c
> @@ -1314,7 +1314,7 @@ static struct config_set_element *configset_find_element(struct config_set *cs,
>  	 * `key` may come from the user, so normalize it before using it
>  	 * for querying entries from the hashmap.
>  	 */
> -	ret = git_config_parse_key(key, &normalized_key, NULL);
> +	ret = git_config_parse_key(key, &normalized_key, NULL, CONFIG_ERROR_QUIET);

Hmm, I am not sure if this is correct, or it is trying to do things
at too low a level.

configset_add_value() calls configset_find_element().

A NULL return from find_element() could be because parse_key()
errored out due to bad name, or the key genuinely did not exist in
the hashmap, and the caller cannot tell.  So add_value() can end up
adding a new <key,value> pair under a bogus key, which is not a new
problem, but what makes me cautious is that it happens silently with
the updated code.

In fact, git_configset_add_file() uses git_config_from_file() with
configset_add_value() as its callback function, and the error that
is squelched with this CONFIG_ERROR_QUIET would be the only thing
that tells the user that the config file being read is malformed.

Right now, "git config" does not seem to use the full configset API
so nobody would notice, but still...

I wonder if alias_lookup() and check_pager_config(), two functions
that *know* that the string they have, cmd, could be invalid and
unusable key to give to the config API, should be doing an extra
effort (e.g. call parse_key() with QUIET option and refrain from
calling git_config_get_value()).  It feels that for existing callers
of parse_key(), not passing QUIET would be the right thing to do.

Of course, I am OK if git_config_get_value() and friends took the
QUIET flag and and passed it all the way down to parse_key(); that
would be a much more correct approach to address this issue (these
two callers do not have to effectively call parse_key() twice that
way), but at the same time, that would be a lot more involved
change.