git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 5/6] lock_ref_for_update(): make error handling more uniform

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Jun 10, 2016, 08:14 UTC
Message-ID
<6cc13da6767c5fd9dbf1b372634adf7c3a353464.1465544913.git.mhagger@alum.mit.edu>
In-Reply-To
<cover.1465544913.git.mhagger@alum.mit.edu>

To aid the effort, extract a new function, check_old_oid(), and use it in the two places where the read value of the reference has to be checked against update->old_sha1.

Update tests to reflect the improvements.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c         | 77 ++++++++++++++++++++++++++------------------
 t/t1404-update-ref-errors.sh | 14 ++++----
 2 files changed, 52 insertions(+), 39 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1230dfb..98c8b95 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3389,6 +3389,41 @@ static const char *original_update_refname(struct ref_update *update)
 }
 
 /*
+ * Check whether the REF_HAVE_OLD and old_oid values stored in update
+ * are consistent with the result read for the reference. error is
+ * true iff there was an error reading the reference; otherwise, oid
+ * is the value read for the reference.
+ *
+ * If there was a problem, write an error message to err and return
+ * -1.
+ */
+static int check_old_oid(struct ref_update *update, struct object_id *oid,
+			 struct strbuf *err)
+{
+	if (!(update->flags & REF_HAVE_OLD) ||
+		   !hashcmp(oid->hash, update->old_sha1))
+		return 0;
+
+	if (is_null_sha1(update->old_sha1))
+		strbuf_addf(err, "cannot lock ref '%s': "
+			    "reference already exists",
+			    original_update_refname(update));
+	else if (is_null_oid(oid))
+		strbuf_addf(err, "cannot lock ref '%s': "
+			    "reference is missing but expected %s",
+			    original_update_refname(update),
+			    sha1_to_hex(update->old_sha1));
+	else
+		strbuf_addf(err, "cannot lock ref '%s': "
+			    "is at %s but expected %s",
+			    original_update_refname(update),
+			    oid_to_hex(oid),
+			    sha1_to_hex(update->old_sha1));
+
+	return -1;
+}
+
+/*
  * Prepare for carrying out update:
  * - Lock the reference referred to by update.
  * - Read the reference under lock.
@@ -3432,7 +3467,7 @@ static int lock_ref_for_update(struct ref_update *update,
 
 		reason = strbuf_detach(err, NULL);
 		strbuf_addf(err, "cannot lock ref '%s': %s",
-			    update->refname, reason);
+			    original_update_refname(update), reason);
 		free(reason);
 		return ret;
 	}
@@ -3446,28 +3481,17 @@ static int lock_ref_for_update(struct ref_update *update,
 			 * the transaction, so we have to read it here
 			 * to record and possibly check old_sha1:
 			 */
