Thread (43 messages) 43 messages, 5 authors, 2025-09-17

Re: [PATCH v2 2/4] refs/files: use correct error type when lock exists

From: Karthik Nayak <hidden>
Date: 2025-09-11 10:14:22

Patrick Steinhardt [off-list ref] writes:
On Mon, Sep 08, 2025 at 02:37:36PM +0200, Karthik Nayak wrote:
quoted
When fetching references into a repository, if a lock for a particular
reference exists, then `lock_raw_ref()` throws the generic error
'REF_TRANSACTION_ERROR_GENERIC'. This causes the entire set of batched
updates to fail.
This isn't quite accurate anymore as we may also raise
`REF_TRANSACTION_ERROR_CASE_CONFLICT` now.
Good catch, will fixup.
quoted
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 58005d2732..2730713d23 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -790,9 +790,13 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
 			goto retry;
 		} else {
 			unable_to_lock_message(ref_file.buf, myerr, err);
-			if (myerr == EEXIST && ignore_case &&
-			    duplicate_reference_case_cmp(transaction, update))
-				ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
+			if (myerr == EEXIST) {
+				if (ignore_case && duplicate_reference_case_cmp(transaction, update))
+					ret = REF_TRANSACTION_ERROR_CASE_CONFLICT;
+				else
+					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
+			}
+
 			goto error_return;
 		}
 	}
Hm. So if I understand correctly, we now return CREATE_EXISTS in case we
have a conflict with a preexisting case-conflicting reference, but we
return CASE_CONFLICT in case we have a conflict with an update in the
same transaction?
It's not a reference, rather the lock that causes the conflict. So a
preexisting case-conflicting reference lock would raise the GENERIC
error. Which would fail all updates.
It feels awkward, but I guess that's the best thing we can do. We
happily overwrite case-conflicting preexisting refs, so we wouldn't even
see EEXIST in that case. So the only case where we still see that error
is on a D/F conflict, and in that case it makes sense to return
CREATE_EXISTS.

I feel like this should be added as a comment though, as it gives some
important non-obvious context.
Yeah, it would be beneficial, will add some comments here.
Patrick

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help