[PATCH 0/5] config: support --type=bool-or-auto for "tristate" parsing

[PATCH 0/5] config: support --type=bool-or-auto for "tristate" parsing

[Ævar Arnfjörð Bjarmason]

Refactor some of our internal C code to use a new
git_config_tristate() functino instead of checking for "auto" or
"bool" in several places.

This isn't just Yet Another Ævar Series, I'm using this to re-roll my
outstanding userdiff series, the start of that topic was a partial
refactor of userdiff.c which is better done here.

While writing this I discovered that the recently added --bool-or-type
option added in
https://lore.kernel.org/git/pull.781.v18.git.git.1599895268433.gitgitgadget@gmail.com/
didn't have any tests (and we didn't notice in 18! iterations of it:)

2/5 adds that, 1/5 adds a comment on some bool parsing code that's
puzzled me for the Nth time.

Ævar Arnfjörð Bjarmason (5):
  config.c: add a comment about why value=NULL is true
  config tests: test for --bool-or-str
  git-config: document --bool-or-str and --type=bool-or-str
  config.c: add a "tristate" helper
  config: add --type=bool-or-auto switch

 Documentation/git-config.txt |   7 ++
 builtin/config.c             |  19 ++++++
 builtin/log.c                |  13 ++--
 compat/mingw.c               |   6 +-
 config.c                     |  20 ++++++
 config.h                     |  12 ++++
 http.c                       |   5 +-
 mergetools/meld              |   2 +-
 t/t1300-config.sh            | 121 +++++++++++++++++++++++++++++++++++
 userdiff.c                   |   6 +-
 10 files changed, 195 insertions(+), 16 deletions(-)

-- 
2.31.1.527.g9b8f7de2547

[PATCH 1/5] config.c: add a comment about why value=NULL is true

[Ævar Arnfjörð Bjarmason]

It's not very intuitive that git_parse_maybe_bool_text() would
consider NULL to be a true value. Add a small comment about it.

See a789ca70e7 (config: teach "git -c" to recognize an empty string,
2014-08-04) for the behavior of "git -c", but we'll end up here in
both the config file parsing and command-line parsing cases.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 config.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/config.c b/config.c
index 6428393a41..fc28dbd97c 100644
--- a/config.c
+++ b/config.c
@@ -1229,6 +1229,10 @@ ssize_t git_config_ssize_t(const char *name, const char *value)
 static int git_parse_maybe_bool_text(const char *value)
 {
 	if (!value)
+		/*
+		 * "[foo]\nbar\n" and "-c foo.bar" on the command-line
+		 * are true.
+		 */
 		return 1;
 	if (!*value)
 		return 0;
-- 
2.31.1.527.g9b8f7de2547

Re: [PATCH 1/5] config.c: add a comment about why value=NULL is true

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Subject: Re: [PATCH 1/5] config.c: add a comment about why value=NULL is true

A few unproductive nitpicks.

The question "why does '[core] ignorecase' mean core.ignorecase is
true?" has only a single answer, which is "because that is how we
designed (loosely imitating .ini format) and implemented."

> It's not very intuitive that git_parse_maybe_bool_text() would
> consider NULL to be a true value. Add a small comment about it.

Do you mean

	if (!value)
		return 1;

is unintuitive?  Or do you mean

	[core]
		ignorecase

being a valid way to say "the filesystem ignores case differences"?

The answer to the question is crucial to justify the title.  Only
when "the config syntax is perfectly understandable, the way the
logic to parse that syntax is expressed in C is not necessarily
intuitive" is true, the comment makes sense, I would think.

> See a789ca70e7 (config: teach "git -c" to recognize an empty string,
> 2014-08-04) for the behavior of "git -c", but we'll end up here in
> both the config file parsing and command-line parsing cases.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  config.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/config.c b/config.c
> index 6428393a41..fc28dbd97c 100644
> --- a/config.c
> +++ b/config.c
> @@ -1229,6 +1229,10 @@ ssize_t git_config_ssize_t(const char *name, const char *value)
>  static int git_parse_maybe_bool_text(const char *value)
>  {
>  	if (!value)
> +		/*
> +		 * "[foo]\nbar\n" and "-c foo.bar" on the command-line
> +		 * are true.
> +		 */

This is a "we do not explain why '[foo] bar' is true, but to be
prepared to parse it, we check if value is NULL" comment.  And
I think it is a good one, except that I do not see the point of
writing "\n" at all.  Replacing them with spaces would make it far
easier to read.

		/*
		 * "[foo] bar" in the configuration file, and
		 * "git -c foo.bar" on the command-line, mean
		 * that the variable foo.bar is true.  A NULL is
		 * passed as 'value' in these cases.
		 */

>  		return 1;
>  	if (!*value)
>  		return 0;

[PATCH 2/5] config tests: test for --bool-or-str

[Ævar Arnfjörð Bjarmason]

Add the missing tests for the --bool-or-str code added in
dbd8c09bfe (mergetool: allow auto-merge for meld to follow the
vim-diff behavior, 2020-05-07).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1300-config.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e0dd5d65ce..a002ec5644 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -802,6 +802,78 @@ test_expect_success 'get --bool-or-int' '
 	test_cmp expect actual
 '
 
+test_expect_success 'get --bool-or-str' '
+	cat >.git/config <<-\EOF &&
+	[bool]
+	true1
+	true2 = true
+	true3 = TRUE
+	true4 = yes
+	true5 = YES
+	true6 = on
+	true7 = ON
+	false1 =
+	false2 = false
+	false3 = FALSE
+	false4 = no
+	false5 = NO
+	false6 = off
+	false7 = OFF
+	[int]
+	int1 = 0
+	int2 = 1
+	int3 = -1
+	[string]
+	string1 = hello
+	string2 = there you
+	EOF
+	cat >expect <<-\EOF &&
+	true
+	true
+	true
+	true
+	true
+	true
+	true
+	false
+	false
+	false
+	false
+	false
+	false
+	false
+	false
+	false
+	true
+	true
+	hello
+	there you
+	EOF
+	{
+		git config --type=bool-or-str bool.true1 &&
+		git config --bool-or-str bool.true2 &&
+		git config --bool-or-str bool.true3 &&
+		git config --bool-or-str bool.true4 &&
+		git config --bool-or-str bool.true5 &&
+		git config --bool-or-str bool.true6 &&
+		git config --bool-or-str bool.true7 &&
+		git config --bool-or-str bool.false1 &&
+		git config --bool-or-str bool.false2 &&
+		git config --bool-or-str bool.false3 &&
+		git config --bool-or-str bool.false4 &&
+		git config --bool-or-str bool.false5 &&
+		git config --bool-or-str bool.false6 &&
+		git config --bool-or-str bool.false6 &&
+		git config --bool-or-str bool.false7 &&
+		git config --bool-or-str int.int1 &&
+		git config --bool-or-str int.int2 &&
+		git config --bool-or-str int.int3 &&
+		git config --bool-or-str string.string1 &&
+		git config --bool-or-str string.string2
+	} >actual &&
+	test_cmp expect actual
+'
+
 cat >expect <<\EOF
 [bool]
 	true1 = true
-- 
2.31.1.527.g9b8f7de2547

Re: [PATCH 2/5] config tests: test for --bool-or-str

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Add the missing tests for the --bool-or-str code added in
> dbd8c09bfe (mergetool: allow auto-merge for meld to follow the
> vim-diff behavior, 2020-05-07).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t1300-config.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index e0dd5d65ce..a002ec5644 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -802,6 +802,78 @@ test_expect_success 'get --bool-or-int' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'get --bool-or-str' '
> +	cat >.git/config <<-\EOF &&
> +	[bool]
> +	true1
> +	true2 = true
> +	true3 = TRUE
> +	true4 = yes
> +	true5 = YES
> +	true6 = on
> +	true7 = ON
> +	false1 =
> +	false2 = false
> +	false3 = FALSE
> +	false4 = no
> +	false5 = NO
> +	false6 = off
> +	false7 = OFF
> +	[int]
> +	int1 = 0
> +	int2 = 1
> +	int3 = -1
> +	[string]
> +	string1 = hello
> +	string2 = there you
> +	EOF

That's fairly complete set (but misses common permutations like
"Yes").  I am not sure, if we try "true" and "TRUE", if it is worth
to check yes/YES and others, but at the same time, I do not know if
reducing the permutations tested would improve the readability,
runtime and/or maintainability of the test.

> +	cat >expect <<-\EOF &&
> +	true
> +	true
> +	true
> +	true
> +	true
> +	true
> +	true
> +	false
> +	false
> +	false
> +	false
> +	false
> +	false
> +	false
> +	false
> +	false
> +	true
> +	true
> +	hello
> +	there you
> +	EOF

The "right answer" is hard to read and maintain.  Can we immediately
spot what happened to int.int3 in this output, for example?

Perhaps with something like

	inspect_config () {
		name=$1
		shift
		printf "%s %s\n" $(git config "$@" "$name") "$name"
	}

we can make these lines to say

	int.int1 false
	int.int2 true
	int.int3 true
	string.string1 hello
	string.string2 there you

to make them easier to follow?  Without such a hint, I would expect
that a failure output from test_cmp at the end would be very hard to
grok, especially with so many permutations tested that produces runs
of "true" and "false".

> +	{
> +		git config --type=bool-or-str bool.true1 &&
> +		git config --bool-or-str bool.true2 &&

This is a bit curious.  We do not do full permutation between
--type=bool-or-str and --bool-or-str here.  We just check both
would work only once.  Feels a bit inconsistent.

My gut-feeling vote is to just try true/TRUE for case insensitivity
and try all the other variants in lowercase, but I can go with the
full permutation if you strongly prefer it.

> ...
> +		git config --bool-or-str int.int1 &&
> +		git config --bool-or-str int.int2 &&
> +		git config --bool-or-str int.int3 &&
> +		git config --bool-or-str string.string1 &&
> +		git config --bool-or-str string.string2
> +	} >actual &&
> +	test_cmp expect actual
> +'

Thanks.

Re: [PATCH 2/5] config tests: test for --bool-or-str

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 08 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Add the missing tests for the --bool-or-str code added in
>> dbd8c09bfe (mergetool: allow auto-merge for meld to follow the
>> vim-diff behavior, 2020-05-07).
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/t1300-config.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 72 insertions(+)
>>
>> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
>> index e0dd5d65ce..a002ec5644 100755
>> --- a/t/t1300-config.sh
>> +++ b/t/t1300-config.sh
>> @@ -802,6 +802,78 @@ test_expect_success 'get --bool-or-int' '
>>  	test_cmp expect actual
>>  '
>>  
>> +test_expect_success 'get --bool-or-str' '
>> +	cat >.git/config <<-\EOF &&
>> +	[bool]
>> +	true1
>> +	true2 = true
>> +	true3 = TRUE
>> +	true4 = yes
>> +	true5 = YES
>> +	true6 = on
>> +	true7 = ON
>> +	false1 =
>> +	false2 = false
>> +	false3 = FALSE
>> +	false4 = no
>> +	false5 = NO
>> +	false6 = off
>> +	false7 = OFF
>> +	[int]
>> +	int1 = 0
>> +	int2 = 1
>> +	int3 = -1
>> +	[string]
>> +	string1 = hello
>> +	string2 = there you
>> +	EOF
>
> That's fairly complete set (but misses common permutations like
> "Yes").  I am not sure, if we try "true" and "TRUE", if it is worth
> to check yes/YES and others, but at the same time, I do not know if
> reducing the permutations tested would improve the readability,
> runtime and/or maintainability of the test.

