[PATCH] refs.c: handle locking failure during transaction better

[PATCH] refs.c: handle locking failure during transaction better

[Stefan Beller]

Change lock_ref_sha1_basic to return an error instead of dying,
when we fail to lock a file during a transaction. This function is
only called from transaction_commit() and it knows how to handle
these failures.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Cherry-picked-to-master: Stefan Beller <sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
 
This was part of the ref-transactions-rename series[1], 
but it makes sense to send this one as a separate patch.

Thanks,
Stefan

[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html

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.2.0.rc2.5.gf7b9fb2

Re: [PATCH] refs.c: handle locking failure during transaction better

[Stefan Beller]

I messed up authorship on this one.
This was of course found and authored by Ronnie.

On Tue, Nov 18, 2014 at 3:17 PM, Stefan Beller <sbeller@google.com> wrote:
> Change lock_ref_sha1_basic to return an error instead of dying,
> when we fail to lock a file during a transaction. This function is
> only called from transaction_commit() and it knows how to handle
> these failures.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Cherry-picked-to-master: Stefan Beller <sbeller@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  refs.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> This was part of the ref-transactions-rename series[1],
> but it makes sense to send this one as a separate patch.
>
> Thanks,
> Stefan
>
> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
>
> 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.2.0.rc2.5.gf7b9fb2
>

[PATCH] refs.c: handle locking failure during transaction better

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

Change lock_ref_sha1_basic to return an error instead of dying,
when we fail to lock a file during a transaction. This function is
only called from transaction_commit() and it knows how to handle
these failures.

[sb: This was part of a larger patch series, cherry-picked to master]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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.2.0.rc2.5.gf7b9fb2

[PATCH 0/4] error cleanups in lock_ref_sha1_basic

[Jeff King]

On Tue, Nov 18, 2014 at 05:13:17PM -0800, Stefan Beller wrote:

> From: Ronnie Sahlberg <sahlberg@google.com>
> 
> Change lock_ref_sha1_basic to return an error instead of dying,
> when we fail to lock a file during a transaction. This function is
> only called from transaction_commit() and it knows how to handle
> these failures.
> 
> [sb: This was part of a larger patch series, cherry-picked to master]
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>

I think this is a good thing to do. I independently wrote the same patch
recently, along with some other cleanups. Here's the series I ended up
with (I added Ronnie as the author of the final one, which replaces
this; even though my discovery was independent, he wrote it first :) ).

  [1/4]: error: save and restore errno
  [2/4]: lock_ref_sha1_basic: simplify errno handling
  [3/4]: lock_ref_sha1_basic: simplify error code path
  [4/4]: lock_ref_sha1_basic: do not die on locking errors

-Peff

[PATCH 1/4] error: save and restore errno

[Jeff King]

It's common to use error() to return from a function, like:

	if (open(...) < 0)
		return error("open failed");

Unfortunately this may clobber the errno from the open()
call. So we often end up with code like this:

        if (open(...) < 0) {
		int saved_errno = errno;
		error("open failed");
		errno = saved_errno;
		return -1;
	}

which is less nice. Let's teach error() to save and restore
errno in each call, so that the original errno is preserved.
This is slightly less efficient for callers which do not
care, but error code paths are generally not performance
critical anyway.

Signed-off-by: Jeff King <peff@peff.net>
---
It's pretty minor to just handle errno in the callers, but I feel like
I've wanted this at least a dozen times, and it seems like it cannot
possibly hurt (i.e., I imagine there are callers where we _should_ be
doing the errno dance but have not realized it).

 usage.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/usage.c b/usage.c
index ed14645..ee44d57 100644
--- a/usage.c
+++ b/usage.c
@@ -142,10 +142,13 @@ void NORETURN die_errno(const char *fmt, ...)
 int error(const char *err, ...)
 {
 	va_list params;
+	int saved_errno = errno;
 
 	va_start(params, err);
 	error_routine(err, params);
 	va_end(params);
+
+	errno = saved_errno;
 	return -1;
 }
 
-- 
2.1.2.596.g7379948

[PATCH 2/4] lock_ref_sha1_basic: simplify errno handling

[Jeff King]

