Junio C Hamano <gitster@xxxxxxxxx> writes:
> Karthik Nayak <karthik.188@xxxxxxxxx> 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
> 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.
Attachment:
signature.asc
Description: PGP signature
