[PATCH v2 0/6] Improve test coverage of update-ref error messages

[PATCH v2 0/6] Improve test coverage of update-ref error messages

[Michael Haggerty]

This is v2 of the patch series. Thanks to Johannes Sixt and Junio for
their comments about v1 [1].

Changes since v1:

* In t1404, don't use non-portable `local`: change function
  test_update_rejected to refer to the global `$prefix` variable
  rather than taking it as a parameter.

* Because that code change has become more involved, split off the
  addition of documentation for the function into a separate patch.

These patches are also available from my GitHub account [2] as branch
update-ref-errors.

Michael

[1] http://thread.gmane.org/gmane.comp.version-control.git/296626
[2] https://github.com/mhagger/git

Michael Haggerty (6):
  t1404: rename file to t1404-update-ref-errors.sh
  t1404: remove "prefix" argument to test_update_rejected
  t1404: document function test_update_rejected
  t1404: add more tests of update-ref error handling
  lock_ref_for_update(): make error handling more uniform
  lock_ref_for_update(): avoid a symref resolution

 refs/files-backend.c               |  77 ++++---
 t/t1404-update-ref-df-conflicts.sh | 181 -----------------
 t/t1404-update-ref-errors.sh       | 407 +++++++++++++++++++++++++++++++++++++
 3 files changed, 452 insertions(+), 213 deletions(-)
 delete mode 100755 t/t1404-update-ref-df-conflicts.sh
 create mode 100755 t/t1404-update-ref-errors.sh

-- 
2.8.1

[PATCH v2 1/6] t1404: rename file to t1404-update-ref-errors.sh

[Michael Haggerty]

I want to broaden the scope of this test file, so rename it accordingly.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/{t1404-update-ref-df-conflicts.sh => t1404-update-ref-errors.sh} | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename t/{t1404-update-ref-df-conflicts.sh => t1404-update-ref-errors.sh} (98%)

diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-errors.sh
similarity index 98%
rename from t/t1404-update-ref-df-conflicts.sh
rename to t/t1404-update-ref-errors.sh
index 6d869d1..2818460 100755
--- a/t/t1404-update-ref-df-conflicts.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='Test git update-ref with D/F conflicts'
+test_description='Test git update-ref error handling'
 . ./test-lib.sh
 
 test_update_rejected () {
-- 
2.8.1

[PATCH v2 2/6] t1404: remove "prefix" argument to test_update_rejected

[Michael Haggerty]

The tests already set a variable called prefix and passed its value as
the first argument to this function. The old argument handling was
overwriting the global variable with its same value rather than creating
a local variable.

So change test_update_rejected to refer to the global variable rather
than taking the prefix as an argument.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1404-update-ref-errors.sh | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 2818460..541cad1 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -4,11 +4,10 @@ test_description='Test git update-ref error handling'
 . ./test-lib.sh
 
 test_update_rejected () {
-	prefix="$1" &&
-	before="$2" &&
-	pack="$3" &&
-	create="$4" &&
-	error="$5" &&
+	before="$1" &&
+	pack="$2" &&
+	create="$3" &&
+	error="$4" &&
 	printf "create $prefix/%s $C\n" $before |
 	git update-ref --stdin &&
 	git for-each-ref $prefix >unchanged &&
@@ -37,7 +36,7 @@ test_expect_success 'setup' '
 test_expect_success 'existing loose ref is a simple prefix of new' '
 
 	prefix=refs/1l &&
-	test_update_rejected $prefix "a c e" false "b c/x d" \
+	test_update_rejected "a c e" false "b c/x d" \
 		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x$Q"
 
 '
@@ -45,7 +44,7 @@ test_expect_success 'existing loose ref is a simple prefix of new' '
 test_expect_success 'existing packed ref is a simple prefix of new' '
 
 	prefix=refs/1p &&
-	test_update_rejected $prefix "a c e" true "b c/x d" \
+	test_update_rejected "a c e" true "b c/x d" \
 		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x$Q"
 
 '
@@ -53,7 +52,7 @@ test_expect_success 'existing packed ref is a simple prefix of new' '
 test_expect_success 'existing loose ref is a deeper prefix of new' '
 
 	prefix=refs/2l &&
-	test_update_rejected $prefix "a c e" false "b c/x/y d" \
+	test_update_rejected "a c e" false "b c/x/y d" \
 		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x/y$Q"
 
 '
@@ -61,7 +60,7 @@ test_expect_success 'existing loose ref is a deeper prefix of new' '
 test_expect_success 'existing packed ref is a deeper prefix of new' '
 
 	prefix=refs/2p &&
-	test_update_rejected $prefix "a c e" true "b c/x/y d" \
+	test_update_rejected "a c e" true "b c/x/y d" \
 		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x/y$Q"
 
 '
@@ -69,7 +68,7 @@ test_expect_success 'existing packed ref is a deeper prefix of new' '
 test_expect_success 'new ref is a simple prefix of existing loose' '
 
 	prefix=refs/3l &&
-	test_update_rejected $prefix "a c/x e" false "b c d" \
+	test_update_rejected "a c/x e" false "b c d" \
 		"$Q$prefix/c/x$Q exists; cannot create $Q$prefix/c$Q"
 
 '
@@ -77,7 +76,7 @@ test_expect_success 'new ref is a simple prefix of existing loose' '
 test_expect_success 'new ref is a simple prefix of existing packed' '
 
 	prefix=refs/3p &&
-	test_update_rejected $prefix "a c/x e" true "b c d" \
+	test_update_rejected "a c/x e" true "b c d" \
 		"$Q$prefix/c/x$Q exists; cannot create $Q$prefix/c$Q"
 
 '
@@ -85,7 +84,7 @@ test_expect_success 'new ref is a simple prefix of existing packed' '
 test_expect_success 'new ref is a deeper prefix of existing loose' '
 
 	prefix=refs/4l &&
-	test_update_rejected $prefix "a c/x/y e" false "b c d" \
+	test_update_rejected "a c/x/y e" false "b c d" \
 		"$Q$prefix/c/x/y$Q exists; cannot create $Q$prefix/c$Q"
 
 '
@@ -93,7 +92,7 @@ test_expect_success 'new ref is a deeper prefix of existing loose' '
 test_expect_success 'new ref is a deeper prefix of existing packed' '
 
 	prefix=refs/4p &&
-	test_update_rejected $prefix "a c/x/y e" true "b c d" \
+	test_update_rejected "a c/x/y e" true "b c d" \
 		"$Q$prefix/c/x/y$Q exists; cannot create $Q$prefix/c$Q"
 
 '
@@ -101,7 +100,7 @@ test_expect_success 'new ref is a deeper prefix of existing packed' '
 test_expect_success 'one new ref is a simple prefix of another' '
 
 	prefix=refs/5 &&
-	test_update_rejected $prefix "a e" false "b c c/x d" \
+	test_update_rejected "a e" false "b c c/x d" \
 		"cannot process $Q$prefix/c$Q and $Q$prefix/c/x$Q at the same time"
 
 '
-- 
2.8.1

[PATCH v2 3/6] t1404: document function test_update_rejected

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1404-update-ref-errors.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 541cad1..7cfeaa9 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -3,6 +3,16 @@
 test_description='Test git update-ref error handling'
 . ./test-lib.sh
 
+# Create some references, perhaps run pack-refs --all, then try to
+# create some more references. Ensure that the second creation fails
+# with the correct error message.
+# Usage: test_update_rejected <before> <pack> <create> <error>
+#   <before> is a ws-separated list of refs to create before the test
+#   <pack> (true or false) tells whether to pack the refs before the test
+#   <create> is a list of variables to attempt creating
+#   <error> is a string to look for in the stderr of update-ref.
+# All references are created in the namespace specified by the current
+# value of $prefix.
 test_update_rejected () {
 	before="$1" &&
 	pack="$2" &&
-- 
2.8.1

[PATCH v2 4/6] t1404: add more tests of update-ref error handling

[Michael Haggerty]

Some of the error messages will be improved in subsequent commits.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1404-update-ref-errors.sh | 221 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 219 insertions(+), 2 deletions(-)

diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 7cfeaa9..5234b41f1 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -39,8 +39,9 @@ test_expect_success 'setup' '
 	git commit --allow-empty -m Initial &&
 	C=$(git rev-parse HEAD) &&
 	git commit --allow-empty -m Second &&
-	D=$(git rev-parse HEAD)
-
+	D=$(git rev-parse HEAD) &&
+	git commit --allow-empty -m Third &&
+	E=$(git rev-parse HEAD)
 '
 
 test_expect_success 'existing loose ref is a simple prefix of new' '
@@ -187,4 +188,220 @@ test_expect_success 'empty directory should not fool 1-arg delete' '
 	git update-ref --stdin
 '
 
+# Test various errors when reading the old values of references...
+
+test_expect_success 'missing old value blocks update' '
+	prefix=refs/missing-update &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q
+	EOF
+	printf "%s\n" "update $prefix/foo $E $D" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'incorrect old value blocks update' '
+	prefix=refs/incorrect-update &&
+	git update-ref $prefix/foo $C &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $Q$prefix/foo$Q: is at $C but expected $D
+	EOF
+	printf "%s\n" "update $prefix/foo $E $D" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'existing old value blocks create' '
+	prefix=refs/existing-create &&
+	git update-ref $prefix/foo $C &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $Q$prefix/foo$Q: reference already exists
+	EOF
+	printf "%s\n" "create $prefix/foo $E" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'incorrect old value blocks delete' '
+	prefix=refs/incorrect-delete &&
+	git update-ref $prefix/foo $C &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $Q$prefix/foo$Q: is at $C but expected $D
+	EOF
+	printf "%s\n" "delete $prefix/foo $D" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+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
+	EOF
+	printf "%s\n" "update $prefix/symref $E $D" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'incorrect old value blocks indirect update' '
+	prefix=refs/incorrect-indirect-update &&
+	git symbolic-ref $prefix/symref $prefix/foo &&
+	git update-ref $prefix/foo $C &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D
+	EOF
+	printf "%s\n" "update $prefix/symref $E $D" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'existing old value blocks indirect create' '
+	prefix=refs/existing-indirect-create &&
+	git symbolic-ref $prefix/symref $prefix/foo &&
+	git update-ref $prefix/foo $C &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $Q$prefix/symref$Q: reference already exists
+	EOF
+	printf "%s\n" "create $prefix/symref $E" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'incorrect old value blocks indirect delete' '
+	prefix=refs/incorrect-indirect-delete &&
+	git symbolic-ref $prefix/symref $prefix/foo &&
+	git update-ref $prefix/foo $C &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D
+	EOF
+	printf "%s\n" "delete $prefix/symref $D" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+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
+	EOF
+	printf "%s\n" "option no-deref" "update $prefix/symref $E $D" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'incorrect old value blocks indirect no-deref update' '
+	prefix=refs/incorrect-noderef-update &&
+	git symbolic-ref $prefix/symref $prefix/foo &&
+	git update-ref $prefix/foo $C &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $Q$prefix/symref$Q: is at $C 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 &&
+	test_cmp expected output.err
+'
+
+test_expect_failure '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 &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $Q$prefix/symref$Q: reference already exists
+	EOF
+	printf "%s\n" "option no-deref" "create $prefix/symref $E" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'incorrect old value blocks indirect no-deref delete' '
+	prefix=refs/incorrect-noderef-delete &&
+	git symbolic-ref $prefix/symref $prefix/foo &&
+	git update-ref $prefix/foo $C &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D
+	EOF
+	printf "%s\n" "option no-deref" "delete $prefix/symref $D" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'non-empty directory blocks create' '
+	prefix=refs/ne-create &&
+	mkdir -p .git/$prefix/foo/bar &&
+	: >.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
+	EOF
+	printf "%s\n" "update $prefix/foo $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
+	EOF
+	printf "%s\n" "update $prefix/foo $D $C" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'broken reference blocks create' '
+	prefix=refs/broken-create &&
+	mkdir -p .git/$prefix &&
+	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
+	EOF
+	printf "%s\n" "update $prefix/foo $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
+	EOF
+	printf "%s\n" "update $prefix/foo $D $C" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'non-empty directory blocks indirect create' '
+	prefix=refs/ne-indirect-create &&
+	git symbolic-ref $prefix/symref $prefix/foo &&
+	mkdir -p .git/$prefix/foo/bar &&
+	: >.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
+	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
+	EOF
+	printf "%s\n" "update $prefix/symref $D $C" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'broken reference blocks indirect create' '
+	prefix=refs/broken-indirect-create &&
+	git symbolic-ref $prefix/symref $prefix/foo &&
+	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
+	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
+	EOF
+	printf "%s\n" "update $prefix/symref $D $C" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
 test_done
-- 
2.8.1

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

[Michael Haggerty]

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

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

[David Turner]

On Fri, 2016-06-10 at 10:14 +0200, Michael Haggerty wrote:

>  /*
> + * 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

"error" is not a thing here?

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

[Michael Haggerty]

On 06/10/2016 09:01 PM, David Turner wrote:
> On Fri, 2016-06-10 at 10:14 +0200, Michael Haggerty wrote:
> 
>>  /*
>> + * 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
> 
> "error" is not a thing here?

You're right; thanks for the feedback. I'll include it in the reroll
that I'm about to do.

Michael

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

[Michael Haggerty]

On 06/13/2016 09:16 AM, Michael Haggerty wrote:
> On 06/10/2016 09:01 PM, David Turner wrote:
>> On Fri, 2016-06-10 at 10:14 +0200, Michael Haggerty wrote:
>>
>>>  /*
>>> + * 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
>>
>> "error" is not a thing here?
> 
> You're right; thanks for the feedback. I'll include it in the reroll
> that I'm about to do.

I have changed the docstring as follows:

>  /*
>   * 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.
> + * are consistent with oid, which is the reference's current value. If
> + * everything is OK, return 0; otherwise, write an error message to
> + * err and return -1.
>   */

I've folded that change into the patch series on branch
update-ref-errors on my GitHub fork [1]. I won't sent a re-roll to the
mailing list unless other changes are necessary (or unless somebody
requests it).

Michael

[1] https://github.com/mhagger/git

[PATCH v2 6/6] lock_ref_for_update(): avoid a symref resolution

[Michael Haggerty]

If we're overwriting a symref with a SHA-1, we need to resolve the value
of the symref (1) to check against update->old_sha1 and (2) to write to
its reflog. However, we've already read the symref itself and know its
referent. So there is no need to read the symref's value through the
symref; we can read the referent directly.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 98c8b95..b8d7a9a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3481,7 +3481,7 @@ 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, 0,
+			if (read_ref_full(referent.buf, 0,
 					  lock->old_oid.hash, NULL)) {
 				if (update->flags & REF_HAVE_OLD) {
 					strbuf_addf(err, "cannot lock ref '%s': "
-- 
2.8.1