Now that error() does not clobber errno, we do not have to
take pains to save it ourselves.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 5ff457e..169a46d 100644
--- a/refs.c
+++ b/refs.c
@@ -2232,7 +2232,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	char *ref_file;
 	const char *orig_refname = refname;
 	struct ref_lock *lock;
-	int last_errno = 0;
 	int type, lflags;
 	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
 	int resolve_flags = 0;
@@ -2260,7 +2259,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		 */
 		ref_file = git_path("%s", orig_refname);
 		if (remove_empty_directories(ref_file)) {
-			last_errno = errno;
 			error("there are still refs under '%s'", orig_refname);
 			goto error_return;
 		}
@@ -2270,7 +2268,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	if (type_p)
 	    *type_p = type;
 	if (!refname) {
-		last_errno = errno;
 		error("unable to resolve reference %s: %s",
 			orig_refname, strerror(errno));
 		goto error_return;
@@ -2283,7 +2280,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	 */
 	if (missing &&
 	     !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {
-		last_errno = ENOTDIR;
+		errno = ENOTDIR;
 		goto error_return;
 	}
 
@@ -2311,7 +2308,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			goto retry;
 		/* fall through */
 	default:
-		last_errno = errno;
 		error("unable to create directory for %s", ref_file);
 		goto error_return;
 	}
@@ -2332,7 +2328,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 
  error_return:
 	unlock_ref(lock);
-	errno = last_errno;
 	return NULL;
 }
 
-- 
2.1.2.596.g7379948

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

[Jeff King]

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).

We can just drop the goto completely and return NULL as
appropriate.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 169a46d..fafae7e 100644
--- a/refs.c
+++ b/refs.c
@@ -2260,7 +2260,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		ref_file = git_path("%s", orig_refname);
 		if (remove_empty_directories(ref_file)) {
 			error("there are still refs under '%s'", orig_refname);
-			goto error_return;
+			return NULL;
 		}
 		refname = resolve_ref_unsafe(orig_refname, resolve_flags,
 					     lock->old_sha1, &type);
@@ -2270,7 +2270,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	if (!refname) {
 		error("unable to resolve reference %s: %s",
 			orig_refname, strerror(errno));
-		goto error_return;
+		return NULL;
 	}
 	missing = is_null_sha1(lock->old_sha1);
 	/* When the ref did not exist and we are creating it,
@@ -2281,7 +2281,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	if (missing &&
 	     !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {
 		errno = ENOTDIR;
-		goto error_return;
+		return NULL;
 	}
 
 	lock->lk = xcalloc(1, sizeof(struct lock_file));
@@ -2309,7 +2309,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		/* fall through */
 	default:
 		error("unable to create directory for %s", ref_file);
-		goto error_return;
+		return NULL;
 	}
 
 	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
@@ -2325,10 +2325,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			unable_to_lock_die(ref_file, errno);
 	}
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
-
- error_return:
-	unlock_ref(lock);
-	return NULL;
 }
 
 struct ref_lock *lock_any_ref_for_update(const char *refname,
-- 
2.1.2.596.g7379948

[PATCH 4/4] lock_ref_sha1_basic: do not die on locking errors

[Jeff King]

From: Ronnie Sahlberg <sahlberg@google.com>

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 and rebased on
 nearby cleanups.]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
It's a little sad to have to do the errno dance here after our earlier
cleanups. But while error() is safe, unable_to_lock_message is anything
but (the original patch from Ronnie did not need this, because it leaves
the goto and last_errno in place). So I dunno. Maybe my cleanups were
all for naught, and this end result is worse.

 refs.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index fafae7e..1f7e136 100644
--- a/refs.c
+++ b/refs.c
@@ -2321,8 +2321,15 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			 * again:
 			 */
 			goto retry;
-		else
-			unable_to_lock_die(ref_file, errno);
+		else {
+			int saved_errno = errno;
+			struct strbuf buf = STRBUF_INIT;
+			unable_to_lock_message(ref_file, errno, &buf);
+			error("%s", buf.buf);
+			strbuf_release(&buf);
+			errno = saved_errno;
+			return NULL;
+		}
 	}
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
 }
-- 
2.1.2.596.g7379948

Re: [PATCH 1/4] error: save and restore errno

