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

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> When fsck_commit() identifies a problem with the commit, it should try
> to make it possible to continue checking the commit object, in case the
> user wants to demote the detected errors to mere warnings.

That makes sense.

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

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?