BUG: 'error: invalid key: pager.show_ref' on 'git show_ref'

Hi all,

there seems to be a regression in the behaviour of 'git show_ref' (note the underscore). In v2.0.3-711-g586f414 it starts to say:

  $ ./git show_ref
  error: invalid key: pager.show_ref
  git: 'show_ref' is not a git command. See 'git --help'.

and somewhere (probably two commits, judging the diffs) later that changes again to:

  $ git show_ref
  error: invalid key: pager.show_ref
  error: invalid key: alias.show_ref
  git: 'show_ref' is not a git command. See 'git --help'.

Apparently we need to squelch this message from within git_config_get_* in this case?

Still present in 2.3.0.

Andreas

"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

This is highlighting the problem with "pager.*" that Junio mentioned recently, which is that the keyname has arbitrary data, but syntactically is limited to alnum and "-". This should have been:

pager.show_ref.enabled

from the beginning. But of course it was not. Even if we transition, we would want to support pager.* for a while.

I don't think squelching the messages is quite the right approach. They come from git_config_parse_key, which barfs on parsing the syntactically invalid keyname. So not only are we complaining, but we are not actually looking up the value. I don't think that's technically a regression in 586f414, though. The reader started to complain, but AFAICT git would not agree to parse a file containing:

[pager]
show_ref = true

in the first place. So it is not a new problem, but it is a bug that you cannot set pager config for such a command or alias.

I can think of a few possible paths forward:

1. Squelch the messages, and declare "show_ref" and friends out-of-luck for pager config or aliases.

2. Relax the syntactic rules for config keys to allow more characters. We cannot make this perfect (e.g., we cannot allow "." for reasons of ambiguity), but I imagine we could cover most practical cases.

Note that we would need the matching loosening on the file-parsing side.

3. Start phasing in pager.*.enabled (and I guess pager.*.command). We would still do the lookup of pager.* for backwards compatibility, but we would be careful to do so only when it is syntactically valid. IOW, this looks like (1), except the path forward for "show_ref" is to use the new, more robust, syntax.

-Peff

Yes, even if it is not "arbitrary" (imagine we limit ourselves to the official set of commands we know about), the naming rule for the "git" subcommand names should not be dictated by the naming rule for the configuration variables, as they are unrelated.

That is one of the reasons why I had the "unbounded set, including the ones under our control such as subcommand names" in the draft update for the guideline. I dropped that part after the discussion to keep other "obviously agreed" parts moving, but we may have to revisit it later.

I think this may be the heart of where we were disagreeing. I took "unbounded set" to mean "a set where you might keep adding things forever". So fsck errors would count in that. But if you mean it as "a set where the syntax may be unbounded", then yeah, we definitely would not want it in the key name, as that becomes an unnecessary restriction.

A list of enum-like values where we are OK confining the names to the alnums is OK to use as an unbounded set of key values. Just like we have color.branch.*, we just pick a name within that syntax for any new values we add (and that is not even a burden; alnum names are what we would have picked anyway).

-Peff

I would say that color.branch.<slot> names are very different from subcommand names. The latter is exposed to the end users who do not have to know that they can be used and must be usable as config keys.

color.branch.<slot> names were invented _only_ to be used to interact with the config, and nowhere else. Of course you can just pick a name within that "syntax for configuration variables" and be happy with it, because the users are very aware that they are using that name to name a configuration variable.

The names of the subcommands are very different in that they are not just for accessing configuration variables---if the user does not have pager.<cmd>, the user will not use it as configuration keys anywhere in the system.

Yeah, again, sorry if I wasn't clear. That was the same contrast I was making. Of the examples given in this thread, color.branch.<slot> and fsck.* names are in one boat ("OK to give them configuration-friendly names, they are just a list") and arbitrary commands are in another.

-Peff

What I mean is "possible keys are unbounded and its syntax is not under control of the 'config' subsystem". The syntax does not have to be unbounded; as long as it is wrong for the config subsystem to dictate what shape the possible values may take, it shouldn't be used as the top or the bottom level in the variable namespace where it has its own syntax restriction that may or may not match the requirement of the using code of the config subsystem.

