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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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