From: Duy Nguyen Date: Sun, 29 Jun 2014 00:40:19 GMT Subject: Re: [PATCH v2 2/4] fsck: do not die when not enough memory to examine a pack entry Message-ID: In-Reply-To: On Fri, Jun 27, 2014 at 1:09 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy 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