On Fri, Aug 21, 2015 at 03:13:38PM -0700, Junio C Hamano wrote: > > 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. I assumed that we would only get well-formed keys here. That is, the config parser should be enforcing the rules already in config.c:get_parse_source and friends. In that sense, the parse_key in the configset_add_value path _should_ be a noop (and this patch does make it worse by quieting a warning we would get for a potential bug). For example: $ echo "utter_crap = true" >.git/config $ git config foo.bar fatal: bad config file line 6 in .git/config It looks like the "-c" code is more forgiving, though, and does rely on this check: $ git -c utter_crap=true log >/dev/null error: key does not contain a section: utter_crap So the patch is a regression there. > Right now, "git config" does not seem to use the full configset API > so nobody would notice, but still... Hmm. I don't think that matters for bad config files. Even after we move to configset there, it will still have to run that same parsing code. But when I say: $ git config utter_crap error: key does not contain a section: utter_crap I think we would end up relying on this code to tell me that my request was bogus. And that needs to keep being noisy, to tell the user what's going on (as opposed to check_pager_config(), which really wants to say "I'm _aware_ I might be feeding you junk"). > 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. Yeah, I agree these are the only two sane fixes. Plumbing the quiet flag through does seem really invasive; every "git_config_get_foo" variant would have to learn it. Probably it's too gross to have a global like: config_lax_mode = 1; git_config_get_string(key.buf, &v); config_lax_mode = 0; That makes a nice tidy patch, but I have a feeling we would regret it later. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html