Re: "lock file exists" when fetching in bare clone of repository

From: Karthik Nayak <karthik.188@gmail.com>
Date: Aug 28, 2025, 13:51 UTC
Message-ID: <CAOLa=ZRizMKRsP2wo-RJUksXAv1Hgx3XDGCO_ZGOwKpf6kAbQQ@mail.gmail.com>
In-Reply-To: <xmqq8qjbxn8n.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 19 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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

Show 11 quoted lines

> 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.

Show 14 quoted lines

> 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.