Re: [PATCH v11 39/41] refs.c: propagate any errno==ENOTDIR from _commit back to the callers
From: Ronnie Sahlberg <hidden>
Date: 2016-06-15 23:01:27
On Fri, May 30, 2014 at 5:22 PM, Jonathan Nieder [off-list ref] wrote:
Hi, Ronnie Sahlberg wrote:quoted
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.
Thanks. Updated.
Unfortunately, the value of errno after calling lock_ref_sha1_basic is not reliable.
I have changed the patch for lock_ref_sha1_basic so it now does :
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
errno = EINVAL;
return NULL;
}
so this function should no longer fail without changing errno to a,
hopefully, sane value.
quoted hunk ↗ jump to hunk
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 10diff --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);