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

Hi Junio,

On 2015-06-19 22:12, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

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.

quoted

Signed-off-by: Johannes Schindelin <redacted>
---
 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