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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 19, 2015, 19:43 UTC
Message-ID: <8eb30cab0d359420bc037ab89b21c4b3@www.dscho.org>
In-Reply-To: <xmqqfv5na9hd.fsf@gitster.dls.corp.google.com>

Hi Junio,

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

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

Show 15 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
>> 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?
> 
> 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.

Show 13 quoted lines

>>>     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.

Show 8 quoted lines

>>> 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