{"thread":{"id":"42554","subject":"[PATCH 0/5] Improve test coverage of update-ref error messages","startedAt":"2016-06-07T11:50:05Z","lastAt":"2016-06-16T02:19:50Z","messageCount":11,"participants":["Michael Haggerty","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"288547","messageId":"cover.1465299118.git.mhagger@alum.mit.edu","threadId":"42554","inReplyTo":null,"subject":"[PATCH 0/5] Improve test coverage of update-ref error messages","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-07T11:50:05Z","receivedAt":"2016-06-07T11:50:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The logic for deciding what error to emit under various update-ref\nfailure scenarios is nontrivial. Add a bunch of tests of these error\nmessages. Then improve a few error messages that were not ideal. One\nor two other error messages could still be improved, but that would\ntake more work so I will skip it for now.\n\nThis patch series applies on top of mh/split-under-lock. Improving the\ntesting of that patch series was in fact the main motivation for this\none. It didn't turn up any serious errors, so I don't think that this\npatch series needs to hold up the other one. On the other hand, each\nof the patch series changes some error messages, so if possible it\nwould be preferable to ship them in the same Git release.\n\nHappily, this branch doesn't conflict with either of the other large\npatch series that are queued up in this area (mh/ref-iterators [2] and\nref-store [3]).\n\nThese patches are also available from my GitHub account [4] as branch\nupdate-ref-errors.\n\nMichael\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/293800\n[2] http://thread.gmane.org/gmane.comp.version-control.git/296322\n[3] http://thread.gmane.org/gmane.comp.version-control.git/296409\n[4] https://github.com/mhagger/git\n\nMichael Haggerty (5):\n  t1404: rename file to t1404-update-ref-errors.sh\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       | 402 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 447 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":"288550","messageId":"13a5d2e8b84bebaa6d826dd5b7cb78be057874c4.1465299118.git.mhagger@alum.mit.edu","threadId":"42554","inReplyTo":"cover.1465299118.git.mhagger@alum.mit.edu","subject":"[PATCH 1/5] t1404: rename file to t1404-update-ref-errors.sh","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-07T11:50:06Z","receivedAt":"2016-06-07T11:50:06Z","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":"288551","messageId":"2561c38f85648144d665e870ca42c133a3a78909.1465299118.git.mhagger@alum.mit.edu","threadId":"42554","inReplyTo":"cover.1465299118.git.mhagger@alum.mit.edu","subject":"[PATCH 2/5] t1404: document function test_update_rejected","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-07T11:50:07Z","receivedAt":"2016-06-07T11:50:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"And declare its variables to be local.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t1404-update-ref-errors.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex 2818460..a62d62a 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -3,7 +3,11 @@\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 test_update_rejected () {\n+\tlocal prefix before pack create error &&\n \tprefix=\"$1\" &&\n \tbefore=\"$2\" &&\n \tpack=\"$3\" &&\n-- \n2.8.1\n"},{"id":"288549","messageId":"9077546cf7c0fd3968f4cca34e75b92e395f88e1.1465299118.git.mhagger@alum.mit.edu","threadId":"42554","inReplyTo":"cover.1465299118.git.mhagger@alum.mit.edu","subject":"[PATCH 3/5] t1404: add more tests of update-ref error handling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-07T11:50:08Z","receivedAt":"2016-06-07T11:50:08Z","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 a62d62a..0dcc16c 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -34,8 +34,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@@ -182,4 +183,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":"288552","messageId":"4976e72fb492d5cfd2999a1fda4bc28f4ab92ae1.1465299118.git.mhagger@alum.mit.edu","threadId":"42554","inReplyTo":"cover.1465299118.git.mhagger@alum.mit.edu","subject":"[PATCH 4/5] lock_ref_for_update(): make error handling more uniform","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-07T11:50:09Z","receivedAt":"2016-06-07T11:50:09Z","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 0dcc16c..b4602df 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -232,7 +232,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@@ -279,7 +279,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@@ -298,7 +298,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@@ -367,13 +367,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@@ -386,13 +386,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":"288548","messageId":"dd34c6a5ef249f4ffb8c8cb7238889e84e19f82c.1465299118.git.mhagger@alum.mit.edu","threadId":"42554","inReplyTo":"cover.1465299118.git.mhagger@alum.mit.edu","subject":"[PATCH 5/5] lock_ref_for_update(): avoid a symref resolution","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-07T11:50:10Z","receivedAt":"2016-06-07T11:50:10Z","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":"288618","messageId":"5756FCF3.7050509@kdbg.org","threadId":"42554","inReplyTo":"2561c38f85648144d665e870ca42c133a3a78909.1465299118.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 2/5] t1404: document function test_update_rejected","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-06-07T16:57:23Z","receivedAt":"2016-06-07T16:57:23Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 07.06.2016 um 13:50 schrieb Michael Haggerty:\n>   test_update_rejected () {\n> +\tlocal prefix before pack create error &&\n\nDo we want to add more of unportable 'local' declarations?\n\n>   \tprefix=\"$1\" &&\n>   \tbefore=\"$2\" &&\n>   \tpack=\"$3\" &&\n\n-- Hannes\n"},{"id":"288633","messageId":"xmqqeg88pzri.fsf@gitster.mtv.corp.google.com","threadId":"42554","inReplyTo":"5756FCF3.7050509@kdbg.org","subject":"Re: [PATCH 2/5] t1404: document function test_update_rejected","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-07T19:25:37Z","receivedAt":"2016-06-07T19:25:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Am 07.06.2016 um 13:50 schrieb Michael Haggerty:\n>>   test_update_rejected () {\n>> +\tlocal prefix before pack create error &&\n>\n> Do we want to add more of unportable 'local' declarations?\n\nNot really ;-)  Thanks for catching.\n\n>>   \tprefix=\"$1\" &&\n>>   \tbefore=\"$2\" &&\n>>   \tpack=\"$3\" &&\n>\n> -- Hannes\n"},{"id":"288816","messageId":"57598F2F.9050806@alum.mit.edu","threadId":"42554","inReplyTo":"5756FCF3.7050509@kdbg.org","subject":"Re: [PATCH 2/5] t1404: document function test_update_rejected","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-09T15:45:51Z","receivedAt":"2016-06-16T02:19:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/07/2016 06:57 PM, Johannes Sixt wrote:\n> Am 07.06.2016 um 13:50 schrieb Michael Haggerty:\n>>   test_update_rejected () {\n>> +    local prefix before pack create error &&\n> \n> Do we want to add more of unportable 'local' declarations?\n\nSorry, I forgot that `local` is not in the POSIX standard.\n\nAccording to [1] it's nevertheless very portable. Do you object to\n`local` based only on its absence from POSIX, or because it's known to\ncause problems in the real world? I ask because it is a convenient\nfeature (and its lack is a common cause of mysterious errors), so it\nwould be nice if we could allow its use.\n\nMichael\n\n[1] http://apenwarr.ca/log/?m=201102#28\n"},{"id":"288818","messageId":"xmqqa8iujqkv.fsf@gitster.mtv.corp.google.com","threadId":"42554","inReplyTo":"57598F2F.9050806@alum.mit.edu","subject":"Re: [PATCH 2/5] t1404: document function test_update_rejected","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-09T16:05:04Z","receivedAt":"2016-06-16T02:19:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> On 06/07/2016 06:57 PM, Johannes Sixt wrote:\n>> Am 07.06.2016 um 13:50 schrieb Michael Haggerty:\n>>>   test_update_rejected () {\n>>> +    local prefix before pack create error &&\n>> \n>> Do we want to add more of unportable 'local' declarations?\n>\n> Sorry, I forgot that `local` is not in the POSIX standard.\n\nRegarding portability we say three things.\n\n * It is supported practically everywhere, and it is even in POSIX,\n   so let's use it.\n\n * Even this is not in POSIX, it is supported practically\n   everywhere, and it is too cumbersome to do things without using\n   it, so let's use it.\n\n * It is not available in some platforms we (collectively) still\n   care; it is not even in POSIX.  Don't use it.\n\nI think \"local\" falls into the third one.\n\n: bash; ksh\n$ v=1\n$ f () { local v; v=2; echo f; }\n$ f\nksh: local: not found [No such file or directory]\nf\n$ echo $v\n2\n$ exit\n: bash;\n"},{"id":"288865","messageId":"575A5E9C.2050308@alum.mit.edu","threadId":"42554","inReplyTo":"xmqqa8iujqkv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/5] t1404: document function test_update_rejected","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-10T06:30:52Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/09/2016 06:05 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> On 06/07/2016 06:57 PM, Johannes Sixt wrote:\n>>> Am 07.06.2016 um 13:50 schrieb Michael Haggerty:\n>>>>   test_update_rejected () {\n>>>> +    local prefix before pack create error &&\n>>>\n>>> Do we want to add more of unportable 'local' declarations?\n>>\n>> Sorry, I forgot that `local` is not in the POSIX standard.\n> \n> Regarding portability we say three things.\n> \n>  * It is supported practically everywhere, and it is even in POSIX,\n>    so let's use it.\n> \n>  * Even this is not in POSIX, it is supported practically\n>    everywhere, and it is too cumbersome to do things without using\n>    it, so let's use it.\n> \n>  * It is not available in some platforms we (collectively) still\n>    care; it is not even in POSIX.  Don't use it.\n> \n> I think \"local\" falls into the third one.\n> \n> : bash; ksh\n> $ v=1\n> $ f () { local v; v=2; echo f; }\n> $ f\n> ksh: local: not found [No such file or directory]\n> f\n> $ echo $v\n> 2\n> $ exit\n> : bash;\n\nOK, I didn't realize that we still support ksh. I will make the change.\n\nMichael\n"}]}