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

From: Jeff King <peff@peff.net>
Date: Apr 9, 2021, 20:05 UTC
Message-ID: <YHCzhcBkqdeEMrWA@coredump.intra.peff.net>
In-Reply-To: <xmqqa6q8tymu.fsf@gitster.g>

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

Show 9 quoted lines

> > +/**
> > + * 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