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

[Junio C Hamano <gitster@pobox.com>]

Æ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.