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?
Patrick