-			if (read_ref_full(update->refname,
-					  mustexist ? RESOLVE_REF_READING : 0,
+			if (read_ref_full(update->refname, 0,
 					  lock->old_oid.hash, NULL)) {
 				if (update->flags & REF_HAVE_OLD) {
 					strbuf_addf(err, "cannot lock ref '%s': "
-						    "can't resolve old value",
-						    update->refname);
-					return TRANSACTION_GENERIC_ERROR;
-				} else {
-					hashclr(lock->old_oid.hash);
+						    "error reading reference",
+						    original_update_refname(update));
+					return -1;
 				}
-			}
-			if ((update->flags & REF_HAVE_OLD) &&
-			    hashcmp(lock->old_oid.hash, update->old_sha1)) {
-				strbuf_addf(err, "cannot lock ref '%s': "
-					    "is at %s but expected %s",
-					    update->refname,
-					    sha1_to_hex(lock->old_oid.hash),
-					    sha1_to_hex(update->old_sha1));
+			} else if (check_old_oid(update, &lock->old_oid, err)) {
 				return TRANSACTION_GENERIC_ERROR;
 			}
-
 		} else {
 			/*
 			 * Create a new update for the reference this
@@ -3484,6 +3508,9 @@ static int lock_ref_for_update(struct ref_update *update,
 	} else {
 		struct ref_update *parent_update;
 
+		if (check_old_oid(update, &lock->old_oid, err))
+			return TRANSACTION_GENERIC_ERROR;
+
 		/*
 		 * If this update is happening indirectly because of a
 		 * symref update, record the old SHA-1 in the parent
@@ -3494,20 +3521,6 @@ static int lock_ref_for_update(struct ref_update *update,
 		     parent_update = parent_update->parent_update) {
 			oidcpy(&parent_update->lock->old_oid, &lock->old_oid);
 		}
-
-		if ((update->flags & REF_HAVE_OLD) &&
-		    hashcmp(lock->old_oid.hash, update->old_sha1)) {
-			if (is_null_sha1(update->old_sha1))
-				strbuf_addf(err, "cannot lock ref '%s': reference already exists",
-					    original_update_refname(update));
-			else
-				strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
-					    original_update_refname(update),
-					    sha1_to_hex(lock->old_oid.hash),
-					    sha1_to_hex(update->old_sha1));
-
-			return TRANSACTION_GENERIC_ERROR;
-		}
 	}
 
 	if ((update->flags & REF_HAVE_NEW) &&
@@ -3529,7 +3542,7 @@ static int lock_ref_for_update(struct ref_update *update,
 			 */
 			update->lock = NULL;
 			strbuf_addf(err,
-				    "cannot update the ref '%s': %s",
+				    "cannot update ref '%s': %s",
 				    update->refname, write_err);
 			free(write_err);
 			return TRANSACTION_GENERIC_ERROR;
diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 5234b41f1..c34ece4 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -237,7 +237,7 @@ test_expect_success 'missing old value blocks indirect update' '
 	prefix=refs/missing-indirect-update &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q
+	fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q
 	EOF
 	printf "%s\n" "update $prefix/symref $E $D" |
 	test_must_fail git update-ref --stdin 2>output.err &&
@@ -284,7 +284,7 @@ test_expect_success 'missing old value blocks indirect no-deref update' '
 	prefix=refs/missing-noderef-update &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $Q$prefix/symref$Q: can${Q}t resolve old value
+	fatal: cannot lock ref $Q$prefix/symref$Q: reference is missing but expected $D
 	EOF
 	printf "%s\n" "option no-deref" "update $prefix/symref $E $D" |
 	test_must_fail git update-ref --stdin 2>output.err &&
@@ -303,7 +303,7 @@ test_expect_success 'incorrect old value blocks indirect no-deref update' '
 	test_cmp expected output.err
 '
 
-test_expect_failure 'existing old value blocks indirect no-deref create' '
+test_expect_success 'existing old value blocks indirect no-deref create' '
 	prefix=refs/existing-noderef-create &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	git update-ref $prefix/foo $C &&
@@ -372,13 +372,13 @@ test_expect_success 'non-empty directory blocks indirect create' '
 	: >.git/$prefix/foo/bar/baz.lock &&
 	test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $Q$prefix/foo$Q: there is a non-empty directory $Q.git/$prefix/foo$Q blocking reference $Q$prefix/foo$Q
+	fatal: cannot lock ref $Q$prefix/symref$Q: there is a non-empty directory $Q.git/$prefix/foo$Q blocking reference $Q$prefix/foo$Q
 	EOF
 	printf "%s\n" "update $prefix/symref $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q
+	fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q
 	EOF
 	printf "%s\n" "update $prefix/symref $D $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
@@ -391,13 +391,13 @@ test_expect_success 'broken reference blocks indirect create' '
 	echo "gobbledigook" >.git/$prefix/foo &&
 	test_when_finished "rm -f .git/$prefix/foo" &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken
+	fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken
 	EOF
 	printf "%s\n" "update $prefix/symref $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken
+	fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken
 	EOF
 	printf "%s\n" "update $prefix/symref $D $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&

-- 
2.8.1
Previous: Michael HaggertyNext: David Turner
Message 7 of 10 in “Improve test coverage of update-ref error messages”
  1. 0/6 Improve test coverage of update-ref error messagesMichael Haggerty, Jun 10, 2016
  2. 1/6 t1404: rename file to t1404-update-ref-errors.shMichael Haggerty, Jun 10, 2016
  3. 4/6 t1404: add more tests of update-ref error handlingMichael Haggerty, Jun 10, 2016
  4. 2/6 t1404: remove "prefix" argument to test_update_rejectedMichael Haggerty, Jun 10, 2016
  5. 3/6 t1404: document function test_update_rejectedMichael Haggerty, Jun 10, 2016
  6. 6/6 lock_ref_for_update(): avoid a symref resolutionMichael Haggerty, Jun 10, 2016
  7. 5/6 lock_ref_for_update(): make error handling more uniformMichael Haggerty, Jun 10, 2016
  8. David TurnerJun 10, 2016
  9. Michael HaggertyJun 13, 2016
  10. Michael HaggertyJun 13, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.