Re: [PATCH] builtin-fast-export.c: add default case to avoid crash on corrupt repo

[Erik Faye-Lund <kusmabite@googlemail.com>]

On Sun, Mar 22, 2009 at 12:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> A tag can point at anything, so this is not an issue about "crash on a
> _corrupt_ repository".

Ah, my bad. I wrongly assumed corrupted repos were the only ways of
triggering this issue. I quite easily managed to reproduce the crash
by setting up some tags to trees and tag objects to trees.

> I am not very familiar with this program, but the codepath involved should
> be prepared to _see_ any type of object instead of dying.
>
> What to do after _seeing_ a type of object is a different matter.  It
> appears that there is no way to feed a tree object to fast-import, but I
> think the fast-import language can represent a tag that points at another
> tag just fine.  So the best you can do is perhaps to issue a warning
> "skipping a tag that points at a tree object" and impoement a proper
> handling of a tag that points at a tag.
>

My patch simply applied the same error that was already present for
tags to tag objects, but yeah. Handling tagged tags and warning
instead of erroring-out makes more sense to me as well. I'll see if I
can write it up, and resubmit a patch.

After looking some more at the code, it seems that there's an attempt
to handle tags to tags there already, but it doesn't seem to work
properly; the program error out with a "fatal: Tag [tagname] points
nowhere?". This seems to be because the tagged-pointer of the second
tag-object is NULL. Now, I'm no expert, but from browsing some code,
it seems that "parse_object(tag->object.sha1)" should have been
performed on the tag before looking up the tagged object. Does this
make sense?

Also, I guess this calls for a couple of test-cases or something. I
haven't written any tests yet, so I might need some time to figuring
it out.

Anyway, I guess this makes the most sense as a four-patch series:
1) Add test-cases for tags of tag objects, tag objects of tag objects,
tags of trees and tag objects of trees.
2) Turn the existing error into a warning
3) Add the missing warning and remove the crash
4) Fix fast-export to export tags pointing to tags.

Makes sense?

Additionally, to reduce the chance of similar bugs in the future, the
code could be refactored a bit to have a handle_commit()-function, so
what goes on becomes a bit more obvious. Since this doesn't really
change any functionality, I guess it could be handed in as a separate
patch.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656