Those who name Git subcommands will be limited to sane looking subcommand names that do not have SP in it, for example, but just because config subsystem does not want to see "_" in its keys, it should not force its world view to those who name subcommands.

If the names are not "unbounded", it becomes easier to live with such a third-party limitation (imposed by config subsystem), but otherwise, "we just pick a name within that syntax" becomes an unnecessary and artificial limitation.

Hmm, I think these are two separate issues.

(1) you cannot define "alias.my_merge" because that is not a valid key. We cannot add a new official subcommand "git c_m_d" because users cannot define "pager.c_m_d" for it for the same reason.

(2) "git no-such-command" does not get these extraneous error messages, but "git no_such_command" does.

Solution to (1) would be to move to "alias.my_merge.command = ..." and "pager.c_m_d.enabled = true". But I do not think that would solve (1) until we transition and start ignoring alias.my_merge and pager.c_m_d, and I do not think of a way other than squelching the messages to solve (1) during the transition period.

I guess I ended up reaching the same conclusion; 3. with also "alias.*.command" as the longer-term goal.

Yeah, sorry, if I wasn't clear. The error messages are definitely a separate and newer issue, and need to be silenced one way or the other. It is just that they are notifying us of a deeper problem that has existed for a long time, and it probably makes sense to deal with both.

-Peff

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 per-sanitized key directly from the user so it becomes necessary to raise an error specifying what went wrong when the entered key is defective.

Other callers like configset_find_element() get their keys from the 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 show_err flag to suppress errors in such cases.

---

Hi,

I just saw your mail late in the night (I didn't had net for a week). This patch just squelches the error message, I will take a better look tomorrow morning.

-Tanay

 builtin/config.c |  2 +-
 cache.h          |  2 +-
 config.c         | 19 ++++++++++++-------
 3 files changed, 14 insertions(+), 9 deletions(-)

index 15a7bea..d5070d7 100644
@@ -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, 1)) {
 			ret = CONFIG_INVALID_KEY;
 			goto free_strings;
 		}

index f704af5..1c0914d 100644
@@ -1358,7 +1358,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 *, 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 *);

index 752e2e2..074a671 100644
@@ -1309,7 +1309,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, 0);
 
 	if (ret)
 		return NULL;

@@ -1842,8 +1842,9 @@ 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
+ * show_err - toggle whether the function raises an error on a defective 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_, int show_err)
 {
 	int i, dot, baselen;
 	const char *last_dot = strrchr(key, '.');

@@ -1854,12 +1855,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 (show_err)
+			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 (show_err)
+			error("key does not contain variable name: %s", key);
 		return -CONFIG_NO_SECTION_OR_NAME;
 	}
 

@@ -1881,12 +1884,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 (show_err)
+					error("invalid key: %s", key);
 				goto out_free_ret_1;
 			}
 			c = tolower(c);
 		} else if (c == '\n') {
-			error("invalid key (newline): %s", key);
+			if (show_err)
+				error("invalid key (newline): %s", key);
 			goto out_free_ret_1;
 		}
 		(*store_key)[i] = c;

