Re: [PATCH v2 4/4] refs/files: handle D/F conflicts during locking

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 12, 2025, 11:53 UTC
Message-ID: <CAOLa=ZRt=HY_MrKHxGrC1g1QjOjngSYENzMwNDgUpi1gbg5yEA@mail.gmail.com>
In-Reply-To: <aL_SznrF61hbUMBo@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 5 quoted lines

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

Show 16 quoted lines

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

Show 51 quoted lines

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

Show 5 quoted lines

> 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