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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 14, 2008, 07:23 UTC
Message-ID: <7vk5mclvk3.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <12002478702664-git-send-email-mkoegler@auto.tuwien.ac.at>

Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:

Show 5 quoted lines

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