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

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

[Andreas Krey]

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

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

[Jeff King]

On Fri, Feb 06, 2015 at 01:45:28PM +0100, Andreas Krey wrote:

> 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?

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Feb 06, 2015 at 01:45:28PM +0100, Andreas Krey wrote:
>
>>   $ 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?
> ...
> So it is not a new problem, but it is a bug that you
> cannot set pager config for such a command or alias.

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

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

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

[Jeff King]

On Fri, Feb 06, 2015 at 11:44:38AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Feb 06, 2015 at 01:45:28PM +0100, Andreas Krey wrote:
> >
> >>   $ 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?
> > ...
> > So it is not a new problem, but it is a bug that you
> > cannot set pager config for such a command or alias.
> 
> Hmm, I think these are two separate issues.

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

[PATCH] config: add show_err flag to git_config_parse_key()

[Tanay Abhra]

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

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---

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(-)

diff --git a/builtin/config.c b/builtin/config.c
index 15a7bea..d5070d7 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, 1)) {
 			ret = CONFIG_INVALID_KEY;
 			goto free_strings;
 		}
diff --git a/cache.h b/cache.h
index f704af5..1c0914d 100644
--- a/cache.h
+++ b/cache.h
@@ -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 *);
diff --git a/config.c b/config.c
index 752e2e2..074a671 100644
--- a/config.c
+++ b/config.c
@@ -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

Re: [PATCH] config: add show_err flag to git_config_parse_key()

[Jeff King]

On Wed, Feb 11, 2015 at 01:15:05AM +0530, Tanay Abhra wrote:

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

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.

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

Here's a test that can be squashed in:

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index da958a8..a28a2fd 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -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

Re: [PATCH] config: add show_err flag to git_config_parse_key()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Feb 11, 2015 at 01:15:05AM +0530, Tanay Abhra wrote:
>
>> 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.
>
> 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).

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.

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

Sensible.

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

Concurred.

[PATCH v2] add a flag to supress errors in git_config_parse_key()

[Tanay Abhra]

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

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
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(-)

diff --git a/builtin/config.c b/builtin/config.c
index d32c532..326d3d3 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 f704af5..9073ee2 100644
--- a/cache.h
+++ b/cache.h
@@ -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 *);
diff --git a/config.c b/config.c
index e5e64dc..7e23bb9 100644
--- a/config.c
+++ b/config.c
@@ -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;

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index da958a8..2dd71c0 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
-- 
1.9.0.GIT

Re: [PATCH v2] add a flag to supress errors in git_config_parse_key()

[Jeff King]

On Mon, Feb 16, 2015 at 01:28:07PM +0530, Tanay Abhra wrote:

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

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.

>  	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);

The intent of the flag variable is that you would check:

  if (!(flags & CONFIG_ERROR_QUIET))

here. I know that there are no other flags yet, so the two are
equivalent. But when somebody adds a new flag later, you would not want
them to have to tweak each of these sites.

-Peff

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

[Mikael Magnusson]

On Fri, Feb 6, 2015 at 8:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Fri, Feb 06, 2015 at 01:45:28PM +0100, Andreas Krey wrote:
>>
>>>   $ 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?

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

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

[Jeff King]

On Sat, Feb 07, 2015 at 01:03:15AM +0100, Mikael Magnusson wrote:

> On Fri, Feb 6, 2015 at 8:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Jeff King <peff@peff.net> writes:
> >
> >> On Fri, Feb 06, 2015 at 01:45:28PM +0100, Andreas Krey wrote:
> >>
> >>>   $ 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?
> 
> 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

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.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This is highlighting the problem with "pager.*" that Junio mentioned
> recently, which is that the keyname has arbitrary data,...

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.

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

[Jeff King]

On Fri, Feb 06, 2015 at 12:14:35PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This is highlighting the problem with "pager.*" that Junio mentioned
> > recently, which is that the keyname has arbitrary data,...
> 
> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

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

[Jeff King]

On Fri, Feb 06, 2015 at 02:27:31PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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).
> 
> 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.

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

Re: [PATCH] config: add show_err flag to git_config_parse_key()

[Tanay Abhra]

>From c87ddf6397964154932d49385ed1433b62631f30 Mon Sep 17 00:00:00 2001
From: Tanay Abhra <tanayabh@gmail.com>
Date: Thu, 30 Oct 2014 08:54:58 -0700
Subject: [PATCH] config: add show_err flag to 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 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.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---

