Re: [PATCH 1/3] fetch: use batched reference updates

> On Thu, May 15, 2025 at 11:13:32AM +0000, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> >> +	 * since pruning must be an independent step, to avoid F/D conflicts.
>> >> +	 */
>> >> +	if (!transaction) {
>> >> +		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> >> +							  REF_TRANSACTION_ALLOW_FAILURE, &err);
>> >> +		if (!transaction) {
>> >> +			retcode = -1;
>> >> +			goto cleanup;
>> >> +		}
>> >> +	}
>> >> +
>> >>  	if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
>> >>  				   &fetch_head, config)) {
>> >>  		retcode = 1;
>> >
>> > Don't transactions handle D/F conflicts for us? Isn't that the sole
>> > reason why for example `refs_verify_refname_available()` accepts an
>> > "extras" parameter that is supposed to contain refs that are about to be
>> > deleted?
>> >
>>
>> My understanding was a little different, from the documentation for the
>> function:
>>
>>   If extras is non-NULL, it is a list of additional refnames with which
>>   refname is not allowed to conflict.
>>
>> This is to capture additional conflicts. We want a way to avoid said
>> conflicts. That said, there is a 'skip' parameter which does exactly
>> what you're saying.
>
> Oh, right, my mistake -- that's what I actually meant.
>
>> But the transaction logic doesn't incorporate this
>> entirely. Specifically in the files backend, where we create a lock in
>> the filesystem, this would cause a conflict, consider the following:
>>
>>   ❯ eza --tree .git/refs/remotes/
>>   .git/refs/remotes
>>   └── origin
>>       ├── dir
>>       │   └── file.lock
>>       ├── dir.lock
>>       └── HEAD
>>
>> This is from the test 'branchname D/F conflict resolved by --prune', the
>> test prunes the existing reference 'refs/remotes/origin/dir/file' while
>> adding 'refs/remotes/origin/dir'. In 'lock_raw_ref()' we lock both and
>> read the reference, but this causes an issue since
>> 'refs/remotes/origin/dir' exists as a directory already.
>>
>> I would say this is logically solvable if we start treating conflict
>> resolution within updates as a first class problem. But perhaps that's
>> something of a patch series in itself and better solved outside of this?
>
> Fair enough, makes sense. Might be worth it to add a TODO comment there
> then?
>