{"thread":{"id":"56602","subject":"[PATCH] config.c: remove unused git_config_key_is_valid()","startedAt":"2021-09-28T12:56:09Z","lastAt":"2021-09-29T18:25:20Z","messageCount":3,"participants":["Ævar Arnfjörð Bjarmason","Jeff King","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"437285","messageId":"patch-1.1-ba40601a511-20210928T125516Z-avarab@gmail.com","threadId":"56602","inReplyTo":null,"subject":"[PATCH] config.c: remove unused git_config_key_is_valid()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T12:56:03Z","receivedAt":"2021-09-28T12:56:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The git_config_key_is_valid() function got left behind in a\nrefactoring in a9bcf6586d1 (alias: use the early config machinery to\nexpand aliases, 2017-06-14),\n\nIt previously had two users when it was added in 9e9de18f1ad (config:\nsilence warnings for command names with invalid keys, 2015-08-24), and\nafter 6a1e1bc0a15 (pager: use callbacks instead of configset,\n2016-09-12) only one remained.\n\nBy removing it we can get rid of the \"quiet\" branches in this\nfunction, as well as cases where \"store_key\" is NULL, for which there\nare no other users.\n\nOut of the 5 callers of git_config_parse_key() only one needs to pass\na non-NULL \"size_t *baselen_\", so we could remove the third parameter\nfrom the public interface. I did not find that potential\nsimplification to be worthwhile.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nI noticed this while looking at the \"env\" includes, we can get some\neasy dead code removal with this cleanup.\n\n config.c | 34 ++++++++--------------------------\n config.h |  1 -\n 2 files changed, 8 insertions(+), 27 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 2edf835262f..2dcbe901b6b 100644\n--- a/config.c\n+++ b/config.c\n@@ -425,7 +425,7 @@ static inline int iskeychar(int c)\n  * baselen - pointer to size_t which will hold the length of the\n  *           section + subsection part, can be NULL\n  */\n-static int git_config_parse_key_1(const char *key, char **store_key, size_t *baselen_, int quiet)\n+int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)\n {\n \tsize_t i, baselen;\n \tint dot;\n@@ -437,14 +437,12 @@ static int git_config_parse_key_1(const char *key, char **store_key, size_t *bas\n \t */\n \n \tif (last_dot == NULL || last_dot == key) {\n-\t\tif (!quiet)\n-\t\t\terror(_(\"key does not contain a section: %s\"), key);\n+\t\terror(_(\"key does not contain a section: %s\"), key);\n \t\treturn -CONFIG_NO_SECTION_OR_NAME;\n \t}\n \n \tif (!last_dot[1]) {\n-\t\tif (!quiet)\n-\t\t\terror(_(\"key does not contain variable name: %s\"), key);\n+\t\terror(_(\"key does not contain variable name: %s\"), key);\n \t\treturn -CONFIG_NO_SECTION_OR_NAME;\n \t}\n \n@@ -455,8 +453,7 @@ static int git_config_parse_key_1(const char *key, char **store_key, size_t *bas\n \t/*\n \t * Validate the key and while at it, lower case it for matching.\n \t */\n-\tif (store_key)\n-\t\t*store_key = xmallocz(strlen(key));\n+\t*store_key = xmallocz(strlen(key));\n \n \tdot = 0;\n \tfor (i = 0; key[i]; i++) {\n@@ -467,39 +464,24 @@ static int git_config_parse_key_1(const char *key, char **store_key, size_t *bas\n \t\tif (!dot || i > baselen) {\n \t\t\tif (!iskeychar(c) ||\n \t\t\t    (i == baselen + 1 && !isalpha(c))) {\n-\t\t\t\tif (!quiet)\n-\t\t\t\t\terror(_(\"invalid key: %s\"), key);\n+\t\t\t\terror(_(\"invalid key: %s\"), key);\n \t\t\t\tgoto out_free_ret_1;\n \t\t\t}\n \t\t\tc = tolower(c);\n \t\t} else if (c == '\\n') {\n-\t\t\tif (!quiet)\n-\t\t\t\terror(_(\"invalid key (newline): %s\"), key);\n+\t\t\terror(_(\"invalid key (newline): %s\"), key);\n \t\t\tgoto out_free_ret_1;\n \t\t}\n-\t\tif (store_key)\n-\t\t\t(*store_key)[i] = c;\n+\t\t(*store_key)[i] = c;\n \t}\n \n \treturn 0;\n \n out_free_ret_1:\n-\tif (store_key) {\n-\t\tFREE_AND_NULL(*store_key);\n-\t}\n+\tFREE_AND_NULL(*store_key);\n \treturn -CONFIG_INVALID_KEY;\n }\n \n-int git_config_parse_key(const char *key, char **store_key, size_t *baselen)\n-{\n-\treturn git_config_parse_key_1(key, store_key, baselen, 0);\n-}\n-\n-int git_config_key_is_valid(const char *key)\n-{\n-\treturn !git_config_parse_key_1(key, NULL, NULL, 1);\n-}\n-\n static int config_parse_pair(const char *key, const char *value,\n \t\t\t  config_fn_t fn, void *data)\n {\ndiff --git a/config.h b/config.h\nindex 147f5e0490e..8b79441ee0c 100644\n--- a/config.h\n+++ b/config.h\n@@ -259,7 +259,6 @@ int git_config_set_gently(const char *, const char *);\n void git_config_set(const char *, const char *);\n \n int git_config_parse_key(const char *, char **, size_t *);\n-int git_config_key_is_valid(const char *key);\n \n /*\n  * The following macros specify flag bits that alter the behavior\n-- \n2.33.0.1340.ge9f77250f2b\n\n"},{"id":"437341","messageId":"YVNY9OMJmXqs8EIl@coredump.intra.peff.net","threadId":"56602","inReplyTo":"patch-1.1-ba40601a511-20210928T125516Z-avarab@gmail.com","subject":"Re: [PATCH] config.c: remove unused git_config_key_is_valid()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-28T18:03:32Z","receivedAt":"2021-09-28T18:03:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 28, 2021 at 02:56:03PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> The git_config_key_is_valid() function got left behind in a\n> refactoring in a9bcf6586d1 (alias: use the early config machinery to\n> expand aliases, 2017-06-14),\n> \n> It previously had two users when it was added in 9e9de18f1ad (config:\n> silence warnings for command names with invalid keys, 2015-08-24), and\n> after 6a1e1bc0a15 (pager: use callbacks instead of configset,\n> 2016-09-12) only one remained.\n> \n> By removing it we can get rid of the \"quiet\" branches in this\n> function, as well as cases where \"store_key\" is NULL, for which there\n> are no other users.\n\nYeah, the patch here is trivially correct. The greater question is\nwhether we're likely to need this quiet parameter again. I kind of doubt\nit given the history, so this seems like a sensible cleanup.\n\n> Out of the 5 callers of git_config_parse_key() only one needs to pass\n> a non-NULL \"size_t *baselen_\", so we could remove the third parameter\n> from the public interface. I did not find that potential\n> simplification to be worthwhile.\n\nYeah, I think stopping here is good for now.\n\n-Peff\n"},{"id":"437472","messageId":"YVSvix9mY/jOz5K9@nand.local","threadId":"56602","inReplyTo":"patch-1.1-ba40601a511-20210928T125516Z-avarab@gmail.com","subject":"Re: [PATCH] config.c: remove unused git_config_key_is_valid()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-29T18:25:15Z","receivedAt":"2021-09-29T18:25:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 28, 2021 at 02:56:03PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> The git_config_key_is_valid() function got left behind in a\n> refactoring in a9bcf6586d1 (alias: use the early config machinery to\n> expand aliases, 2017-06-14),\n>\n> It previously had two users when it was added in 9e9de18f1ad (config:\n> silence warnings for command names with invalid keys, 2015-08-24), and\n> after 6a1e1bc0a15 (pager: use callbacks instead of configset,\n> 2016-09-12) only one remained.\n>\n> By removing it we can get rid of the \"quiet\" branches in this\n> function, as well as cases where \"store_key\" is NULL, for which there\n> are no other users.\n\nThis was the part that I paid most attention to. The only caller which\npassed \"quiet == 1\" was git_config_key_is_valid(), which is gone. So all\nof those \"if (!quiet)\" conditionals can go away. And the remaining\ncallers all pass a non-null pointer to store_key, so those guard can go\naway too.\n\n> Out of the 5 callers of git_config_parse_key() only one needs to pass\n> a non-NULL \"size_t *baselen_\", so we could remove the third parameter\n> from the public interface. I did not find that potential\n> simplification to be worthwhile.\n\nBut the guard for baselen being NULL needs to stay, and it does. I agree\nwith you (and Peff) that stopping here is fine.\n\nThanks,\nTaylor\n"}]}