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

From: Duy Nguyen <pclouds@gmail.com>
Date: Jun 29, 2014, 00:40 UTC
Message-ID: <CACsJy8C8TkQLNXdz=D8YC8TjSEP1nth8LmvftJYSG0bTRvqU7A@mail.gmail.com>
In-Reply-To: <xmqqa98z5yrg.fsf@gitster.dls.corp.google.com>

On Fri, Jun 27, 2014 at 1:09 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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.

Show 7 quoted lines

> 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