From: Jeff King <peff@peff.net>
Date: Wed, 19 Nov 2014 02:04:52 GMT
Subject: Re: [PATCH 3/4] lock_ref_sha1_basic: simplify error code path
Message-ID: <20141119020451.GA2734@peff.net>
In-Reply-To: <20141119020009.GR6527@google.com>

On Tue, Nov 18, 2014 at 06:00:09PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > For most errors, we jump to a goto label that unlocks the
> > ref and returns NULL. However, in none of these error paths
> > would we ever have actually locked the ref. By the time we
> > actually take the lock, we follow a different path that does
> > not ever hit this goto (we rely on verify_lock to unlock if
> > it finds an error).
> [...]
> >  refs.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> Wouldn't this leak lock (in all cases) and lock->ref_name and
> lock->orig_ref_name (on safe_create_leading_directories failure)?

Ah, you're right. I totally missed that unlock_ref is not just about
unlocking, but about free()ing. We do need to keep the goto/unlock.

Hmph. Should we just abandon my series in favor of taking Ronnie's
original patch, then? We can apply the "save/restore errno in error()"
patch independently if we like.

-Peff