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@xxxxxx>
> ---
> 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;
> }
