Re: [PATCH 3/4] lock_ref_sha1_basic: simplify error code path

[Jeff King <peff@peff.net>]

On Tue, Nov 18, 2014 at 06:07:13PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > 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.
> 
> I liked patches 1 and 2 and the explanation from patch 4.  Perhaps
> patch 3 should be replaced with a patch renaming unlock_ref to
> free_ref_lock or something.

I took a look at this, and it ends up not being very useful.

The "return NULL" from the final patch has to become a "goto
error_return", so that it can call unlock_ref(). But that means it
cannot save and restore errno itself, because unlock_ref may clobber
errno[1].

So we still have to keep the last_errno handling in error_return.
Meaning that we need to drop patch 2 (even though the other cases don't
need errno saved/restore, since the goto does it unconditionally, we
still need to set last_errno). And therefore patch 1 is not helping
anyone (we could still apply it, but there's no immediate benefit).

I also looked at renaming unlock_ref, but it is called from other places
where they _do_ care about unlocking. So renaming is out. We could build
a free_ref_lock that unlock_ref builds on, but that brings up another
confusion: can we or can we not free the "struct lockfile" pointer it
contains (and which should free_ref_lock do)? Whether it is safe to do
so depends on whether we have actually fed it to hold_lock_file_for_update
or not (even if it fails, it takes ownership of the lock). So we
actually leak it in some cases, but the only case we could fix is when
safe_create_leading_directories the _first_ time (before we have ever
tried to lock and failed, at which point we loop). Yeesh.

So I really think we are better off leaving it as-is, and just applying
some form of Ronnie's patch (which does the right thing with errno). The
other cleanups end up making things worse, and the unlock_ref thing was
just my misunderstanding.

So here is that patch, with my explanation. Thanks for your patience in
my running around in circles. :)

-- >8 --
Subject: lock_ref_sha1_basic: do not die on locking errors

lock_ref_sha1_basic is inconsistent about when it calls
die() and when it returns NULL to signal an error. This is
annoying to any callers that want to recover from a locking
error.

This seems to be mostly historical accident. It was added in
4bd18c4 (Improve abstraction of ref lock/write.,
2006-05-17), which returned an error in all cases except
calling safe_create_leading_directories, in which case it
died.  Later, 40aaae8 (Better error message when we are
unable to lock the index file, 2006-08-12) asked
hold_lock_file_for_update to die for us, leaving the
resolve_ref code-path the only one which returned NULL.

We tried to correct that in 5cc3cef (lock_ref_sha1(): do not
sometimes error() and sometimes die()., 2006-09-30),
by converting all of the die() calls into returns. But we
missed the "die" flag passed to the lock code, leaving us
inconsistent. This state persisted until e5c223e
(lock_ref_sha1_basic(): if locking fails with ENOENT, retry,
2014-01-18). Because of its retry scheme, it does not ask
the lock code to die, but instead manually dies with
unable_to_lock_die().

We can make this consistent with the other return paths by
converting this to use unable_to_lock_message(), and
returning NULL. This is safe to do because all callers
already needed to check the return value of the function,
since it could fail (and return NULL) for other reasons.

[jk: Added excessive history explanation]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 5ff457e..0347328 100644
--- a/refs.c
+++ b/refs.c
@@ -2318,6 +2318,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 
 	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
 	if (lock->lock_fd < 0) {
+		last_errno = errno;
 		if (errno == ENOENT && --attempts_remaining > 0)
 			/*
 			 * Maybe somebody just deleted one of the
@@ -2325,8 +2326,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			 * again:
 			 */
 			goto retry;
-		else
-			unable_to_lock_die(ref_file, errno);
+		else {
+			struct strbuf err = STRBUF_INIT;
+			unable_to_lock_message(ref_file, errno, &err);
+			error("%s", err.buf);
+			strbuf_reset(&err);
+			goto error_return;
+		}
 	}
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
 
-- 
2.1.2.596.g7379948