From 651ab9f553a1fd8bb847bd42922dacb14f8ec77f Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Wed, 16 Apr 2014 11:56:52 -0700 Subject: [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 explicitly, 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() failed 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 Signed-off-by: Junio C Hamano --- sequencer.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sequencer.c b/sequencer.c index bde5f047b0..0a80c58d11 100644 --- a/sequencer.c +++ b/sequencer.c @@ -281,8 +281,12 @@ 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) + return error(_("Failed to lock HEAD during fast_forward_to")); + strbuf_addf(&sb, "%s: fast-forward", action_name(opts)); ret = write_ref_sha1(ref_lock, to, sb.buf); + strbuf_release(&sb); return ret; } From 55a5c8d72b03ad3727b89900069a1d58664fe5e4 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Wed, 16 Apr 2014 11:56:53 -0700 Subject: [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 Signed-off-by: Junio C Hamano --- builtin/commit.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 9cfef6c6cc..f0b790640d 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"));