On Fri, Jun 27, 2014 at 1:09 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclo...@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.

> 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.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to