Re: [PATCH 04/18] Offer a function to demote fsck errors to warnings

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5711 bytes --]

Hi Junio,

On Mon, 22 Dec 2014, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > For example, try to spot the error here:
> >
> > 	...
> > 	F(ALMOST_HAPPY, INFO) \
> > 	F(CANNOT_RECOVER, ERROR) \
> > 	F(COFFEE_IS_EMPTY, WARN) \
> > 	F(JUST_BEING_CHATTY, INFO) \
> > 	F(LIFE_IS_GOOD, INFO) \
> > 	F(MISSING_SOMETHING_VITAL, FATAL_ERROR) \
> > 	F(NEED_TO_SLEEP, WARN) \
> > 	F(SOMETHING_WENT_WRONG, ERROR) \
> > 	...
> 
> But that is not what is being suggested at all.  I already said that
> FIRST_SOMETHING is fine as a measure to initialize, didn't I?
> 
> I am only saying that if you have a place to store customized level,
> you should initialize that part with default levels and always look
> it up from that place at runtime.  It is perfectly fine for the
> initialization step to take advantage of the ordering and
> FIRST_SOMETHING constants.

Thanks for clarifying, I was worried that you wanted to encode the
severity levels explicitly (F(ID, LEVEL) instead of F(ID) in the correct
order). The DRY principle also suggests that we should not encode the
severity level in two ways (which would leave the door open for
inconsistencies). That means that we should not initialize a static array
of severity levels, but initialize the array using a loop.

Okay, now that we have established that the initial ordering by severity
makes sense, let's examine the initialization step.

Basically, our approaches differ only in *when* to initialize that array
of severity levels: you want to initialize it always, and I want to
initialize it only when the severity levels are customized by the caller.

Now, let's have a look how the fsck_options are currently initialized. My
code follows the convention established with strbufs, argv_arrays, etc:
there is a preprocessor definition (_DEFAULT, imitating the _INIT
definitions) that allows us to initialize such structs very conveniently.
Please note that no loop is required, and certainly no extra code has to
be called to initialize the struct. We get away with initializing that
array lazily in the fsck_strict_mode() function when we detect that it
needs to be initialized, being populated by the very same function that
provides the default settings before customization. This is a very robust
setup as the knowledge about, say, the size of that array is confined
strictly to fsck.c.

However, if we had to change the lookup such that it uses an array always,
we would have to introduce a function to initialize the struct, always, in
particular we would have to find a place to call that initialization
function in, say, builtin/fsck.c (actually, in every code path that calls
into the fsck machinery). Arguably, the code would get more complex –
introducing new call paths just to initialize the fsck_options struct –
and I would argue further that there is no gain from an elegance,
readability and maintenance point of view: whether the array is
initialized lazily or not, it will be initialized the exact same way. All
it means is that we have to introduce separate code paths because we would
separate explicitly the initialization from the configuration step.

Therefore, I do not believe that introducing an fsck_options_init() is
what you would really want.

An alternative would be to initialize the array at compile time – we would
have to violate the DRY principle for that, repeating the severity levels
many times over, and we could no longer confine the visibility to the
message IDs to fsck.c because not only the size of the array of severity
levels would have to be known to every user of fsck.h, but the exact
default severity levels themselves, to be able to initialize the struct.
But we could initialize the struct with a known set of settings via the
_DEFAULTS definition that way.

However, you already expressed slight disagreement with the preprocessor
magic needed to initialize both the enum and the array of message ID
strings from the same list in a way that lets the compiler ensure
consistency; I am afraid that if I were to modify _DEFAULTS to populate the
entire severity level array, the resulting code would find your utter
contempt.

I believe, therefore, that this is also not what you want.

So that leaves only one alternative: to initialize a global array with the
default severity levels at *some* stage. I have no idea what that stage
would be, therefore we would have to either establish ugly, and
DRY-violating, compile time initialization, or we would have to call a
function before using any of the fsck machinery – but that is fragile: it
is too easy to forget one call path and access an uninitialized array!
Worse, even if we *had* a fully initialized array of default severity
levels, we would still have to have an on-demand copy (i.e. lazy
initialization) of said array lest we modify the global defaults in
fsck_strict_mode()! Essentially, we would just *add* complexity to the
current solution, not replace it with anything simpler.

Therefore, I believe that you cannot be a fan of this alternative, either.

In short, I still find it much more elegant to determine the default
severity levels from the numeric enum values, and initialize the severity
level array only on demand, than to introduce a separate call to
initialize the array always, in particular since we would have to execute
that initialization loop *all* the time in the latter case – even if we do
not customize, let alone look up, any value – or clutter the code with
ugly, ugly preprocessor constructs.

And I hope that my arguments convinced you, too!

Ciao,
Dscho