Sure, I was trying to do just enough to cover strcasecmp(). I don't
think we need to do black-box testing here.

>> +	cat >expect <<-\EOF &&
>> +	true
>> +	true
>> +	true
>> +	true
>> +	true
>> +	true
>> +	true
>> +	false
>> +	false
>> +	false
>> +	false
>> +	false
>> +	false
>> +	false
>> +	false
>> +	false
>> +	true
>> +	true
>> +	hello
>> +	there you
>> +	EOF
>
> The "right answer" is hard to read and maintain.  Can we immediately
> spot what happened to int.int3 in this output, for example?
>
> Perhaps with something like
>
> 	inspect_config () {
> 		name=$1
> 		shift
> 		printf "%s %s\n" $(git config "$@" "$name") "$name"
> 	}
>
> we can make these lines to say
>
> 	int.int1 false
> 	int.int2 true
> 	int.int3 true
> 	string.string1 hello
> 	string.string2 there you
>
> to make them easier to follow?  Without such a hint, I would expect
> that a failure output from test_cmp at the end would be very hard to
> grok, especially with so many permutations tested that produces runs
> of "true" and "false".

It's a general established pattern in t1300-config.sh used by several
other existing tests, e.g. the one for bool, path etc. I'd rather not
get into a general refactoring of that file.

>> +	{
>> +		git config --type=bool-or-str bool.true1 &&
>> +		git config --bool-or-str bool.true2 &&
>
> This is a bit curious.  We do not do full permutation between
> --type=bool-or-str and --bool-or-str here.  We just check both
> would work only once.  Feels a bit inconsistent.

Yeah, I was just trying to stick a --type=X v.s. --X test somewhere. I
can add it to another test_expect_success or something.

> My gut-feeling vote is to just try true/TRUE for case insensitivity
> and try all the other variants in lowercase, but I can go with the
> full permutation if you strongly prefer it.
>
>> ...
>> +		git config --bool-or-str int.int1 &&
>> +		git config --bool-or-str int.int2 &&
>> +		git config --bool-or-str int.int3 &&
>> +		git config --bool-or-str string.string1 &&
>> +		git config --bool-or-str string.string2
>> +	} >actual &&
>> +	test_cmp expect actual
>> +'
>
> Thanks.

[PATCH 3/5] git-config: document --bool-or-str and --type=bool-or-str

[Ævar Arnfjörð Bjarmason]

Document the new "bool-or-str" facility added in
dbd8c09bfe (mergetool: allow auto-merge for meld to follow the
vim-diff behavior, 2020-05-07).

Unfortunately that commit also added a --bool-or-str option, even
though we've preferred to deprecate that form ever since
fb0dc3bac1 (builtin/config.c: support `--type=<type>` as preferred
alias for `--<type>`, 2018-04-18).

Since we've got it already let's document it along with the preferred
--type=* form, and change our own code to use the --type=bool-or-str
form over --bool-or-str.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-config.txt | 3 +++
 mergetools/meld              | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4b4cc5c5e8..4ae9ef210c 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -187,6 +187,8 @@ Valid `<type>`'s include:
   1073741824 upon input.
 - 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
   above.