*Resend*
I am behind a firewall and gmail web interface butchered my previous mail.
This is a resend, if the patch again corrupts I will send it tomorrow through
a proper internet connection.

Hi,

You were right, one of the functions was calling git_config_parse_key()
which was leaking errors to the console. git_config_parse_key() was
meant for sanitizing user provided keys only but it was being used
internally in a place where only a return value would be enough.

Thanks for bringing this to our attention.

Cheers,
Tanay Abhra.

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

diff --git a/builtin/config.c b/builtin/config.c
index 8cc2604..51635dc 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, 1)) {
 			ret = CONFIG_INVALID_KEY;
 			goto free_strings;
 		}
diff --git a/cache.h b/cache.h
index 99ed096..8129590 100644
--- a/cache.h
+++ b/cache.h
@@ -1362,7 +1362,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 *);
diff --git a/config.c b/config.c
index 15a2983..eb9058c 100644
--- a/config.c
+++ b/config.c
@@ -1299,7 +1299,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;
@@ -1832,8 +1832,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, '.');
@@ -1844,12 +1845,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;
 	}

@@ -1871,12 +1874,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;
@@ -1926,7 +1931,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

[PATCH] config: add show_err flag to git_config_parse_key()

[Tanay Abhra]

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

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---

Hi,

You were right, one of the functions was calling git_config_parse_key()
which was leaking errors to the console. git_config_parse_key() was
meant for sanitizing user provided keys only but it was being used
internally in a place where only a return value would be enough.

Thanks for bringing this to our attention.

Cheers,
Tanay Abhra.

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

diff --git a/builtin/config.c b/builtin/config.c
index 8cc2604..51635dc 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, 1)) {
             ret = CONFIG_INVALID_KEY;
             goto free_strings;
         }
diff --git a/cache.h b/cache.h
index 99ed096..8129590 100644
--- a/cache.h
+++ b/cache.h
@@ -1362,7 +1362,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 *);
diff --git a/config.c b/config.c
index 15a2983..eb9058c 100644
--- a/config.c
+++ b/config.c
@@ -1299,7 +1299,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;
@@ -1832,8 +1832,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, '.');
@@ -1844,12 +1845,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;
     }

@@ -1871,12 +1874,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;
@@ -1926,7 +1931,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

Re: [PATCH] config: add show_err flag to git_config_parse_key()

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> `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 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.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>
> Hi,
>
> You were right, one of the functions was calling git_config_parse_key()
> which was leaking errors to the console. git_config_parse_key() was
> meant for sanitizing user provided keys only but it was being used
> internally in a place where only a return value would be enough.
>
> Thanks for bringing this to our attention.
>
> Cheers,
> Tanay Abhra.

Who are *you* in the above, and what was the bug report about (if it
was a bug report)?  Perhaps summarize it in a form of a few new tests
in t/t13XX series is in order?

Thanks.

>
>  builtin/config.c |  2 +-
>  cache.h          |  2 +-
>  config.c         | 19 ++++++++++++-------
>  3 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index 8cc2604..51635dc 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, 1)) {
>              ret = CONFIG_INVALID_KEY;
>              goto free_strings;
>          }
> diff --git a/cache.h b/cache.h
> index 99ed096..8129590 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1362,7 +1362,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 *);
> diff --git a/config.c b/config.c
> index 15a2983..eb9058c 100644
> --- a/config.c
> +++ b/config.c
> @@ -1299,7 +1299,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;
> @@ -1832,8 +1832,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, '.');
> @@ -1844,12 +1845,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;
>      }
>
> @@ -1871,12 +1874,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;
> @@ -1926,7 +1931,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;

Re: [PATCH] config: add show_err flag to git_config_parse_key()

[Tanay Abhra]

>> ---
>>
>> Hi,
>>
>> You were right, one of the functions was calling git_config_parse_key()
>> which was leaking errors to the console. git_config_parse_key() was
>> meant for sanitizing user provided keys only but it was being used
>> internally in a place where only a return value would be enough.
>>
>> Thanks for bringing this to our attention.
>>
>> Cheers,
>> Tanay Abhra.
>
> Who are *you* in the above, and what was the bug report about (if it
> was a bug report)?  Perhaps summarize it in a form of a few new tests
> in t/t13XX series is in order?
>
> Thanks.
>

Sorry about that, I am behind a firewall and had to use the gmail web interface.
The patches are butchered, I will send new ones with a proper connection
tomorrow.

The original bug report is at [1].

http://thread.gmane.org/gmane.comp.version-control.git/258886