On Fri, Apr 02, 2010 at 12:12:22AM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > This patch just pushes the fill_mmfile call a bit later, so we can avoid
> > populating the filespec in some cases. There is one thing to note that
> > looks like a bug but isn't. We push the fill_mmfile down into the first
> > branch of a conditional. It seems like we would need it on the other
> > branch, too, but we don't; fill_textconv does it for us (in fact, before
> > this, we were just writing over the results of the fill_mmfile on that
> > branch).
>
> ... and leaking the memory in the "cached" codepath, perhaps?
I don't think so. fill_mmfile never allocates, but rather points to
what's in the diff_filespec. So though we overwrote what was in the
mmfile, that data was still pointed to and eventually freed by the
diff_filespec struct.
fill_textconv will allocate if and only if we are doing a textconv
(either a buffer we get from read_sha1_file in the case of a cached
entry, or the detached strbuf from run-textconv otherwise). If we're not
doing a textconv, it falls back to basically doing the same as
fill_mmfile. And that's the reason for the
if (textconv_one)
free(data_one);
seen in patch 2/7 (and the matching one in builtin_diff from before).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html