Re: [PATCH v2 06/10] fsck.h: move FSCK_{FATAL,INFO,ERROR,WARN,IGNORE} into an enum

Jeff King [off-list ref] writes:

On Thu, Feb 18, 2021 at 11:58:36AM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

Move the FSCK_{FATAL,INFO,ERROR,WARN,IGNORE} defines into a new
fsck_msg_type enum.

Makes sense. As with my previous comment, I wonder if "severity" is a
more descriptive term.

quoted

diff --git a/fsck.h b/fsck.h
index 0c75789d219..c77e8ddf10b 100644
--- a/fsck.h
+++ b/fsck.h

@@ -3,10 +3,13 @@
 
 #include "oidset.h"
 
-#define FSCK_ERROR 1
-#define FSCK_WARN 2
-#define FSCK_IGNORE 3
-
+enum fsck_msg_type {
+	FSCK_INFO = -2,
+	FSCK_FATAL = -1,
+	FSCK_ERROR = 1,
+	FSCK_WARN,
+	FSCK_IGNORE
+};

You kept the values the same as they were before, which is good in a
refactoring step, but...wow, the ordering is weird and confusing.

In FATAL/ERROR/WARN/IGNORE the number increases as severity decreases.
Maybe reversed from how I'd do it, but at least the order makes sense.
But somehow INFO is on the far side of FATAL?

Again, not something to address in this patch, but I hope something we
could maybe deal with in the longer term (perhaps along with fixing the
weird "INFO is a warning from the user's perspective, but WARNING is
generally an error" behavior).

I also know that this is assigning WARN and IGNORE based on
counting-by-one from ERROR, so it's correct. But I think it would be
more obvious if you simply filled in the values manually, so a reader
does not have to wonder why some are assigned and some are not.

I had the same reaction, plus "Wow, we had FSCK_* constants in two
different places and without colliding?  Have we been lucky?
Declaring it in one place, whether we use enum or not (as enum is
not very useful in C as a type checking vehicle), makes a lot of
sense but why does this come this late in the series, instead of
being at the front as a trivial low-hanging fruit?"

Thanks.