Minor bug with help.autocorrect.

Minor bug with help.autocorrect.

[Bjørnar Snoksrud]

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

-- 
bjornar@snoksrud.no

Re: Minor bug with help.autocorrect.

[Junio C Hamano]

Bjørnar Snoksrud <snoksrud@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-<.

Re: Minor bug with help.autocorrect.

[Jeff King]

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 <tanayabh@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 <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
Signed-off-by: Jeff King <peff@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 name and the real
@@ -1859,12 +1862,14 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	 */
 
 	if (last_dot == NULL || last_dot == key) {
-		error("key does not contain a section: %s", key);
+		if (!quiet)
+			error("key does not contain a section: %s", key);
 		return -CONFIG_NO_SECTION_OR_NAME;
 	}
 
 	if (!last_dot[1]) {
-		error("key does not contain variable name: %s", key);
+		if (!quiet)
+			error("key does not contain variable name: %s", key);
 		return -CONFIG_NO_SECTION_OR_NAME;
 	}
 
@@ -1886,12 +1891,14 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 		if (!dot || i > baselen) {
 			if (!iskeychar(c) ||
 			    (i == baselen + 1 && !isalpha(c))) {
-				error("invalid key: %s", key);
+				if (!quiet)
+					error("invalid key: %s", key);
 				goto out_free_ret_1;
 			}
 			c = tolower(c);
 		} else if (c == '\n') {
-			error("invalid key (newline): %s", key);
+			if (!quiet)
+				error("invalid key (newline): %s", key);
 			goto out_free_ret_1;
 		}
 		(*store_key)[i] = c;
@@ -1943,7 +1950,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	size_t contents_sz;
 
 	/* parse-key returns negative; flip the sign to feed exit(3) */
-	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
+	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen, 0);
 	if (ret)
 		goto out_free;
 
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 947b690..6ea7ac4 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -447,4 +447,13 @@ test_expect_success TTY 'external command pagers override sub-commands' '
 	test_cmp expect actual
 '
 
+test_expect_success 'command with underscores does not complain' '
+	write_script git-under_score <<-\EOF &&
+	echo ok
+	EOF
+	git --exec-path=. under_score >actual 2>&1 &&
+	echo ok >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0.685.g0ca4974

Re: Minor bug with help.autocorrect.

[Junio C Hamano]

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.

Re: Minor bug with help.autocorrect.

[Jeff King]

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

Re: Minor bug with help.autocorrect.

[Jeff King]

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 <peff@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 || last_dot == key) {
-		error("key does not contain a section: %s", key);
+		if (!quiet)
+			error("key does not contain a section: %s", key);
 		return -CONFIG_NO_SECTION_OR_NAME;
 	}
 
 	if (!last_dot[1]) {
-		error("key does not contain variable name: %s", key);
+		if (!quiet)
+			error("key does not contain variable name: %s", key);
 		return -CONFIG_NO_SECTION_OR_NAME;
 	}
 
@@ -1875,7 +1877,8 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	/*
 	 * Validate the key and while at it, lower case it for matching.
 	 */
-	*store_key = xmalloc(strlen(key) + 1);
+	if (store_key)
+		*store_key = xmalloc(strlen(key) + 1);
 
 	dot = 0;
 	for (i = 0; key[i]; i++) {
@@ -1886,26 +1889,42 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 		if (!dot || i > baselen) {
 			if (!iskeychar(c) ||
 			    (i == baselen + 1 && !isalpha(c))) {
-				error("invalid key: %s", key);
+				if (!quiet)
+					error("invalid key: %s", key);
 				goto out_free_ret_1;
 			}
 			c = tolower(c);
 		} else if (c == '\n') {
-			error("invalid key (newline): %s", key);
+			if (!quiet)
+				error("invalid key (newline): %s", key);
 			goto out_free_ret_1;
 		}
-		(*store_key)[i] = c;
+		if (store_key)
+			(*store_key)[i] = c;
 	}
-	(*store_key)[i] = 0;
+	if (store_key)
+		(*store_key)[i] = 0;
 
 	return 0;
 
 out_free_ret_1:
-	free(*store_key);
-	*store_key = NULL;
+	if (store_key) {
+		free(*store_key);
+		*store_key = NULL;
+	}
 	return -CONFIG_INVALID_KEY;
 }
 
+int git_config_parse_key(const char *key, char **store_key, int *baselen)
+{
+	return git_config_parse_key_1(key, store_key, baselen, 0);
+}
+
+int git_config_key_is_valid(const char *key)
+{
+	return !git_config_parse_key_1(key, NULL, NULL, 1);
+}
+
 /*
  * If value==NULL, unset in (remove from) config,
  * if value_regex!=NULL, disregard key/value pairs where value does not match.
diff --git a/pager.c b/pager.c
index 070dc11..27d4c8a 100644
--- a/pager.c
+++ b/pager.c
@@ -150,7 +150,8 @@ int check_pager_config(const char *cmd)
 	struct strbuf key = STRBUF_INIT;
 	const char *value = NULL;
 	strbuf_addf(&key, "pager.%s", cmd);
-	if (!git_config_get_value(key.buf, &value)) {
+	if (git_config_key_is_valid(key.buf) &&
+	    !git_config_get_value(key.buf, &value)) {
 		int b = git_config_maybe_bool(key.buf, value);
 		if (b >= 0)
 			want = b;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 947b690..6ea7ac4 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -447,4 +447,13 @@ test_expect_success TTY 'external command pagers override sub-commands' '
 	test_cmp expect actual
 '
 
+test_expect_success 'command with underscores does not complain' '
+	write_script git-under_score <<-\EOF &&
+	echo ok
+	EOF
+	git --exec-path=. under_score >actual 2>&1 &&
+	echo ok >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0.685.g0ca4974

Re: Minor bug with help.autocorrect.

[Junio C Hamano]

Jeff King <peff@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 ;-).