Jeff King <peff@xxxxxxxx> writes:
> 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.
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.