[Stefan Beller]

This one makes my day.
A really good fix as it minimizes maintenance burden for checking
incoming patches for that pattern.

Reviewed-by: Stefan Beller <sbeller@google.com>


On Tue, Nov 18, 2014 at 5:37 PM, Jeff King <peff@peff.net> wrote:
> It's common to use error() to return from a function, like:
>
>         if (open(...) < 0)
>                 return error("open failed");
>
> Unfortunately this may clobber the errno from the open()
> call. So we often end up with code like this:
>
>         if (open(...) < 0) {
>                 int saved_errno = errno;
>                 error("open failed");
>                 errno = saved_errno;
>                 return -1;
>         }
>
> which is less nice. Let's teach error() to save and restore
> errno in each call, so that the original errno is preserved.
> This is slightly less efficient for callers which do not
> care, but error code paths are generally not performance
> critical anyway.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> It's pretty minor to just handle errno in the callers, but I feel like
> I've wanted this at least a dozen times, and it seems like it cannot
> possibly hurt (i.e., I imagine there are callers where we _should_ be
> doing the errno dance but have not realized it).
>
>  usage.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/usage.c b/usage.c
> index ed14645..ee44d57 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -142,10 +142,13 @@ void NORETURN die_errno(const char *fmt, ...)
>  int error(const char *err, ...)
>  {
>         va_list params;
> +       int saved_errno = errno;
>
>         va_start(params, err);
>         error_routine(err, params);
>         va_end(params);
> +
> +       errno = saved_errno;
>         return -1;
>  }
>
> --
> 2.1.2.596.g7379948
>

Re: [PATCH 1/4] error: save and restore errno

[Jonathan Nieder]

Jeff King wrote:

> It's common to use error() to return from a function, like:
>
> 	if (open(...) < 0)
> 		return error("open failed");
>
> Unfortunately this may clobber the errno from the open()
> call. So we often end up with code like this:
>
>         if (open(...) < 0) {
> 		int saved_errno = errno;
> 		error("open failed");
> 		errno = saved_errno;
> 		return -1;
> 	}
>
> which is less nice.

What the above doesn't explain is why the caller cares about errno.
Are they going to print another message with strerror(errno)?  Or are
they going to consider some errors non-errors (like ENOENT when trying
to unlink a file), in which case why is printing a message to stderr
okay?

All that said, given that there are real examples of code already
doing this, the patch seems sane.

[...]
>  usage.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 1/4] error: save and restore errno

[Jeff King]

On Tue, Nov 18, 2014 at 05:43:44PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > It's common to use error() to return from a function, like:
> >
> > 	if (open(...) < 0)
> > 		return error("open failed");
> >
> > Unfortunately this may clobber the errno from the open()
> > call. So we often end up with code like this:
> >
> >         if (open(...) < 0) {
> > 		int saved_errno = errno;
> > 		error("open failed");
> > 		errno = saved_errno;
> > 		return -1;
> > 	}
> >
> > which is less nice.
> 
> What the above doesn't explain is why the caller cares about errno.
> Are they going to print another message with strerror(errno)?  Or are
> they going to consider some errors non-errors (like ENOENT when trying
> to unlink a file), in which case why is printing a message to stderr
> okay?

I guess the unsaid bit is:

  Unfortunately this may clobber the errno from the open() call. Even
  though error() sees the correct errno, the caller to which we are
  returning may see a bogus errno value.

-Peff

Re: [PATCH 2/4] lock_ref_sha1_basic: simplify errno handling

[Jonathan Nieder]

Jeff King wrote:

> Now that error() does not clobber errno, we do not have to
> take pains to save it ourselves.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  refs.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 5ff457e..169a46d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2232,7 +2232,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,

All the caller to lock_ref_sha1_basic cares about is whether errno ==
ENOTDIR, since in that case we can print a message suggesting running
"git remote prune".

Longer term, I suspect the caller (transaction_commit) should call
is_refname_available to check for conflicting refs and return early to
give "git fetch" a chance to print its advice.

If a conflicting ref appears after that point, then just printing a
reasonable error message is enough --- it is not so useful to give the
'remote prune' advice when people are doing funny things like running
fetch in one terminal and a ref update creating a D/F conflict against
that fetch in another terminal.[*]