+- 'bool-or-str: canonicalize according to either 'bool' (as described
+  above), or emit the value as-is.
 - 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
   `~user` to the home directory for the specified user. This specifier has no
   effect when setting the value (but you can use `git config section.variable
@@ -202,6 +204,7 @@ Valid `<type>`'s include:
 --bool::
 --int::
 --bool-or-int::
+--bool-or-str::
 --path::
 --expiry-date::
   Historical options for selecting a type specifier. Prefer instead `--type`
diff --git a/mergetools/meld b/mergetools/meld
index aab4ebb935..8386e0574e 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -59,7 +59,7 @@ check_meld_for_features () {
 	if test -z "$meld_use_auto_merge_option"
 	then
 		meld_use_auto_merge_option=$(
-			git config --bool-or-str mergetool.meld.useAutoMerge
+			git config --type=bool-or-str mergetool.meld.useAutoMerge
 		)
 		case "$meld_use_auto_merge_option" in
 		true | false)
-- 
2.31.1.527.g9b8f7de2547

Re: [PATCH 3/5] git-config: document --bool-or-str and --type=bool-or-str

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Document the new "bool-or-str" facility added in
> dbd8c09bfe (mergetool: allow auto-merge for meld to follow the
> vim-diff behavior, 2020-05-07).
>
> Unfortunately that commit also added a --bool-or-str option, even
> though we've preferred to deprecate that form ever since
> fb0dc3bac1 (builtin/config.c: support `--type=<type>` as preferred
> alias for `--<type>`, 2018-04-18).
>
> Since we've got it already let's document it along with the preferred
> --type=* form, and change our own code to use the --type=bool-or-str
> form over --bool-or-str.

It was a mistake to introduce a new option that is immediately
deprecated X-<.

Thanks for spotting and correcting.

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/git-config.txt | 3 +++
>  mergetools/meld              | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 4b4cc5c5e8..4ae9ef210c 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -187,6 +187,8 @@ Valid `<type>`'s include:
>    1073741824 upon input.
>  - 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
>    above.
> +- 'bool-or-str: canonicalize according to either 'bool' (as described
> +  above), or emit the value as-is.
>  - 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
>    `~user` to the home directory for the specified user. This specifier has no
>    effect when setting the value (but you can use `git config section.variable
> @@ -202,6 +204,7 @@ Valid `<type>`'s include:
>  --bool::
>  --int::
>  --bool-or-int::
> +--bool-or-str::
>  --path::
>  --expiry-date::
>    Historical options for selecting a type specifier. Prefer instead `--type`
> diff --git a/mergetools/meld b/mergetools/meld
> index aab4ebb935..8386e0574e 100644
> --- a/mergetools/meld
> +++ b/mergetools/meld
> @@ -59,7 +59,7 @@ check_meld_for_features () {
>  	if test -z "$meld_use_auto_merge_option"
>  	then
>  		meld_use_auto_merge_option=$(
> -			git config --bool-or-str mergetool.meld.useAutoMerge
> +			git config --type=bool-or-str mergetool.meld.useAutoMerge
>  		)
>  		case "$meld_use_auto_merge_option" in
>  		true | false)

[PATCH 4/5] config.c: add a "tristate" helper

[Ævar Arnfjörð Bjarmason]

Add "tristate" functions to go along with the "bool" functions and
migrate the common pattern of checking if something is "bool" or
"auto" in various places over to the new functions.

We also have e.g. "repo_config_get_bool" and
"config_error_nonbool". I'm not adding corresponding "tristate"
functions as they're not needed by anything, but we could add those in
the future if they are.

I'm not migrating over "core.abbrev" parsing as part of this
change. When "core.abbrev" was made optionally boolean in
a9ecaa06a7 (core.abbrev=no disables abbreviations, 2020-09-01) the
"die if empty" code added in g48d5014dd4 (config.abbrev: document the
new default that auto-scales, 2016-11-01) wasn't adjusted. It thus
behaves unlike all other "maybe bool" config variables.

I have a planned series to start adding some tests for "core.abbrev",
but AFAICT there's not even a test for "core.abbrev=true", and I'd
like to focus on thing that have no behavior change here, so let's
leave it for now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/log.c  | 13 +++++++------
 compat/mingw.c |  6 +++---
 config.c       | 16 ++++++++++++++++
 config.h       | 12 ++++++++++++
 http.c         |  5 +++--
 userdiff.c     |  6 ++----
 6 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 8acd285daf..0d945313d8 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -868,11 +868,12 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (!strcmp(var, "format.numbered")) {
-		if (value && !strcasecmp(value, "auto")) {
+		int tristate = git_config_tristate(var, value);
+		if (tristate == 2) {
 			auto_number = 1;
 			return 0;
 		}
-		numbered = git_config_bool(var, value);
+		numbered = tristate;
 		auto_number = auto_number && numbered;
 		return 0;
 	}
@@ -904,11 +905,11 @@ static int git_format_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "format.signaturefile"))
 		return git_config_pathname(&signature_file, var, value);
 	if (!strcmp(var, "format.coverletter")) {
-		if (value && !strcasecmp(value, "auto")) {
+		int tristate = git_config_tristate(var, value);
+		if (tristate == 2)
 			config_cover_letter = COVER_AUTO;
-			return 0;
-		}
-		config_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;
+		else
+			config_cover_letter = tristate ? COVER_ON : COVER_OFF;
 		return 0;
 	}
 	if (!strcmp(var, "format.outputdirectory"))
diff --git a/compat/mingw.c b/compat/mingw.c
index a43599841c..e6e85ae99a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -247,11 +247,11 @@ int mingw_core_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "core.restrictinheritedhandles")) {
-		if (value && !strcasecmp(value, "auto"))
+		int tristate = git_config_tristate(var, value);
+		if (tristate == 2)
 			core_restrict_inherited_handles = -1;
 		else
-			core_restrict_inherited_handles =
-				git_config_bool(var, value);
+			core_restrict_inherited_handles = tristate;
 		return 0;
 	}
 
diff --git a/config.c b/config.c
index fc28dbd97c..74d2b2c0df 100644
--- a/config.c
+++ b/config.c
@@ -1257,6 +1257,14 @@ int git_parse_maybe_bool(const char *value)
 	return -1;
 }
 
+int git_parse_maybe_tristate(const char *value)
+{
+	int v = git_parse_maybe_bool(value);
+	if (v < 0 && !strcasecmp(value, "auto"))
+		return 2;
+	return v;
+}
+
 int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 {
 	int v = git_parse_maybe_bool_text(value);
@@ -1268,6 +1276,14 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 	return git_config_int(name, value);
 }
 
