On Fri, Jun 27, 2014 at 1:09 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes: > >> fsck is a tool that error() is more preferred than die(), but many > > "more preferred" without justifying why it is "more preferred" is > not quite a justification, is it? Also, an object failing to load > in-core is not a missing object, so if your aim is to let "fsck" > diagnose a too-large-to-load object as missing and let it continue, > I do not know if it is "more preferred" in the first place. Adding > a "too large--cannot check" bin of objects may be needed for it to > be useful. Also, we might need to give at the end "oh by the way, > because we couldn't read some objects to even determine its type, > the unreachable report from this fsck run is totally useless." Fair enough. I think avoiding dying in xmalloc() in this code path is still a good thing. At least "failed to read object %s" is more informative than simply "Out of memory". The error cascading effect in fsck is something I think we already have. I'll try to rephrase the commit message. But if you think this is not a good direction, dropping it is not so bad. I'm going to look at xmalloc() in unpack-objects. That's where we really should not abort because of memory shortage as the user may try to get as many objects as possible out of the pack. > The log message tries to justify that this may be a good thing for > "fsck", but the patch actually tries to change the behaviour of all > code paths that try to load an object in-core without considering > the ramifications of such a change. I _think_ all callers should be > prepared to receive NULL when we encounter a corrupt object (and > otherwise we should fix them), but it is unclear how much audit of > the callers (if any) was done to prepare this change. -- Duy -- 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