Junio C Hamano [off-list ref] writes:

Karthik Nayak [off-list ref] writes:

quoted

The fix itself isn't too involved:

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 088b52c740..5c31b02e6b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -776,6 +776,8 @@ 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)
+				ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
            goto error_return;
        }
    }

Let me preface my response by saying that in my quick to respond bug
fix, I think the actual assigned error should be
'REF_TRANSACTION_ERROR_CREATE_EXISTS'.

We assume that the existing lock is what _we_ created to lock the
ref in the other case, not somebody else locked-and-died some time
ago, or somebody else locked-and-about-to-update-competing-with-us?

We don't really change the path of exit, but rather just categorize the
error. So by marking it as 'REF_TRANSACTION_ERROR_CREATE_EXISTS', we
don't really say what kind of error it is.

In batched updates, the transaction allows failures unless a GENERIC
error is observed, wherein the transaction would fail. By marking the
error as 'REF_TRANSACTION_ERROR_CREATE_EXISTS', we allow batched updates
to allow this failure and carry on. Which I think it makes sense for all
the scenarios:

  - existing lock created by us due to being on a case insensitive FS
  - somebody else locked-and-died some time ago
  - somebody else locked-and-about-to-update-competing-with-us

Without this change we'd return REF_TRANSACTION_ERROR_GENERIC; does
the caller treat NAME_CONFLICT any specially?  Or is the "fix" you
talk about just about giving a different message and no other
behaviour changes involved?  I guess a more specific message that is
99% of the time more correct is an improvement over an overly
generic "some error happened" message.  But I thought the original
issue was that the user cannot make any progress when the ref
updates are transactional.  Does returning NAME_CONFLICT from here
tell the machinery that we are allowed to break transactional
semantics somehow?

Yes, without this, we'd return REF_TRANSACTION_ERROR_GENERIC. With this
change, we'd return REF_TRANSACTION_ERROR_CREATE_EXISTS.

This error type is bubbled up to `files_transaction_prepare()` which
tries to lock each reference update by calling `lock_ref_for_update()`.
So if the locking fails, we check if the rejection type can be ignored,
which is done by calling `ref_transaction_maybe_set_rejected()`.

Only during batched updates would errors be ignore and only for
non-generic errors. So this change would specifically only apply for
batched updates. Currently that is used by:

    1. git fetch
    2. git receive-pack
    3. git update-ref --batch-updates

And for all three scenarios I think it makes sense to add this in.

In any case, I wonder if refs_verify_refnames_available() should
notice that we are using files backend on a case challenged
filesystem, and change the behaviour a bit.  This additional check
needs to be implemented as a backend call via refs->be->something()
to tighten the outcome.  It appears to me that the function in the
current form does not even notice a D/F conflict when there is a
branch 'd' and the transaction requests to create a new branch 'D/f'
on a case challenged system if files backend is in use, since the
function is in the generic layer and behaves case sensitively (which
is the right thing to do---we are talking about detecting backend
specific glitches here).

Thanks.

Absolutely, this whole thing with the files backend and case-insensitive
file-systems is a mess. Yes we'd need to go around and fix that too.