By the way, Stefan was mentioning the other day that it might make
sense for transaction_commit to prevent a conflicting ref from
appearing mid-transaction by locking 'refs/heads' in addition to
'refs/heads/master'.

Anyway, in the meantime this is a nice cleanup.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[*] If we want to handle that, the 'struct strbuf *err' could be
replaced with a larger struct with room for structured data, like
Junio was hinting at in another thread.

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

[Jonathan Nieder]

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)?

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

[Jeff King]

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

Re: [PATCH 4/4] lock_ref_sha1_basic: do not die on locking errors

[Jonathan Nieder]

Jeff King wrote:

> From: Ronnie Sahlberg <sahlberg@google.com>
>
> 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.

And in addition to the modern transaction stuff, rename_ref() has been
such a caller for a long time.

Thanks for a pleasant explanation.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[...]
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

No need for my sign-off here --- it was just an artifact of my
forwarding the patch to the list before.

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

[Jonathan Nieder]

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.

Thanks,
Jonathan

Re: [PATCH 1/4] error: save and restore errno

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Nov 18, 2014 at 05:43:44PM -0800, Jonathan Nieder wrote:
>
>> Jeff King wrote:
>> 
>> > It's common to use error() to return from a function, like:
>> >
>> > 	if (open(...) < 0)
>> > 		return error("open failed");
>> >
>> > Unfortunately this may clobber the errno from the open()
>> > call. So we often end up with code like this:
>> >
>> >         if (open(...) < 0) {
>> > 		int saved_errno = errno;
>> > 		error("open failed");
>> > 		errno = saved_errno;
>> > 		return -1;
>> > 	}
>> >
>> > which is less nice.
>> 
>> What the above doesn't explain is why the caller cares about errno.
>> Are they going to print another message with strerror(errno)?  Or are
>> they going to consider some errors non-errors (like ENOENT when trying
>> to unlink a file), in which case why is printing a message to stderr
>> okay?
>
> I guess the unsaid bit is:
>
>   Unfortunately this may clobber the errno from the open() call. Even
>   though error() sees the correct errno, the caller to which we are
>   returning may see a bogus errno value.
>
> -Peff

I am not sure if that answers the question asked.

If you have

	int frotz(...) {
		int fd = open(...);
        	if (fd < 0)
                	return error("open failed (%s)", strerror(errno));
		return fd;
	}

and the caller calls it and cares about the errno from this open,
what does the caller do?  Jonathan's worried about a codepath that
may be familiar to us as we recently saw a patch similar to it:

	int fd = frotz(...);
        if (fd < 0) {
        	if (errno == ENOENT || errno == EISDIR)
                	; /* not quite an error */
		else
			exit(1);
	}

If ENOENT/EISDIR is expected and a non-error, it is not useful for
frotz() to give an error message on its own.

I think a more appropriate answer to Jonathan's question is why is
the callee (i.e. frotz()) calling error() in the first place if an
unconditional error message is an issue.

Re: [PATCH 1/4] error: save and restore errno

[Jeff King]

On Wed, Nov 19, 2014 at 10:14:17AM -0800, Junio C Hamano wrote:

> >> What the above doesn't explain is why the caller cares about errno.
> >> Are they going to print another message with strerror(errno)?  Or are
> >> they going to consider some errors non-errors (like ENOENT when trying
> >> to unlink a file), in which case why is printing a message to stderr
> >> okay?
> >
> > I guess the unsaid bit is:
> >
> >   Unfortunately this may clobber the errno from the open() call. Even
> >   though error() sees the correct errno, the caller to which we are
> >   returning may see a bogus errno value.
> >
> I am not sure if that answers the question asked.
> 
> If you have
> 
> 	int frotz(...) {
> 		int fd = open(...);
>         	if (fd < 0)
>                 	return error("open failed (%s)", strerror(errno));
> 		return fd;
> 	}
> 
> and the caller calls it and cares about the errno from this open,
> what does the caller do?  Jonathan's worried about a codepath that
> may be familiar to us as we recently saw a patch similar to it:
> 
> 	int fd = frotz(...);
>         if (fd < 0) {
>         	if (errno == ENOENT || errno == EISDIR)
>                 	; /* not quite an error */
> 		else
> 			exit(1);
> 	}
> 
> If ENOENT/EISDIR is expected and a non-error, it is not useful for
> frotz() to give an error message on its own.

