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

Hi Junio,

first of all: the improvements discussed here are already part of v6.

On 2015-06-19 19:33, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

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

quoted

Some of the questionable groups are:

    BAD_DATE DATE_OVERFLOW

I guess it should be BAD_DATE_OVERFLOW to be more consistent?

I am not sure about "consistency", but surely a common prefix would
help readers to group things.  But for this particular group, I was
wondering if singling out "integer overflow", "zero stuffed
timestamp", etc. into such a finer sub-errors of "you have a bad
timestamp" was beneficial.

Well, someone thought it a good idea to print different error messages, and I took that as an indicator that there is merit in being able to distinguish these issues from one another.
 

quoted

quoted

    BAD_TREE_SHA1 INVALID_OBJECT_SHA1 INVALID_TREE

    BAD_PARENT_SHA1 INVALID_OBJECT_SHA1

So how about s/INVALID_/BAD_/g?

It is not just about distinction between INVAID and BAD.

I was basically wondering what rule decides which one among
BAD_TREE_SHA1, INVALID_OBJECT_SHA1 and INVALID_TREE I would get when
I have a random non-hexdigit string in various places, e.g. after
'tree ' in the object header of a commit object, after 'tag ' in a
tag object that says 'type tree', etc.

To be honest, I think the IDs do not really matter as much as your comment makes it sound: the IDs purpose is solely to be able to configure the message type (read: whether to error out, warn or ignore the issue). The real information is in the actual message (and I did not change any message, therefore I could not make things worse than they are right now).

Example: you would never read BAD_TREE_SHA1, but instead: "badtreesha1: invalid 'tree' line format - bad sha1".

For BAD_OBJECT_SHA1 (as it is now called), there are actually two code paths generating the error: "invalid 'object' line format - bad sha1" and "no valid object to fsck".

And for BAD_TREE it is: "could not load commit's tree <SHA1>".

Thus, from the error message it should be really clear what is going on.

quoted

quoted

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?

If that error is specific to tree sorting order, then that would be
a definite improvement.

Yes, that is the case. Tree objects are assumed to list their contents in order, and this ID applies to the problem where a tree object's list is out of order.

As I said, I already made both discussed changes part of v6.

Ciao,
Dscho