Re: [PATCH v2 06/10] fsck.h: move FSCK_{FATAL,INFO,ERROR,WARN,IGNORE} into an enum
From: Jeff King <hidden>
Date: 2021-02-18 19:55:50
On Thu, Feb 18, 2021 at 11:58:36AM +0100, Ævar Arnfjörð Bjarmason wrote:
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 hunk ↗ jump to hunk
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. -Peff