Re: [PATCH 7/7] diff: avoid useless filespec population

From: Jeff King <peff@peff.net>
Date: Apr 2, 2010, 07:24 UTC
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:

Show 11 quoted lines

> 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