+int git_config_tristate(const char *name, const char *value)
+{
+	int v = git_parse_maybe_tristate(value);
+	if (v < 0)
+		die(_("bad tristate config value '%s' for '%s'"), value, name);
+	return v;
+}
+
 int git_config_bool(const char *name, const char *value)
 {
 	int v = git_parse_maybe_bool(value);
diff --git a/config.h b/config.h
index 19a9adbaa9..c5129e4392 100644
--- a/config.h
+++ b/config.h
@@ -197,6 +197,12 @@ int git_parse_ulong(const char *, unsigned long *);
  */
 int git_parse_maybe_bool(const char *);
 
+/**
+ * Same as `git_parse_maybe_bool`, except that "auto" is recognized and
+ * will return "2".
+ */
+int git_parse_maybe_tristate(const char *);
+
 /**
  * Parse the string to an integer, including unit factors. Dies on error;
  * otherwise, returns the parsed result.
@@ -226,6 +232,12 @@ int git_config_bool_or_int(const char *, const char *, int *);
  */
 int git_config_bool(const char *, const char *);
 
+/**
+ * Like git_config_bool() except "auto" is also recognized and will
+ * return "2"
+ */
+int git_config_tristate(const char *, const char *);
+
 /**
  * Allocates and copies the value string into the `dest` parameter; if no
  * string is given, prints an error message and returns -1.
diff --git a/http.c b/http.c
index 406410f884..b54a232e90 100644
--- a/http.c
+++ b/http.c
@@ -406,10 +406,11 @@ static int http_options(const char *var, const char *value, void *cb)
 		return git_config_string(&user_agent, var, value);
 
 	if (!strcmp("http.emptyauth", var)) {
-		if (value && !strcmp("auto", value))
+		int tristate = git_config_tristate(var, value);
+		if (tristate == 2)
 			curl_empty_auth = -1;
 		else
-			curl_empty_auth = git_config_bool(var, value);
+			curl_empty_auth = tristate;
 		return 0;
 	}
 
diff --git a/userdiff.c b/userdiff.c
index 3f81a2261c..7ff010961f 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -277,10 +277,8 @@ static int parse_funcname(struct userdiff_funcname *f, const char *k,
 
 static int parse_tristate(int *b, const char *k, const char *v)
 {
-	if (v && !strcasecmp(v, "auto"))
-		*b = -1;
-	else
-		*b = git_config_bool(k, v);
+	int tristate = git_config_tristate(k, v);
+	*b = tristate == 2 ? -1 : tristate;
 	return 0;
 }
 
-- 
2.31.1.527.g9b8f7de2547

Re: [PATCH 4/5] config.c: add a "tristate" helper

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Add "tristate" functions to go along with the "bool" functions and
> migrate the common pattern of checking if something is "bool" or
> "auto" in various places over to the new functions.
>
> We also have e.g. "repo_config_get_bool" and
> "config_error_nonbool". I'm not adding corresponding "tristate"
> functions as they're not needed by anything, but we could add those in
> the future if they are.
>
> I'm not migrating over "core.abbrev" parsing as part of this
> change. When "core.abbrev" was made optionally boolean in
> a9ecaa06a7 (core.abbrev=no disables abbreviations, 2020-09-01) the
> "die if empty" code added in g48d5014dd4 (config.abbrev: document the
> new default that auto-scales, 2016-11-01) wasn't adjusted. It thus
> behaves unlike all other "maybe bool" config variables.
>
> I have a planned series to start adding some tests for "core.abbrev",
> but AFAICT there's not even a test for "core.abbrev=true", and I'd
> like to focus on thing that have no behavior change here, so let's
> leave it for now.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/log.c  | 13 +++++++------
>  compat/mingw.c |  6 +++---
>  config.c       | 16 ++++++++++++++++
>  config.h       | 12 ++++++++++++
>  http.c         |  5 +++--
>  userdiff.c     |  6 ++----
>  6 files changed, 43 insertions(+), 15 deletions(-)

> diff --git a/config.c b/config.c
> index fc28dbd97c..74d2b2c0df 100644
> --- a/config.c
> +++ b/config.c
> @@ -1257,6 +1257,14 @@ int git_parse_maybe_bool(const char *value)
>  	return -1;
>  }
>  
> +int git_parse_maybe_tristate(const char *value)
> +{
> +	int v = git_parse_maybe_bool(value);
> +	if (v < 0 && !strcasecmp(value, "auto"))
> +		return 2;
> +	return v;
> +}

This is not parse_mayb_bool_text(), so "1" and "-1" written in the
configuration file are "true", "0" is "false", like the "bool" case.

I wonder if written without an unnecessary extra variable, i.e.

	if (value && !strcasecmp(value, "auto"))
		return 2;
	return git_parse_maybe_bool(value);

is easier to follow, though, as it is quite clear that it is mostly
the same as maybe_bool and the only difference is when "auto" is
given.

> +int git_config_tristate(const char *name, const char *value)
> +{
> +	int v = git_parse_maybe_tristate(value);
> +	if (v < 0)
> +		die(_("bad tristate config value '%s' for '%s'"), value, name);
> +	return v;
> +}

OK.

> diff --git a/config.h b/config.h
> index 19a9adbaa9..c5129e4392 100644
> --- a/config.h
> +++ b/config.h
> @@ -197,6 +197,12 @@ int git_parse_ulong(const char *, unsigned long *);
>   */
>  int git_parse_maybe_bool(const char *);
>  
> +/**
> + * Same as `git_parse_maybe_bool`, except that "auto" is recognized and
> + * will return "2".
> + */
> +int git_parse_maybe_tristate(const char *);

A false being 0 and a true being 1 is understandable for readers
without symbolic constant, but "2" deserves to have a symbolic
constant, doesn't it?

> @@ -226,6 +232,12 @@ int git_config_bool_or_int(const char *, const char *, int *);
>   */
>  int git_config_bool(const char *, const char *);
>  
> +/**
> + * Like git_config_bool() except "auto" is also recognized and will
> + * return "2"
> + */
> +int git_config_tristate(const char *, const char *);

Likewise.

Thanks.

Re: [PATCH 4/5] config.c: add a "tristate" helper

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 08 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Add "tristate" functions to go along with the "bool" functions and
>> migrate the common pattern of checking if something is "bool" or
>> "auto" in various places over to the new functions.
>>
>> We also have e.g. "repo_config_get_bool" and
>> "config_error_nonbool". I'm not adding corresponding "tristate"
>> functions as they're not needed by anything, but we could add those in
>> the future if they are.
>>
>> I'm not migrating over "core.abbrev" parsing as part of this
>> change. When "core.abbrev" was made optionally boolean in
>> a9ecaa06a7 (core.abbrev=no disables abbreviations, 2020-09-01) the
>> "die if empty" code added in g48d5014dd4 (config.abbrev: document the
>> new default that auto-scales, 2016-11-01) wasn't adjusted. It thus
>> behaves unlike all other "maybe bool" config variables.
>>
>> I have a planned series to start adding some tests for "core.abbrev",
>> but AFAICT there's not even a test for "core.abbrev=true", and I'd
>> like to focus on thing that have no behavior change here, so let's
>> leave it for now.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  builtin/log.c  | 13 +++++++------
>>  compat/mingw.c |  6 +++---
>>  config.c       | 16 ++++++++++++++++
>>  config.h       | 12 ++++++++++++
>>  http.c         |  5 +++--
>>  userdiff.c     |  6 ++----
>>  6 files changed, 43 insertions(+), 15 deletions(-)
>
>> diff --git a/config.c b/config.c
>> index fc28dbd97c..74d2b2c0df 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1257,6 +1257,14 @@ int git_parse_maybe_bool(const char *value)
>>  	return -1;
>>  }
>>  
>> +int git_parse_maybe_tristate(const char *value)
>> +{
>> +	int v = git_parse_maybe_bool(value);
>> +	if (v < 0 && !strcasecmp(value, "auto"))
>> +		return 2;
>> +	return v;
>> +}
>
> This is not parse_mayb_bool_text(), so "1" and "-1" written in the
> configuration file are "true", "0" is "false", like the "bool" case.
>
> I wonder if written without an unnecessary extra variable, i.e.
>
> 	if (value && !strcasecmp(value, "auto"))
> 		return 2;
> 	return git_parse_maybe_bool(value);
>
> is easier to follow, though, as it is quite clear that it is mostly
> the same as maybe_bool and the only difference is when "auto" is
> given.

I guess it could be either way around, I think it makes more sense to
have git_parse_maybe_bool() handle as much as possible right from the
get-go, since it already handles NULL if we call it first we just need
to care about cases where it's looked at all the "is this bool-like"
permutations and decided to reject it.

>> +int git_config_tristate(const char *name, const char *value)
>> +{
>> +	int v = git_parse_maybe_tristate(value);
>> +	if (v < 0)
>> +		die(_("bad tristate config value '%s' for '%s'"), value, name);
>> +	return v;
>> +}
>
> OK.
>
>> diff --git a/config.h b/config.h
>> index 19a9adbaa9..c5129e4392 100644
>> --- a/config.h
>> +++ b/config.h
>> @@ -197,6 +197,12 @@ int git_parse_ulong(const char *, unsigned long *);
>>   */
>>  int git_parse_maybe_bool(const char *);
>>  
>> +/**
>> + * Same as `git_parse_maybe_bool`, except that "auto" is recognized and
>> + * will return "2".
>> + */
>> +int git_parse_maybe_tristate(const char *);
>
> A false being 0 and a true being 1 is understandable for readers
> without symbolic constant, but "2" deserves to have a symbolic
> constant, doesn't it?
>
>> @@ -226,6 +232,12 @@ int git_config_bool_or_int(const char *, const char *, int *);
>>   */
>>  int git_config_bool(const char *, const char *);
>>  
>> +/**
>> + * Like git_config_bool() except "auto" is also recognized and will
>> + * return "2"
>> + */
>> +int git_config_tristate(const char *, const char *);
>
> Likewise.

