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.
> 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
