From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 25 Jul 2016 21:13:32 +0000 (-0700)
Subject: Merge branch 'mh/update-ref-errors'
X-Git-Tag: v2.10.0-rc0~97
X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/702ebbf4e2937accbac8184f87932f961e626a63?ds=inline;hp=-c

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
---

702ebbf4e2937accbac8184f87932f961e626a63
diff --combined refs/files-backend.c
index 4dd72b42c8,769e5c4100..1bf643025c
--- a/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;