threads / patch / 36416

patch, 2 partsCheck for lock failures early

Subject: [PATCH 0/2] Check for lock failures early

## tl;dr

4 messages between Apr 15, 2014 and Apr 16, 2014. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Ronnie Sahlberg· Apr 15, 2014, 23:46 UTC · lore

Callers outside of refs.c use either lock_ref_sha1() or lock_any_ref_for_update() to lock a ref during an update.

Two of these places we do not immediately check the lock for failure making reading the code harder.

One place we do some unrelated string manipulation fucntions before we check for failure and the other place we rely on that write_ref_sha1() will check the lock for failure and return an error.

These two patches updates these two places so that we immediately check the lock for failure and act on it. It does not change any functionality or logic but makes the code easier to read by being more consistent.

Ronnie Sahlberg (2):
  sequencer.c: check for lock failure and bail early in fast_forward_to
  commit.c: check for lock error and return early

 builtin/commit.c | 8 ++++----
 sequencer.c      | 7 +++++++
 2 files changed, 11 insertions(+), 4 deletions(-)

-- 
1.9.1.503.ge4c3920.dirty

Ronnie Sahlberg· Apr 15, 2014, 23:46 UTC · re: Ronnie Sahlberg · lore

[PATCH 1/2] sequencer.c: check for lock failure and bail early in fast_forward_to

Change fast_forward_to() to check if locking the ref failed, print a nice
error message and bail out early.
The old code did not check if ref_lock was NULL and relied on the fact
that the write_ref_sha1() would safely detect this condition and set the
return variable ret to indicate an error.
While that is safe, it makes the code harder to read for two reasons:
* Inconsistency.  Almost all other places we do check the lock for NULL
  explicitely, so the naive reader is confused "why don't we check here".
* And relying on write_ref_sha1() to detect and return an error for when
  a previous lock_any_ref_for_update() feels obfuscated.

This change should not change any functionality or logic aside from adding an extra error message when this condition is triggered. (write_ref_sha1() returns an error silently for this condition)

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 sequencer.c | 7 +++++++
 1 file changed, 7 insertions(+)

Show changes to sequencer.c +7 −0

diff --git a/sequencer.c b/sequencer.c
index bde5f04..6aa3b50 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -281,8 +281,15 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 		exit(1); /* the callee should have complained already */
 	ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
 					   0, NULL);
+	if (!ref_lock) {
+		ret = error(_("Failed to lock HEAD during fast_forward_to"));
+		goto leave;
+	}
+
 	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
 	ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+leave:
 	strbuf_release(&sb);
 	return ret;
 }

-- 
1.9.1.503.ge4c3920.dirty

Brandon Casey· Apr 16, 2014, 01:37 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 1/2] sequencer.c: check for lock failure and bail early in fast_forward_to

On Tue, Apr 15, 2014 at 4:46 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:

<snip well-worded commit message>

Show 18 quoted lines

> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  sequencer.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index bde5f04..6aa3b50 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -281,8 +281,15 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
>                 exit(1); /* the callee should have complained already */
>         ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
>                                            0, NULL);
> +       if (!ref_lock) {
> +               ret = error(_("Failed to lock HEAD during fast_forward_to"));
> +               goto leave;
> +       }
> +

Or just:

   if (!ref_lock)
       return error(_("Failed to lock HEAD ..."));

We don't need to strbuf_release() since the strbuf has not been modified at this point. We've only initialized with a static initializer.

Show 8 quoted lines

>         strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
>         ret = write_ref_sha1(ref_lock, to, sb.buf);
> +
> +leave:
>         strbuf_release(&sb);
>         return ret;
>  }
> --

-Brandon

Ronnie Sahlberg· Apr 15, 2014, 23:46 UTC · re: Ronnie Sahlberg · lore

[PATCH 2/2] commit.c: check for lock error and return early

Move the check for the lock failure to happen immediately after lock_any_ref_for_update(). Previously the lock and the check-if-lock-failed was separated by a handful of string manipulation statements.

Moving the check to occur immediately after the failed lock makes the
code slightly easier to read and makes it follow the pattern of
 try-to-take-a-lock()
 if (check-if-lock-failed){
    error
 }

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/commit.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Show changes to builtin/commit.c +4 −4

diff --git a/builtin/commit.c b/builtin/commit.c
index d9550c5..c6320f1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1672,6 +1672,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 					   ? NULL
 					   : current_head->object.sha1,
 					   0, NULL);
+	if (!ref_lock) {
+		rollback_index_files();
+		die(_("cannot lock HEAD ref"));
+	}
 
 	nl = strchr(sb.buf, '\n');
 	if (nl)
@@ -1681,10 +1685,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
 	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
 
-	if (!ref_lock) {
-		rollback_index_files();
-		die(_("cannot lock HEAD ref"));
-	}
 	if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
 		rollback_index_files();
 		die(_("cannot update HEAD ref"));

-- 
1.9.1.503.ge4c3920.dirty