From bf0c6603ff809b035bd3b2049597e2273e9d86ed Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Tue, 7 Jun 2016 10:13:04 +0200
Subject: t1404: rename file to t1404-update-ref-errors.sh

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

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t1404-update-ref-df-conflicts.sh | 181 -------------------------------------
 t/t1404-update-ref-errors.sh       | 181 +++++++++++++++++++++++++++++++++++++
 2 files changed, 181 insertions(+), 181 deletions(-)
 delete mode 100755 t/t1404-update-ref-df-conflicts.sh
 create mode 100755 t/t1404-update-ref-errors.sh

diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh
deleted file mode 100755
index 6d869d128..000000000
--- a/t/t1404-update-ref-df-conflicts.sh
+++ /dev/null
@@ -1,181 +0,0 @@
-#!/bin/sh
-
-test_description='Test git update-ref with D/F conflicts'
-. ./test-lib.sh
-
-test_update_rejected () {
-	prefix="$1" &&
-	before="$2" &&
-	pack="$3" &&
-	create="$4" &&
-	error="$5" &&
-	printf "create $prefix/%s $C\n" $before |
-	git update-ref --stdin &&
-	git for-each-ref $prefix >unchanged &&
-	if $pack
-	then
-		git pack-refs --all
-	fi &&
-	printf "create $prefix/%s $C\n" $create >input &&
-	test_must_fail git update-ref --stdin <input 2>output.err &&
-	grep -F "$error" output.err &&
-	git for-each-ref $prefix >actual &&
-	test_cmp unchanged actual
-}
-
-Q="'"
-
-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)
-
-'
-
-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" \
-		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x$Q"
-
-'
-
-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" \
-		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x$Q"
-
-'
-
-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" \
-		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x/y$Q"
-
-'
-
-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" \
-		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x/y$Q"
-
-'
-
-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" \
-		"$Q$prefix/c/x$Q exists; cannot create $Q$prefix/c$Q"
-
-'
-
-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" \
-		"$Q$prefix/c/x$Q exists; cannot create $Q$prefix/c$Q"
-
-'
-
-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" \
-		"$Q$prefix/c/x/y$Q exists; cannot create $Q$prefix/c$Q"
-
-'
-
-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" \
-		"$Q$prefix/c/x/y$Q exists; cannot create $Q$prefix/c$Q"
-
-'
-
-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" \
-		"cannot process $Q$prefix/c$Q and $Q$prefix/c/x$Q at the same time"
-
-'
-
-test_expect_success 'empty directory should not fool rev-parse' '
-	prefix=refs/e-rev-parse &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	echo "$C" >expected &&
-	git rev-parse $prefix/foo >actual &&
-	test_cmp expected actual
-'
-
-test_expect_success 'empty directory should not fool for-each-ref' '
-	prefix=refs/e-for-each-ref &&
-	git update-ref $prefix/foo $C &&
-	git for-each-ref $prefix >expected &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	git for-each-ref $prefix >actual &&
-	test_cmp expected actual
-'
-
-test_expect_success 'empty directory should not fool create' '
-	prefix=refs/e-create &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	printf "create %s $C\n" $prefix/foo |
-	git update-ref --stdin
-'
-
-test_expect_success 'empty directory should not fool verify' '
-	prefix=refs/e-verify &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	printf "verify %s $C\n" $prefix/foo |
-	git update-ref --stdin
-'
-
-test_expect_success 'empty directory should not fool 1-arg update' '
-	prefix=refs/e-update-1 &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	printf "update %s $D\n" $prefix/foo |
-	git update-ref --stdin
-'
-
-test_expect_success 'empty directory should not fool 2-arg update' '
-	prefix=refs/e-update-2 &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	printf "update %s $D $C\n" $prefix/foo |
-	git update-ref --stdin
-'
-
-test_expect_success 'empty directory should not fool 0-arg delete' '
-	prefix=refs/e-delete-0 &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	printf "delete %s\n" $prefix/foo |
-	git update-ref --stdin
-'
-
-test_expect_success 'empty directory should not fool 1-arg delete' '
-	prefix=refs/e-delete-1 &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	printf "delete %s $C\n" $prefix/foo |
-	git update-ref --stdin
-'
-
-test_done
diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
new file mode 100755
index 000000000..2818460ba
--- /dev/null
+++ b/t/t1404-update-ref-errors.sh
@@ -0,0 +1,181 @@
+#!/bin/sh
+
+test_description='Test git update-ref error handling'
+. ./test-lib.sh
+
+test_update_rejected () {
+	prefix="$1" &&
+	before="$2" &&
+	pack="$3" &&
+	create="$4" &&
+	error="$5" &&
+	printf "create $prefix/%s $C\n" $before |
+	git update-ref --stdin &&
+	git for-each-ref $prefix >unchanged &&
+	if $pack
+	then
+		git pack-refs --all
+	fi &&
+	printf "create $prefix/%s $C\n" $create >input &&
+	test_must_fail git update-ref --stdin <input 2>output.err &&
+	grep -F "$error" output.err &&
+	git for-each-ref $prefix >actual &&
+	test_cmp unchanged actual
+}
+
+Q="'"
+
+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)
+
+'
+
+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" \
+		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x$Q"
+
+'
+
+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" \
+		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x$Q"
+
+'
+
+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" \
+		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x/y$Q"
+
+'
+
+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" \
+		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x/y$Q"
+
+'
+
+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" \
+		"$Q$prefix/c/x$Q exists; cannot create $Q$prefix/c$Q"
+
+'
+
+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" \
+		"$Q$prefix/c/x$Q exists; cannot create $Q$prefix/c$Q"
+
+'
+
+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" \
+		"$Q$prefix/c/x/y$Q exists; cannot create $Q$prefix/c$Q"
+
+'
+
+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" \
+		"$Q$prefix/c/x/y$Q exists; cannot create $Q$prefix/c$Q"
+
+'
+
+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" \
+		"cannot process $Q$prefix/c$Q and $Q$prefix/c/x$Q at the same time"
+
+'
+
+test_expect_success 'empty directory should not fool rev-parse' '
+	prefix=refs/e-rev-parse &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	echo "$C" >expected &&
+	git rev-parse $prefix/foo >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'empty directory should not fool for-each-ref' '
+	prefix=refs/e-for-each-ref &&
+	git update-ref $prefix/foo $C &&
+	git for-each-ref $prefix >expected &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	git for-each-ref $prefix >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'empty directory should not fool create' '
+	prefix=refs/e-create &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	printf "create %s $C\n" $prefix/foo |
+	git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool verify' '
+	prefix=refs/e-verify &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	printf "verify %s $C\n" $prefix/foo |
+	git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool 1-arg update' '
+	prefix=refs/e-update-1 &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	printf "update %s $D\n" $prefix/foo |
+	git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool 2-arg update' '
+	prefix=refs/e-update-2 &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	printf "update %s $D $C\n" $prefix/foo |
+	git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool 0-arg delete' '
+	prefix=refs/e-delete-0 &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	printf "delete %s\n" $prefix/foo |
+	git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool 1-arg delete' '
+	prefix=refs/e-delete-1 &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	printf "delete %s $C\n" $prefix/foo |
+	git update-ref --stdin
+'
+
+test_done
-- 
cgit v1.2.1


