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

Johannes Schindelin [off-list ref] writes:

At the moment, the git-fsck's integrity checks are targeted toward the
end user, i.e. the error messages are really just messages, intended for
human consumption.

Under certain circumstances, some of those errors should be allowed to
be turned into mere warnings, though, because the cost of fixing the
issues might well be larger than the cost of carrying those flawed
objects.

Interdiff below the diffstat. It's huge. Sorry.

Heh, no need to say sorry, though.  A large interdiff means you did
a lot more work, after all.

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?

Some of the questionable groups are:

    BAD_DATE DATE_OVERFLOW

    BAD_TREE_SHA1 INVALID_OBJECT_SHA1 INVALID_TREE

    BAD_PARENT_SHA1 INVALID_OBJECT_SHA1

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.

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.

Thanks.

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