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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 22, 2009, 21:58 UTC
Message-ID: <7vwsahcj8i.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <40aa078e0903220522g66cf2172l9f1a43ed562cc4d3@mail.gmail.com>

Erik Faye-Lund <kusmabite@googlemail.com> writes:

Show 8 quoted lines

> 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?

Yes.

I suspect we will hear an argument that say the dying is deliberate because pretending to have produced a faithful export while some information is lacking is bad and warnings are easy to miss, and I would certainly understand that argument.

We have similar issue with signed tags and filter-branch, which strips the signature when it rewrites the history. I recall the code originally died for the same reasoning, but asking "it died; what can the user do now?" made us realize that it is the best we can do to make best-effort reproduction with a warning when losing information.

This falls into the same category. I think "we die by default if we cannot give a faithful reproduction, and the user needs to give an explicit permission (e.g. --force option) to lose information", is a very sensible thing to do, but "we die if we cannot export some things, and we refuse to produce an approximation." without an escape hatch is not.