Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

From: Jeff King <peff@peff.net>
Date: Aug 29, 2017, 18:53 UTC
Message-ID: <20170829185341.s3xlsx4uym7lcluc@sigill.intra.peff.net>
In-Reply-To: <B1E291F2-86FF-4982-A092-92FAED65385C@gmail.com>

On Tue, Aug 29, 2017 at 06:51:52PM +0100, Lars Schneider wrote:

Show 5 quoted lines

> I set $TOOL_OPTIONS in valgrind.sh: to 
> '--leak-check=full --errors-for-leak-kinds=definite'
> 
> ... but I also had to adjust t/test-lib-functions.sh:test_create_repo
> as I ran into the error "cannot run git init -- have you built things yet?".

Yeah, this is a general problem with run-time analyzers. If we're not mostly error free than the setup code often breaks before you even get to the interesting part of the test.

It looks like the config code has a minor-ish leak. Patch to follow.

Show 11 quoted lines

> What if we run a few selected tests with valgrind and count all files that
> valgrind mentions (a single leak has multiple file mentions because of
> the stack trace and other leak indicators). We record these counts and let 
> TravisCI scream if one of the numbers increases.
> 
> I wonder how stable/fragile such a metric would be as a simple refactoring 
> could easily change these numbers. Below I ran valgrind on t5510 before and
> after Martin's patch. The diff below clearly shows the pkt-line leak.
> 
> Would it make sense to pursue something like this in TravisCI to avoid 
> "pkt-line" kind of leaks in the future?

I don't think that would work, because simply adding new tests would bump the leak count, without the code actually growing worse.

But think about your strategy for a moment: what you're really trying to do is say "these existing leaks are OK because they are too many for us to count, but we want to make sure we don't add _new_ ones". We already have two techniques for distinguishing old ones from new ones:

  1. Mark existing ones with valgrind suppressions so they do not warn
     at all.

  2. Fix them, so that the "existing" count drops to zero.

Option (2), of course, has the added side effect that it's actually fixing potential problems. :)

-Peff