Re: [REGRESSION, BISECTED] `git checkout <branch>` started to be memory hog

Jeff King [off-list ref] writes:

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.

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.

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

Jeff King [off-list ref] writes:

quoted

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