[PATCH] refs.c: clean up write_ref_sha1 returns

[Stefan Beller <sbeller@google.com>]

write_ref_sha1 now either returns 0 for a successful write or !=0 if an
error occurred. This cleanup results in cleaning the code at other places
as well where we had to set force_write to make the write_ref_sha1(...)
|| commit_ref(...) combination work. Also the checks for the optimisation
of old and new sha1 values being the same has been moved to a helper
function so that check is not part of write_ref_sha1 or commit_ref any
more.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    v1:
    
    applies on top of origin/sb/atomic-push-fix (79370dd9656)
    
    This undoes some of the changes of previous patches
    such as removing the check if old and new sha1 are equal
    and not being forced to rewrite the value.
    
    Junio, I think this patch adresses the concerns you raised.
    I can redo the atomic-push-fix series with this cleanup merged
    into the appropriate patches or you could just queue it on top 
    of said series.

 refs.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index 2594b23..c8fd4a4 100644
--- a/refs.c
+++ b/refs.c
@@ -2817,9 +2817,6 @@ static int close_ref(struct ref_lock *lock)
 
 static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
 {
-	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
-		return 0;
-
 	if (commit_lock_file(lock->lk))
 		return -1;
 	return 0;
@@ -2880,7 +2877,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 		error("unable to lock %s for update", newrefname);
 		goto rollback;
 	}
-	lock->force_write = 1;
 	hashcpy(lock->old_sha1, orig_sha1);
 	if (write_ref_sha1(lock, orig_sha1, logmsg)
 	    || commit_ref(lock, orig_sha1)) {
@@ -2899,7 +2895,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 		goto rollbacklog;
 	}
 
-	lock->force_write = 1;
 	flag = log_all_ref_updates;
 	log_all_ref_updates = 0;
 	if (write_ref_sha1(lock, orig_sha1, NULL)
@@ -3062,12 +3057,22 @@ int is_branch(const char *refname)
 	return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
 }
 
+static int should_write_ref_sha1(struct ref_lock *lock,
+				 const unsigned char *new_sha1)
+{
+	/* If the old and new sha1 are equal only write if forced to. */
+	if (!lock->force_write && !hashcmp(lock->old_sha1, new_sha1))
+		return 0;
+	/* null sha indicates deletion, so don't write it */
+	return !is_null_sha1(new_sha1);
+}
+
 /*
  * Write sha1 into the ref specified by the lock. Make sure that errno
  * is sane on error.
  */
-static int write_ref_sha1(struct ref_lock *lock,
-	const unsigned char *sha1, const char *logmsg)
+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
+			  const char *logmsg)
 {
 	static char term = '\n';
 	struct object *o;
@@ -3076,8 +3081,6 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = EINVAL;
 		return -1;
 	}
-	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
-		return 0;
 
 	o = parse_object(sha1);
 	if (!o) {
@@ -3752,7 +3755,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				    update->refname);
 			goto cleanup;
 		}
-		if (!is_null_sha1(update->new_sha1)) {
+		if (should_write_ref_sha1(update->lock, update->new_sha1)) {
 			if (write_ref_sha1(update->lock, update->new_sha1,
 					   update->msg)) {
 				strbuf_addf(err, "Cannot write to the ref lock '%s'.",
@@ -3769,7 +3772,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		if (!is_null_sha1(update->new_sha1)) {
+		if (should_write_ref_sha1(update->lock, update->new_sha1)) {
 			if (commit_ref(update->lock, update->new_sha1)) {
 				strbuf_addf(err, "Cannot update the ref '%s'.",
 					    update->refname);
@@ -3785,7 +3788,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		if (update->lock) {
+		if (is_null_sha1(update->new_sha1)) {
 			if (delete_ref_loose(update->lock, update->type, err)) {
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
-- 
2.2.1.62.g3f15098