Patrick Steinhardt <ps@xxxxxx> writes:
> On Mon, Sep 08, 2025 at 02:37:38PM +0200, Karthik Nayak wrote:
>> The previous commit, added the necessary validation and checks for F/D
>
> s/commit,/commit/
>
Thanks, will change.
>> diff --git a/refs.c b/refs.c
>> index 4c1c339ed9..ec4f0e9502 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1232,6 +1232,12 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
>> if (err == REF_TRANSACTION_ERROR_GENERIC)
>> return 0;
>>
>> + /*
>> + * Remove this refname from the list of refnames used for validation
>> + */
>
> Nit: it's obvious that we remove the refname from that list, so the
> comment is not helping much. It's much more important to explain _why_
> we do that though to give readers the necessary context.
>
Indeed, I'll add this instead
Rejected refnames shouldn't be considered in the availability checks,
so remove them from the list.
>> + string_list_remove(&transaction->refnames,
>> + transaction->updates[update_idx]->refname, 0);
>> +
>> transaction->updates[update_idx]->rejection_err = err;
>> ALLOC_GROW(transaction->rejections->update_indices,
>> transaction->rejections->nr + 1,
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 85f2e14e93..ceeec272ff 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -852,6 +852,7 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
>> goto error_return;
>> } else if (remove_dir_recursively(&ref_file,
>> REMOVE_DIR_EMPTY_ONLY)) {
>> + ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
>> if (refs_verify_refname_available(
>> &refs->base, refname,
>> extras, NULL, 0, err)) {
>> @@ -859,7 +860,6 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
>> * The error message set by
>> * verify_refname_available() is OK.
>> */
>> - ret = REF_TRANSACTION_ERROR_NAME_CONFLICT;
>> goto error_return;
>> } else {
>> /*
>
> Hm, interesting. Previously we'd have returned a generic error in the
> `else` branch, which reads like this:
>
> } else {
> /*
> * We can't delete the directory,
> * but we also don't know of any
> * references that it should
> * contain.
> */
> strbuf_addf(err, "there is a non-empty directory '%s' "
> "blocking reference '%s'",
> ref_file.buf, refname);
> goto error_return;
> }
>
> So that directory contains something, even though we've previously
> verified that it shouldn't, if I understand correctly. The test case you
> add does seem to indicate that there are valid cases though where this
> can happen on a case insensitive filesystem?
>
> If so, the comment definitely needs to be updated to explain this
> additional error case.
>
Yeah I think that comment needs to be rewritten.
> One more question is whether it's the correct thing to unconditionally
> return REF_TRANSACTION_ERROR_NAME_CONFLICT in that case now. Could it be
> that the directory exists but contains some garbage?
>
> Patrick
This does affect case-sensitive systems too. I should've added a test
there as well to showcase this.
Basically if there is a lock file in a directory
'refs/heads/foo/bar.lock' while fetching 'refs/heads/foo', this would
reach this section of the code. Pre batched-updates, we would skip this
ref and update other refs. So it makes sense to retain this behavior.
I'll add in a test and amend the commit message to reflect this
behavior.
- Karthik
Attachment:
signature.asc
Description: PGP signature
