Re: [PATCH] sha1_loose_object_info: handle errors from unpack_sha1_rest

[Jeff King <peff@peff.net>]

On Fri, Oct 06, 2017 at 01:19:21PM +0900, Junio C Hamano wrote:

> > But note that the leak in (2) is actually older than that.
> > The original unpack_sha1_file() directly returned the result
> > of unpack_sha1_rest() to its caller, when it should have
> > been closing the zlib stream itself on error.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> 
> Obviously correct.  (2) is as old as Git itself; it eventually
> blames down to e83c5163 ("Initial revision of "git", the information
> manager from hell", 2005-04-07), where read-cache.c::unpack_sha1_file()
> liberally returns NULL without cleaning up the zstream.

Thanks, I as too lazy to dig down further, but I'm always interested to
see the roots of these things (especially "bug in the original" versus
"introduced by a careless refactor").

I have a feeling that the world would be a better place if
unpack_sha1_rest() just always promised to close the zstream, since no
callers seem to want to look at it in the error case. But I wanted to go
for the minimal fix first.

-Peff