Re: [PATCH 1/2] parse_commit_buffer: don't parse invalid commits

Martin Koegler [off-list ref] writes:

+	if (!parse_commit_date(bufptr, tail, &item->date))
+		return error("bogus commit date in object %s", sha1_to_hex(item->object.sha1));
 
 	if (track_object_refs) {
 		unsigned i = 0;

I suspect this might be an undesirable regression.

If somebody managed to create a commit with a bogus "author"
line and wanted to clean up the history, your previous one at
least gave something usable back, even though it had to come up
with a bogus date.  It gave the rest of the data back without
barfing.  And it was easy to see which "resurrected" commit had
a missing author date (bogus ones always gave 0 timestamp).

This round you made it to error out, and callers that check the
return value of parse_commit() would stop traversing the
history, even if the commit in question has perfectly valid
"parent " lines, thinking "ah, this commit object is faulty".
It actively interferes with attempts to resurrect data from
history that contains a faulty commit.

Your previous version was much better with respect to this
issue.  It was about being more careful not to read outside the
commit object buffer, while still allowing the data from a
history that has an unfortunate commit with broken author line
to be resurrected more easily.

I do not think the checks done by fsck and parse_commit should
share the same strictness.  They serve different purposes.