Re: [PATCH v11 39/41] refs.c: propagate any errno==ENOTDIR from _commit back to the callers

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Ronnie Sahlberg wrote:

> For these cases, save the errno value and abort and make sure that the caller
> can see errno==ENOTDIR.
>
> Also start defining specific return codes for _commit, assign -1 as a generic
> error and -2 as the error that refers to a name conflict. Callers can (and
> should) use that return value inspecting errno directly.

Heh.  Here's a patch on top to hopefully finish that part of the job.

Unfortunately, the value of errno after calling lock_ref_sha1_basic is
not reliable.
http://thread.gmane.org/gmane.comp.version-control.git/249159/focus=249407
lists the error paths that are broken (marked with "[!]" in that
message).

diff --git i/builtin/fetch.c w/builtin/fetch.c
index b13e8f9..0a01cda 100644
--- i/builtin/fetch.c
+++ w/builtin/fetch.c
@@ -377,6 +377,7 @@ static int s_update_ref(const char *action,
 	char *rla = getenv("GIT_REFLOG_ACTION");
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
+	int ret, df_conflict = 0;
 
 	if (dry_run)
 		return 0;
@@ -387,16 +388,22 @@ static int s_update_ref(const char *action,
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref->name, ref->new_sha1,
-				   ref->old_sha1, 0, check_old, msg, &err) ||
-	    ref_transaction_commit(transaction, &err)) {
-		ref_transaction_free(transaction);
-		error("%s", err.buf);
-		strbuf_release(&err);
-		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
-					  STORE_REF_ERROR_OTHER;
-	}
+				   ref->old_sha1, 0, check_old, msg, &err))
+		goto fail;
+
+	ret = ref_transaction_commit(transaction, &err);
+	if (ret == UPDATE_REFS_NAME_CONFLICT)
+		df_conflict = 1;
+	if (ret)
+		goto fail;
 	ref_transaction_free(transaction);
 	return 0;
+fail:
+	ref_transaction_free(transaction);
+	error("%s", err.buf);
+	strbuf_release(&err);
+	return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
+			   : STORE_REF_ERROR_OTHER;
 }
 
 #define REFCOL_WIDTH  10
diff --git i/refs.c w/refs.c
index dbaabba..b22b99b 100644
--- i/refs.c
+++ w/refs.c
@@ -3499,7 +3499,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
-	int ret = 0, delnum = 0, i, save_errno = 0;
+	int ret = 0, delnum = 0, i, df_conflict = 0;
 	const char **delnames;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
@@ -3535,7 +3535,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 						   delnames, delnum);
 		if (!update->lock) {
 			if (errno == ENOTDIR)
-				save_errno = errno;
+				df_conflict = 1;
 			if (err)
 				strbuf_addf(err, "Cannot lock the ref '%s'.",
 					    update->refname);
@@ -3590,8 +3590,7 @@ cleanup:
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
 	free(delnames);
-	errno = save_errno;
-	if (save_errno == ENOTDIR)
+	if (df_conflict)
 		ret = -2;
 	return ret;
 }
diff --git i/refs.h w/refs.h
index 88732a1..1583097 100644
--- i/refs.h
+++ w/refs.h
@@ -325,9 +325,11 @@ int ref_transaction_delete(struct ref_transaction *transaction,
  * problem.
  * If the transaction is already in failed state this function will return
  * an error.
- * Function returns 0 on success, -1 for generic failures and -2 if the
- * failure was due to a name collision (ENOTDIR).
+ * Function returns 0 on success, -1 for generic failures and
+ * UPDATE_REFS_NAME_CONFLICT (-2) if the failure was due to a name
+ * collision (ENOTDIR).
  */
+#define UPDATE_REFS_NAME_CONFLICT -2
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err);