Re: [PATCH] config: use a static lock_file struct

From: Jeff King <peff@peff.net>
Date: Aug 29, 2017, 19:12 UTC
Message-ID: <20170829191217.dt65wazf7qh5qs3k@sigill.intra.peff.net>
In-Reply-To: <20170829190928.GD131745@google.com>

On Tue, Aug 29, 2017 at 12:09:28PM -0700, Brandon Williams wrote:

Show 15 quoted lines

> > -- >8 --
> > Subject: [PATCH] config: use a static lock_file struct
> > 
> > When modifying git config, we xcalloc() a struct lock_file
> > but never free it. This is necessary because the tempfile
> > code (upon which the locking code is built) requires that
> > the resulting struct remain valid through the life of the
> > program. However, it also confuses leak-checkers like
> > valgrind because only the inner "struct tempfile" is still
> > reachable; no pointer to the outer lock_file is kept.
> 
> Is this just due to a limitation in the tempfile code?  Would it be
> possible to improve the tempfile code such that we don't need to require
> that a tempfile, once created, is required to exist for the remaining
> life of the program?

Yes. Like I wrote below:

Show 5 quoted lines

> > ---
> > In the long run we may want to drop the "tempfiles must remain forever"
> > rule. This is certainly not the first time it has caused confusion or
> > leaks. And I don't think it's a fundamental issue, just the way the code
> > is written. But in the interim, this fix is probably worth doing.

The main issue is that the caller needs to make sure they're removed from the list (via commit or rollback) before being freed.

As far as I know anyway. This restriction dates back to the very early days of the lockfile code and has been carried through the various tempfile-cleanup refactorings over the years (mostly because each was afraid to make functional changes).

+cc Michael, who did most comprehensive cleanup of that code.

-Peff