Re: [PATCH 1/2] parse_commit_buffer: don't parse invalid commits
- From
- Martin Koegler <mkoegler@auto.tuwien.ac.at>
- Date
- Jan 14, 2008, 20:58 UTC
- Message-ID
- <20080114205826.GB5058@auto.tuwien.ac.at>
- In-Reply-To
- <7vk5mclvk3.fsf@gitster.siamese.dyndns.org>
On Sun, Jan 13, 2008 at 11:23:40PM -0800, Junio C Hamano wrote:
Show 9 quoted lines
> Martin Koegler <mkoegler@auto.tuwien.ac.at> 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.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 ;-)
Show 13 quoted lines
> 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.
Show 5 quoted lines
> 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