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

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

[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

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

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