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

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

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