I'd prefer to just make these "enum", which means we'll have the aid of
the compiler in checking all the callsites, as in the patch-on-top
(which I can squash appropriately, need to update the doc comments
though) at the end of this E-Mail.

It's a bit of boilerplate, and it's unfortunate that subset/supersets of
enums aren't better supported in C (so e.g. you could have different
types share some of the same label names), but I think being able to
look at any given use and know the compiler is checking whether the case
statements (there's no "default") are exhaustively enumerated is worth
it.

But I wasn't sure you'd prefet that, especially (and maybe I read too
much into it) with me replacing -1 with OBJ_BAD in another topic, as
GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_BAD does here.

In this case most of the callsites don't need to deal with the "BAD"
value, but for other things in config.c and this sort of code in general
if we're going to insist on "v < 0" over explicit labels the benefit of
using enum/switch pretty much goes away.

I mean, we could not do the enum/switch/case implementation and just
have a "#define" to give "2" a pretty name, but at that point anything
beyond that is pretty pointless, i.e. we can just make the function
return an "int" if we're not hoping to have the compiler check the enum
values.

diff --git a/builtin/config.c b/builtin/config.c
index 039a4f0961..a5d7efc8bc 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -268,11 +268,18 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 			else
 				strbuf_addstr(buf, v ? "true" : "false");
 		} else if (type == TYPE_BOOL_OR_AUTO) {
-			int v = git_config_tristate(key_, value_);
-			if (v == 2)
+			enum git_config_type_bool_or_auto v = git_config_tristate(key_, value_);
+			switch (v) {
+			case GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE:
+				strbuf_addstr(buf, "false");
+				break;
+			case GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE:
+				strbuf_addstr(buf, "true");
+				break;
+			case GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO:
 				strbuf_addstr(buf, "auto");
-			else
-				strbuf_addstr(buf, v ? "true" : "false");
+				break;
+			}
 		} else if (type == TYPE_PATH) {
 			const char *v;
 			if (git_config_pathname(&v, key_, value_) < 0)
@@ -446,13 +453,17 @@ static char *normalize_value(const char *key, const char *value)
 			return xstrdup(v ? "true" : "false");
 	}
 	if (type == TYPE_BOOL_OR_AUTO) {
-		int v = git_parse_maybe_tristate(value);
-		if (v < 0)
+		enum git_config_type_maybe_bool_or_auto v = git_parse_maybe_tristate(value); 
+		switch (v) {
+		case GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_BAD:
 			return xstrdup(value);
-		else if (v == 2)
-			xstrdup("auto");
-		else
-			return xstrdup(v ? "true" : "false");
+		case GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_FALSE:
+			return xstrdup("false");
+		case GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_TRUE:
+			return xstrdup("true");
+		case GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_AUTO:
+			return xstrdup("auto");
+		}
 	}
 	if (type == TYPE_COLOR) {
 		char v[COLOR_MAXLEN];
diff --git a/builtin/log.c b/builtin/log.c
index 0d945313d8..f363c841a7 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -868,14 +868,17 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (!strcmp(var, "format.numbered")) {
-		int tristate = git_config_tristate(var, value);
-		if (tristate == 2) {
+		enum git_config_type_bool_or_auto v = git_config_tristate(var, value);
+		switch (v) {
+		case GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE:
 			auto_number = 1;
 			return 0;
+		case GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE:
+		case GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO:
+			numbered = v;
+			auto_number = auto_number && numbered;
+			return 0;
 		}
-		numbered = tristate;
-		auto_number = auto_number && numbered;
-		return 0;
 	}
 	if (!strcmp(var, "format.attach")) {
 		if (value && *value)
@@ -905,12 +908,18 @@ static int git_format_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "format.signaturefile"))
 		return git_config_pathname(&signature_file, var, value);
 	if (!strcmp(var, "format.coverletter")) {
-		int tristate = git_config_tristate(var, value);
-		if (tristate == 2)
+		enum git_config_type_bool_or_auto v = git_config_tristate(var, value);
+		switch (v) {
+		case GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE:
+			config_cover_letter = COVER_OFF;
+			return 0;
+		case GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE:
+			config_cover_letter = COVER_ON;
+			return 0;
+		case GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO:
 			config_cover_letter = COVER_AUTO;
-		else
-			config_cover_letter = tristate ? COVER_ON : COVER_OFF;
-		return 0;
+			return 0;
+		}
 	}
 	if (!strcmp(var, "format.outputdirectory"))
 		return git_config_string(&config_output_directory, var, value);
diff --git a/compat/mingw.c b/compat/mingw.c
index e6e85ae99a..b76ccc0a15 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -247,12 +247,16 @@ int mingw_core_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "core.restrictinheritedhandles")) {
-		int tristate = git_config_tristate(var, value);
-		if (tristate == 2)
+		enum git_config_type_bool_or_auto v = git_config_tristate(var, value);
+		switch (v) {
+		case GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE:
+		case GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE:
+			core_restrict_inherited_handles = v;
+			return 0;
+		case GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO:
 			core_restrict_inherited_handles = -1;
-		else
-			core_restrict_inherited_handles = tristate;
-		return 0;
+			return 0;
+		}
 	}
 
 	return 0;
diff --git a/config.c b/config.c
index 74d2b2c0df..3375895b80 100644
--- a/config.c
+++ b/config.c
@@ -1257,11 +1257,11 @@ int git_parse_maybe_bool(const char *value)
 	return -1;
 }
 
-int git_parse_maybe_tristate(const char *value)
+enum git_config_type_maybe_bool_or_auto git_parse_maybe_tristate(const char *value)
 {
 	int v = git_parse_maybe_bool(value);
 	if (v < 0 && !strcasecmp(value, "auto"))
-		return 2;
+		return GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO;
 	return v;
 }
 
@@ -1276,9 +1276,10 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 	return git_config_int(name, value);
 }
 
