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

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

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.

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.