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

On Sun, Jan 13, 2008 at 11:23:40PM -0800, Junio C Hamano wrote:

Martin Koegler [off-list ref] writes:

quoted

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

You seem to have missed my reply to your last mail:
http://marc.info/?l=git&m=119969163624138&w=2

I asked you, what you would think about this change, but got no
answer. Anyway, now I know your opinion ;-)

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.

On the other hand, it is possible, that somebody pushed such a commit
out, if he does not notice it. Then its difficult to get rid of the
broken commit. [Hasn't happend a broken commit on pu recently?]

parse_commit_date is not very strict, so its likely, that it miss such
a commit. Commit parsing is too common function to slow it down with
further checks.

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'll repost a version, which reverts this change.

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

Maybe I can improve fsck.

mfg Martin Kögler