{"thread":{"id":"55454","subject":"[PATCH 1/5] config.c: add a comment about why value=NULL is true","startedAt":"2021-04-08T13:34:42Z","lastAt":"2021-04-10T01:43:57Z","messageCount":21,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"421212","messageId":"patch-1.6-bd70181ffb-20210408T133125Z-avarab@gmail.com","threadId":"55454","inReplyTo":"cover-0.6-0000000000-20210408T133125Z-avarab@gmail.com","subject":"[PATCH 1/5] config.c: add a comment about why value=NULL is true","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-08T13:34:25Z","receivedAt":"2021-04-08T13:34:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"It's not very intuitive that git_parse_maybe_bool_text() would\nconsider NULL to be a true value. Add a small comment about it.\n\nSee a789ca70e7 (config: teach \"git -c\" to recognize an empty string,\n2014-08-04) for the behavior of \"git -c\", but we'll end up here in\nboth the config file parsing and command-line parsing cases.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n config.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 6428393a41..fc28dbd97c 100644\n--- a/config.c\n+++ b/config.c\n@@ -1229,6 +1229,10 @@ ssize_t git_config_ssize_t(const char *name, const char *value)\n static int git_parse_maybe_bool_text(const char *value)\n {\n \tif (!value)\n+\t\t/*\n+\t\t * \"[foo]\\nbar\\n\" and \"-c foo.bar\" on the command-line\n+\t\t * are true.\n+\t\t */\n \t\treturn 1;\n \tif (!*value)\n \t\treturn 0;\n-- \n2.31.1.527.g9b8f7de2547\n\n"},{"id":"421213","messageId":"cover-0.6-0000000000-20210408T133125Z-avarab@gmail.com","threadId":"55454","inReplyTo":null,"subject":"[PATCH 0/5] config: support --type=bool-or-auto for \"tristate\" parsing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-08T13:34:24Z","receivedAt":"2021-04-08T13:34:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor some of our internal C code to use a new\ngit_config_tristate() functino instead of checking for \"auto\" or\n\"bool\" in several places.\n\nThis isn't just Yet Another Ævar Series, I'm using this to re-roll my\noutstanding userdiff series, the start of that topic was a partial\nrefactor of userdiff.c which is better done here.\n\nWhile writing this I discovered that the recently added --bool-or-type\noption added in\nhttps://lore.kernel.org/git/pull.781.v18.git.git.1599895268433.gitgitgadget@gmail.com/\ndidn't have any tests (and we didn't notice in 18! iterations of it:)\n\n2/5 adds that, 1/5 adds a comment on some bool parsing code that's\npuzzled me for the Nth time.\n\nÆvar Arnfjörð Bjarmason (5):\n config.c: add a comment about why value=NULL is true\n config tests: test for --bool-or-str\n git-config: document --bool-or-str and --type=bool-or-str\n config.c: add a \"tristate\" helper\n config: add --type=bool-or-auto switch\n\n Documentation/git-config.txt | 7 ++\n builtin/config.c | 19 ++++++\n builtin/log.c | 13 ++--\n compat/mingw.c | 6 +-\n config.c | 20 ++++++\n config.h | 12 ++++\n http.c | 5 +-\n mergetools/meld | 2 +-\n t/t1300-config.sh | 121 +++++++++++++++++++++++++++++++++++\n userdiff.c | 6 +-\n 10 files changed, 195 insertions(+), 16 deletions(-)\n\n-- \n2.31.1.527.g9b8f7de2547\n\n"},{"id":"421214","messageId":"patch-2.6-2f6c2de050-20210408T133125Z-avarab@gmail.com","threadId":"55454","inReplyTo":"cover-0.6-0000000000-20210408T133125Z-avarab@gmail.com","subject":"[PATCH 2/5] config tests: test for --bool-or-str","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-08T13:34:26Z","receivedAt":"2021-04-08T13:34:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add the missing tests for the --bool-or-str code added in\ndbd8c09bfe (mergetool: allow auto-merge for meld to follow the\nvim-diff behavior, 2020-05-07).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1300-config.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 72 insertions(+)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex e0dd5d65ce..a002ec5644 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -802,6 +802,78 @@ test_expect_success 'get --bool-or-int' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'get --bool-or-str' '\n+\tcat >.git/config <<-\\EOF &&\n+\t[bool]\n+\ttrue1\n+\ttrue2 = true\n+\ttrue3 = TRUE\n+\ttrue4 = yes\n+\ttrue5 = YES\n+\ttrue6 = on\n+\ttrue7 = ON\n+\tfalse1 =\n+\tfalse2 = false\n+\tfalse3 = FALSE\n+\tfalse4 = no\n+\tfalse5 = NO\n+\tfalse6 = off\n+\tfalse7 = OFF\n+\t[int]\n+\tint1 = 0\n+\tint2 = 1\n+\tint3 = -1\n+\t[string]\n+\tstring1 = hello\n+\tstring2 = there you\n+\tEOF\n+\tcat >expect <<-\\EOF &&\n+\ttrue\n+\ttrue\n+\ttrue\n+\ttrue\n+\ttrue\n+\ttrue\n+\ttrue\n+\tfalse\n+\tfalse\n+\tfalse\n+\tfalse\n+\tfalse\n+\tfalse\n+\tfalse\n+\tfalse\n+\tfalse\n+\ttrue\n+\ttrue\n+\thello\n+\tthere you\n+\tEOF\n+\t{\n+\t\tgit config --type=bool-or-str bool.true1 &&\n+\t\tgit config --bool-or-str bool.true2 &&\n+\t\tgit config --bool-or-str bool.true3 &&\n+\t\tgit config --bool-or-str bool.true4 &&\n+\t\tgit config --bool-or-str bool.true5 &&\n+\t\tgit config --bool-or-str bool.true6 &&\n+\t\tgit config --bool-or-str bool.true7 &&\n+\t\tgit config --bool-or-str bool.false1 &&\n+\t\tgit config --bool-or-str bool.false2 &&\n+\t\tgit config --bool-or-str bool.false3 &&\n+\t\tgit config --bool-or-str bool.false4 &&\n+\t\tgit config --bool-or-str bool.false5 &&\n+\t\tgit config --bool-or-str bool.false6 &&\n+\t\tgit config --bool-or-str bool.false6 &&\n+\t\tgit config --bool-or-str bool.false7 &&\n+\t\tgit config --bool-or-str int.int1 &&\n+\t\tgit config --bool-or-str int.int2 &&\n+\t\tgit config --bool-or-str int.int3 &&\n+\t\tgit config --bool-or-str string.string1 &&\n+\t\tgit config --bool-or-str string.string2\n+\t} >actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat >expect <<\\EOF\n [bool]\n \ttrue1 = true\n-- \n2.31.1.527.g9b8f7de2547\n\n"},{"id":"421215","messageId":"patch-3.6-8aa59b3118-20210408T133125Z-avarab@gmail.com","threadId":"55454","inReplyTo":"cover-0.6-0000000000-20210408T133125Z-avarab@gmail.com","subject":"[PATCH 3/5] git-config: document --bool-or-str and --type=bool-or-str","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-08T13:34:27Z","receivedAt":"2021-04-08T13:34:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Document the new \"bool-or-str\" facility added in\ndbd8c09bfe (mergetool: allow auto-merge for meld to follow the\nvim-diff behavior, 2020-05-07).\n\nUnfortunately that commit also added a --bool-or-str option, even\nthough we've preferred to deprecate that form ever since\nfb0dc3bac1 (builtin/config.c: support `--type=` as preferred\nalias for `--`, 2018-04-18).\n\nSince we've got it already let's document it along with the preferred\n--type=* form, and change our own code to use the --type=bool-or-str\nform over --bool-or-str.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-config.txt | 3 +++\n mergetools/meld | 2 +-\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 4b4cc5c5e8..4ae9ef210c 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -187,6 +187,8 @@ Valid ``'s include:\n 1073741824 upon input.\n - 'bool-or-int': canonicalize according to either 'bool' or 'int', as described\n above.\n+- 'bool-or-str: canonicalize according to either 'bool' (as described\n+ above), or emit the value as-is.\n - 'path': canonicalize by adding a leading `~` to the value of `$HOME` and\n `~user` to the home directory for the specified user. This specifier has no\n effect when setting the value (but you can use `git config section.variable\n@@ -202,6 +204,7 @@ Valid ``'s include:\n --bool::\n --int::\n --bool-or-int::\n+--bool-or-str::\n --path::\n --expiry-date::\n Historical options for selecting a type specifier. Prefer instead `--type`\ndiff --git a/mergetools/meld b/mergetools/meld\nindex aab4ebb935..8386e0574e 100644\n--- a/mergetools/meld\n+++ b/mergetools/meld\n@@ -59,7 +59,7 @@ check_meld_for_features () {\n \tif test -z \"$meld_use_auto_merge_option\"\n \tthen\n \t\tmeld_use_auto_merge_option=$(\n-\t\t\tgit config --bool-or-str mergetool.meld.useAutoMerge\n+\t\t\tgit config --type=bool-or-str mergetool.meld.useAutoMerge\n \t\t)\n \t\tcase \"$meld_use_auto_merge_option\" in\n \t\ttrue | false)\n-- \n2.31.1.527.g9b8f7de2547\n\n"},{"id":"421216","messageId":"patch-4.6-222e91e11b-20210408T133125Z-avarab@gmail.com","threadId":"55454","inReplyTo":"cover-0.6-0000000000-20210408T133125Z-avarab@gmail.com","subject":"[PATCH 4/5] config.c: add a \"tristate\" helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-08T13:34:28Z","receivedAt":"2021-04-08T13:34:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add \"tristate\" functions to go along with the \"bool\" functions and\nmigrate the common pattern of checking if something is \"bool\" or\n\"auto\" in various places over to the new functions.\n\nWe also have e.g. \"repo_config_get_bool\" and\n\"config_error_nonbool\". I'm not adding corresponding \"tristate\"\nfunctions as they're not needed by anything, but we could add those in\nthe future if they are.\n\nI'm not migrating over \"core.abbrev\" parsing as part of this\nchange. When \"core.abbrev\" was made optionally boolean in\na9ecaa06a7 (core.abbrev=no disables abbreviations, 2020-09-01) the\n\"die if empty\" code added in g48d5014dd4 (config.abbrev: document the\nnew default that auto-scales, 2016-11-01) wasn't adjusted. It thus\nbehaves unlike all other \"maybe bool\" config variables.\n\nI have a planned series to start adding some tests for \"core.abbrev\",\nbut AFAICT there's not even a test for \"core.abbrev=true\", and I'd\nlike to focus on thing that have no behavior change here, so let's\nleave it for now.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/log.c | 13 +++++++------\n compat/mingw.c | 6 +++---\n config.c | 16 ++++++++++++++++\n config.h | 12 ++++++++++++\n http.c | 5 +++--\n userdiff.c | 6 ++----\n 6 files changed, 43 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8acd285daf..0d945313d8 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -868,11 +868,12 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"format.numbered\")) {\n-\t\tif (value && !strcasecmp(value, \"auto\")) {\n+\t\tint tristate = git_config_tristate(var, value);\n+\t\tif (tristate == 2) {\n \t\t\tauto_number = 1;\n \t\t\treturn 0;\n \t\t}\n-\t\tnumbered = git_config_bool(var, value);\n+\t\tnumbered = tristate;\n \t\tauto_number = auto_number && numbered;\n \t\treturn 0;\n \t}\n@@ -904,11 +905,11 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"format.signaturefile\"))\n \t\treturn git_config_pathname(&signature_file, var, value);\n \tif (!strcmp(var, \"format.coverletter\")) {\n-\t\tif (value && !strcasecmp(value, \"auto\")) {\n+\t\tint tristate = git_config_tristate(var, value);\n+\t\tif (tristate == 2)\n \t\t\tconfig_cover_letter = COVER_AUTO;\n-\t\t\treturn 0;\n-\t\t}\n-\t\tconfig_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;\n+\t\telse\n+\t\t\tconfig_cover_letter = tristate ? COVER_ON : COVER_OFF;\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"format.outputdirectory\"))\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex a43599841c..e6e85ae99a 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -247,11 +247,11 @@ int mingw_core_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (!strcmp(var, \"core.restrictinheritedhandles\")) {\n-\t\tif (value && !strcasecmp(value, \"auto\"))\n+\t\tint tristate = git_config_tristate(var, value);\n+\t\tif (tristate == 2)\n \t\t\tcore_restrict_inherited_handles = -1;\n \t\telse\n-\t\t\tcore_restrict_inherited_handles =\n-\t\t\t\tgit_config_bool(var, value);\n+\t\t\tcore_restrict_inherited_handles = tristate;\n \t\treturn 0;\n \t}\n \ndiff --git a/config.c b/config.c\nindex fc28dbd97c..74d2b2c0df 100644\n--- a/config.c\n+++ b/config.c\n@@ -1257,6 +1257,14 @@ int git_parse_maybe_bool(const char *value)\n \treturn -1;\n }\n \n+int git_parse_maybe_tristate(const char *value)\n+{\n+\tint v = git_parse_maybe_bool(value);\n+\tif (v < 0 && !strcasecmp(value, \"auto\"))\n+\t\treturn 2;\n+\treturn v;\n+}\n+\n int git_config_bool_or_int(const char *name, const char *value, int *is_bool)\n {\n \tint v = git_parse_maybe_bool_text(value);\n@@ -1268,6 +1276,14 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool)\n \treturn git_config_int(name, value);\n }\n \n+int git_config_tristate(const char *name, const char *value)\n+{\n+\tint v = git_parse_maybe_tristate(value);\n+\tif (v < 0)\n+\t\tdie(_(\"bad tristate config value '%s' for '%s'\"), value, name);\n+\treturn v;\n+}\n+\n int git_config_bool(const char *name, const char *value)\n {\n \tint v = git_parse_maybe_bool(value);\ndiff --git a/config.h b/config.h\nindex 19a9adbaa9..c5129e4392 100644\n--- a/config.h\n+++ b/config.h\n@@ -197,6 +197,12 @@ int git_parse_ulong(const char *, unsigned long *);\n */\n int git_parse_maybe_bool(const char *);\n \n+/**\n+ * Same as `git_parse_maybe_bool`, except that \"auto\" is recognized and\n+ * will return \"2\".\n+ */\n+int git_parse_maybe_tristate(const char *);\n+\n /**\n * Parse the string to an integer, including unit factors. Dies on error;\n * otherwise, returns the parsed result.\n@@ -226,6 +232,12 @@ int git_config_bool_or_int(const char *, const char *, int *);\n */\n int git_config_bool(const char *, const char *);\n \n+/**\n+ * Like git_config_bool() except \"auto\" is also recognized and will\n+ * return \"2\"\n+ */\n+int git_config_tristate(const char *, const char *);\n+\n /**\n * Allocates and copies the value string into the `dest` parameter; if no\n * string is given, prints an error message and returns -1.\ndiff --git a/http.c b/http.c\nindex 406410f884..b54a232e90 100644\n--- a/http.c\n+++ b/http.c\n@@ -406,10 +406,11 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\treturn git_config_string(&user_agent, var, value);\n \n \tif (!strcmp(\"http.emptyauth\", var)) {\n-\t\tif (value && !strcmp(\"auto\", value))\n+\t\tint tristate = git_config_tristate(var, value);\n+\t\tif (tristate == 2)\n \t\t\tcurl_empty_auth = -1;\n \t\telse\n-\t\t\tcurl_empty_auth = git_config_bool(var, value);\n+\t\t\tcurl_empty_auth = tristate;\n \t\treturn 0;\n \t}\n \ndiff --git a/userdiff.c b/userdiff.c\nindex 3f81a2261c..7ff010961f 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -277,10 +277,8 @@ static int parse_funcname(struct userdiff_funcname *f, const char *k,\n \n static int parse_tristate(int *b, const char *k, const char *v)\n {\n-\tif (v && !strcasecmp(v, \"auto\"))\n-\t\t*b = -1;\n-\telse\n-\t\t*b = git_config_bool(k, v);\n+\tint tristate = git_config_tristate(k, v);\n+\t*b = tristate == 2 ? -1 : tristate;\n \treturn 0;\n }\n \n-- \n2.31.1.527.g9b8f7de2547\n\n"},{"id":"421217","messageId":"patch-5.6-9f8996a888-20210408T133125Z-avarab@gmail.com","threadId":"55454","inReplyTo":"cover-0.6-0000000000-20210408T133125Z-avarab@gmail.com","subject":"[PATCH 5/5] config: add --type=bool-or-auto switch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-08T13:34:29Z","receivedAt":"2021-04-08T13:34:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Now that we're using git_config_tristate() internally let's expose it\nvia \"git config\" like we do \"bool\", \"int\" etc for completeness, and so\nthat we can easily test it.\n\nUnlike the --type=bool-or-str option added in dbd8c09bfe (mergetool:\nallow auto-merge for meld to follow the vim-diff behavior, 2020-05-07)\nwe don't have or anticipate any in-tree user of this except the tests.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-config.txt | 4 +++\n builtin/config.c | 19 ++++++++++++++\n t/t1300-config.sh | 49 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 72 insertions(+)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 4ae9ef210c..1af8222e82 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -189,6 +189,10 @@ Valid ``'s include:\n above.\n - 'bool-or-str: canonicalize according to either 'bool' (as described\n above), or emit the value as-is.\n+- 'bool-or-auto: canonicalize according to either 'bool', as described\n+ above, or whether the value is \"auto\". This is used by various\n+ \"tristate\" variables such as `core.restrictInheritedHandles`,\n+ `format.numbered` etc.\n - 'path': canonicalize by adding a leading `~` to the value of `$HOME` and\n `~user` to the home directory for the specified user. This specifier has no\n effect when setting the value (but you can use `git config section.variable\ndiff --git a/builtin/config.c b/builtin/config.c\nindex f71fa39b38..039a4f0961 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -68,6 +68,7 @@ static int fixed_value;\n #define TYPE_EXPIRY_DATE\t5\n #define TYPE_COLOR\t\t6\n #define TYPE_BOOL_OR_STR\t7\n+#define TYPE_BOOL_OR_AUTO\t8\n \n #define OPT_CALLBACK_VALUE(s, l, v, h, i) \\\n \t{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \\\n@@ -99,6 +100,8 @@ static int option_parse_type(const struct option *opt, const char *arg,\n \t\t\tnew_type = TYPE_BOOL_OR_INT;\n \t\telse if (!strcmp(arg, \"bool-or-str\"))\n \t\t\tnew_type = TYPE_BOOL_OR_STR;\n+\t\telse if (!strcmp(arg, \"bool-or-auto\"))\n+\t\t\tnew_type = TYPE_BOOL_OR_AUTO;\n \t\telse if (!strcmp(arg, \"path\"))\n \t\t\tnew_type = TYPE_PATH;\n \t\telse if (!strcmp(arg, \"expiry-date\"))\n@@ -156,6 +159,7 @@ static struct option builtin_config_options[] = {\n \tOPT_CALLBACK_VALUE(0, \"int\", &type, N_(\"value is decimal number\"), TYPE_INT),\n \tOPT_CALLBACK_VALUE(0, \"bool-or-int\", &type, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n \tOPT_CALLBACK_VALUE(0, \"bool-or-str\", &type, N_(\"value is --bool or string\"), TYPE_BOOL_OR_STR),\n+\t/* No bool-or-auto! The -- form is deprecated in favor of --type= */\n \tOPT_CALLBACK_VALUE(0, \"path\", &type, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n \tOPT_CALLBACK_VALUE(0, \"expiry-date\", &type, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n \tOPT_GROUP(N_(\"Other\")),\n@@ -263,6 +267,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\t\tstrbuf_addstr(buf, value_);\n \t\t\telse\n \t\t\t\tstrbuf_addstr(buf, v ? \"true\" : \"false\");\n+\t\t} else if (type == TYPE_BOOL_OR_AUTO) {\n+\t\t\tint v = git_config_tristate(key_, value_);\n+\t\t\tif (v == 2)\n+\t\t\t\tstrbuf_addstr(buf, \"auto\");\n+\t\t\telse\n+\t\t\t\tstrbuf_addstr(buf, v ? \"true\" : \"false\");\n \t\t} else if (type == TYPE_PATH) {\n \t\t\tconst char *v;\n \t\t\tif (git_config_pathname(&v, key_, value_) < 0)\n@@ -435,6 +445,15 @@ static char *normalize_value(const char *key, const char *value)\n \t\telse\n \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n \t}\n+\tif (type == TYPE_BOOL_OR_AUTO) {\n+\t\tint v = git_parse_maybe_tristate(value);\n+\t\tif (v < 0)\n+\t\t\treturn xstrdup(value);\n+\t\telse if (v == 2)\n+\t\t\txstrdup(\"auto\");\n+\t\telse\n+\t\t\treturn xstrdup(v ? \"true\" : \"false\");\n+\t}\n \tif (type == TYPE_COLOR) {\n \t\tchar v[COLOR_MAXLEN];\n \t\tif (git_config_color(v, key, value))\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex a002ec5644..952d9e9ed9 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -874,6 +874,55 @@ test_expect_success 'get --bool-or-str' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'there is no --bool-or-auto, -- is deprecated in favor of --type=' '\n+\ttest_expect_code 129 git config --bool-or-auto\n+'\n+\n+test_expect_success 'get --type=bool-or-auto' '\n+\tcat >.git/config <<-\\EOF &&\n+\t[bool]\n+\ttrue1\n+\ttrue2 = true\n+\tfalse = false\n+\t[int]\n+\tint1 = 0\n+\tint2 = 1\n+\tint3 = -1\n+\t[string]\n+\tstring1 = hello\n+\tstring2 = there you\n+\t[auto]\n+\tauto1 = auto\n+\tauto2 = AUTO\n+\t[bad-auto]\n+\tbad-auto1 = AUTOMATIC\n+\tEOF\n+\tcat >expect <<-\\EOF &&\n+\ttrue\n+\ttrue\n+\tfalse\n+\tfalse\n+\ttrue\n+\ttrue\n+\tauto\n+\tauto\n+\tEOF\n+\t{\n+\t\tgit config --type=bool-or-auto bool.true1 &&\n+\t\tgit config --type=bool-or-auto bool.true2 &&\n+\t\tgit config --type=bool-or-auto bool.false &&\n+\t\tgit config --type=bool-or-auto int.int1 &&\n+\t\tgit config --type=bool-or-auto int.int2 &&\n+\t\tgit config --type=bool-or-auto int.int3 &&\n+\t\tgit config --type=bool-or-auto auto.auto1 &&\n+\t\tgit config --type=bool-or-auto auto.auto2\n+\t} >actual &&\n+\ttest_cmp expect actual &&\n+\n+\ttest_must_fail git config --type=bool-or-auto --get bad-auto.bad-auto1 2>err &&\n+\tgrep \"bad tristate config value\" err\n+'\n+\n cat >expect <<\\EOF\n [bool]\n \ttrue1 = true\n-- \n2.31.1.527.g9b8f7de2547\n\n"},{"id":"421251","messageId":"xmqqmtu8tzo5.fsf@gitster.g","threadId":"55454","inReplyTo":"patch-1.6-bd70181ffb-20210408T133125Z-avarab@gmail.com","subject":"Re: [PATCH 1/5] config.c: add a comment about why value=NULL is true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T18:10:50Z","receivedAt":"2021-04-08T18:10:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Subject: Re: [PATCH 1/5] config.c: add a comment about why value=NULL is true\n\nA few unproductive nitpicks.\n\nThe question \"why does '[core] ignorecase' mean core.ignorecase is\ntrue?\" has only a single answer, which is \"because that is how we\ndesigned (loosely imitating .ini format) and implemented.\"\n\n> It's not very intuitive that git_parse_maybe_bool_text() would\n> consider NULL to be a true value. Add a small comment about it.\n\nDo you mean\n\n\tif (!value)\n\t\treturn 1;\n\nis unintuitive? Or do you mean\n\n\t[core]\n\t\tignorecase\n\nbeing a valid way to say \"the filesystem ignores case differences\"?\n\nThe answer to the question is crucial to justify the title. Only\nwhen \"the config syntax is perfectly understandable, the way the\nlogic to parse that syntax is expressed in C is not necessarily\nintuitive\" is true, the comment makes sense, I would think.\n\n> See a789ca70e7 (config: teach \"git -c\" to recognize an empty string,\n> 2014-08-04) for the behavior of \"git -c\", but we'll end up here in\n> both the config file parsing and command-line parsing cases.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> config.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/config.c b/config.c\n> index 6428393a41..fc28dbd97c 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1229,6 +1229,10 @@ ssize_t git_config_ssize_t(const char *name, const char *value)\n> static int git_parse_maybe_bool_text(const char *value)\n> {\n> \tif (!value)\n> +\t\t/*\n> +\t\t * \"[foo]\\nbar\\n\" and \"-c foo.bar\" on the command-line\n> +\t\t * are true.\n> +\t\t */\n\nThis is a \"we do not explain why '[foo] bar' is true, but to be\nprepared to parse it, we check if value is NULL\" comment. And\nI think it is a good one, except that I do not see the point of\nwriting \"\\n\" at all. Replacing them with spaces would make it far\neasier to read.\n\n\t\t/*\n\t\t * \"[foo] bar\" in the configuration file, and\n\t\t * \"git -c foo.bar\" on the command-line, mean\n\t\t * that the variable foo.bar is true. A NULL is\n\t\t * passed as 'value' in these cases.\n\t\t */\n\n> \t\treturn 1;\n> \tif (!*value)\n> \t\treturn 0;\n"},{"id":"421252","messageId":"xmqqim4wtz5k.fsf@gitster.g","threadId":"55454","inReplyTo":"patch-2.6-2f6c2de050-20210408T133125Z-avarab@gmail.com","subject":"Re: [PATCH 2/5] config tests: test for --bool-or-str","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T18:21:59Z","receivedAt":"2021-04-08T18:22:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Add the missing tests for the --bool-or-str code added in\n> dbd8c09bfe (mergetool: allow auto-merge for meld to follow the\n> vim-diff behavior, 2020-05-07).\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/t1300-config.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 72 insertions(+)\n>\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index e0dd5d65ce..a002ec5644 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -802,6 +802,78 @@ test_expect_success 'get --bool-or-int' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'get --bool-or-str' '\n> +\tcat >.git/config <<-\\EOF &&\n> +\t[bool]\n> +\ttrue1\n> +\ttrue2 = true\n> +\ttrue3 = TRUE\n> +\ttrue4 = yes\n> +\ttrue5 = YES\n> +\ttrue6 = on\n> +\ttrue7 = ON\n> +\tfalse1 =\n> +\tfalse2 = false\n> +\tfalse3 = FALSE\n> +\tfalse4 = no\n> +\tfalse5 = NO\n> +\tfalse6 = off\n> +\tfalse7 = OFF\n> +\t[int]\n> +\tint1 = 0\n> +\tint2 = 1\n> +\tint3 = -1\n> +\t[string]\n> +\tstring1 = hello\n> +\tstring2 = there you\n> +\tEOF\n\nThat's fairly complete set (but misses common permutations like\n\"Yes\"). I am not sure, if we try \"true\" and \"TRUE\", if it is worth\nto check yes/YES and others, but at the same time, I do not know if\nreducing the permutations tested would improve the readability,\nruntime and/or maintainability of the test.\n\n> +\tcat >expect <<-\\EOF &&\n> +\ttrue\n> +\ttrue\n> +\ttrue\n> +\ttrue\n> +\ttrue\n> +\ttrue\n> +\ttrue\n> +\tfalse\n> +\tfalse\n> +\tfalse\n> +\tfalse\n> +\tfalse\n> +\tfalse\n> +\tfalse\n> +\tfalse\n> +\tfalse\n> +\ttrue\n> +\ttrue\n> +\thello\n> +\tthere you\n> +\tEOF\n\nThe \"right answer\" is hard to read and maintain. Can we immediately\nspot what happened to int.int3 in this output, for example?\n\nPerhaps with something like\n\n\tinspect_config () {\n\t\tname=$1\n\t\tshift\n\t\tprintf \"%s %s\\n\" $(git config \"$@\" \"$name\") \"$name\"\n\t}\n\nwe can make these lines to say\n\n\tint.int1 false\n\tint.int2 true\n\tint.int3 true\n\tstring.string1 hello\n\tstring.string2 there you\n\nto make them easier to follow? Without such a hint, I would expect\nthat a failure output from test_cmp at the end would be very hard to\ngrok, especially with so many permutations tested that produces runs\nof \"true\" and \"false\".\n\n> +\t{\n> +\t\tgit config --type=bool-or-str bool.true1 &&\n> +\t\tgit config --bool-or-str bool.true2 &&\n\nThis is a bit curious. We do not do full permutation between\n--type=bool-or-str and --bool-or-str here. We just check both\nwould work only once. Feels a bit inconsistent.\n\nMy gut-feeling vote is to just try true/TRUE for case insensitivity\nand try all the other variants in lowercase, but I can go with the\nfull permutation if you strongly prefer it.\n\n> ...\n> +\t\tgit config --bool-or-str int.int1 &&\n> +\t\tgit config --bool-or-str int.int2 &&\n> +\t\tgit config --bool-or-str int.int3 &&\n> +\t\tgit config --bool-or-str string.string1 &&\n> +\t\tgit config --bool-or-str string.string2\n> +\t} >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThanks.\n"},{"id":"421253","messageId":"xmqqeefktz3z.fsf@gitster.g","threadId":"55454","inReplyTo":"patch-3.6-8aa59b3118-20210408T133125Z-avarab@gmail.com","subject":"Re: [PATCH 3/5] git-config: document --bool-or-str and --type=bool-or-str","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T18:22:56Z","receivedAt":"2021-04-08T18:23:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Document the new \"bool-or-str\" facility added in\n> dbd8c09bfe (mergetool: allow auto-merge for meld to follow the\n> vim-diff behavior, 2020-05-07).\n>\n> Unfortunately that commit also added a --bool-or-str option, even\n> though we've preferred to deprecate that form ever since\n> fb0dc3bac1 (builtin/config.c: support `--type=` as preferred\n> alias for `--`, 2018-04-18).\n>\n> Since we've got it already let's document it along with the preferred\n> --type=* form, and change our own code to use the --type=bool-or-str\n> form over --bool-or-str.\n\nIt was a mistake to introduce a new option that is immediately\ndeprecated X-<.\n\nThanks for spotting and correcting.\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/git-config.txt | 3 +++\n> mergetools/meld | 2 +-\n> 2 files changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index 4b4cc5c5e8..4ae9ef210c 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -187,6 +187,8 @@ Valid ``'s include:\n> 1073741824 upon input.\n> - 'bool-or-int': canonicalize according to either 'bool' or 'int', as described\n> above.\n> +- 'bool-or-str: canonicalize according to either 'bool' (as described\n> + above), or emit the value as-is.\n> - 'path': canonicalize by adding a leading `~` to the value of `$HOME` and\n> `~user` to the home directory for the specified user. This specifier has no\n> effect when setting the value (but you can use `git config section.variable\n> @@ -202,6 +204,7 @@ Valid ``'s include:\n> --bool::\n> --int::\n> --bool-or-int::\n> +--bool-or-str::\n> --path::\n> --expiry-date::\n> Historical options for selecting a type specifier. Prefer instead `--type`\n> diff --git a/mergetools/meld b/mergetools/meld\n> index aab4ebb935..8386e0574e 100644\n> --- a/mergetools/meld\n> +++ b/mergetools/meld\n> @@ -59,7 +59,7 @@ check_meld_for_features () {\n> \tif test -z \"$meld_use_auto_merge_option\"\n> \tthen\n> \t\tmeld_use_auto_merge_option=$(\n> -\t\t\tgit config --bool-or-str mergetool.meld.useAutoMerge\n> +\t\t\tgit config --type=bool-or-str mergetool.meld.useAutoMerge\n> \t\t)\n> \t\tcase \"$meld_use_auto_merge_option\" in\n> \t\ttrue | false)\n"},{"id":"421254","messageId":"xmqqa6q8tymu.fsf@gitster.g","threadId":"55454","inReplyTo":"patch-4.6-222e91e11b-20210408T133125Z-avarab@gmail.com","subject":"Re: [PATCH 4/5] config.c: add a \"tristate\" helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T18:33:13Z","receivedAt":"2021-04-08T18:33:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Add \"tristate\" functions to go along with the \"bool\" functions and\n> migrate the common pattern of checking if something is \"bool\" or\n> \"auto\" in various places over to the new functions.\n>\n> We also have e.g. \"repo_config_get_bool\" and\n> \"config_error_nonbool\". I'm not adding corresponding \"tristate\"\n> functions as they're not needed by anything, but we could add those in\n> the future if they are.\n>\n> I'm not migrating over \"core.abbrev\" parsing as part of this\n> change. When \"core.abbrev\" was made optionally boolean in\n> a9ecaa06a7 (core.abbrev=no disables abbreviations, 2020-09-01) the\n> \"die if empty\" code added in g48d5014dd4 (config.abbrev: document the\n> new default that auto-scales, 2016-11-01) wasn't adjusted. It thus\n> behaves unlike all other \"maybe bool\" config variables.\n>\n> I have a planned series to start adding some tests for \"core.abbrev\",\n> but AFAICT there's not even a test for \"core.abbrev=true\", and I'd\n> like to focus on thing that have no behavior change here, so let's\n> leave it for now.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/log.c | 13 +++++++------\n> compat/mingw.c | 6 +++---\n> config.c | 16 ++++++++++++++++\n> config.h | 12 ++++++++++++\n> http.c | 5 +++--\n> userdiff.c | 6 ++----\n> 6 files changed, 43 insertions(+), 15 deletions(-)\n\n> diff --git a/config.c b/config.c\n> index fc28dbd97c..74d2b2c0df 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1257,6 +1257,14 @@ int git_parse_maybe_bool(const char *value)\n> \treturn -1;\n> }\n> \n> +int git_parse_maybe_tristate(const char *value)\n> +{\n> +\tint v = git_parse_maybe_bool(value);\n> +\tif (v < 0 && !strcasecmp(value, \"auto\"))\n> +\t\treturn 2;\n> +\treturn v;\n> +}\n\nThis is not parse_mayb_bool_text(), so \"1\" and \"-1\" written in the\nconfiguration file are \"true\", \"0\" is \"false\", like the \"bool\" case.\n\nI wonder if written without an unnecessary extra variable, i.e.\n\n\tif (value && !strcasecmp(value, \"auto\"))\n\t\treturn 2;\n\treturn git_parse_maybe_bool(value);\n\nis easier to follow, though, as it is quite clear that it is mostly\nthe same as maybe_bool and the only difference is when \"auto\" is\ngiven.\n\n> +int git_config_tristate(const char *name, const char *value)\n> +{\n> +\tint v = git_parse_maybe_tristate(value);\n> +\tif (v < 0)\n> +\t\tdie(_(\"bad tristate config value '%s' for '%s'\"), value, name);\n> +\treturn v;\n> +}\n\nOK.\n\n> diff --git a/config.h b/config.h\n> index 19a9adbaa9..c5129e4392 100644\n> --- a/config.h\n> +++ b/config.h\n> @@ -197,6 +197,12 @@ int git_parse_ulong(const char *, unsigned long *);\n> */\n> int git_parse_maybe_bool(const char *);\n> \n> +/**\n> + * Same as `git_parse_maybe_bool`, except that \"auto\" is recognized and\n> + * will return \"2\".\n> + */\n> +int git_parse_maybe_tristate(const char *);\n\nA false being 0 and a true being 1 is understandable for readers\nwithout symbolic constant, but \"2\" deserves to have a symbolic\nconstant, doesn't it?\n\n> @@ -226,6 +232,12 @@ int git_config_bool_or_int(const char *, const char *, int *);\n> */\n> int git_config_bool(const char *, const char *);\n> \n> +/**\n> + * Like git_config_bool() except \"auto\" is also recognized and will\n> + * return \"2\"\n> + */\n> +int git_config_tristate(const char *, const char *);\n\nLikewise.\n\nThanks.\n"},{"id":"421255","messageId":"xmqq5z0wtyhw.fsf@gitster.g","threadId":"55454","inReplyTo":"patch-5.6-9f8996a888-20210408T133125Z-avarab@gmail.com","subject":"Re: [PATCH 5/5] config: add --type=bool-or-auto switch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T18:36:11Z","receivedAt":"2021-04-08T18:36:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Now that we're using git_config_tristate() internally let's expose it\n> via \"git config\" like we do \"bool\", \"int\" etc for completeness, and so\n> that we can easily test it.\n>\n> Unlike the --type=bool-or-str option added in dbd8c09bfe (mergetool:\n> allow auto-merge for meld to follow the vim-diff behavior, 2020-05-07)\n> we don't have or anticipate any in-tree user of this except the tests.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n\nOK.\n\n\n> @@ -263,6 +267,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n> \t\t\t\tstrbuf_addstr(buf, value_);\n> \t\t\telse\n> \t\t\t\tstrbuf_addstr(buf, v ? \"true\" : \"false\");\n> +\t\t} else if (type == TYPE_BOOL_OR_AUTO) {\n> +\t\t\tint v = git_config_tristate(key_, value_);\n> +\t\t\tif (v == 2)\n> +\t\t\t\tstrbuf_addstr(buf, \"auto\");\n> +\t\t\telse\n> +\t\t\t\tstrbuf_addstr(buf, v ? \"true\" : \"false\");\n\nMakes sense.\n\n> +test_expect_success 'there is no --bool-or-auto, -- is deprecated in favor of --type=' '\n> +\ttest_expect_code 129 git config --bool-or-auto\n> +'\n> +\n> +test_expect_success 'get --type=bool-or-auto' '\n> +\tcat >.git/config <<-\\EOF &&\n> +\t[bool]\n> +\ttrue1\n> +\ttrue2 = true\n> +\tfalse = false\n> +\t[int]\n> +\tint1 = 0\n> +\tint2 = 1\n> +\tint3 = -1\n> +\t[string]\n> +\tstring1 = hello\n> +\tstring2 = there you\n> +\t[auto]\n> +\tauto1 = auto\n> +\tauto2 = AUTO\n> +\t[bad-auto]\n> +\tbad-auto1 = AUTOMATIC\n> +\tEOF\n> +\tcat >expect <<-\\EOF &&\n> +\ttrue\n> +\ttrue\n> +\tfalse\n> +\tfalse\n> +\ttrue\n> +\ttrue\n> +\tauto\n> +\tauto\n> +\tEOF\n\nAlmost the same comment applies to the expected output as the\nearlier patch.\n\nOther than that, (and adjusting for 2 that should be turned into\nsymbolic constant in an earlier step in a reroll), this step looks\nquite sensible.\n\nThanks.\n\n> +\t{\n> +\t\tgit config --type=bool-or-auto bool.true1 &&\n> +\t\tgit config --type=bool-or-auto bool.true2 &&\n> +\t\tgit config --type=bool-or-auto bool.false &&\n> +\t\tgit config --type=bool-or-auto int.int1 &&\n> +\t\tgit config --type=bool-or-auto int.int2 &&\n> +\t\tgit config --type=bool-or-auto int.int3 &&\n> +\t\tgit config --type=bool-or-auto auto.auto1 &&\n> +\t\tgit config --type=bool-or-auto auto.auto2\n> +\t} >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\ttest_must_fail git config --type=bool-or-auto --get bad-auto.bad-auto1 2>err &&\n> +\tgrep \"bad tristate config value\" err\n> +'\n> +\n> cat >expect <<\\EOF\n> [bool]\n> \ttrue1 = true\n"},{"id":"421295","messageId":"878s5sid6x.fsf@evledraar.gmail.com","threadId":"55454","inReplyTo":"xmqqim4wtz5k.fsf@gitster.g","subject":"Re: [PATCH 2/5] config tests: test for --bool-or-str","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-08T23:11:50Z","receivedAt":"2021-04-08T23:11:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 08 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Add the missing tests for the --bool-or-str code added in\n>> dbd8c09bfe (mergetool: allow auto-merge for meld to follow the\n>> vim-diff behavior, 2020-05-07).\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> t/t1300-config.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++\n>> 1 file changed, 72 insertions(+)\n>>\n>> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n>> index e0dd5d65ce..a002ec5644 100755\n>> --- a/t/t1300-config.sh\n>> +++ b/t/t1300-config.sh\n>> @@ -802,6 +802,78 @@ test_expect_success 'get --bool-or-int' '\n>> \ttest_cmp expect actual\n>> '\n>> \n>> +test_expect_success 'get --bool-or-str' '\n>> +\tcat >.git/config <<-\\EOF &&\n>> +\t[bool]\n>> +\ttrue1\n>> +\ttrue2 = true\n>> +\ttrue3 = TRUE\n>> +\ttrue4 = yes\n>> +\ttrue5 = YES\n>> +\ttrue6 = on\n>> +\ttrue7 = ON\n>> +\tfalse1 =\n>> +\tfalse2 = false\n>> +\tfalse3 = FALSE\n>> +\tfalse4 = no\n>> +\tfalse5 = NO\n>> +\tfalse6 = off\n>> +\tfalse7 = OFF\n>> +\t[int]\n>> +\tint1 = 0\n>> +\tint2 = 1\n>> +\tint3 = -1\n>> +\t[string]\n>> +\tstring1 = hello\n>> +\tstring2 = there you\n>> +\tEOF\n>\n> That's fairly complete set (but misses common permutations like\n> \"Yes\"). I am not sure, if we try \"true\" and \"TRUE\", if it is worth\n> to check yes/YES and others, but at the same time, I do not know if\n> reducing the permutations tested would improve the readability,\n> runtime and/or maintainability of the test.\n\nSure, I was trying to do just enough to cover strcasecmp(). I don't\nthink we need to do black-box testing here.\n\n>> +\tcat >expect <<-\\EOF &&\n>> +\ttrue\n>> +\ttrue\n>> +\ttrue\n>> +\ttrue\n>> +\ttrue\n>> +\ttrue\n>> +\ttrue\n>> +\tfalse\n>> +\tfalse\n>> +\tfalse\n>> +\tfalse\n>> +\tfalse\n>> +\tfalse\n>> +\tfalse\n>> +\tfalse\n>> +\tfalse\n>> +\ttrue\n>> +\ttrue\n>> +\thello\n>> +\tthere you\n>> +\tEOF\n>\n> The \"right answer\" is hard to read and maintain. Can we immediately\n> spot what happened to int.int3 in this output, for example?\n>\n> Perhaps with something like\n>\n> \tinspect_config () {\n> \t\tname=$1\n> \t\tshift\n> \t\tprintf \"%s %s\\n\" $(git config \"$@\" \"$name\") \"$name\"\n> \t}\n>\n> we can make these lines to say\n>\n> \tint.int1 false\n> \tint.int2 true\n> \tint.int3 true\n> \tstring.string1 hello\n> \tstring.string2 there you\n>\n> to make them easier to follow? Without such a hint, I would expect\n> that a failure output from test_cmp at the end would be very hard to\n> grok, especially with so many permutations tested that produces runs\n> of \"true\" and \"false\".\n\nIt's a general established pattern in t1300-config.sh used by several\nother existing tests, e.g. the one for bool, path etc. I'd rather not\nget into a general refactoring of that file.\n\n>> +\t{\n>> +\t\tgit config --type=bool-or-str bool.true1 &&\n>> +\t\tgit config --bool-or-str bool.true2 &&\n>\n> This is a bit curious. We do not do full permutation between\n> --type=bool-or-str and --bool-or-str here. We just check both\n> would work only once. Feels a bit inconsistent.\n\nYeah, I was just trying to stick a --type=X v.s. --X test somewhere. I\ncan add it to another test_expect_success or something.\n\n> My gut-feeling vote is to just try true/TRUE for case insensitivity\n> and try all the other variants in lowercase, but I can go with the\n> full permutation if you strongly prefer it.\n>\n>> ...\n>> +\t\tgit config --bool-or-str int.int1 &&\n>> +\t\tgit config --bool-or-str int.int2 &&\n>> +\t\tgit config --bool-or-str int.int3 &&\n>> +\t\tgit config --bool-or-str string.string1 &&\n>> +\t\tgit config --bool-or-str string.string2\n>> +\t} >actual &&\n>> +\ttest_cmp expect actual\n>> +'\n>\n> Thanks.\n\n"},{"id":"421297","messageId":"875z0wicmp.fsf@evledraar.gmail.com","threadId":"55454","inReplyTo":"xmqqa6q8tymu.fsf@gitster.g","subject":"Re: [PATCH 4/5] config.c: add a \"tristate\" helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-08T23:23:58Z","receivedAt":"2021-04-08T23:24:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 08 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Add \"tristate\" functions to go along with the \"bool\" functions and\n>> migrate the common pattern of checking if something is \"bool\" or\n>> \"auto\" in various places over to the new functions.\n>>\n>> We also have e.g. \"repo_config_get_bool\" and\n>> \"config_error_nonbool\". I'm not adding corresponding \"tristate\"\n>> functions as they're not needed by anything, but we could add those in\n>> the future if they are.\n>>\n>> I'm not migrating over \"core.abbrev\" parsing as part of this\n>> change. When \"core.abbrev\" was made optionally boolean in\n>> a9ecaa06a7 (core.abbrev=no disables abbreviations, 2020-09-01) the\n>> \"die if empty\" code added in g48d5014dd4 (config.abbrev: document the\n>> new default that auto-scales, 2016-11-01) wasn't adjusted. It thus\n>> behaves unlike all other \"maybe bool\" config variables.\n>>\n>> I have a planned series to start adding some tests for \"core.abbrev\",\n>> but AFAICT there's not even a test for \"core.abbrev=true\", and I'd\n>> like to focus on thing that have no behavior change here, so let's\n>> leave it for now.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> builtin/log.c | 13 +++++++------\n>> compat/mingw.c | 6 +++---\n>> config.c | 16 ++++++++++++++++\n>> config.h | 12 ++++++++++++\n>> http.c | 5 +++--\n>> userdiff.c | 6 ++----\n>> 6 files changed, 43 insertions(+), 15 deletions(-)\n>\n>> diff --git a/config.c b/config.c\n>> index fc28dbd97c..74d2b2c0df 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -1257,6 +1257,14 @@ int git_parse_maybe_bool(const char *value)\n>> \treturn -1;\n>> }\n>> \n>> +int git_parse_maybe_tristate(const char *value)\n>> +{\n>> +\tint v = git_parse_maybe_bool(value);\n>> +\tif (v < 0 && !strcasecmp(value, \"auto\"))\n>> +\t\treturn 2;\n>> +\treturn v;\n>> +}\n>\n> This is not parse_mayb_bool_text(), so \"1\" and \"-1\" written in the\n> configuration file are \"true\", \"0\" is \"false\", like the \"bool\" case.\n>\n> I wonder if written without an unnecessary extra variable, i.e.\n>\n> \tif (value && !strcasecmp(value, \"auto\"))\n> \t\treturn 2;\n> \treturn git_parse_maybe_bool(value);\n>\n> is easier to follow, though, as it is quite clear that it is mostly\n> the same as maybe_bool and the only difference is when \"auto\" is\n> given.\n\nI guess it could be either way around, I think it makes more sense to\nhave git_parse_maybe_bool() handle as much as possible right from the\nget-go, since it already handles NULL if we call it first we just need\nto care about cases where it's looked at all the \"is this bool-like\"\npermutations and decided to reject it.\n\n>> +int git_config_tristate(const char *name, const char *value)\n>> +{\n>> +\tint v = git_parse_maybe_tristate(value);\n>> +\tif (v < 0)\n>> +\t\tdie(_(\"bad tristate config value '%s' for '%s'\"), value, name);\n>> +\treturn v;\n>> +}\n>\n> OK.\n>\n>> diff --git a/config.h b/config.h\n>> index 19a9adbaa9..c5129e4392 100644\n>> --- a/config.h\n>> +++ b/config.h\n>> @@ -197,6 +197,12 @@ int git_parse_ulong(const char *, unsigned long *);\n>> */\n>> int git_parse_maybe_bool(const char *);\n>> \n>> +/**\n>> + * Same as `git_parse_maybe_bool`, except that \"auto\" is recognized and\n>> + * will return \"2\".\n>> + */\n>> +int git_parse_maybe_tristate(const char *);\n>\n> A false being 0 and a true being 1 is understandable for readers\n> without symbolic constant, but \"2\" deserves to have a symbolic\n> constant, doesn't it?\n>\n>> @@ -226,6 +232,12 @@ int git_config_bool_or_int(const char *, const char *, int *);\n>> */\n>> int git_config_bool(const char *, const char *);\n>> \n>> +/**\n>> + * Like git_config_bool() except \"auto\" is also recognized and will\n>> + * return \"2\"\n>> + */\n>> +int git_config_tristate(const char *, const char *);\n>\n> Likewise.\n\nI'd prefer to just make these \"enum\", which means we'll have the aid of\nthe compiler in checking all the callsites, as in the patch-on-top\n(which I can squash appropriately, need to update the doc comments\nthough) at the end of this E-Mail.\n\nIt's a bit of boilerplate, and it's unfortunate that subset/supersets of\nenums aren't better supported in C (so e.g. you could have different\ntypes share some of the same label names), but I think being able to\nlook at any given use and know the compiler is checking whether the case\nstatements (there's no \"default\") are exhaustively enumerated is worth\nit.\n\nBut I wasn't sure you'd prefet that, especially (and maybe I read too\nmuch into it) with me replacing -1 with OBJ_BAD in another topic, as\nGIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_BAD does here.\n\nIn this case most of the callsites don't need to deal with the \"BAD\"\nvalue, but for other things in config.c and this sort of code in general\nif we're going to insist on \"v < 0\" over explicit labels the benefit of\nusing enum/switch pretty much goes away.\n\nI mean, we could not do the enum/switch/case implementation and just\nhave a \"#define\" to give \"2\" a pretty name, but at that point anything\nbeyond that is pretty pointless, i.e. we can just make the function\nreturn an \"int\" if we're not hoping to have the compiler check the enum\nvalues.\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 039a4f0961..a5d7efc8bc 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -268,11 +268,18 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\telse\n \t\t\t\tstrbuf_addstr(buf, v ? \"true\" : \"false\");\n \t\t} else if (type == TYPE_BOOL_OR_AUTO) {\n-\t\t\tint v = git_config_tristate(key_, value_);\n-\t\t\tif (v == 2)\n+\t\t\tenum git_config_type_bool_or_auto v = git_config_tristate(key_, value_);\n+\t\t\tswitch (v) {\n+\t\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE:\n+\t\t\t\tstrbuf_addstr(buf, \"false\");\n+\t\t\t\tbreak;\n+\t\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE:\n+\t\t\t\tstrbuf_addstr(buf, \"true\");\n+\t\t\t\tbreak;\n+\t\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO:\n \t\t\t\tstrbuf_addstr(buf, \"auto\");\n-\t\t\telse\n-\t\t\t\tstrbuf_addstr(buf, v ? \"true\" : \"false\");\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t} else if (type == TYPE_PATH) {\n \t\t\tconst char *v;\n \t\t\tif (git_config_pathname(&v, key_, value_) < 0)\n@@ -446,13 +453,17 @@ static char *normalize_value(const char *key, const char *value)\n \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n \t}\n \tif (type == TYPE_BOOL_OR_AUTO) {\n-\t\tint v = git_parse_maybe_tristate(value);\n-\t\tif (v < 0)\n+\t\tenum git_config_type_maybe_bool_or_auto v = git_parse_maybe_tristate(value); \n+\t\tswitch (v) {\n+\t\tcase GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_BAD:\n \t\t\treturn xstrdup(value);\n-\t\telse if (v == 2)\n-\t\t\txstrdup(\"auto\");\n-\t\telse\n-\t\t\treturn xstrdup(v ? \"true\" : \"false\");\n+\t\tcase GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_FALSE:\n+\t\t\treturn xstrdup(\"false\");\n+\t\tcase GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_TRUE:\n+\t\t\treturn xstrdup(\"true\");\n+\t\tcase GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_AUTO:\n+\t\t\treturn xstrdup(\"auto\");\n+\t\t}\n \t}\n \tif (type == TYPE_COLOR) {\n \t\tchar v[COLOR_MAXLEN];\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 0d945313d8..f363c841a7 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -868,14 +868,17 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"format.numbered\")) {\n-\t\tint tristate = git_config_tristate(var, value);\n-\t\tif (tristate == 2) {\n+\t\tenum git_config_type_bool_or_auto v = git_config_tristate(var, value);\n+\t\tswitch (v) {\n+\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE:\n \t\t\tauto_number = 1;\n \t\t\treturn 0;\n+\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE:\n+\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO:\n+\t\t\tnumbered = v;\n+\t\t\tauto_number = auto_number && numbered;\n+\t\t\treturn 0;\n \t\t}\n-\t\tnumbered = tristate;\n-\t\tauto_number = auto_number && numbered;\n-\t\treturn 0;\n \t}\n \tif (!strcmp(var, \"format.attach\")) {\n \t\tif (value && *value)\n@@ -905,12 +908,18 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"format.signaturefile\"))\n \t\treturn git_config_pathname(&signature_file, var, value);\n \tif (!strcmp(var, \"format.coverletter\")) {\n-\t\tint tristate = git_config_tristate(var, value);\n-\t\tif (tristate == 2)\n+\t\tenum git_config_type_bool_or_auto v = git_config_tristate(var, value);\n+\t\tswitch (v) {\n+\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE:\n+\t\t\tconfig_cover_letter = COVER_OFF;\n+\t\t\treturn 0;\n+\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE:\n+\t\t\tconfig_cover_letter = COVER_ON;\n+\t\t\treturn 0;\n+\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO:\n \t\t\tconfig_cover_letter = COVER_AUTO;\n-\t\telse\n-\t\t\tconfig_cover_letter = tristate ? COVER_ON : COVER_OFF;\n-\t\treturn 0;\n+\t\t\treturn 0;\n+\t\t}\n \t}\n \tif (!strcmp(var, \"format.outputdirectory\"))\n \t\treturn git_config_string(&config_output_directory, var, value);\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex e6e85ae99a..b76ccc0a15 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -247,12 +247,16 @@ int mingw_core_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (!strcmp(var, \"core.restrictinheritedhandles\")) {\n-\t\tint tristate = git_config_tristate(var, value);\n-\t\tif (tristate == 2)\n+\t\tenum git_config_type_bool_or_auto v = git_config_tristate(var, value);\n+\t\tswitch (v) {\n+\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE:\n+\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE:\n+\t\t\tcore_restrict_inherited_handles = v;\n+\t\t\treturn 0;\n+\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO:\n \t\t\tcore_restrict_inherited_handles = -1;\n-\t\telse\n-\t\t\tcore_restrict_inherited_handles = tristate;\n-\t\treturn 0;\n+\t\t\treturn 0;\n+\t\t}\n \t}\n \n \treturn 0;\ndiff --git a/config.c b/config.c\nindex 74d2b2c0df..3375895b80 100644\n--- a/config.c\n+++ b/config.c\n@@ -1257,11 +1257,11 @@ int git_parse_maybe_bool(const char *value)\n \treturn -1;\n }\n \n-int git_parse_maybe_tristate(const char *value)\n+enum git_config_type_maybe_bool_or_auto git_parse_maybe_tristate(const char *value)\n {\n \tint v = git_parse_maybe_bool(value);\n \tif (v < 0 && !strcasecmp(value, \"auto\"))\n-\t\treturn 2;\n+\t\treturn GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO;\n \treturn v;\n }\n \n@@ -1276,9 +1276,10 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool)\n \treturn git_config_int(name, value);\n }\n \n-int git_config_tristate(const char *name, const char *value)\n+enum git_config_type_bool_or_auto git_config_tristate(const char *name,\n+\t\t\t\t\t\t const char *value)\n {\n-\tint v = git_parse_maybe_tristate(value);\n+\tenum git_config_type_maybe_bool_or_auto v = git_parse_maybe_tristate(value);\n \tif (v < 0)\n \t\tdie(_(\"bad tristate config value '%s' for '%s'\"), value, name);\n \treturn v;\ndiff --git a/config.h b/config.h\nindex c5129e4392..70684ecb3c 100644\n--- a/config.h\n+++ b/config.h\n@@ -201,7 +201,14 @@ int git_parse_maybe_bool(const char *);\n * Same as `git_parse_maybe_bool`, except that \"auto\" is recognized and\n * will return \"2\".\n */\n-int git_parse_maybe_tristate(const char *);\n+enum git_config_type_maybe_bool_or_auto {\n+\tGIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_BAD = -1,\n+\tGIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_FALSE = 0,\n+\tGIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_TRUE = 1,\n+\tGIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_AUTO = 2,\n+};\n+\t\n+enum git_config_type_maybe_bool_or_auto git_parse_maybe_tristate(const char *);\n \n /**\n * Parse the string to an integer, including unit factors. Dies on error;\n@@ -236,7 +243,13 @@ int git_config_bool(const char *, const char *);\n * Like git_config_bool() except \"auto\" is also recognized and will\n * return \"2\"\n */\n-int git_config_tristate(const char *, const char *);\n+enum git_config_type_bool_or_auto {\n+\tGIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE = GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_FALSE,\n+\tGIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE = GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_TRUE,\n+\tGIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO = GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_AUTO,\n+};\n+enum git_config_type_bool_or_auto git_config_tristate(const char *name,\n+\t\t\t\t\t\t const char *value);\n \n /**\n * Allocates and copies the value string into the `dest` parameter; if no\ndiff --git a/http.c b/http.c\nindex b54a232e90..6dd6191517 100644\n--- a/http.c\n+++ b/http.c\n@@ -406,12 +406,16 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\treturn git_config_string(&user_agent, var, value);\n \n \tif (!strcmp(\"http.emptyauth\", var)) {\n-\t\tint tristate = git_config_tristate(var, value);\n-\t\tif (tristate == 2)\n+\t\tenum git_config_type_bool_or_auto v = git_config_tristate(var, value);\n+\t\tswitch (v) {\n+\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE:\n+\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE:\n+\t\t\tcurl_empty_auth = v;\n+\t\t\treturn 0;\n+\t\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO:\n \t\t\tcurl_empty_auth = -1;\n-\t\telse\n-\t\t\tcurl_empty_auth = tristate;\n-\t\treturn 0;\n+\t\t\treturn 0;\n+\t\t}\n \t}\n \n \tif (!strcmp(\"http.delegation\", var)) {\ndiff --git a/userdiff.c b/userdiff.c\nindex 7ff010961f..fe001563c3 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -277,8 +277,16 @@ static int parse_funcname(struct userdiff_funcname *f, const char *k,\n \n static int parse_tristate(int *b, const char *k, const char *v)\n {\n-\tint tristate = git_config_tristate(k, v);\n-\t*b = tristate == 2 ? -1 : tristate;\n+\tenum git_config_type_bool_or_auto tristate = git_config_tristate(k, v);\n+\tswitch (tristate) {\n+\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE:\n+\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE:\n+\t\t*b = tristate;\n+\t\tbreak;\n+\tcase GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO:\n+\t\t*b = -1;\n+\t\tbreak;\n+\t}\n \treturn 0;\n }\n \n\n"},{"id":"421308","messageId":"xmqqtuogpc6f.fsf@gitster.g","threadId":"55454","inReplyTo":"875z0wicmp.fsf@evledraar.gmail.com","subject":"Re: [PATCH 4/5] config.c: add a \"tristate\" helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T23:51:52Z","receivedAt":"2021-04-08T23:51:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I'd prefer to just make these \"enum\", which means we'll have the aid of\n> the compiler in checking all the callsites, as in the patch-on-top\n> (which I can squash appropriately, need to update the doc comments\n> though) at the end of this E-Mail.\n\nI think enum is oversold by some people (not me). C Compilers won't\ndo much when you use them interchangeably with int, simply because\nthey are designed to be used that way, no?\n\nIf existing code used 0 as false and 1 as true, and it learns an\n\"auto\" value with a new definition,\n\n #define TRISTATE_AUTO 2\n\nwithout TRISTATE_{TRUE,FALSE} defined to 0 and 1, that would be a\ngood place to stop. I'd be quite happy with that.\n\n"},{"id":"421310","messageId":"xmqqsg40pc1l.fsf@gitster.g","threadId":"55454","inReplyTo":"875z0wicmp.fsf@evledraar.gmail.com","subject":"Re: [PATCH 4/5] config.c: add a \"tristate\" helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T23:54:46Z","receivedAt":"2021-04-08T23:54:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>>> +int git_parse_maybe_tristate(const char *value)\n>>> +{\n>>> +\tint v = git_parse_maybe_bool(value);\n>>> +\tif (v < 0 && !strcasecmp(value, \"auto\"))\n>>> +\t\treturn 2;\n>>> +\treturn v;\n>>> +}\n>>\n>> This is not parse_maybe_bool_text(), so \"1\" and \"-1\" written in the\n>> configuration file are \"true\", \"0\" is \"false\", like the \"bool\" case.\n>>\n>> I wonder if written without an unnecessary extra variable, i.e.\n>>\n>> \tif (value && !strcasecmp(value, \"auto\"))\n>> \t\treturn 2;\n>> \treturn git_parse_maybe_bool(value);\n>>\n>> is easier to follow, though, as it is quite clear that it is mostly\n>> the same as maybe_bool and the only difference is when \"auto\" is\n>> given.\n>\n> I guess it could be either way around,...\n\nHaving seen another example in the current code recently,\n\n static int parse_tristate(int *b, const char *k, const char *v)\n {\n-\tif (v && !strcasecmp(v, \"auto\"))\n-\t\t*b = -1;\n-\telse\n-\t\t*b = git_config_bool(k, v);\n\nI upgrade my earlier \"I wonder\" to \"I do think that\". Let's swap\nthe order so that it is clear that we are special-casing \"auto\".\n\n"},{"id":"421324","messageId":"87o8eogs2r.fsf@evledraar.gmail.com","threadId":"55454","inReplyTo":"xmqqtuogpc6f.fsf@gitster.g","subject":"Re: [PATCH 4/5] config.c: add a \"tristate\" helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-09T01:33:16Z","receivedAt":"2021-04-09T01:33:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 09 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> I'd prefer to just make these \"enum\", which means we'll have the aid of\n>> the compiler in checking all the callsites, as in the patch-on-top\n>> (which I can squash appropriately, need to update the doc comments\n>> though) at the end of this E-Mail.\n>\n> I think enum is oversold by some people (not me). C Compilers won't\n> do much when you use them interchangeably with int, simply because\n> they are designed to be used that way, no?\n\nThey are just ints, not a seperate type like in C++.\n\n> If existing code used 0 as false and 1 as true, and it learns an\n> \"auto\" value with a new definition,\n>\n> #define TRISTATE_AUTO 2\n>\n> without TRISTATE_{TRUE,FALSE} defined to 0 and 1, that would be a\n> good place to stop. I'd be quite happy with that.\n\nThe benefit in this case is to human readers of the code who know\nthey're being helped by the enum checking in \"case\" arms.\n\nSo you can immediately look at code like:\n\n enum foo x = git_config_foo();\n switch (x)\n [...]\n\nAnd know without having to jump to the definition of \"git_config_foo()\"\nthat all the return values are being handled.\n\nJust to clarify, do you have a dislike of the sort of code in\nhttp://lore.kernel.org/git/875z0wicmp.fsf@evledraar.gmail.com for all\nuses in the codebase, or in this case because the diff of adapting\nexisting code is larger as a result?\n\nI think if we're not going to use enum/switch for this and similar\nthings in config.c in the future it makes sense to pass a pointer to a\n\"is_auto\" parameter to these new tristate() functions, similar to\ne.g. the existing git_config_bool_or_int().\n"},{"id":"421386","messageId":"xmqq35vzoc0e.fsf@gitster.g","threadId":"55454","inReplyTo":"87o8eogs2r.fsf@evledraar.gmail.com","subject":"Re: [PATCH 4/5] config.c: add a \"tristate\" helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-09T12:53:05Z","receivedAt":"2021-04-09T12:53:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The benefit in this case is to human readers of the code who know\n> they're being helped by the enum checking in \"case\" arms.\n\nWell, do we have tristate that is handled with switch/case? And\nmore importantly, do tristates benefit from getting handled with\nswitch/case?\n\nThe one I cited as an existing example to decide that we should\nfavour \"special case 'auto' and then let it fall through to the\nnormal bool\" is in userdiff.c\n\n static int parse_tristate(int *b, const char *k, const char *v)\n {\n if (v && !strcasecmp(v, \"auto\"))\n *b = -1;\n else\n *b = git_config_bool(k, v);\n return 0;\n }\n\nand the caller uses it to set -1/0/1 in driver->binary member.\n\nAnd the member is often used like so:\n\n\t... else if (userdiff->binary != -1) {\n\t\tis_binary = userdiff->binary;\n\t} else {\n\t\tis_binary = auto_detect_based_on_contents(...);\n\t}\n\tif (is_binary) {\n\t\t... do the binary thing ...\n\t}\n\nThis is because \"auto\" is the only value among the three that needs\n\"preprocessing\" before it is turned into a concrete yes/no that is\nusable in the downstream code. So with AUTO being the only spelled\nout value, a construct like this:\n\n\tif ((driver->binary != TRISTATE_AUTO) \n\t ? driver->binary : auto_detect())\n\t\t...; /* do the binary thing */\n\telse\n\t\t...; /* do the non-binary thing */\n\nwould be sufficient to help human readers. You do not need enum\nto help, and you do not need switch/case.\n\nYou could use switch/case with enum if you really wanted to, but\n\n\tswitch (temp = driver->binary) {\n\tcase TRISTATE_AUTO:\n\t\ttemp = auto_detect();\n break; /* ????? */\n\tcase TRISTATE_FALSE:\n\t\t/* do the non-binary thing */\n\t\t...;\n\t\tbreak;\n\tcase TRISTATE_TRUE:\n\t\t/* do the binary thing */\n\t\t...;\n\t\tbreak;\n\t}\n\nwould not be a useful construct for the \"auto\" use case, where\ntristate is used to mean \"we cannot decide at the configuration\ntime, as the appropriate value depends on other factors that are\navailable when it actually is used, e.g. the contents in the buffer,\nif fd=1 is connected to the terminal, etc.\"\n\nIf you really wanted to, you could still use switch/case for such a\nuse case, perhaps like this:\n\n\tswitch (temp = driver->binary) {\n\tcase TRISTATE_AUTO:\n\t\ttemp = auto_detect();\n\t\t/* fallthrough */\n\tdefault:\n\t\tif (!temp) { /* TRISTATE_FALSE */\n\t\t\t/* do the non-binary thing */\n\t\t\t...;\n\t\t} else { /* TRISTATE_TRUE */\n\t\t\t/* do the binary thing */\n\t\t\t...;\n\t\t}\n\t\tbreak;\n\t}\n\nand switch may help making sure that all enum values are handled,\nbut I do not see a value in it. The earlier code that used \"if\nauto, run autodetect, otherwise use the value as is\" as the\ncondition for an if/else statement would be far easier to follow,\nand equally safe.\n\n> ... in config.c in the future it makes sense to pass a pointer to a\n> \"is_auto\" parameter to these new tristate() functions, similar to\n> e.g. the existing git_config_bool_or_int().\n\nI am not sure what you are trying to gain by introducing is_auto\nhere.\n\nFor bool_or_int(), is_bool pointer makes perfect sense, because the\nvalue spelled as \"true\" cannot be anything but bool, but \"1\" can be\neither boolean true or an integer. An extra is_bool bit can be used\nby callers that care if the user said \"true/yes\" or \"1\" and want to\nbehave differently, when the configuration can take either bool or\ninteger. For example, if you originally have a \"do you want this\noperation to be verbose?\" yes/no variable, and later extended it to\nadd verbosity level, \"yes/true\" might mean \"yes, please use the\ndefault verbosity level\", while \"1\", \"2\", ... would mean \"yes, and\nplease set the verbosity level to this number\". And the default\nverbosity level may not be \"1\", so the distinction between \"yes\" and\n\"1\" does matter.\n\nI do not see such a disambiguation need for tristate parsing, so the\nimmense usefulness of is_bool is not a good analogy to draw value for\nthe proposed is_auto from.\n\nThanks.\n"},{"id":"421472","messageId":"YHCzhcBkqdeEMrWA@coredump.intra.peff.net","threadId":"55454","inReplyTo":"xmqqa6q8tymu.fsf@gitster.g","subject":"Re: [PATCH 4/5] config.c: add a \"tristate\" helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-09T20:05:25Z","receivedAt":"2021-04-09T20:05:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 08, 2021 at 11:33:13AM -0700, Junio C Hamano wrote:\n\n> > +/**\n> > + * Same as `git_parse_maybe_bool`, except that \"auto\" is recognized and\n> > + * will return \"2\".\n> > + */\n> > +int git_parse_maybe_tristate(const char *);\n> \n> A false being 0 and a true being 1 is understandable for readers\n> without symbolic constant, but \"2\" deserves to have a symbolic\n> constant, doesn't it?\n\nI had the same thought.\n\nThis \"tristate\" really has four outcomes: true, false, auto, or error.\nSince the caller has to then check for error or for \"auto\" themselves,\nit feels like it is not actually making their lives any easier. I.e.,\nwithout it:\n\n if (value && !strcmp(value, \"auto\"))\n\tdo_auto();\n else {\n\tint b = git_parse_maybe_bool(value);\n\tif (b < 0)\n\t\tdo_error();\n\telse if (b)\n\t\tdo_true();\n\telse\n\t\tdo_false();\n }\n\nbut with it:\n\n b = git_parse_maybe_tristate(value);\n if (b < 0)\n do_error();\n else if (b == 2)\n do_auto();\n else if (b)\n do_true();\n else\n do_false();\n\nwhich is the same thing, swapping string comparison for a numeric one.\n\nAnd a caller of git-config has the same thing: it still has to check for\n\"auto\" in the output it gets (which it could just do via bool-or-str).\n\nIt seems like it would be more convenient if you could pass it a bool\nvalue to turn the \"auto\" into. E.g., if you could do:\n\n b = git_parse_maybe_tristate(value, 1); /* default to \"1\" */\n if (b < 0)\n do_error();\n if (b)\n do_true(); /* true, or maybe \"auto\" */\n else\n do_false();\n\nI dunno. That would probably be hard to represent via \"git config\n--type\". And some callers probably do care about \"auto\" versus \"true\".\n\nIt also feels funny calling this \"tristate\". Aren't there other\ntristates we could have besides \"auto\"? The command-line option is\nbool-or-auto, which is descriptive. Should this use a similar name?\n\n-Peff\n"},{"id":"421482","messageId":"xmqqv98vm7lt.fsf@gitster.g","threadId":"55454","inReplyTo":"YHCzhcBkqdeEMrWA@coredump.intra.peff.net","subject":"Re: [PATCH 4/5] config.c: add a \"tristate\" helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-09T22:11:10Z","receivedAt":"2021-04-09T22:11:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It seems like it would be more convenient if you could pass it a bool\n> value to turn the \"auto\" into. E.g., if you could do:\n>\n> b = git_parse_maybe_tristate(value, 1); /* default to \"1\" */\n> if (b < 0)\n> do_error();\n> if (b)\n> do_true(); /* true, or maybe \"auto\" */\n> else\n> do_false();\n>\n> I dunno. That would probably be hard to represent via \"git config\n> --type\". And some callers probably do care about \"auto\" versus \"true\".\n\nIt would work well for codepaths where computing the default value\nis cheap (or even possible).\n\nI think the point of using \"auto\" is to delay the decision as late\nas possible. E.g. in-core parsed config and attribute may still\nwant to stay \"auto\", until we actually get our hands on the blob\ncontents to see if it is binary, until we know how heavily loaded\nthe system is, until we know isatty(1), etc. Some are cheap to\ncompute in advance, some are expensive and impossible until we meet\nthe data.\n\nSo I still think the canonical use pattern for the \"auto\" thing is\n\n\tis_frotz = git_parse_bool_or_auto(value);\n\n\t... arbitrary number of things can happen here\n\t... the above may even be done in a git_config()\n\t... callback, and is_frotz may not even be used.\n\n\tif (is_frotz == AUTO)\n\t\tis_frotz = auto_detect_frotz();\n\n\tif (is_frotz)\n\t\t...; /* do the frotz thing */\n\telse\n\t\t...; /* do the non-frotz thing */\n\n> It also feels funny calling this \"tristate\". Aren't there other\n> tristates we could have besides \"auto\"? The command-line option is\n> bool-or-auto, which is descriptive. Should this use a similar name?\n\nI like that bool-or-auto name very much.\n"},{"id":"421505","messageId":"YHD+BMo35krbH9rx@coredump.intra.peff.net","threadId":"55454","inReplyTo":"xmqqv98vm7lt.fsf@gitster.g","subject":"Re: [PATCH 4/5] config.c: add a \"tristate\" helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-10T01:23:16Z","receivedAt":"2021-04-10T01:23:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 09, 2021 at 03:11:10PM -0700, Junio C Hamano wrote:\n\n> > I dunno. That would probably be hard to represent via \"git config\n> > --type\". And some callers probably do care about \"auto\" versus \"true\".\n> \n> It would work well for codepaths where computing the default value\n> is cheap (or even possible).\n> \n> I think the point of using \"auto\" is to delay the decision as late\n> as possible. E.g. in-core parsed config and attribute may still\n> want to stay \"auto\", until we actually get our hands on the blob\n> contents to see if it is binary, until we know how heavily loaded\n> the system is, until we know isatty(1), etc. Some are cheap to\n> compute in advance, some are expensive and impossible until we meet\n> the data.\n\nHmm, yeah, that's the \"do care about auto versus true\" thing.\n\n> So I still think the canonical use pattern for the \"auto\" thing is\n> \n> \tis_frotz = git_parse_bool_or_auto(value);\n> \n> \t... arbitrary number of things can happen here\n> \t... the above may even be done in a git_config()\n> \t... callback, and is_frotz may not even be used.\n> \n> \tif (is_frotz == AUTO)\n> \t\tis_frotz = auto_detect_frotz();\n> \n> \tif (is_frotz)\n> \t\t...; /* do the frotz thing */\n> \telse\n> \t\t...; /* do the non-frotz thing */\n\nThat makes sense. Usually we represent \"undecided\" in such a tristate\nwith -1, so something that returned -1/0/1 would feel very natural to me\n(and probably wouldn't need symbolic constants even). But -1 is also\nerror.\n\nSo a function like:\n\n int git_parse_tristate(const char *value, int *out);\n\nwhich returned success/error via its return value, and put the value\ninto \"out\" would feel pretty natural to me.\n\nI dunno. I admit I don't care _that_ much, but somehow Ævar's series\nhave a way of sniping me into responding anyway. :)\n\n-Peff\n"},{"id":"421510","messageId":"xmqq8s5qlxr9.fsf@gitster.g","threadId":"55454","inReplyTo":"YHD+BMo35krbH9rx@coredump.intra.peff.net","subject":"Re: [PATCH 4/5] config.c: add a \"tristate\" helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-10T01:43:54Z","receivedAt":"2021-04-10T01:43:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So a function like:\n>\n> int git_parse_tristate(const char *value, int *out);\n>\n> which returned success/error via its return value, and put the value\n> into \"out\" would feel pretty natural to me.\n\nYeah, with s/tristate/bool-or-auto/, and if we do this throughout\nthe types, that would be ideal. FWIW git_parse_ulong() and friends\nfor sized numerics already follow that pattern, but helpers for\nboolean like git_parse_maybe_bool() don't, which is unfortunate.\n"}]}