Re: [PATCH v2 2/4] fsck: do not die when not enough memory to examine a pack entry

On Fri, Jun 27, 2014 at 1:09 AM, Junio C Hamano [off-list ref] wrote:

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

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