From: Jeff King <peff@peff.net>
Date: Fri, 22 Jul 2011 17:36:02 GMT
Subject: Re: [REGRESSION, BISECTED] `git checkout <branch>` started to be memory hog
Message-ID: <20110722173601.GA9422@sigill.intra.peff.net>
In-Reply-To: <7vipquz0d0.fsf@alter.siamese.dyndns.org>

On Fri, Jul 22, 2011 at 10:33:31AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > From my quick look, I came up with the fix below. It removes the leak
> > and doesn't trigger any memory errors according to valgrind. So it
> > _must_ be right. :)
> >
> > -Peff
> >
> > ---
> > diff --git a/streaming.c b/streaming.c
> > index 565f000..f3acc5d 100644
> > --- a/streaming.c
> > +++ b/streaming.c
> > @@ -93,7 +93,9 @@ struct git_istream {
> >  
> >  int close_istream(struct git_istream *st)
> >  {
> > -	return st->vtbl->close(st);
> > +	int r = st->vtbl->close(st);
> > +	free(st);
> > +	return r;
> >  }
> 
> I do not think of a reason any future callers would want to close the
> stream first and then inspect some attribute of the stream afterwards (if
> this were an output stream, there might be things like output stats that
> may want such an interface, but even in such a case, the caller should say
> "flush" first to drain whatever is pending, grab stats and then close), so
> freeing the resource at close time seems sensible to me.

Yeah. My impression was that these istreams would operate much like
"FILE *" streams. And closing those frees all resources, and you can
throw away the pointer afterwards.

> Both the external caller in entry.c and the internal caller (a filtered
> istream reads from its underlying istream, and when it is getting closed,
> closes the underlying istream) were leaking the istream exactly the same
> way, so this fix should plug both of them.
> 
> Thanks.

Did you want me to write a commit message? I think you might do a better
job of writing a coherent one for this particular patch.

-Peff