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>

diff --git a/refs/files-backend.c b/refs/files-backend.c
index bbf96ad83a5abc494fbbd99d2bd9cceef6252e97..6f8fecd024d177afe2427d58fae603c52a4ee156 100644 (file)
--- 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;
 }
 
        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.
 /*
  * 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",
 
                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;
        }
                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:
                         */
                         * 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': "
                                          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;
                        }
                                return TRANSACTION_GENERIC_ERROR;
                        }

-

                } else {
                        /*
                         * Create a new update for the reference this
                } 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;
 
        } 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
                /*
                 * 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);
                }
                     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) &&
        }
 
        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,
                         */
                        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;
                                    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 5234b41f1452fda8a7ef48287b0e304cee477005..c34ece48f5ad77a6d53b5f0a1dcf7d53d7413837 100755 (executable)
--- 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 &&
        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 &&
        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 &&
        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 &&
        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_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 &&
        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 &&
        : >.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 &&
        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 &&
        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 &&
        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 &&
        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 &&
        EOF
        printf "%s\n" "update $prefix/symref $D $C" |
        test_must_fail git update-ref --stdin 2>output.err &&