Re: [PATCH 2/3] setup_path(): Free temporary buffer

From: Jeff King <peff@peff.net>
Date: Mar 15, 2011, 01:12 UTC
Message-ID: <20110315011230.GA31865@sigill.intra.peff.net>
In-Reply-To: <1300140119.4320.38.camel@bee.lab.cmartin.tk>

On Mon, Mar 14, 2011 at 11:01:53PM +0100, Carlos Martín Nieto wrote:

Show 13 quoted lines

> > I tend to think they are Ok, but then I also tend to think one leak of
> > exec-path return value in setup_path() is perfectly fine for the same
> > reason, so in that sense I don't see a point in this patch...
> 
>  Which brings us to the matter of whether we actually care about memory
> leaks, as the processes are short-lived and the system is going to clean
> up after us. Do we, unless the leaks are huge? As there is built-in
> valgrind support in the test suite, I went in with the assumption that
> we did. It seems however that hardly any code paths free their memory,
> other than when using strbuf.
> 
>  In case we don't, valgrind should be told not to bother reporting leaks
> (and maybe mention in some document that small leaks are not an issue).

Don't we already use --leak-check=no in our valgrind support for the test suite? The valgrind support is there primarily to find memory access errors, not leaks.

It would be nice if it could find leaks, too, but there is currently a lot of noise due to uninteresting leaks like this. I haven't looked at valgrind's support for suppressing leak reporting, but presumably we could build a suppression file that would drop the uninteresting ones. We could also fix them, of course, but if they are one-time-per-program allocations, it might not be worth cluttering the code.

-Peff