Sure, but isn't that out-of-scope for this patch? We know that some
functions _are_ calling error() and taking care to keep errno valid, and
we would prefer to do that with less work.

If you are arguing "there are literally zero cases where that makes
sense; functions should either complain themselves, _or_ they should
pass on a valid errno so the caller can decide whether to complain, but
doing both is a recipe for pointless and unwanted error messages", then
this patch is counter-productive (and we should be fixing those call
sites of error() instead).

But I am not sure that there are zero legitimate cases. Just as a
hypothetical, imagine you wanted to complain to stderr _and_ propagate
the error value into a specific exit code. You'd want something like:

  fd = frotz(...);
  error("frotz failed (%s)", strerror(errno));
  exit(errno == ENOENT ? 1 : 2);

Granted, I do not think we do that particular pattern in git, but we do
take pains to save errno across some error() calls. I do not know which
of those are legitimate and which should be refactored. In the meantime,
I do not think this patch makes anything worse.

> I think a more appropriate answer to Jonathan's question is why is
> the callee (i.e. frotz()) calling error() in the first place if an
> unconditional error message is an issue.

Exactly. It is not error()'s business to know what the caller wants to
do, or whether or why it cares about retaining errno. But error(), since
it is designed to be called in error codepaths, should aim to be
minimally intrusive.

-Peff

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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.

Concurred.

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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).
> ...
> So here is that patch, with my explanation. Thanks for your patience in
> my running around in circles. :)

My head has been spinning, but I think I can relax now ~@~ ;-)

> Subject: lock_ref_sha1_basic: do not die on locking errors

Will queue; thanks.

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

[Jeff King]

On Wed, Nov 19, 2014 at 02:34:12PM -0800, Junio C Hamano wrote:

> > Subject: lock_ref_sha1_basic: do not die on locking errors
> 
> Will queue; thanks.

I just noticed that when I pasted it into the mail, I dropped the

  From: Ronnie Sahlberg <sahlberg@google.com>

header. Can you please make sure to credit Ronnie as you apply?

Thanks.

-Peff

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

[Jonathan Nieder]

Jeff King wrote:

> 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(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

(modulo the author attribution nit you noticed)

Re: [PATCH 2/4] lock_ref_sha1_basic: simplify errno handling

[Michael Haggerty]

On 11/19/2014 02:37 AM, Jeff King wrote:
> Now that error() does not clobber errno, we do not have to
> take pains to save it ourselves.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  refs.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 5ff457e..169a46d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2232,7 +2232,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  	char *ref_file;
>  	const char *orig_refname = refname;
>  	struct ref_lock *lock;
> -	int last_errno = 0;
>  	int type, lflags;
>  	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
>  	int resolve_flags = 0;
> @@ -2260,7 +2259,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  		 */
>  		ref_file = git_path("%s", orig_refname);
>  		if (remove_empty_directories(ref_file)) {
> -			last_errno = errno;
>  			error("there are still refs under '%s'", orig_refname);
>  			goto error_return;
>  		}
> @@ -2270,7 +2268,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  	if (type_p)
>  	    *type_p = type;
>  	if (!refname) {
> -		last_errno = errno;
>  		error("unable to resolve reference %s: %s",
>  			orig_refname, strerror(errno));
>  		goto error_return;
> @@ -2283,7 +2280,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  	 */
>  	if (missing &&
>  	     !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {
> -		last_errno = ENOTDIR;
> +		errno = ENOTDIR;
>  		goto error_return;
>  	}
>  
> @@ -2311,7 +2308,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  			goto retry;
>  		/* fall through */
>  	default:
> -		last_errno = errno;
>  		error("unable to create directory for %s", ref_file);
>  		goto error_return;
>  	}
> @@ -2332,7 +2328,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  
>   error_return:
>  	unlock_ref(lock);
> -	errno = last_errno;
>  	return NULL;
>  }

error() doesn't clobber errno anymore, but unlock_ref() still might.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu