Jeff King <p...@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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
