Merge branch 'mh/update-ref-errors'

Error handling in the codepaths that updates refs has been
improved.

* mh/update-ref-errors:
lock_ref_for_update(): avoid a symref resolution
lock_ref_for_update(): make error handling more uniform
t1404: add more tests of update-ref error handling
t1404: document function test_update_rejected
t1404: remove "prefix" argument to test_update_rejected
t1404: rename file to t1404-update-ref-errors.sh

diff --combined refs/files-backend.c
index 4dd72b42c88e4e125ec5fa108d3e81bfc8a44483,769e5c410043afd2f0d4304fbef3850662437ed0..1bf643025cf25f34f3f0ea3f28822459a0b0a11a
--- 1/refs/files-backend.c
--- 2/refs/files-backend.c
+++ b/refs/files-backend.c
@@@ -1929,14 -1929,14 +1929,14 @@@ static int verify_lock(struct ref_lock 
                        errno = save_errno;
                        return -1;
                } else {
 -                      hashclr(lock->old_oid.hash);
 +                      oidclr(&lock->old_oid);
                        return 0;
                }
        }
        if (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) {
                strbuf_addf(err, "ref '%s' is at %s but expected %s",
                            lock->ref_name,
 -                          sha1_to_hex(lock->old_oid.hash),
 +                          oid_to_hex(&lock->old_oid),
                            sha1_to_hex(old_sha1));
                errno = EBUSY;
                return -1;
@@@ -3388,6 -3388,38 +3388,38 @@@ static const char *original_update_refn
        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 +3465,7 @@@ static int lock_ref_for_update(struct r
  
                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 +3479,17 @@@
                         * 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(referent.buf, 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 +3506,9 @@@
        } 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 +3519,6 @@@
                     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 +3540,7 @@@
                         */
                        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;