Re: [PATCH v6 08/19] fsck: Make fsck_commit() warn-friendly

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

Hi Junio,

On 2015-06-19 22:12, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
>> Note that some problems are too problematic to simply ignore. For
>> example, when the header lines are mixed up, we punt after encountering
>> an incorrect line. Therefore, demoting certain warnings to errors can
>> hide other problems. Example: demoting the missingauthor error to
>> a warning would hide a problematic committer line.
> 
> Is this a warning to end-users (which should be better in the doc),
> or "because some of them are too problematic to ignore" that forgot
> to add the explanation "hence we do not keep going in this code"
> (which should be in the log message if that is what is going on)?

It was intended to offer the explanation for the design decision you commented on later:

> I notice that there are many instances of
> 
> 	if (object does not pass some test)
> 		return report(...);
> 
> that do not do "err = report(); if (err) return;" in this function
> after applying this patch.
> 
> I think that answers the above question.  The answer is "because
> some are too problematic, even after this patch, we give up parsing
> the remainder of the commit object once we hit certain errors,
> leaving some other errors that appear later in the object
> undetected".
> 
> I think that is a sensible design decision, but the proposed log
> message forgets to say so.
> 
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>>  fsck.c | 28 ++++++++++++++++++++--------
>>  1 file changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/fsck.c b/fsck.c
>> index 9faaf53..9fe9f48 100644
>> --- a/fsck.c
>> +++ b/fsck.c
>> @@ -534,12 +534,18 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
>>
>>  	if (!skip_prefix(buffer, "tree ", &buffer))
>>  		return report(options, &commit->object, FSCK_MSG_MISSING_TREE, "invalid format - expected 'tree' line");
>> -	if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
>> -		return report(options, &commit->object, FSCK_MSG_BAD_TREE_SHA1, "invalid 'tree' line format - bad sha1");
>> +	if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n') {
>> +		err = report(options, &commit->object, FSCK_MSG_BAD_TREE_SHA1, "invalid 'tree' line format - bad sha1");
>> +		if (err)
>> +			return err;
>> +	}
> 
> I do not think this "if (err) return err;" that uses the return
> value of report(), makes sense.
> 
> As all the errors that use this pattern are isolated ones that does
> not break parsing of the remainder (e.g. author ident had an extra >
> in it may break "author " but that does not prevent us from checking
> "committer ").
> 
> Your report() switches its return value based on the user setting;
> specifically, it returns 0 if the user tells us to ignore/skip or
> warn.  Which means that the user will see all warnings, but we stop
> at the first error.
> 
> Shouldn't we continue regardless of the end-user setting in order to
> show errors on other fields, too?

I can make that happen, but please note that this is a change of behavior: we always stopped upon the first error.

It was my intention not to change behavior in that way without a proper reason, and I saw none.

I actually see a really good reason to *keep* the current behavior: one of the most prominent users of this code path is `git receive-pack --strict`. It is used heavily by GitHub to ensure at least a certain level of validity of pushed objects. Now, for this use case it is easy to see that you want to stop *as soon as an error was encountered*. And as GitHub sponsors my work on this patch series, my main aim is to support their use case.

Having said that, I agree that it could actually make sense for `git fsck` to show all errors, or at least to have an option to do so.

But that is a story for another night ;-)
Dscho