Re: [PATCH 2/2] fsck: document msg-id

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

Good observation. "help --config" only gives names and no description, so maintaining the description like this new file does make sense.

Can you also add a referencing comment to fsck.h to tell that folks who add a new error type that they need to update the -msgids.txt file as well?

Does this list in the new file cover all existing messages, by the way?

I also wonder if the order of the entries in this file should be alphabetical, unlike the entries in fsck.h where more severe ones come earlier than the less severe ones. In a sense, matching the order of two lists makes it easier to maintain, and grouping by severity in the doc might or might not find it easier to scan and find what they are, but one of the biggest reason why users will come to this list is when they see a particular error message and want to understand what that means, no? At that point it would be easier to look things up if

 (1) the list contains EVERYTHING, even the ones that you are not
     supposed to configure away.

 (2) the list is sorted alphabetically, regardless of the severity.

The first point suggests that it may be a mistake to have the main list appear in the "what configuration knobs are available for squelching fsck error messages". It is OK to refer from there to the main list, but the main list should list everything, with comments like "(this error cannot be configured away)", no?

In other words, I think it is better to have a master list of everything, with their message ID and textual description, which essentially is your fsck-msgids.txt but additionally mention which ones are by default FATAL and cannot be configured away (even better, we can mention what severity level each of them is by default, by mirroring that is in fsck.h).

And that master list should NOT be made a part of fsck.<msg-type> configuration item, like this patch did. It should be a separate section in "git fsck --help" output whose section title is "Fsck errors" or something.

Then the existing description of fsck.<msg-type> configuration can refer to that "Fsck errors" section for what msg types exist.

Another thing that I noticed while thinking about the patch, but is better left outside the scope of this patch, is that an attempt to configure fatal ones away is prevented by fsck.c::fsck_set_msg_type() but "git help config | grep '^fsck\.'" lists them. I think the "help config" should be fixed.

I almost suggested to extend the FOREACH_FSCK_MSG_ID() definition in fsck.h so that fsck-msgids.txt gets auto-generated (what is missing in fsck.h that prevents us from doing so is the textual explanation you added in the new file in your patch---they could come from comments on the same line, for example, and can be extracted via a Perl or sed script at build time). I do not know if it is a good idea, especially if we forsee a future in which we may be translating the documentation, so decided against making such a suggestion.

But at least, we could _lint_ the manually curated fsck-msgids.txt using what is in fsck.h to see if a new MSG_ID added to fsck.h is missing from the doc, or a MSG_ID whose default severity is updated in fsck.h is stale in the doc, etc. That can be left for the future updates, but we should at least instruct developers to keep them in sync in fsck.h by adding a comment.

Thanks.