From: Michael Haggerty <mhagger@alum.mit.edu>
To: Junio C Hamano <gitster@pobox.com>,
David Turner <dturner@twopensource.com>
Cc: "Jeff King" <peff@peff.net>,
"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
git@vger.kernel.org, "Michael Haggerty" <mhagger@alum.mit.edu>
Subject: [PATCH 4/5] lock_ref_for_update(): make error handling more uniform
Date: Tue, 7 Jun 2016 13:50:09 +0200 [thread overview]
Message-ID: <4976e72fb492d5cfd2999a1fda4bc28f4ab92ae1.1465299118.git.mhagger@alum.mit.edu> (raw)
In-Reply-To: <cover.1465299118.git.mhagger@alum.mit.edu>
To aid the effort, extract a new function, check_old_oid(), and use it
in the two places where the read value of the reference has to be
checked against update->old_sha1.
Update tests to reflect the improvements.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 77 ++++++++++++++++++++++++++------------------
t/t1404-update-ref-errors.sh | 14 ++++----
2 files changed, 52 insertions(+), 39 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1230dfb..98c8b95 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3389,6 +3389,41 @@ static const char *original_update_refname(struct ref_update *update)
}
/*
+ * Check whether the REF_HAVE_OLD and old_oid values stored in update
+ * are consistent with the result read for the reference. error is
+ * true iff there was an error reading the reference; otherwise, oid
+ * is the value read for the reference.
+ *
+ * If there was a problem, write an error message to err and return
+ * -1.
+ */
+static int check_old_oid(struct ref_update *update, struct object_id *oid,
+ struct strbuf *err)
+{
+ if (!(update->flags & REF_HAVE_OLD) ||
+ !hashcmp(oid->hash, update->old_sha1))
+ return 0;
+
+ if (is_null_sha1(update->old_sha1))
+ strbuf_addf(err, "cannot lock ref '%s': "
+ "reference already exists",
+ original_update_refname(update));
+ else if (is_null_oid(oid))
+ strbuf_addf(err, "cannot lock ref '%s': "
+ "reference is missing but expected %s",
+ original_update_refname(update),
+ sha1_to_hex(update->old_sha1));
+ else
+ strbuf_addf(err, "cannot lock ref '%s': "
+ "is at %s but expected %s",
+ original_update_refname(update),
+ oid_to_hex(oid),
+ sha1_to_hex(update->old_sha1));
+
+ return -1;
+}
+
+/*
* Prepare for carrying out update:
* - Lock the reference referred to by update.
* - Read the reference under lock.
@@ -3432,7 +3467,7 @@ static int lock_ref_for_update(struct ref_update *update,
reason = strbuf_detach(err, NULL);
strbuf_addf(err, "cannot lock ref '%s': %s",
- update->refname, reason);
+ original_update_refname(update), reason);
free(reason);
return ret;
}
@@ -3446,28 +3481,17 @@ static int lock_ref_for_update(struct ref_update *update,
* the transaction, so we have to read it here
* to record and possibly check old_sha1:
*/
- if (read_ref_full(update->refname,
- mustexist ? RESOLVE_REF_READING : 0,
+ if (read_ref_full(update->refname, 0,
lock->old_oid.hash, NULL)) {
if (update->flags & REF_HAVE_OLD) {
strbuf_addf(err, "cannot lock ref '%s': "
- "can't resolve old value",
- update->refname);
- return TRANSACTION_GENERIC_ERROR;
- } else {
- hashclr(lock->old_oid.hash);
+ "error reading reference",
+ original_update_refname(update));
+ return -1;
}
- }
- if ((update->flags & REF_HAVE_OLD) &&
- hashcmp(lock->old_oid.hash, update->old_sha1)) {
- strbuf_addf(err, "cannot lock ref '%s': "
- "is at %s but expected %s",
- update->refname,
- sha1_to_hex(lock->old_oid.hash),
- sha1_to_hex(update->old_sha1));
+ } else if (check_old_oid(update, &lock->old_oid, err)) {
return TRANSACTION_GENERIC_ERROR;
}
-
} else {
/*
* Create a new update for the reference this
@@ -3484,6 +3508,9 @@ static int lock_ref_for_update(struct ref_update *update,
} else {
struct ref_update *parent_update;
+ if (check_old_oid(update, &lock->old_oid, err))
+ return TRANSACTION_GENERIC_ERROR;
+
/*
* If this update is happening indirectly because of a
* symref update, record the old SHA-1 in the parent
@@ -3494,20 +3521,6 @@ static int lock_ref_for_update(struct ref_update *update,
parent_update = parent_update->parent_update) {
oidcpy(&parent_update->lock->old_oid, &lock->old_oid);
}
-
- if ((update->flags & REF_HAVE_OLD) &&
- hashcmp(lock->old_oid.hash, update->old_sha1)) {
- if (is_null_sha1(update->old_sha1))
- strbuf_addf(err, "cannot lock ref '%s': reference already exists",
- original_update_refname(update));
- else
- strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
- original_update_refname(update),
- sha1_to_hex(lock->old_oid.hash),
- sha1_to_hex(update->old_sha1));
-
- return TRANSACTION_GENERIC_ERROR;
- }
}
if ((update->flags & REF_HAVE_NEW) &&
@@ -3529,7 +3542,7 @@ static int lock_ref_for_update(struct ref_update *update,
*/
update->lock = NULL;
strbuf_addf(err,
- "cannot update the ref '%s': %s",
+ "cannot update ref '%s': %s",
update->refname, write_err);
free(write_err);
return TRANSACTION_GENERIC_ERROR;
diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 0dcc16c..b4602df 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -232,7 +232,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 &&
@@ -279,7 +279,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 &&
@@ -298,7 +298,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 &&
@@ -367,13 +367,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 &&
@@ -386,13 +386,13 @@ test_expect_success 'broken reference blocks indirect create' '
echo "gobbledigook" >.git/$prefix/foo &&
test_when_finished "rm -f .git/$prefix/foo" &&
cat >expected <<-EOF &&
- fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken
+ fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken
EOF
printf "%s\n" "update $prefix/symref $C" |
test_must_fail git update-ref --stdin 2>output.err &&
test_cmp expected output.err &&
cat >expected <<-EOF &&
- fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken
+ fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken
EOF
printf "%s\n" "update $prefix/symref $D $C" |
test_must_fail git update-ref --stdin 2>output.err &&
--
2.8.1
next prev parent reply other threads:[~2016-06-07 11:50 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-07 11:50 [PATCH 0/5] Improve test coverage of update-ref error messages Michael Haggerty
2016-06-07 11:50 ` [PATCH 1/5] t1404: rename file to t1404-update-ref-errors.sh Michael Haggerty
2016-06-07 11:50 ` [PATCH 2/5] t1404: document function test_update_rejected Michael Haggerty
2016-06-07 16:57 ` Johannes Sixt
2016-06-07 19:25 ` Junio C Hamano
2016-06-09 15:45 ` Michael Haggerty
2016-06-09 16:05 ` Junio C Hamano
2016-06-10 6:30 ` Michael Haggerty
2016-06-07 11:50 ` [PATCH 3/5] t1404: add more tests of update-ref error handling Michael Haggerty
2016-06-07 11:50 ` Michael Haggerty [this message]
2016-06-07 11:50 ` [PATCH 5/5] lock_ref_for_update(): avoid a symref resolution Michael Haggerty
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4976e72fb492d5cfd2999a1fda4bc28f4ab92ae1.1465299118.git.mhagger@alum.mit.edu \
--to=mhagger@alum.mit.edu \
--cc=dturner@twopensource.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).