From 0e4b63b5a8b8d369720f0671040113e347221042 Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Fri, 10 Jun 2016 08:50:53 +0200
Subject: t1404: remove "prefix" argument to test_update_rejected

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 2818460ba..541cad12a 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"
 
 '
-- 
cgit v1.2.1


From 017f7221abe6129a41c6a7d2b4ce990f477be74f Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Fri, 10 Jun 2016 08:55:40 +0200
Subject: t1404: document function test_update_rejected

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 541cad12a..7cfeaa99b 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" &&
-- 
cgit v1.2.1


From c5119dcf493a7b13b6a3e586e8d771a9e1d4975e Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Tue, 7 Jun 2016 12:29:02 +0200
Subject: t1404: add more tests of update-ref error handling

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

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 7cfeaa99b..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
-- 
cgit v1.2.1


From e3f510393c9d373f2969badc2b8afe179803a0fa Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Tue, 7 Jun 2016 09:29:23 +0200
Subject: lock_ref_for_update(): make error handling more uniform

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs/files-backend.c         | 74 +++++++++++++++++++++++++-------------------
 t/t1404-update-ref-errors.sh | 14 ++++-----
 2 files changed, 49 insertions(+), 39 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index bbf96ad83..6f8fecd02 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3388,6 +3388,38 @@ static const char *original_update_refname(struct ref_update *update)
 	return update->refname;
 }
 
+/*
+ * Check whether the REF_HAVE_OLD and old_oid values stored in update
+ * 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.
+ */
+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.
@@ -3433,7 +3465,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;
 	}
@@ -3447,28 +3479,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
@@ -3485,6 +3506,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
@@ -3495,20 +3519,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) &&
@@ -3530,7 +3540,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..c34ece48f 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 &&
-- 
cgit v1.2.1


From 841caad903f2b160e9f5ff05f961d20ad9085ddc Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Tue, 7 Jun 2016 09:32:08 +0200
Subject: lock_ref_for_update(): avoid a symref resolution

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 6f8fecd02..769e5c410 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3479,7 +3479,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': "
-- 
cgit v1.2.1