-int git_config_tristate(const char *name, const char *value)
+enum git_config_type_bool_or_auto git_config_tristate(const char *name,
+						      const char *value)
 {
-	int v = git_parse_maybe_tristate(value);
+	enum git_config_type_maybe_bool_or_auto v = git_parse_maybe_tristate(value);
 	if (v < 0)
 		die(_("bad tristate config value '%s' for '%s'"), value, name);
 	return v;
diff --git a/config.h b/config.h
index c5129e4392..70684ecb3c 100644
--- a/config.h
+++ b/config.h
@@ -201,7 +201,14 @@ int git_parse_maybe_bool(const char *);
  * Same as `git_parse_maybe_bool`, except that "auto" is recognized and
  * will return "2".
  */
-int git_parse_maybe_tristate(const char *);
+enum git_config_type_maybe_bool_or_auto {
+	GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_BAD   = -1,
+	GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_FALSE = 0,
+	GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_TRUE  = 1,
+	GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_AUTO  = 2,
+};
+	
+enum git_config_type_maybe_bool_or_auto git_parse_maybe_tristate(const char *);
 
 /**
  * Parse the string to an integer, including unit factors. Dies on error;
@@ -236,7 +243,13 @@ int git_config_bool(const char *, const char *);
  * Like git_config_bool() except "auto" is also recognized and will
  * return "2"
  */
-int git_config_tristate(const char *, const char *);
+enum git_config_type_bool_or_auto {
+	GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE = GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_FALSE,
+	GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE  = GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_TRUE,
+	GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO  = GIT_CONFIG_TYPE_MAYBE_BOOL_OR_AUTO_AUTO,
+};
+enum git_config_type_bool_or_auto git_config_tristate(const char *name,
+						      const char *value);
 
 /**
  * Allocates and copies the value string into the `dest` parameter; if no
diff --git a/http.c b/http.c
index b54a232e90..6dd6191517 100644
--- a/http.c
+++ b/http.c
@@ -406,12 +406,16 @@ static int http_options(const char *var, const char *value, void *cb)
 		return git_config_string(&user_agent, var, value);
 
 	if (!strcmp("http.emptyauth", var)) {
-		int tristate = git_config_tristate(var, value);
-		if (tristate == 2)
+		enum git_config_type_bool_or_auto v = git_config_tristate(var, value);
+		switch (v) {
+		case GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE:
+		case GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE:
+			curl_empty_auth = v;
+			return 0;
+		case GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO:
 			curl_empty_auth = -1;
-		else
-			curl_empty_auth = tristate;
-		return 0;
+			return 0;
+		}
 	}
 
 	if (!strcmp("http.delegation", var)) {
diff --git a/userdiff.c b/userdiff.c
index 7ff010961f..fe001563c3 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -277,8 +277,16 @@ static int parse_funcname(struct userdiff_funcname *f, const char *k,
 
 static int parse_tristate(int *b, const char *k, const char *v)
 {
-	int tristate = git_config_tristate(k, v);
-	*b = tristate == 2 ? -1 : tristate;
+	enum git_config_type_bool_or_auto tristate = git_config_tristate(k, v);
+	switch (tristate) {
+	case GIT_CONFIG_TYPE_BOOL_OR_AUTO_FALSE:
+	case GIT_CONFIG_TYPE_BOOL_OR_AUTO_TRUE:
+		*b = tristate;
+		break;
+	case GIT_CONFIG_TYPE_BOOL_OR_AUTO_AUTO:
+		*b = -1;
+		break;
+	}
 	return 0;
 }
 

Re: [PATCH 4/5] config.c: add a "tristate" helper

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I'd prefer to just make these "enum", which means we'll have the aid of
> the compiler in checking all the callsites, as in the patch-on-top
> (which I can squash appropriately, need to update the doc comments
> though) at the end of this E-Mail.

I think enum is oversold by some people (not me).  C Compilers won't
do much when you use them interchangeably with int, simply because
they are designed to be used that way, no?

If existing code used 0 as false and 1 as true, and it learns an
"auto" value with a new definition,

    #define TRISTATE_AUTO 2

without TRISTATE_{TRUE,FALSE} defined to 0 and 1, that would be a
good place to stop.  I'd be quite happy with that.

Re: [PATCH 4/5] config.c: add a "tristate" helper

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 09 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> I'd prefer to just make these "enum", which means we'll have the aid of
>> the compiler in checking all the callsites, as in the patch-on-top
>> (which I can squash appropriately, need to update the doc comments
>> though) at the end of this E-Mail.
>
> I think enum is oversold by some people (not me).  C Compilers won't
> do much when you use them interchangeably with int, simply because
> they are designed to be used that way, no?

They are just ints, not a seperate type like in C++.

> If existing code used 0 as false and 1 as true, and it learns an
> "auto" value with a new definition,
>
>     #define TRISTATE_AUTO 2
>
> without TRISTATE_{TRUE,FALSE} defined to 0 and 1, that would be a
> good place to stop.  I'd be quite happy with that.

The benefit in this case is to human readers of the code who know
they're being helped by the enum checking in "case" arms.

So you can immediately look at code like:

    enum foo x = git_config_foo();
    switch (x)
    [...]

And know without having to jump to the definition of "git_config_foo()"
that all the return values are being handled.

Just to clarify, do you have a dislike of the sort of code in
http://lore.kernel.org/git/875z0wicmp.fsf@evledraar.gmail.com for all
uses in the codebase, or in this case because the diff of adapting
existing code is larger as a result?

I think if we're not going to use enum/switch for this and similar
things in config.c in the future it makes sense to pass a pointer to a
"is_auto" parameter to these new tristate() functions, similar to
e.g. the existing git_config_bool_or_int().

Re: [PATCH 4/5] config.c: add a "tristate" helper

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> The benefit in this case is to human readers of the code who know
> they're being helped by the enum checking in "case" arms.

Well, do we have tristate that is handled with switch/case?  And
more importantly, do tristates benefit from getting handled with
switch/case?

The one I cited as an existing example to decide that we should
favour "special case 'auto' and then let it fall through to the
normal bool" is in userdiff.c

        static int parse_tristate(int *b, const char *k, const char *v)
        {
                if (v && !strcasecmp(v, "auto"))
                        *b = -1;
                else
                        *b = git_config_bool(k, v);
                return 0;
        }

and the caller uses it to set -1/0/1 in driver->binary member.

And the member is often used like so:

	... else if (userdiff->binary != -1) {
		is_binary = userdiff->binary;
	} else {
		is_binary = auto_detect_based_on_contents(...);
	}
	if (is_binary) {
		... do the binary thing ...
	}

This is because "auto" is the only value among the three that needs
"preprocessing" before it is turned into a concrete yes/no that is
usable in the downstream code.  So with AUTO being the only spelled
out value, a construct like this:

	if ((driver->binary != TRISTATE_AUTO) 
	    ? driver->binary : auto_detect())
		...; /* do the binary thing */
	else
		...; /* do the non-binary thing */

would be sufficient to help human readers.  You do not need enum
to help, and you do not need switch/case.

You could use switch/case with enum if you really wanted to, but

	switch (temp = driver->binary) {
	case TRISTATE_AUTO:
		temp = auto_detect();
                break; /* ????? */
	case TRISTATE_FALSE:
		/* do the non-binary thing */
		...;
		break;
	case TRISTATE_TRUE:
		/* do the binary thing */
		...;
		break;
	}

would not be a useful construct for the "auto" use case, where
tristate is used to mean "we cannot decide at the configuration
time, as the appropriate value depends on other factors that are
available when it actually is used, e.g. the contents in the buffer,
if fd=1 is connected to the terminal, etc."

If you really wanted to, you could still use switch/case for such a
use case, perhaps like this:

	switch (temp = driver->binary) {
	case TRISTATE_AUTO:
		temp = auto_detect();
		/* fallthrough */
	default:
		if (!temp) { /* TRISTATE_FALSE */
			/* do the non-binary thing */
			...;
		} else { /* TRISTATE_TRUE */
			/* do the binary thing */
			...;
		}
		break;
	}

and switch may help making sure that all enum values are handled,
but I do not see a value in it.  The earlier code that used "if
auto, run autodetect, otherwise use the value as is" as the
condition for an if/else statement would be far easier to follow,
and equally safe.

> ... in config.c in the future it makes sense to pass a pointer to a
> "is_auto" parameter to these new tristate() functions, similar to
> e.g. the existing git_config_bool_or_int().

I am not sure what you are trying to gain by introducing is_auto
here.

For bool_or_int(), is_bool pointer makes perfect sense, because the
value spelled as "true" cannot be anything but bool, but "1" can be
either boolean true or an integer.  An extra is_bool bit can be used
by callers that care if the user said "true/yes" or "1" and want to
behave differently, when the configuration can take either bool or
integer.  For example, if you originally have a "do you want this
operation to be verbose?" yes/no variable, and later extended it to
add verbosity level, "yes/true" might mean "yes, please use the
default verbosity level", while "1", "2", ... would mean "yes, and
please set the verbosity level to this number".  And the default
verbosity level may not be "1", so the distinction between "yes" and
"1" does matter.

I do not see such a disambiguation need for tristate parsing, so the
immense usefulness of is_bool is not a good analogy to draw value for
the proposed is_auto from.

Thanks.

Re: [PATCH 4/5] config.c: add a "tristate" helper

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>> +int git_parse_maybe_tristate(const char *value)
>>> +{
>>> +	int v = git_parse_maybe_bool(value);
>>> +	if (v < 0 && !strcasecmp(value, "auto"))
>>> +		return 2;
>>> +	return v;
>>> +}
>>
>> This is not parse_maybe_bool_text(), so "1" and "-1" written in the
>> configuration file are "true", "0" is "false", like the "bool" case.
>>
>> I wonder if written without an unnecessary extra variable, i.e.
>>
>> 	if (value && !strcasecmp(value, "auto"))
>> 		return 2;
>> 	return git_parse_maybe_bool(value);
>>
>> is easier to follow, though, as it is quite clear that it is mostly
>> the same as maybe_bool and the only difference is when "auto" is
>> given.
>
> I guess it could be either way around,...

Having seen another example in the current code recently,

 static int parse_tristate(int *b, const char *k, const char *v)
 {
-	if (v && !strcasecmp(v, "auto"))
-		*b = -1;
-	else
-		*b = git_config_bool(k, v);

I upgrade my earlier "I wonder" to "I do think that".  Let's swap
the order so that it is clear that we are special-casing "auto".

Re: [PATCH 4/5] config.c: add a "tristate" helper

[Jeff King]

On Thu, Apr 08, 2021 at 11:33:13AM -0700, Junio C Hamano wrote:

> > +/**
> > + * Same as `git_parse_maybe_bool`, except that "auto" is recognized and
> > + * will return "2".
> > + */
> > +int git_parse_maybe_tristate(const char *);
> 
> A false being 0 and a true being 1 is understandable for readers
> without symbolic constant, but "2" deserves to have a symbolic
> constant, doesn't it?

I had the same thought.

This "tristate" really has four outcomes: true, false, auto, or error.
Since the caller has to then check for error or for "auto" themselves,
it feels like it is not actually making their lives any easier. I.e.,
without it:

  if (value && !strcmp(value, "auto"))
	do_auto();
  else {
	int b = git_parse_maybe_bool(value);
	if (b < 0)
		do_error();
	else if (b)
		do_true();
	else
		do_false();
  }

but with it:

  b = git_parse_maybe_tristate(value);
  if (b < 0)
        do_error();
  else if (b == 2)
        do_auto();
  else if (b)
        do_true();
  else
        do_false();

which is the same thing, swapping string comparison for a numeric one.

And a caller of git-config has the same thing: it still has to check for
"auto" in the output it gets (which it could just do via bool-or-str).

It seems like it would be more convenient if you could pass it a bool
value to turn the "auto" into. E.g., if you could do:

  b = git_parse_maybe_tristate(value, 1); /* default to "1" */
  if (b < 0)
          do_error();
  if (b)
          do_true(); /* true, or maybe "auto" */
  else
          do_false();

I dunno. That would probably be hard to represent via "git config
--type". And some callers probably do care about "auto" versus "true".

It also feels funny calling this "tristate". Aren't there other
tristates we could have besides "auto"? The command-line option is
bool-or-auto, which is descriptive. Should this use a similar name?

-Peff

Re: [PATCH 4/5] config.c: add a "tristate" helper

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It seems like it would be more convenient if you could pass it a bool
> value to turn the "auto" into. E.g., if you could do:
>
>   b = git_parse_maybe_tristate(value, 1); /* default to "1" */
>   if (b < 0)
>           do_error();
>   if (b)
>           do_true(); /* true, or maybe "auto" */
>   else
>           do_false();
>
> I dunno. That would probably be hard to represent via "git config
> --type". And some callers probably do care about "auto" versus "true".

It would work well for codepaths where computing the default value
is cheap (or even possible).

I think the point of using "auto" is to delay the decision as late
as possible.  E.g. in-core parsed config and attribute may still
want to stay "auto", until we actually get our hands on the blob
contents to see if it is binary, until we know how heavily loaded
the system is, until we know isatty(1), etc.  Some are cheap to
compute in advance, some are expensive and impossible until we meet
the data.

So I still think the canonical use pattern for the "auto" thing is

	is_frotz = git_parse_bool_or_auto(value);

	... arbitrary number of things can happen here
	... the above may even be done in a git_config()
	... callback, and is_frotz may not even be used.

	if (is_frotz == AUTO)
		is_frotz = auto_detect_frotz();

	if (is_frotz)
		...; /* do the frotz thing */
	else
		...; /* do the non-frotz thing */

> It also feels funny calling this "tristate". Aren't there other
> tristates we could have besides "auto"? The command-line option is
> bool-or-auto, which is descriptive. Should this use a similar name?

I like that bool-or-auto name very much.

Re: [PATCH 4/5] config.c: add a "tristate" helper

[Jeff King]

On Fri, Apr 09, 2021 at 03:11:10PM -0700, Junio C Hamano wrote:

> > I dunno. That would probably be hard to represent via "git config
> > --type". And some callers probably do care about "auto" versus "true".
> 
> It would work well for codepaths where computing the default value
> is cheap (or even possible).
> 
> I think the point of using "auto" is to delay the decision as late
> as possible.  E.g. in-core parsed config and attribute may still
> want to stay "auto", until we actually get our hands on the blob
> contents to see if it is binary, until we know how heavily loaded
> the system is, until we know isatty(1), etc.  Some are cheap to
> compute in advance, some are expensive and impossible until we meet
> the data.

Hmm, yeah, that's the "do care about auto versus true" thing.

> So I still think the canonical use pattern for the "auto" thing is
> 
> 	is_frotz = git_parse_bool_or_auto(value);
> 
> 	... arbitrary number of things can happen here
> 	... the above may even be done in a git_config()
> 	... callback, and is_frotz may not even be used.
> 
> 	if (is_frotz == AUTO)
> 		is_frotz = auto_detect_frotz();
> 
> 	if (is_frotz)
> 		...; /* do the frotz thing */
> 	else
> 		...; /* do the non-frotz thing */

That makes sense. Usually we represent "undecided" in such a tristate
with -1, so something that returned -1/0/1 would feel very natural to me
(and probably wouldn't need symbolic constants even). But -1 is also
error.

So a function like:

  int git_parse_tristate(const char *value, int *out);

which returned success/error via its return value, and put the value
into "out" would feel pretty natural to me.

I dunno. I admit I don't care _that_ much, but somehow Ævar's series
have a way of sniping me into responding anyway. :)

-Peff

Re: [PATCH 4/5] config.c: add a "tristate" helper

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So a function like:
>
>   int git_parse_tristate(const char *value, int *out);
>
> which returned success/error via its return value, and put the value
> into "out" would feel pretty natural to me.

Yeah, with s/tristate/bool-or-auto/, and if we do this throughout
the types, that would be ideal.  FWIW git_parse_ulong() and friends
for sized numerics already follow that pattern, but helpers for
boolean like git_parse_maybe_bool() don't, which is unfortunate.

[PATCH 5/5] config: add --type=bool-or-auto switch

[Ævar Arnfjörð Bjarmason]

Now that we're using git_config_tristate() internally let's expose it
via "git config" like we do "bool", "int" etc for completeness, and so
that we can easily test it.

Unlike the --type=bool-or-str option added in dbd8c09bfe (mergetool:
allow auto-merge for meld to follow the vim-diff behavior, 2020-05-07)
we don't have or anticipate any in-tree user of this except the tests.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-config.txt |  4 +++
 builtin/config.c             | 19 ++++++++++++++
 t/t1300-config.sh            | 49 ++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4ae9ef210c..1af8222e82 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -189,6 +189,10 @@ Valid `<type>`'s include:
   above.
 - 'bool-or-str: canonicalize according to either 'bool' (as described
   above), or emit the value as-is.
+- 'bool-or-auto: canonicalize according to either 'bool', as described
+  above, or whether the value is "auto". This is used by various
+  "tristate" variables such as `core.restrictInheritedHandles`,
+  `format.numbered` etc.
 - 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
   `~user` to the home directory for the specified user. This specifier has no
   effect when setting the value (but you can use `git config section.variable
diff --git a/builtin/config.c b/builtin/config.c
index f71fa39b38..039a4f0961 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -68,6 +68,7 @@ static int fixed_value;
 #define TYPE_EXPIRY_DATE	5
 #define TYPE_COLOR		6
 #define TYPE_BOOL_OR_STR	7
+#define TYPE_BOOL_OR_AUTO	8
 
 #define OPT_CALLBACK_VALUE(s, l, v, h, i) \
 	{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
@@ -99,6 +100,8 @@ static int option_parse_type(const struct option *opt, const char *arg,
 			new_type = TYPE_BOOL_OR_INT;
 		else if (!strcmp(arg, "bool-or-str"))
 			new_type = TYPE_BOOL_OR_STR;
+		else if (!strcmp(arg, "bool-or-auto"))
+			new_type = TYPE_BOOL_OR_AUTO;
 		else if (!strcmp(arg, "path"))
 			new_type = TYPE_PATH;
 		else if (!strcmp(arg, "expiry-date"))
@@ -156,6 +159,7 @@ static struct option builtin_config_options[] = {
 	OPT_CALLBACK_VALUE(0, "int", &type, N_("value is decimal number"), TYPE_INT),
 	OPT_CALLBACK_VALUE(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
 	OPT_CALLBACK_VALUE(0, "bool-or-str", &type, N_("value is --bool or string"), TYPE_BOOL_OR_STR),
+	/* No bool-or-auto! The --<type> form is deprecated in favor of --type=<what> */
 	OPT_CALLBACK_VALUE(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
 	OPT_CALLBACK_VALUE(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
 	OPT_GROUP(N_("Other")),
@@ -263,6 +267,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 				strbuf_addstr(buf, value_);
 			else
 				strbuf_addstr(buf, v ? "true" : "false");
+		} else if (type == TYPE_BOOL_OR_AUTO) {
+			int v = git_config_tristate(key_, value_);
+			if (v == 2)
+				strbuf_addstr(buf, "auto");
+			else
+				strbuf_addstr(buf, v ? "true" : "false");
 		} else if (type == TYPE_PATH) {
 			const char *v;
 			if (git_config_pathname(&v, key_, value_) < 0)
@@ -435,6 +445,15 @@ static char *normalize_value(const char *key, const char *value)
 		else
 			return xstrdup(v ? "true" : "false");
 	}
+	if (type == TYPE_BOOL_OR_AUTO) {
+		int v = git_parse_maybe_tristate(value);
+		if (v < 0)
+			return xstrdup(value);
+		else if (v == 2)
+			xstrdup("auto");
+		else
+			return xstrdup(v ? "true" : "false");
+	}
 	if (type == TYPE_COLOR) {
 		char v[COLOR_MAXLEN];
 		if (git_config_color(v, key, value))
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index a002ec5644..952d9e9ed9 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -874,6 +874,55 @@ test_expect_success 'get --bool-or-str' '
 	test_cmp expect actual
 '
 
+test_expect_success 'there is no --bool-or-auto, --<type> is deprecated in favor of --type=<type>' '
+	test_expect_code 129 git config --bool-or-auto
+'
+
+test_expect_success 'get --type=bool-or-auto' '
+	cat >.git/config <<-\EOF &&
+	[bool]
+	true1
+	true2 = true
+	false = false
+	[int]
+	int1 = 0
+	int2 = 1
+	int3 = -1
+	[string]
+	string1 = hello
+	string2 = there you
+	[auto]
+	auto1 = auto
+	auto2 = AUTO
+	[bad-auto]
+	bad-auto1 = AUTOMATIC
+	EOF
+	cat >expect <<-\EOF &&
+	true
+	true
+	false
+	false
+	true
+	true
+	auto
+	auto
+	EOF
+	{
+		git config --type=bool-or-auto bool.true1 &&
+		git config --type=bool-or-auto bool.true2 &&
+		git config --type=bool-or-auto bool.false &&
+		git config --type=bool-or-auto int.int1 &&
+		git config --type=bool-or-auto int.int2 &&
+		git config --type=bool-or-auto int.int3 &&
+		git config --type=bool-or-auto auto.auto1 &&
+		git config --type=bool-or-auto auto.auto2
+	} >actual &&
+	test_cmp expect actual &&
+
+	test_must_fail git config --type=bool-or-auto --get bad-auto.bad-auto1 2>err &&
+	grep "bad tristate config value" err
+'
+
 cat >expect <<\EOF
 [bool]
 	true1 = true
-- 
2.31.1.527.g9b8f7de2547

Re: [PATCH 5/5] config: add --type=bool-or-auto switch

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Now that we're using git_config_tristate() internally let's expose it
> via "git config" like we do "bool", "int" etc for completeness, and so
> that we can easily test it.
>
> Unlike the --type=bool-or-str option added in dbd8c09bfe (mergetool:
> allow auto-merge for meld to follow the vim-diff behavior, 2020-05-07)
> we don't have or anticipate any in-tree user of this except the tests.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

OK.


> @@ -263,6 +267,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
>  				strbuf_addstr(buf, value_);
>  			else
>  				strbuf_addstr(buf, v ? "true" : "false");
> +		} else if (type == TYPE_BOOL_OR_AUTO) {
> +			int v = git_config_tristate(key_, value_);
> +			if (v == 2)
> +				strbuf_addstr(buf, "auto");
> +			else
> +				strbuf_addstr(buf, v ? "true" : "false");

Makes sense.

> +test_expect_success 'there is no --bool-or-auto, --<type> is deprecated in favor of --type=<type>' '
> +	test_expect_code 129 git config --bool-or-auto
> +'
> +
> +test_expect_success 'get --type=bool-or-auto' '
> +	cat >.git/config <<-\EOF &&
> +	[bool]
> +	true1
> +	true2 = true
> +	false = false
> +	[int]
> +	int1 = 0
> +	int2 = 1
> +	int3 = -1
> +	[string]
> +	string1 = hello
> +	string2 = there you
> +	[auto]
> +	auto1 = auto
> +	auto2 = AUTO
> +	[bad-auto]
> +	bad-auto1 = AUTOMATIC
> +	EOF
> +	cat >expect <<-\EOF &&
> +	true
> +	true
> +	false
> +	false
> +	true
> +	true
> +	auto
> +	auto
> +	EOF

Almost the same comment applies to the expected output as the
earlier patch.

Other than that, (and adjusting for 2 that should be turned into
symbolic constant in an earlier step in a reroll), this step looks
quite sensible.

Thanks.

> +	{
> +		git config --type=bool-or-auto bool.true1 &&
> +		git config --type=bool-or-auto bool.true2 &&
> +		git config --type=bool-or-auto bool.false &&
> +		git config --type=bool-or-auto int.int1 &&
> +		git config --type=bool-or-auto int.int2 &&
> +		git config --type=bool-or-auto int.int3 &&
> +		git config --type=bool-or-auto auto.auto1 &&
> +		git config --type=bool-or-auto auto.auto2
> +	} >actual &&
> +	test_cmp expect actual &&
> +
> +	test_must_fail git config --type=bool-or-auto --get bad-auto.bad-auto1 2>err &&
> +	grep "bad tristate config value" err
> +'
> +
>  cat >expect <<\EOF
>  [bool]
>  	true1 = true