On Thu, Mar 06, 2025 at 04:08:37PM +0100, Patrick Steinhardt wrote:
> @@ -2811,6 +2808,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
> size_t i;
> int ret = 0;
> struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
> + struct string_list refnames_to_check = STRING_LIST_INIT_NODUP;
> char *head_ref = NULL;
> int head_type;
> struct files_transaction_backend_data *backend_data;
> @@ -2898,7 +2896,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,
> struct ref_update *update = transaction->updates[i];
>
> ret = lock_ref_for_update(refs, update, transaction,
> - head_ref, &affected_refnames, err);
> + head_ref, &refnames_to_check,
> + &affected_refnames, err);
> if (ret)
> goto cleanup;
>
> @@ -2930,6 +2929,26 @@ static int files_transaction_prepare(struct ref_store *ref_store,
> }
> }
>
> + /*
> + * Verify that none of the loose reference that we're about to write
> + * conflict with any existing packed references. Ideally, we'd do this
> + * check after the packed-refs are locked so that the file cannot
> + * change underneath our feet. But introducing such a lock now would
> + * probably do more harm than good as users rely on there not being a
> + * global lock with the "files" backend.
> + *
> + * Another alternative would be to do the check after the (optional)
> + * lock, but that would extend the time we spend in the globally-locked
> + * state.
> + *
> + * So instead, we accept the race for now.
> + */
I am curious why we don't sort the `refnames_to_check` here. What is the
difference between the reftable backend and files backend?
> + if (refs_verify_refnames_available(refs->packed_ref_store, &refnames_to_check,
> + &affected_refnames, NULL, 0, err)) {
> + ret = TRANSACTION_NAME_CONFLICT;
> + goto cleanup;
> + }
> +
> if (packed_transaction) {
> if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
> ret = TRANSACTION_GENERIC_ERROR;
