From: Jeff King <peff@peff.net>
Date: Fri, 02 Apr 2010 07:24:55 GMT
Subject: Re: [PATCH 7/7] diff: avoid useless filespec population
Message-ID: <20100402072455.GA2111@sigill.intra.peff.net>
In-Reply-To: <7vvdcal44p.fsf@alter.siamese.dyndns.org>

On Fri, Apr 02, 2010 at 12:12:22AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> 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