Re: [PATCH v5 00/19] Introduce an internal API to interact with the fsck machinery

[Johannes Schindelin <johannes.schindelin@gmx.de>]

Hi Junio,

On 2015-06-19 00:11, Junio C Hamano wrote:

> I haven't had a chance to go through the all the patches, but one
> thing I noticed that did not appear in the interdiff is that some of
> the message IDs are unclear.  For example, there are BAD_something,
> INVALID_something and MISSING_something.  The last one is in a
> different category and is good, but how are the former two
> differenciated?  Do they follow some systematic rules, or they are
> named after the way how they happened to be reported in the original
> textual error message?

I basically made up names on the go, based on the messages.

> Some of the questionable groups are:
> 
>     BAD_DATE DATE_OVERFLOW

I guess it should be BAD_DATE_OVERFLOW to be more consistent?

>     BAD_TREE_SHA1 INVALID_OBJECT_SHA1 INVALID_TREE
> 
>     BAD_PARENT_SHA1 INVALID_OBJECT_SHA1

So how about s/INVALID_/BAD_/g?

> Also it is unclear if NOT_SORTED is to be used ever for any error
> other than a tree object sorted incorrectly, or if we start noticing
> a new error that something is not sorted, we will reuse this one.

s/NOT_SORTED/TREE_&/ maybe?

> I also briefly wondered if fsck.skipList should be finer grained
> than "these are know to be broken, do not bother reporting problems
> with them" (e.g. I know v0.99 lacks "tagger" so I want to squelch
> MISSING_TAGGER_ENTRY for it, but I want to be notified on any other
> errors).  But that only matters if we update Git to a version with a
> new fsck that knows yet more kinds of breakages, so it is not a huge
> issue, and the simplicity of "be silent on these objects" is
> probably better overall.

Well, the idea of skiplist is to say: "I have inspected this object and determined that errors in it should be ignored." As such, it does not really matter what problems future Git versions report because the person populating the skiplist is supposed to test thoroughly, not just asking `git fsck` what is going on.

And yes, the motivation for this feature is to keep it super-simple. ;-)

Ciao,
Dscho