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

[shejialuo <shejialuo@gmail.com>]

On Thu, Mar 06, 2025 at 04:08:38PM +0100, Patrick Steinhardt wrote:
> The "files" backend explicitly carves out special logic for its initial
> transaction so that it can avoid writing out every single reference as
> a loose reference. While the assumption is that there shouldn't be any
> preexisting references, we still have to verify that none of the newly
> written references will conflict with any other new reference in the
> same transaction.
> 
> Refactor the initial transaction to use batched refname availability
> checks. This does not yet have an effect on performance as we still call
> `refs_verify_refname_available()` in a loop. But this will change in
> subsequent commits and then impact performance when cloning a repository
> with many references or when migrating references to the "files" format.
> 
> This will improve performance when cloning a repository with many
> references or when migrating references from any format to the "files"
> format once the availability checks have learned to optimize checks for
> many references in a subsequent commit.

I guess you forgot to delete some sentences for the commit message. This
paragraph is a little redundant. In the second paragraph, I think we
have already talked about this.

> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/files-backend.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6ce79cf0791..11a620ea11a 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3056,6 +3056,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
>  	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;
>  	struct ref_transaction *packed_transaction = NULL;
>  	struct ref_transaction *loose_transaction = NULL;
>  
> @@ -3105,11 +3106,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
>  		    !is_null_oid(&update->old_oid))
>  			BUG("initial ref transaction with old_sha1 set");
>  
> -		if (refs_verify_refname_available(&refs->base, update->refname,
> -						  &affected_refnames, NULL, 1, err)) {
> -			ret = TRANSACTION_NAME_CONFLICT;
> -			goto cleanup;
> -		}
> +		string_list_append(&refnames_to_check, update->refname);
>  
>  		/*
>  		 * packed-refs don't support symbolic refs, root refs and reflogs,
> @@ -3145,8 +3142,19 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
>  		}
>  	}
>  
> -	if (packed_refs_lock(refs->packed_ref_store, 0, err) ||
> -	    ref_transaction_commit(packed_transaction, err)) {
> +	if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
> +		ret = TRANSACTION_GENERIC_ERROR;
> +		goto cleanup;
> +	}
> +

Still the same question: why we don't sort the `refnames_to_check` here.

> +	if (refs_verify_refnames_available(&refs->base, &refnames_to_check,
> +					   &affected_refnames, NULL, 1, err)) {
> +		packed_refs_unlock(refs->packed_ref_store);
> +		ret = TRANSACTION_NAME_CONFLICT;
> +		goto cleanup;
> +	}
> +
> +	if (ref_transaction_commit(packed_transaction, err)) {
>  		ret = TRANSACTION_GENERIC_ERROR;
>  		goto cleanup;
>  	}
> @@ -3167,6 +3175,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
>  		ref_transaction_free(packed_transaction);
>  	transaction->state = REF_TRANSACTION_CLOSED;
>  	string_list_clear(&affected_refnames, 0);
> +	string_list_clear(&refnames_to_check, 0);
>  	return ret;
>  }