Patrick Steinhardt <ps@xxxxxx> writes:
> On Thu, May 15, 2025 at 11:13:32AM +0000, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@xxxxxx> 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?
>
Yeah totally agree, will add in a TODO here.
> Patrick
Attachment:
signature.asc
Description: PGP signature
