git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, "Lin Sun" <lin.sun@zoom.us>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"David Aguilar" <davvid@gmail.com>, "Jeff King" <peff@peff.net>
Subject: Re: [PATCH 4/5] config.c: add a "tristate" helper
Date: Fri, 09 Apr 2021 05:53:05 -0700	[thread overview]
Message-ID: <xmqq35vzoc0e.fsf@gitster.g> (raw)
In-Reply-To: <87o8eogs2r.fsf@evledraar.gmail.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.

  reply	other threads:[~2021-04-09 12:53 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-08 13:34 [PATCH 0/5] config: support --type=bool-or-auto for "tristate" parsing Ævar Arnfjörð Bjarmason
2021-04-08 13:34 ` [PATCH 1/5] config.c: add a comment about why value=NULL is true Ævar Arnfjörð Bjarmason
2021-04-08 18:10   ` Junio C Hamano
2021-04-08 13:34 ` [PATCH 2/5] config tests: test for --bool-or-str Ævar Arnfjörð Bjarmason
2021-04-08 18:21   ` Junio C Hamano
2021-04-08 23:11     ` Ævar Arnfjörð Bjarmason
2021-04-08 13:34 ` [PATCH 3/5] git-config: document --bool-or-str and --type=bool-or-str Ævar Arnfjörð Bjarmason
2021-04-08 18:22   ` Junio C Hamano
2021-04-08 13:34 ` [PATCH 4/5] config.c: add a "tristate" helper Ævar Arnfjörð Bjarmason
2021-04-08 18:33   ` Junio C Hamano
2021-04-08 23:23     ` Ævar Arnfjörð Bjarmason
2021-04-08 23:51       ` Junio C Hamano
2021-04-09  1:33         ` Ævar Arnfjörð Bjarmason
2021-04-09 12:53           ` Junio C Hamano [this message]
2021-04-08 23:54       ` Junio C Hamano
2021-04-09 20:05     ` Jeff King
2021-04-09 22:11       ` Junio C Hamano
2021-04-10  1:23         ` Jeff King
2021-04-10  1:43           ` Junio C Hamano
2021-04-08 13:34 ` [PATCH 5/5] config: add --type=bool-or-auto switch Ævar Arnfjörð Bjarmason
2021-04-08 18:36   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq35vzoc0e.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=congdanhqx@gmail.com \
    --cc=davvid@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=lin.sun@zoom.us \
    --cc=peff@peff.net \
    --subject='Re: [PATCH 4/5] config.c: add a "tristate" helper' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).