On Sun, Jan 13, 2008 at 11:23:40PM -0800, Junio C Hamano wrote:
> Martin Koegler <mkoegler@xxxxxxxxxxxxxxxxx> 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 ;-)
> 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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