@@ -1936,7 +1941,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	char *filename_buf = NULL;
 
 	/* 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, 1);
 	if (ret)
 		goto out_free;
 

1.9.0.GIT

Thanks, this is probably a good first step. We can worry about making the config look actually _work_ as the next step (which does not even have to happen right now; it is not like it hasn't been this way since the very beginning of git).

Another option for this first step would be to actually make git_config_parse_key permissive, rather than just squelching the error. That is, to actually look up pager.under_score rather than silently erroring out with an invalid key whenever we are reading (whereas on the writing side, we _do_ want to make sure we enforce syntactic validity). I doubt it matters, much, though. Such a lookup would never succeed, because the config file parser will also not allow it. So assuming the syntactic rules here match what the config file parser does, they are at worst redundant.

Here's a test that can be squashed in:

index da958a8..a28a2fd 100755
@@ -447,4 +447,14 @@ 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

I was tempted to also add something like:

test_expect_failure TTY 'command with underscores can override pager' '

	test_config pager.under_score "sed s/^/paged://" &&
	git --exec-path=. under_score >actual &&
	echo paged:ok >expect &&
	test_cmp expect actual

'

but I am not sure it is worth adding the test, even as a placeholder. Unless we are planning to relax the config syntax, the correct spelling is more like "pager.under_score.command". It's probably better to just add the test along with the code when we know what the final form will look like.

-Peff

I agree this is probably a good first step in the right direction. As to the implementation, there are a few minor things I would change, but they are both minor:

- "defective" may want to be a bit more descriptive to clarify what kind fo defect is undesired. In the context of this patch, I think Tanay meant (syntactically) "malformed", perhaps?

- "int show_err" should be "unsigned flags" with its bit 01 defined to be used as QUIET bit.

Sensible.

Concurred.

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 the 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.

---
Hi Jeff,

I went through Junio's config guideline patch series and the whole thread of underscore bug report and I also think that pager.*.command is the right path to go.

If you want to relax the syntactic requirement (such as add '_' to the current set of allowed chacters), I can work upon it but most of the comments point that moving towards pager.*.command would be better.

p.s: I hope that I got the unsigned flag suggestion by Junio correctly.

-Tanay

 builtin/config.c |  2 +-
 cache.h          |  4 +++-
 config.c         | 20 +++++++++++++-------
 t/t7006-pager.sh |  9 +++++++++
 4 files changed, 26 insertions(+), 9 deletions(-)

index d32c532..326d3d3 100644
@@ -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;
 		}

index f704af5..9073ee2 100644
@@ -1329,6 +1329,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;

@@ -1358,7 +1360,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 *);

index e5e64dc..7e23bb9 100644
@@ -1309,7 +1309,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;

@@ -1842,8 +1842,10 @@ 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 - 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, '.');

@@ -1854,12 +1856,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 (!flags)
+			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 (!flags)
+			error("key does not contain variable name: %s", key);
 		return -CONFIG_NO_SECTION_OR_NAME;
 	}
 

@@ -1881,12 +1885,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 (!flags)
+					error("invalid key: %s", key);
 				goto out_free_ret_1;
 			}
 			c = tolower(c);
 		} else if (c == '\n') {
-			error("invalid key (newline): %s", key);
+			if (!flags)
+				error("invalid key (newline): %s", key);
 			goto out_free_ret_1;
 		}
 		(*store_key)[i] = c;

@@ -1936,7 +1942,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	char *filename_buf = NULL;
 
 	/* 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;
 

index da958a8..2dd71c0 100755
@@ -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

1.9.0.GIT

No, as silly as I find the "_" restriction, it is not worth doing. One, it would not cover all cases (it is one common case, so it makes the problem more rare but does not eliminate it). And two, there are other parsers of git's config format. Technically we do not need to care about them and they can follow our lead, but we do not need to make things harder on them than is necessary.

I reported this issue a few months ago,
http://permalink.gmane.org/gmane.comp.version-control.git/258886
Someone sent a patch that never went anywhere,
http://comments.gmane.org/gmane.comp.version-control.git/258895

Mikael Magnusson

Thanks. I had thought this all seemed familiar, and I did find your report in the archive, but not the follow-up patch[1].

It looks like that patch just squelches the error message. That fixes the immediate error-message regression, but does not fix the larger problem (that you cannot have an alias with an underscore, or set the pager config for a command with an underscore). But it is at least a start, and unless somebody is excited about taking it further, maybe it is enough for now.

The thread ended with Tanay mentioning that new patches would be forthcoming. I've cc'd him, so hopefully that can still happen.

-Peff

[1] This is a good lesson in why it is nice to make sure that the in-reply-to headers for patches are set properly; it makes it easier later on to find related parts of the discussion. This is something I think that git-send-email doesn't make especially easy.