Re: [PATCH v5 0/9] fetch: introduce machine-parseable output

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 11, 2023, 17:24 UTC
Message-ID: <645d24e37eadb_26011a29488@chronos.notmuch>
In-Reply-To: <ZFzMDCK8rprq6qUs@ncase>

Patrick Steinhardt wrote:

> On Wed, May 10, 2023 at 11:05:19AM -0700, Junio C Hamano wrote:

Show 15 quoted lines

> > OK, this time the familiar "prepare a variable to its default, let
> > config callback to overwrite it by reading configuration variables,
> > and then let the command line option override it" is used and the
> > result easy to follow.  I do not think .display_format is never
> > assigned DISPLAY_FORMAT_UNKNOWN with this change, so [6/9] could
> > lose the value from the enum, I think.  The defensive switch
> > statement that has BUG() to notice an erroneous caller that pass
> > values other than DISPLAY_FORMAT_{FULL,COMPACT} is still a good
> > idea.
> 
> Ah, right, `DISPLAY_FORMAT_UNKNOWN` isn't really needed anymore. I think
> it's still good to have valid values of the enum start with `1` so that
> it becomes easier to detect cases where we accidentally use a default
> initialized variable. But that can be achieved without giving the
> default value an explicit name.

I think it's standard to have an element like that. It could be UNKNOWN, NULL, INVALID, or even DEFAULT.

For example, you could have code like this:

 if (format == DISPLAY_FORMAT_DEFAULT)
        format = DISPLAY_FORMAT_FULL;

This would distinguish cases in which the user did not specify a display format and we choose a default, from those where the user explicitely chose the format that happens to be the default.

-- 
Felipe Contreras