{"thread":{"id":"42573","subject":"[PATCH v2 1/6] t1404: rename file to t1404-update-ref-errors.sh","startedAt":"2016-06-16T02:19:50Z","lastAt":"2016-06-16T02:19:53Z","messageCount":10,"participants":["Michael Haggerty","David Turner"],"isPatch":true,"patchVersion":2,"patchTotal":6},"messages":[{"id":"288871","messageId":"13a5d2e8b84bebaa6d826dd5b7cb78be057874c4.1465544913.git.mhagger@alum.mit.edu","threadId":"42573","inReplyTo":"cover.1465544913.git.mhagger@alum.mit.edu","subject":"[PATCH v2 1/6] t1404: rename file to t1404-update-ref-errors.sh","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-10T08:14:44Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"I want to broaden the scope of this test file, so rename it accordingly.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/{t1404-update-ref-df-conflicts.sh => t1404-update-ref-errors.sh} | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n rename t/{t1404-update-ref-df-conflicts.sh => t1404-update-ref-errors.sh} (98%)\n\ndiff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-errors.sh\nsimilarity index 98%\nrename from t/t1404-update-ref-df-conflicts.sh\nrename to t/t1404-update-ref-errors.sh\nindex 6d869d1..2818460 100755\n--- a/t/t1404-update-ref-df-conflicts.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='Test git update-ref with D/F conflicts'\n+test_description='Test git update-ref error handling'\n . ./test-lib.sh\n \n test_update_rejected () {\n-- \n2.8.1\n"},{"id":"288872","messageId":"cover.1465544913.git.mhagger@alum.mit.edu","threadId":"42573","inReplyTo":null,"subject":"[PATCH v2 0/6] Improve test coverage of update-ref error messages","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-10T08:14:43Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is v2 of the patch series. Thanks to Johannes Sixt and Junio for\ntheir comments about v1 [1].\n\nChanges since v1:\n\n* In t1404, don't use non-portable `local`: change function\n  test_update_rejected to refer to the global `$prefix` variable\n  rather than taking it as a parameter.\n\n* Because that code change has become more involved, split off the\n  addition of documentation for the function into a separate patch.\n\nThese patches are also available from my GitHub account [2] as branch\nupdate-ref-errors.\n\nMichael\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/296626\n[2] https://github.com/mhagger/git\n\nMichael Haggerty (6):\n  t1404: rename file to t1404-update-ref-errors.sh\n  t1404: remove \"prefix\" argument to test_update_rejected\n  t1404: document function test_update_rejected\n  t1404: add more tests of update-ref error handling\n  lock_ref_for_update(): make error handling more uniform\n  lock_ref_for_update(): avoid a symref resolution\n\n refs/files-backend.c               |  77 ++++---\n t/t1404-update-ref-df-conflicts.sh | 181 -----------------\n t/t1404-update-ref-errors.sh       | 407 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 452 insertions(+), 213 deletions(-)\n delete mode 100755 t/t1404-update-ref-df-conflicts.sh\n create mode 100755 t/t1404-update-ref-errors.sh\n\n-- \n2.8.1\n"},{"id":"288873","messageId":"484e4048dbc55314c668dcfb584bb2ad01f587cf.1465544913.git.mhagger@alum.mit.edu","threadId":"42573","inReplyTo":"cover.1465544913.git.mhagger@alum.mit.edu","subject":"[PATCH v2 4/6] t1404: add more tests of update-ref error handling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-10T08:14:47Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Some of the error messages will be improved in subsequent commits.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t1404-update-ref-errors.sh | 221 ++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 219 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex 7cfeaa9..5234b41f1 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -39,8 +39,9 @@ test_expect_success 'setup' '\n \tgit commit --allow-empty -m Initial &&\n \tC=$(git rev-parse HEAD) &&\n \tgit commit --allow-empty -m Second &&\n-\tD=$(git rev-parse HEAD)\n-\n+\tD=$(git rev-parse HEAD) &&\n+\tgit commit --allow-empty -m Third &&\n+\tE=$(git rev-parse HEAD)\n '\n \n test_expect_success 'existing loose ref is a simple prefix of new' '\n@@ -187,4 +188,220 @@ test_expect_success 'empty directory should not fool 1-arg delete' '\n \tgit update-ref --stdin\n '\n \n+# Test various errors when reading the old values of references...\n+\n+test_expect_success 'missing old value blocks update' '\n+\tprefix=refs/missing-update &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q\n+\tEOF\n+\tprintf \"%s\\n\" \"update $prefix/foo $E $D\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'incorrect old value blocks update' '\n+\tprefix=refs/incorrect-update &&\n+\tgit update-ref $prefix/foo $C &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: is at $C but expected $D\n+\tEOF\n+\tprintf \"%s\\n\" \"update $prefix/foo $E $D\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'existing old value blocks create' '\n+\tprefix=refs/existing-create &&\n+\tgit update-ref $prefix/foo $C &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: reference already exists\n+\tEOF\n+\tprintf \"%s\\n\" \"create $prefix/foo $E\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'incorrect old value blocks delete' '\n+\tprefix=refs/incorrect-delete &&\n+\tgit update-ref $prefix/foo $C &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: is at $C but expected $D\n+\tEOF\n+\tprintf \"%s\\n\" \"delete $prefix/foo $D\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'missing old value blocks indirect update' '\n+\tprefix=refs/missing-indirect-update &&\n+\tgit symbolic-ref $prefix/symref $prefix/foo &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q\n+\tEOF\n+\tprintf \"%s\\n\" \"update $prefix/symref $E $D\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'incorrect old value blocks indirect update' '\n+\tprefix=refs/incorrect-indirect-update &&\n+\tgit symbolic-ref $prefix/symref $prefix/foo &&\n+\tgit update-ref $prefix/foo $C &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D\n+\tEOF\n+\tprintf \"%s\\n\" \"update $prefix/symref $E $D\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'existing old value blocks indirect create' '\n+\tprefix=refs/existing-indirect-create &&\n+\tgit symbolic-ref $prefix/symref $prefix/foo &&\n+\tgit update-ref $prefix/foo $C &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: reference already exists\n+\tEOF\n+\tprintf \"%s\\n\" \"create $prefix/symref $E\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'incorrect old value blocks indirect delete' '\n+\tprefix=refs/incorrect-indirect-delete &&\n+\tgit symbolic-ref $prefix/symref $prefix/foo &&\n+\tgit update-ref $prefix/foo $C &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D\n+\tEOF\n+\tprintf \"%s\\n\" \"delete $prefix/symref $D\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'missing old value blocks indirect no-deref update' '\n+\tprefix=refs/missing-noderef-update &&\n+\tgit symbolic-ref $prefix/symref $prefix/foo &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: can${Q}t resolve old value\n+\tEOF\n+\tprintf \"%s\\n\" \"option no-deref\" \"update $prefix/symref $E $D\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'incorrect old value blocks indirect no-deref update' '\n+\tprefix=refs/incorrect-noderef-update &&\n+\tgit symbolic-ref $prefix/symref $prefix/foo &&\n+\tgit update-ref $prefix/foo $C &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D\n+\tEOF\n+\tprintf \"%s\\n\" \"option no-deref\" \"update $prefix/symref $E $D\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_failure 'existing old value blocks indirect no-deref create' '\n+\tprefix=refs/existing-noderef-create &&\n+\tgit symbolic-ref $prefix/symref $prefix/foo &&\n+\tgit update-ref $prefix/foo $C &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: reference already exists\n+\tEOF\n+\tprintf \"%s\\n\" \"option no-deref\" \"create $prefix/symref $E\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'incorrect old value blocks indirect no-deref delete' '\n+\tprefix=refs/incorrect-noderef-delete &&\n+\tgit symbolic-ref $prefix/symref $prefix/foo &&\n+\tgit update-ref $prefix/foo $C &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D\n+\tEOF\n+\tprintf \"%s\\n\" \"option no-deref\" \"delete $prefix/symref $D\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'non-empty directory blocks create' '\n+\tprefix=refs/ne-create &&\n+\tmkdir -p .git/$prefix/foo/bar &&\n+\t: >.git/$prefix/foo/bar/baz.lock &&\n+\ttest_when_finished \"rm -f .git/$prefix/foo/bar/baz.lock\" &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: there is a non-empty directory $Q.git/$prefix/foo$Q blocking reference $Q$prefix/foo$Q\n+\tEOF\n+\tprintf \"%s\\n\" \"update $prefix/foo $C\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q\n+\tEOF\n+\tprintf \"%s\\n\" \"update $prefix/foo $D $C\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'broken reference blocks create' '\n+\tprefix=refs/broken-create &&\n+\tmkdir -p .git/$prefix &&\n+\techo \"gobbledigook\" >.git/$prefix/foo &&\n+\ttest_when_finished \"rm -f .git/$prefix/foo\" &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken\n+\tEOF\n+\tprintf \"%s\\n\" \"update $prefix/foo $C\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken\n+\tEOF\n+\tprintf \"%s\\n\" \"update $prefix/foo $D $C\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'non-empty directory blocks indirect create' '\n+\tprefix=refs/ne-indirect-create &&\n+\tgit symbolic-ref $prefix/symref $prefix/foo &&\n+\tmkdir -p .git/$prefix/foo/bar &&\n+\t: >.git/$prefix/foo/bar/baz.lock &&\n+\ttest_when_finished \"rm -f .git/$prefix/foo/bar/baz.lock\" &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: there is a non-empty directory $Q.git/$prefix/foo$Q blocking reference $Q$prefix/foo$Q\n+\tEOF\n+\tprintf \"%s\\n\" \"update $prefix/symref $C\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q\n+\tEOF\n+\tprintf \"%s\\n\" \"update $prefix/symref $D $C\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n+test_expect_success 'broken reference blocks indirect create' '\n+\tprefix=refs/broken-indirect-create &&\n+\tgit symbolic-ref $prefix/symref $prefix/foo &&\n+\techo \"gobbledigook\" >.git/$prefix/foo &&\n+\ttest_when_finished \"rm -f .git/$prefix/foo\" &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken\n+\tEOF\n+\tprintf \"%s\\n\" \"update $prefix/symref $C\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err &&\n+\tcat >expected <<-EOF &&\n+\tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken\n+\tEOF\n+\tprintf \"%s\\n\" \"update $prefix/symref $D $C\" |\n+\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_cmp expected output.err\n+'\n+\n test_done\n-- \n2.8.1\n"},{"id":"288874","messageId":"cd193d74753c0c5e34995dc5c1df1e8881ba5bcb.1465544913.git.mhagger@alum.mit.edu","threadId":"42573","inReplyTo":"cover.1465544913.git.mhagger@alum.mit.edu","subject":"[PATCH v2 2/6] t1404: remove \"prefix\" argument to test_update_rejected","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-10T08:14:45Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The tests already set a variable called prefix and passed its value as\nthe first argument to this function. The old argument handling was\noverwriting the global variable with its same value rather than creating\na local variable.\n\nSo change test_update_rejected to refer to the global variable rather\nthan taking the prefix as an argument.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t1404-update-ref-errors.sh | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex 2818460..541cad1 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -4,11 +4,10 @@ test_description='Test git update-ref error handling'\n . ./test-lib.sh\n \n test_update_rejected () {\n-\tprefix=\"$1\" &&\n-\tbefore=\"$2\" &&\n-\tpack=\"$3\" &&\n-\tcreate=\"$4\" &&\n-\terror=\"$5\" &&\n+\tbefore=\"$1\" &&\n+\tpack=\"$2\" &&\n+\tcreate=\"$3\" &&\n+\terror=\"$4\" &&\n \tprintf \"create $prefix/%s $C\\n\" $before |\n \tgit update-ref --stdin &&\n \tgit for-each-ref $prefix >unchanged &&\n@@ -37,7 +36,7 @@ test_expect_success 'setup' '\n test_expect_success 'existing loose ref is a simple prefix of new' '\n \n \tprefix=refs/1l &&\n-\ttest_update_rejected $prefix \"a c e\" false \"b c/x d\" \\\n+\ttest_update_rejected \"a c e\" false \"b c/x d\" \\\n \t\t\"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x$Q\"\n \n '\n@@ -45,7 +44,7 @@ test_expect_success 'existing loose ref is a simple prefix of new' '\n test_expect_success 'existing packed ref is a simple prefix of new' '\n \n \tprefix=refs/1p &&\n-\ttest_update_rejected $prefix \"a c e\" true \"b c/x d\" \\\n+\ttest_update_rejected \"a c e\" true \"b c/x d\" \\\n \t\t\"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x$Q\"\n \n '\n@@ -53,7 +52,7 @@ test_expect_success 'existing packed ref is a simple prefix of new' '\n test_expect_success 'existing loose ref is a deeper prefix of new' '\n \n \tprefix=refs/2l &&\n-\ttest_update_rejected $prefix \"a c e\" false \"b c/x/y d\" \\\n+\ttest_update_rejected \"a c e\" false \"b c/x/y d\" \\\n \t\t\"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x/y$Q\"\n \n '\n@@ -61,7 +60,7 @@ test_expect_success 'existing loose ref is a deeper prefix of new' '\n test_expect_success 'existing packed ref is a deeper prefix of new' '\n \n \tprefix=refs/2p &&\n-\ttest_update_rejected $prefix \"a c e\" true \"b c/x/y d\" \\\n+\ttest_update_rejected \"a c e\" true \"b c/x/y d\" \\\n \t\t\"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x/y$Q\"\n \n '\n@@ -69,7 +68,7 @@ test_expect_success 'existing packed ref is a deeper prefix of new' '\n test_expect_success 'new ref is a simple prefix of existing loose' '\n \n \tprefix=refs/3l &&\n-\ttest_update_rejected $prefix \"a c/x e\" false \"b c d\" \\\n+\ttest_update_rejected \"a c/x e\" false \"b c d\" \\\n \t\t\"$Q$prefix/c/x$Q exists; cannot create $Q$prefix/c$Q\"\n \n '\n@@ -77,7 +76,7 @@ test_expect_success 'new ref is a simple prefix of existing loose' '\n test_expect_success 'new ref is a simple prefix of existing packed' '\n \n \tprefix=refs/3p &&\n-\ttest_update_rejected $prefix \"a c/x e\" true \"b c d\" \\\n+\ttest_update_rejected \"a c/x e\" true \"b c d\" \\\n \t\t\"$Q$prefix/c/x$Q exists; cannot create $Q$prefix/c$Q\"\n \n '\n@@ -85,7 +84,7 @@ test_expect_success 'new ref is a simple prefix of existing packed' '\n test_expect_success 'new ref is a deeper prefix of existing loose' '\n \n \tprefix=refs/4l &&\n-\ttest_update_rejected $prefix \"a c/x/y e\" false \"b c d\" \\\n+\ttest_update_rejected \"a c/x/y e\" false \"b c d\" \\\n \t\t\"$Q$prefix/c/x/y$Q exists; cannot create $Q$prefix/c$Q\"\n \n '\n@@ -93,7 +92,7 @@ test_expect_success 'new ref is a deeper prefix of existing loose' '\n test_expect_success 'new ref is a deeper prefix of existing packed' '\n \n \tprefix=refs/4p &&\n-\ttest_update_rejected $prefix \"a c/x/y e\" true \"b c d\" \\\n+\ttest_update_rejected \"a c/x/y e\" true \"b c d\" \\\n \t\t\"$Q$prefix/c/x/y$Q exists; cannot create $Q$prefix/c$Q\"\n \n '\n@@ -101,7 +100,7 @@ test_expect_success 'new ref is a deeper prefix of existing packed' '\n test_expect_success 'one new ref is a simple prefix of another' '\n \n \tprefix=refs/5 &&\n-\ttest_update_rejected $prefix \"a e\" false \"b c c/x d\" \\\n+\ttest_update_rejected \"a e\" false \"b c c/x d\" \\\n \t\t\"cannot process $Q$prefix/c$Q and $Q$prefix/c/x$Q at the same time\"\n \n '\n-- \n2.8.1\n"},{"id":"288875","messageId":"8d628cac9086238c1507c08838963a7900a1cd32.1465544913.git.mhagger@alum.mit.edu","threadId":"42573","inReplyTo":"cover.1465544913.git.mhagger@alum.mit.edu","subject":"[PATCH v2 3/6] t1404: document function test_update_rejected","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-10T08:14:46Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t1404-update-ref-errors.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex 541cad1..7cfeaa9 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -3,6 +3,16 @@\n test_description='Test git update-ref error handling'\n . ./test-lib.sh\n \n+# Create some references, perhaps run pack-refs --all, then try to\n+# create some more references. Ensure that the second creation fails\n+# with the correct error message.\n+# Usage: test_update_rejected <before> <pack> <create> <error>\n+#   <before> is a ws-separated list of refs to create before the test\n+#   <pack> (true or false) tells whether to pack the refs before the test\n+#   <create> is a list of variables to attempt creating\n+#   <error> is a string to look for in the stderr of update-ref.\n+# All references are created in the namespace specified by the current\n+# value of $prefix.\n test_update_rejected () {\n \tbefore=\"$1\" &&\n \tpack=\"$2\" &&\n-- \n2.8.1\n"},{"id":"288876","messageId":"b8c6f36a6291ad5a864d6fc05e3f90da578416b0.1465544913.git.mhagger@alum.mit.edu","threadId":"42573","inReplyTo":"cover.1465544913.git.mhagger@alum.mit.edu","subject":"[PATCH v2 6/6] lock_ref_for_update(): avoid a symref resolution","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-10T08:14:49Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If we're overwriting a symref with a SHA-1, we need to resolve the value\nof the symref (1) to check against update->old_sha1 and (2) to write to\nits reflog. However, we've already read the symref itself and know its\nreferent. So there is no need to read the symref's value through the\nsymref; we can read the referent directly.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 98c8b95..b8d7a9a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3481,7 +3481,7 @@ static int lock_ref_for_update(struct ref_update *update,\n \t\t\t * the transaction, so we have to read it here\n \t\t\t * to record and possibly check old_sha1:\n \t\t\t */\n-\t\t\tif (read_ref_full(update->refname, 0,\n+\t\t\tif (read_ref_full(referent.buf, 0,\n \t\t\t\t\t  lock->old_oid.hash, NULL)) {\n \t\t\t\tif (update->flags & REF_HAVE_OLD) {\n \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n-- \n2.8.1\n"},{"id":"288877","messageId":"6cc13da6767c5fd9dbf1b372634adf7c3a353464.1465544913.git.mhagger@alum.mit.edu","threadId":"42573","inReplyTo":"cover.1465544913.git.mhagger@alum.mit.edu","subject":"[PATCH v2 5/6] lock_ref_for_update(): make error handling more uniform","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-10T08:14:48Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"To aid the effort, extract a new function, check_old_oid(), and use it\nin the two places where the read value of the reference has to be\nchecked against update->old_sha1.\n\nUpdate tests to reflect the improvements.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c         | 77 ++++++++++++++++++++++++++------------------\n t/t1404-update-ref-errors.sh | 14 ++++----\n 2 files changed, 52 insertions(+), 39 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 1230dfb..98c8b95 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3389,6 +3389,41 @@ static const char *original_update_refname(struct ref_update *update)\n }\n \n /*\n+ * Check whether the REF_HAVE_OLD and old_oid values stored in update\n+ * are consistent with the result read for the reference. error is\n+ * true iff there was an error reading the reference; otherwise, oid\n+ * is the value read for the reference.\n+ *\n+ * If there was a problem, write an error message to err and return\n+ * -1.\n+ */\n+static int check_old_oid(struct ref_update *update, struct object_id *oid,\n+\t\t\t struct strbuf *err)\n+{\n+\tif (!(update->flags & REF_HAVE_OLD) ||\n+\t\t   !hashcmp(oid->hash, update->old_sha1))\n+\t\treturn 0;\n+\n+\tif (is_null_sha1(update->old_sha1))\n+\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n+\t\t\t    \"reference already exists\",\n+\t\t\t    original_update_refname(update));\n+\telse if (is_null_oid(oid))\n+\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n+\t\t\t    \"reference is missing but expected %s\",\n+\t\t\t    original_update_refname(update),\n+\t\t\t    sha1_to_hex(update->old_sha1));\n+\telse\n+\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n+\t\t\t    \"is at %s but expected %s\",\n+\t\t\t    original_update_refname(update),\n+\t\t\t    oid_to_hex(oid),\n+\t\t\t    sha1_to_hex(update->old_sha1));\n+\n+\treturn -1;\n+}\n+\n+/*\n  * Prepare for carrying out update:\n  * - Lock the reference referred to by update.\n  * - Read the reference under lock.\n@@ -3432,7 +3467,7 @@ static int lock_ref_for_update(struct ref_update *update,\n \n \t\treason = strbuf_detach(err, NULL);\n \t\tstrbuf_addf(err, \"cannot lock ref '%s': %s\",\n-\t\t\t    update->refname, reason);\n+\t\t\t    original_update_refname(update), reason);\n \t\tfree(reason);\n \t\treturn ret;\n \t}\n@@ -3446,28 +3481,17 @@ static int lock_ref_for_update(struct ref_update *update,\n \t\t\t * the transaction, so we have to read it here\n \t\t\t * to record and possibly check old_sha1:\n \t\t\t */\n-\t\t\tif (read_ref_full(update->refname,\n-\t\t\t\t\t  mustexist ? RESOLVE_REF_READING : 0,\n+\t\t\tif (read_ref_full(update->refname, 0,\n \t\t\t\t\t  lock->old_oid.hash, NULL)) {\n \t\t\t\tif (update->flags & REF_HAVE_OLD) {\n \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n-\t\t\t\t\t\t    \"can't resolve old value\",\n-\t\t\t\t\t\t    update->refname);\n-\t\t\t\t\treturn TRANSACTION_GENERIC_ERROR;\n-\t\t\t\t} else {\n-\t\t\t\t\thashclr(lock->old_oid.hash);\n+\t\t\t\t\t\t    \"error reading reference\",\n+\t\t\t\t\t\t    original_update_refname(update));\n+\t\t\t\t\treturn -1;\n \t\t\t\t}\n-\t\t\t}\n-\t\t\tif ((update->flags & REF_HAVE_OLD) &&\n-\t\t\t    hashcmp(lock->old_oid.hash, update->old_sha1)) {\n-\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n-\t\t\t\t\t    \"is at %s but expected %s\",\n-\t\t\t\t\t    update->refname,\n-\t\t\t\t\t    sha1_to_hex(lock->old_oid.hash),\n-\t\t\t\t\t    sha1_to_hex(update->old_sha1));\n+\t\t\t} else if (check_old_oid(update, &lock->old_oid, err)) {\n \t\t\t\treturn TRANSACTION_GENERIC_ERROR;\n \t\t\t}\n-\n \t\t} else {\n \t\t\t/*\n \t\t\t * Create a new update for the reference this\n@@ -3484,6 +3508,9 @@ static int lock_ref_for_update(struct ref_update *update,\n \t} else {\n \t\tstruct ref_update *parent_update;\n \n+\t\tif (check_old_oid(update, &lock->old_oid, err))\n+\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\n \t\t/*\n \t\t * If this update is happening indirectly because of a\n \t\t * symref update, record the old SHA-1 in the parent\n@@ -3494,20 +3521,6 @@ static int lock_ref_for_update(struct ref_update *update,\n \t\t     parent_update = parent_update->parent_update) {\n \t\t\toidcpy(&parent_update->lock->old_oid, &lock->old_oid);\n \t\t}\n-\n-\t\tif ((update->flags & REF_HAVE_OLD) &&\n-\t\t    hashcmp(lock->old_oid.hash, update->old_sha1)) {\n-\t\t\tif (is_null_sha1(update->old_sha1))\n-\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': reference already exists\",\n-\t\t\t\t\t    original_update_refname(update));\n-\t\t\telse\n-\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': is at %s but expected %s\",\n-\t\t\t\t\t    original_update_refname(update),\n-\t\t\t\t\t    sha1_to_hex(lock->old_oid.hash),\n-\t\t\t\t\t    sha1_to_hex(update->old_sha1));\n-\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n-\t\t}\n \t}\n \n \tif ((update->flags & REF_HAVE_NEW) &&\n@@ -3529,7 +3542,7 @@ static int lock_ref_for_update(struct ref_update *update,\n \t\t\t */\n \t\t\tupdate->lock = NULL;\n \t\t\tstrbuf_addf(err,\n-\t\t\t\t    \"cannot update the ref '%s': %s\",\n+\t\t\t\t    \"cannot update ref '%s': %s\",\n \t\t\t\t    update->refname, write_err);\n \t\t\tfree(write_err);\n \t\t\treturn TRANSACTION_GENERIC_ERROR;\ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex 5234b41f1..c34ece4 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -237,7 +237,7 @@ test_expect_success 'missing old value blocks indirect update' '\n \tprefix=refs/missing-indirect-update &&\n \tgit symbolic-ref $prefix/symref $prefix/foo &&\n \tcat >expected <<-EOF &&\n-\tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/symref $E $D\" |\n \ttest_must_fail git update-ref --stdin 2>output.err &&\n@@ -284,7 +284,7 @@ test_expect_success 'missing old value blocks indirect no-deref update' '\n \tprefix=refs/missing-noderef-update &&\n \tgit symbolic-ref $prefix/symref $prefix/foo &&\n \tcat >expected <<-EOF &&\n-\tfatal: cannot lock ref $Q$prefix/symref$Q: can${Q}t resolve old value\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: reference is missing but expected $D\n \tEOF\n \tprintf \"%s\\n\" \"option no-deref\" \"update $prefix/symref $E $D\" |\n \ttest_must_fail git update-ref --stdin 2>output.err &&\n@@ -303,7 +303,7 @@ test_expect_success 'incorrect old value blocks indirect no-deref update' '\n \ttest_cmp expected output.err\n '\n \n-test_expect_failure 'existing old value blocks indirect no-deref create' '\n+test_expect_success 'existing old value blocks indirect no-deref create' '\n \tprefix=refs/existing-noderef-create &&\n \tgit symbolic-ref $prefix/symref $prefix/foo &&\n \tgit update-ref $prefix/foo $C &&\n@@ -372,13 +372,13 @@ test_expect_success 'non-empty directory blocks indirect create' '\n \t: >.git/$prefix/foo/bar/baz.lock &&\n \ttest_when_finished \"rm -f .git/$prefix/foo/bar/baz.lock\" &&\n \tcat >expected <<-EOF &&\n-\tfatal: cannot lock ref $Q$prefix/foo$Q: there is a non-empty directory $Q.git/$prefix/foo$Q blocking reference $Q$prefix/foo$Q\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: there is a non-empty directory $Q.git/$prefix/foo$Q blocking reference $Q$prefix/foo$Q\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/symref $C\" |\n \ttest_must_fail git update-ref --stdin 2>output.err &&\n \ttest_cmp expected output.err &&\n \tcat >expected <<-EOF &&\n-\tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/symref $D $C\" |\n \ttest_must_fail git update-ref --stdin 2>output.err &&\n@@ -391,13 +391,13 @@ test_expect_success 'broken reference blocks indirect create' '\n \techo \"gobbledigook\" >.git/$prefix/foo &&\n \ttest_when_finished \"rm -f .git/$prefix/foo\" &&\n \tcat >expected <<-EOF &&\n-\tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/symref $C\" |\n \ttest_must_fail git update-ref --stdin 2>output.err &&\n \ttest_cmp expected output.err &&\n \tcat >expected <<-EOF &&\n-\tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken\n+\tfatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/symref $D $C\" |\n \ttest_must_fail git update-ref --stdin 2>output.err &&\n-- \n2.8.1\n"},{"id":"288938","messageId":"1465585279.8278.2.camel@frank","threadId":"42573","inReplyTo":"6cc13da6767c5fd9dbf1b372634adf7c3a353464.1465544913.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 5/6] lock_ref_for_update(): make error handling more uniform","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-06-10T19:01:19Z","receivedAt":"2016-06-16T02:19:51Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2016-06-10 at 10:14 +0200, Michael Haggerty wrote:\n\n>  /*\n> + * Check whether the REF_HAVE_OLD and old_oid values stored in update\n> + * are consistent with the result read for the reference. error is\n> + * true iff there was an error reading the reference; otherwise, oid\n\n\"error\" is not a thing here?\n"},{"id":"289083","messageId":"575E5DB5.8040109@alum.mit.edu","threadId":"42573","inReplyTo":"1465585279.8278.2.camel@frank","subject":"Re: [PATCH v2 5/6] lock_ref_for_update(): make error handling more uniform","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-13T07:16:05Z","receivedAt":"2016-06-16T02:19:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/10/2016 09:01 PM, David Turner wrote:\n> On Fri, 2016-06-10 at 10:14 +0200, Michael Haggerty wrote:\n> \n>>  /*\n>> + * Check whether the REF_HAVE_OLD and old_oid values stored in update\n>> + * are consistent with the result read for the reference. error is\n>> + * true iff there was an error reading the reference; otherwise, oid\n> \n> \"error\" is not a thing here?\n\nYou're right; thanks for the feedback. I'll include it in the reroll\nthat I'm about to do.\n\nMichael\n"},{"id":"289097","messageId":"575E868B.70402@alum.mit.edu","threadId":"42573","inReplyTo":"575E5DB5.8040109@alum.mit.edu","subject":"Re: [PATCH v2 5/6] lock_ref_for_update(): make error handling more uniform","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-13T10:10:19Z","receivedAt":"2016-06-16T02:19:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/13/2016 09:16 AM, Michael Haggerty wrote:\n> On 06/10/2016 09:01 PM, David Turner wrote:\n>> On Fri, 2016-06-10 at 10:14 +0200, Michael Haggerty wrote:\n>>\n>>>  /*\n>>> + * Check whether the REF_HAVE_OLD and old_oid values stored in update\n>>> + * are consistent with the result read for the reference. error is\n>>> + * true iff there was an error reading the reference; otherwise, oid\n>>\n>> \"error\" is not a thing here?\n> \n> You're right; thanks for the feedback. I'll include it in the reroll\n> that I'm about to do.\n\nI have changed the docstring as follows:\n\n>  /*\n>   * Check whether the REF_HAVE_OLD and old_oid values stored in update\n> - * are consistent with the result read for the reference. error is\n> - * true iff there was an error reading the reference; otherwise, oid\n> - * is the value read for the reference.\n> - *\n> - * If there was a problem, write an error message to err and return\n> - * -1.\n> + * are consistent with oid, which is the reference's current value. If\n> + * everything is OK, return 0; otherwise, write an error message to\n> + * err and return -1.\n>   */\n\nI've folded that change into the patch series on branch\nupdate-ref-errors on my GitHub fork [1]. I won't sent a re-roll to the\nmailing list unless other changes are necessary (or unless somebody\nrequests it).\n\nMichael\n\n[1] https://github.com/mhagger/git\n"}]}