Merge branch 'mh/verify-lock-error-report'

Bring consistency to error reporting mechanism used in "refs" API.

* mh/verify-lock-error-report:
  ref_transaction_commit(): do not capitalize error messages
  verify_lock(): do not capitalize error messages
  verify_lock(): report errors via a strbuf
  verify_lock(): on errors, let the caller unlock the lock
  verify_lock(): return 0/-1 rather than struct ref_lock *

1 files changed, 26 insertions, 14 deletions

diff --git a/refs.c b/refs.c
index a742d7925..26d1ac1e3 100644
--- a/refs.c
+++ b/refs.c
@@ -2219,27 +2219,35 @@ static void unlock_ref(struct ref_lock *lock)
 	free(lock);
 }
 
-/* This function should make sure errno is meaningful on error */
-static struct ref_lock *verify_lock(struct ref_lock *lock,
-	const unsigned char *old_sha1, int mustexist)
+/*
+ * Verify that the reference locked by lock has the value old_sha1.
+ * Fail if the reference doesn't exist and mustexist is set. Return 0
+ * on success. On error, write an error message to err, set errno, and
+ * return a negative value.
+ */
+static int verify_lock(struct ref_lock *lock,
+		       const unsigned char *old_sha1, int mustexist,
+		       struct strbuf *err)
 {
+	assert(err);
+
 	if (read_ref_full(lock->ref_name,
 			  mustexist ? RESOLVE_REF_READING : 0,
 			  lock->old_oid.hash, NULL)) {
 		int save_errno = errno;
-		error("Can't verify ref %s", lock->ref_name);
-		unlock_ref(lock);
+		strbuf_addf(err, "can't verify ref %s", lock->ref_name);
 		errno = save_errno;
-		return NULL;
+		return -1;
 	}
 	if (hashcmp(lock->old_oid.hash, old_sha1)) {
-		error("Ref %s is at %s but expected %s", lock->ref_name,
-			oid_to_hex(&lock->old_oid), sha1_to_hex(old_sha1));
-		unlock_ref(lock);
+		strbuf_addf(err, "ref %s is at %s but expected %s",
+			    lock->ref_name,
+			    sha1_to_hex(lock->old_oid.hash),
+			    sha1_to_hex(old_sha1));
 		errno = EBUSY;
-		return NULL;
+		return -1;
 	}
-	return lock;
+	return 0;
 }
 
 static int remove_empty_directories(const char *file)
@@ -2467,7 +2475,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			goto error_return;
 		}
 	}
-	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
+	if (old_sha1 && verify_lock(lock, old_sha1, mustexist, err)) {
+		last_errno = errno;
+		goto error_return;
+	}
+	return lock;
 
  error_return:
 	unlock_ref(lock);
@@ -3912,7 +3924,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				? TRANSACTION_NAME_CONFLICT
 				: TRANSACTION_GENERIC_ERROR;
 			reason = strbuf_detach(err, NULL);
-			strbuf_addf(err, "Cannot lock ref '%s': %s",
+			strbuf_addf(err, "cannot lock ref '%s': %s",
 				    update->refname, reason);
 			free(reason);
 			goto cleanup;
@@ -3935,7 +3947,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				 * write_ref_to_lockfile():
 				 */
 				update->lock = NULL;
-				strbuf_addf(err, "Cannot update the ref '%s'.",
+				strbuf_addf(err, "cannot update the ref '%s'.",
 					    update->refname);
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;