Re: Minor bug with help.autocorrect.
On Mon, Aug 24, 2015 at 01:43:27AM -0400, Jeff King wrote:
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. [...]
Here is a patch to do the first. While it seems a little gross to have
to call parse_key twice, I think it does make sense. The alias.* and
pager.* config trees are mis-designed, and we are papering over the
problem for historical reasons.
-- 8 --
Subject: config: silence warnings for command names with invalid keys
When we are running the git command foo, we may have to
look up the config keys pager.foo and alias.foo. These
config schemes are mis-designed, as the command names can be
anything, but the config syntax has some restrictions. For
example:
$ git foo_bar
error: invalid key: pager.foo_bar
error: invalid key: alias.foo_bar
git: 'foo_bar' is not a git command. See 'git --help'.
You cannot name an alias with an underscore. And if you have
an external command with one, you cannot configure its
pager.
In the long run, we may develop a different config scheme
for these features. But in the near term (and because we'll
need to support the existing scheme indefinitely), we should
at least squelch the error messages shown above.
These errors come from git_config_parse_key. Ideally we
would pass a quiet flag to the config machinery, but there
are many layers between the pager code and the key parsing.
Passing a flag through all of those would be an invasive
change.
Instead, let's provide a config function to report on
whether a key is syntactically valid, and have the pager and
alias code skip lookup for bogus keys. We can build this
easily around the existing git_config_parse_key, with two
minor modifications:
1. We now handle a NULL store_key, to validate but not
write out the normalized key.
2. We accept a quiet flag to avoid writing to stderr.
This doesn't need to be a full-blown public flags
field, because we can make the existing implementation
a static helper function, keeping the mess contained
inside config.c.
Signed-off-by: Jeff King p...@peff.net
---
alias.c | 3 ++-
cache.h | 1 +
config.c | 39 +--
pager.c | 3 ++-
t/t7006-pager.sh | 9 +
5 files changed, 43 insertions(+), 12 deletions(-)
diff --git a/alias.c b/alias.c
index 6aa164a..a11229d 100644
--- a/alias.c
+++ b/alias.c
@@ -5,7 +5,8 @@ char *alias_lookup(const char *alias)
char *v = NULL;
struct strbuf key = STRBUF_INIT;
strbuf_addf(key, alias.%s, alias);
- git_config_get_string(key.buf, v);
+ if (git_config_key_is_valid(key.buf))
+ git_config_get_string(key.buf, v);
strbuf_release(key);
return v;
}
diff --git a/cache.h b/cache.h
index 4e25271..8de519a 100644
--- a/cache.h
+++ b/cache.h
@@ -1440,6 +1440,7 @@ extern int git_config_pathname(const char **, const char
*, const char *);
extern int git_config_set_in_file(const char *, const char *, const char *);
extern int git_config_set(const char *, const char *);
extern int git_config_parse_key(const char *, char **, int *);
+extern int git_config_key_is_valid(const char *key);
extern int git_config_set_multivar(const char *, const char *, const char *,
int);
extern int git_config_set_multivar_in_file(const char *, const char *, const
char *, const char *, int);
extern int git_config_rename_section(const char *, const char *);
diff --git a/config.c b/config.c
index 9fd275f..8adc15a 100644
--- a/config.c
+++ b/config.c
@@ -1848,7 +1848,7 @@ int git_config_set(const char *key, const char *value)
* baselen - pointer to int which will hold the length of the
* section + subsection part, can be NULL
*/
-int git_config_parse_key(const char *key, char **store_key, int *baselen_)
+static int git_config_parse_key_1(const char *key, char **store_key, int
*baselen_, int quiet)
{
int i, dot, baselen;
const char *last_dot = strrchr(key, '.');
@@ -1859,12 +1859,14 @@ int git_config_parse_key(const char *key, char
**store_key, int *baselen_)
*/
if (last_dot == NULL ||
Re: Minor bug with help.autocorrect.
Jeff King p...@peff.net writes: 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. :) Yeah, I do think the double-checking the patch in your follow-up does is not so bad. Thanks for following it through (now I must remember not to drop these patches ;-). -- 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
Re: Minor bug with help.autocorrect.
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Minor bug with help.autocorrect.
On Fri, Aug 21, 2015 at 09:10:35AM -0700, Junio C Hamano wrote:
$ git \#fetch
error: invalid key: pager.#fetch
error: invalid key: alias.#fetch
git: '#fetch' is not a git command. See 'git --help'.
Did you mean this?
fetch
Thanks. I somehow thought that we had some discussion on this
earlier, perhaps
http://thread.gmane.org/gmane.comp.version-control.git/263416/focus=263438
Peff, do you remember what (if anything) we decided to do about
this? I unfortunately don't X-.
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.
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:
-- 8 --
From: Tanay Abhra tanay...@gmail.com
Subject: [PATCH] add a flag to supress errors in git_config_parse_key()
`git_config_parse_key()` is used to sanitize the input key.
Some callers of the function like `git_config_set_multivar_in_file()`
get the pre-sanitized key directly from the user so it becomes
necessary to raise an error specifying what went wrong when the entered
key is syntactically malformed.
Other callers like `configset_find_element()` get their keys from git
itself so a return value signifying error would be enough. The error
output shown to the user is useless and confusing in this case, so add a
flag to suppress errors in such cases.
Helped-by: Junio C Hamano gits...@pobox.com
Helped-by: Jeff King p...@peff.net
Signed-off-by: Tanay Abhra tanay...@gmail.com
Signed-off-by: Jeff King p...@peff.net
---
builtin/config.c | 2 +-
cache.h | 4 +++-
config.c | 21 ++---
t/t7006-pager.sh | 9 +
4 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 7188405..f6cfb10 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -200,7 +200,7 @@ static int get_value(const char *key_, const char *regex_)
goto free_strings;
}
} else {
- if (git_config_parse_key(key_, key, NULL)) {
+ if (git_config_parse_key(key_, key, NULL, 0)) {
ret = CONFIG_INVALID_KEY;
goto free_strings;
}
diff --git a/cache.h b/cache.h
index 4e25271..4b95d7e 100644
--- a/cache.h
+++ b/cache.h
@@ -1410,6 +1410,8 @@ extern int update_server_info(int);
#define CONFIG_REGEX_NONE ((void *)1)
+#define CONFIG_ERROR_QUIET 0x0001
+
struct git_config_source {
unsigned int use_stdin:1;
const char *file;
@@ -1439,7 +1441,7 @@ extern int git_config_string(const char **, const char *,
const char *);
extern int git_config_pathname(const char **, const char *, const char *);
extern int git_config_set_in_file(const char *, const char *, const char *);
extern int git_config_set(const char *, const char *);
-extern int git_config_parse_key(const char *, char **, int *);
+extern int git_config_parse_key(const char *, char **, int *, unsigned int);
extern int git_config_set_multivar(const char *, const char *, const char *,
int);
extern int git_config_set_multivar_in_file(const char *, const char *, const
char *, const char *, int);
extern int git_config_rename_section(const char *, const char *);
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);
if (ret)
return NULL;
@@ -1847,11 +1847,14 @@ int git_config_set(const char *key, const char *value)
* lowercase section and variable name
* baselen - pointer to int which will hold the length of the
* section + subsection part, can be NULL
+ * flags - we respect CONFIG_ERROR_QUIET to toggle whether the function raises
+ * an error on a syntactically malformed key
*/
-int git_config_parse_key(const char *key, char **store_key, int *baselen_)
+int git_config_parse_key(const char *key, char **store_key, int *baselen_,
unsigned int flags)
{
int i, dot, baselen;
const char *last_dot = strrchr(key, '.');
+ int quiet = flags CONFIG_ERROR_QUIET;
/*
* Since key actually contains the section
Re: Minor bug with help.autocorrect.
Bjørnar Snoksrud snoks...@gmail.com writes: If you mis-type a git command starting with a non-letter, git internals will spit out some errors at you. $ git 5fetch error: invalid key: pager.5fetch error: invalid key: alias.5fetch git: '5fetch' is not a git command. See 'git --help'. Did you mean this? fetch $ git \#fetch error: invalid key: pager.#fetch error: invalid key: alias.#fetch git: '#fetch' is not a git command. See 'git --help'. Did you mean this? fetch Thanks. I somehow thought that we had some discussion on this earlier, perhaps http://thread.gmane.org/gmane.comp.version-control.git/263416/focus=263438 Peff, do you remember what (if anything) we decided to do about this? I unfortunately don't X-. -- 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
Re: Minor bug with help.autocorrect.
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
