Re: [PATCH v5 06/16] refs/files: batch refname availability checks for normal transactions

[shejialuo <shejialuo@gmail.com>]

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;