From a0efef144686ca2c46caad98df72507ba2606ce5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 08:51:30 +0100 Subject: refs: allow passing flags when setting up a transaction Allow passing flags when setting up a transaction such that the behaviour of the transaction itself can be altered. This functionality will be used in a subsequent patch. Adapt callers accordingly. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/files-backend.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 0824c0b8a9..df61057c9f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1252,7 +1252,7 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r) if (check_refname_format(r->name, 0)) return; - transaction = ref_store_transaction_begin(&refs->base, &err); + transaction = ref_store_transaction_begin(&refs->base, 0, &err); if (!transaction) goto cleanup; ref_transaction_add_update( @@ -1396,7 +1396,8 @@ static int files_pack_refs(struct ref_store *ref_store, if (!should_pack_refs(refs, opts)) return 0; - transaction = ref_store_transaction_begin(refs->packed_ref_store, &err); + transaction = ref_store_transaction_begin(refs->packed_ref_store, + 0, &err); if (!transaction) return -1; @@ -2867,7 +2868,8 @@ static int files_transaction_prepare(struct ref_store *ref_store, */ if (!packed_transaction) { packed_transaction = ref_store_transaction_begin( - refs->packed_ref_store, err); + refs->packed_ref_store, + transaction->flags, err); if (!packed_transaction) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; @@ -3174,7 +3176,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store, &affected_refnames)) BUG("initial ref transaction called with existing refs"); - packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, err); + packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, + transaction->flags, err); if (!packed_transaction) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; -- cgit v1.2.3 From 83b8ed8bba5fe6655bb835a27b49bbf249314f55 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 08:51:31 +0100 Subject: refs/files: move logic to commit initial transaction Move the logic to commit initial transactions such that we can start to call it in `files_transaction_finish()` in a subsequent commit without requiring a separate function declaration. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/files-backend.c | 202 +++++++++++++++++++++++++-------------------------- 1 file changed, 101 insertions(+), 101 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index df61057c9f..f37c805a34 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2975,6 +2975,107 @@ static int parse_and_write_reflog(struct files_ref_store *refs, return 0; } +static int ref_present(const char *refname, const char *referent UNUSED, + const struct object_id *oid UNUSED, + int flags UNUSED, + void *cb_data) +{ + struct string_list *affected_refnames = cb_data; + + return string_list_has_string(affected_refnames, refname); +} + +static int files_initial_transaction_commit(struct ref_store *ref_store, + struct ref_transaction *transaction, + struct strbuf *err) +{ + struct files_ref_store *refs = + files_downcast(ref_store, REF_STORE_WRITE, + "initial_ref_transaction_commit"); + size_t i; + int ret = 0; + struct string_list affected_refnames = STRING_LIST_INIT_NODUP; + struct ref_transaction *packed_transaction = NULL; + + assert(err); + + if (transaction->state != REF_TRANSACTION_OPEN) + BUG("commit called for transaction that is not open"); + + /* Fail if a refname appears more than once in the transaction: */ + for (i = 0; i < transaction->nr; i++) + string_list_append(&affected_refnames, + transaction->updates[i]->refname); + string_list_sort(&affected_refnames); + if (ref_update_reject_duplicates(&affected_refnames, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; + } + + /* + * It's really undefined to call this function in an active + * repository or when there are existing references: we are + * only locking and changing packed-refs, so (1) any + * simultaneous processes might try to change a reference at + * the same time we do, and (2) any existing loose versions of + * the references that we are setting would have precedence + * over our values. But some remote helpers create the remote + * "HEAD" and "master" branches before calling this function, + * so here we really only check that none of the references + * that we are creating already exists. + */ + if (refs_for_each_rawref(&refs->base, ref_present, + &affected_refnames)) + BUG("initial ref transaction called with existing refs"); + + packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, + transaction->flags, err); + if (!packed_transaction) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; + } + + for (i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; + + if ((update->flags & REF_HAVE_OLD) && + !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, + err)) { + ret = TRANSACTION_NAME_CONFLICT; + goto cleanup; + } + + /* + * Add a reference creation for this reference to the + * packed-refs transaction: + */ + ref_transaction_add_update(packed_transaction, update->refname, + update->flags & ~REF_HAVE_OLD, + &update->new_oid, &update->old_oid, + NULL, NULL, NULL); + } + + if (packed_refs_lock(refs->packed_ref_store, 0, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; + } + + if (initial_ref_transaction_commit(packed_transaction, err)) { + ret = TRANSACTION_GENERIC_ERROR; + } + + packed_refs_unlock(refs->packed_ref_store); +cleanup: + if (packed_transaction) + ref_transaction_free(packed_transaction); + transaction->state = REF_TRANSACTION_CLOSED; + string_list_clear(&affected_refnames, 0); + return ret; +} + static int files_transaction_finish(struct ref_store *ref_store, struct ref_transaction *transaction, struct strbuf *err) @@ -3123,107 +3224,6 @@ static int files_transaction_abort(struct ref_store *ref_store, return 0; } -static int ref_present(const char *refname, const char *referent UNUSED, - const struct object_id *oid UNUSED, - int flags UNUSED, - void *cb_data) -{ - struct string_list *affected_refnames = cb_data; - - return string_list_has_string(affected_refnames, refname); -} - -static int files_initial_transaction_commit(struct ref_store *ref_store, - struct ref_transaction *transaction, - struct strbuf *err) -{ - struct files_ref_store *refs = - files_downcast(ref_store, REF_STORE_WRITE, - "initial_ref_transaction_commit"); - size_t i; - int ret = 0; - struct string_list affected_refnames = STRING_LIST_INIT_NODUP; - struct ref_transaction *packed_transaction = NULL; - - assert(err); - - if (transaction->state != REF_TRANSACTION_OPEN) - BUG("commit called for transaction that is not open"); - - /* Fail if a refname appears more than once in the transaction: */ - for (i = 0; i < transaction->nr; i++) - string_list_append(&affected_refnames, - transaction->updates[i]->refname); - string_list_sort(&affected_refnames); - if (ref_update_reject_duplicates(&affected_refnames, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; - } - - /* - * It's really undefined to call this function in an active - * repository or when there are existing references: we are - * only locking and changing packed-refs, so (1) any - * simultaneous processes might try to change a reference at - * the same time we do, and (2) any existing loose versions of - * the references that we are setting would have precedence - * over our values. But some remote helpers create the remote - * "HEAD" and "master" branches before calling this function, - * so here we really only check that none of the references - * that we are creating already exists. - */ - if (refs_for_each_rawref(&refs->base, ref_present, - &affected_refnames)) - BUG("initial ref transaction called with existing refs"); - - packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, - transaction->flags, err); - if (!packed_transaction) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; - } - - for (i = 0; i < transaction->nr; i++) { - struct ref_update *update = transaction->updates[i]; - - if ((update->flags & REF_HAVE_OLD) && - !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, - err)) { - ret = TRANSACTION_NAME_CONFLICT; - goto cleanup; - } - - /* - * Add a reference creation for this reference to the - * packed-refs transaction: - */ - ref_transaction_add_update(packed_transaction, update->refname, - update->flags & ~REF_HAVE_OLD, - &update->new_oid, &update->old_oid, - NULL, NULL, NULL); - } - - if (packed_refs_lock(refs->packed_ref_store, 0, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; - } - - if (initial_ref_transaction_commit(packed_transaction, err)) { - ret = TRANSACTION_GENERIC_ERROR; - } - - packed_refs_unlock(refs->packed_ref_store); -cleanup: - if (packed_transaction) - ref_transaction_free(packed_transaction); - transaction->state = REF_TRANSACTION_CLOSED; - string_list_clear(&affected_refnames, 0); - return ret; -} - struct expire_reflog_cb { reflog_expiry_should_prune_fn *should_prune_fn; void *policy_cb; -- cgit v1.2.3 From 1c299d03e5551847533019aa32863d2cbe589c7f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 08:51:32 +0100 Subject: refs: introduce "initial" transaction flag There are two different ways to commit a transaction: - `ref_transaction_commit()` can be used to commit a regular transaction and is what almost every caller wants. - `initial_ref_transaction_commit()` can be used when it is known that the ref store that the transaction is committed for is empty and when there are no concurrent processes. This is used when cloning a new repository. Implementing this via two separate functions has a couple of downsides. First, every reference backend needs to implement a separate callback even in the case where they don't special-case the initial transaction. Second, backends are basically forced to reimplement the whole logic for how to commit the transaction like the "files" backend does, even though backends may wish to only tweak certain behaviour of a "normal" commit. Third, it is awkward that callers must never prepare the transaction as this is somewhat different than how a transaction typically works. Refactor the code such that we instead mark initial transactions via a separate flag when starting the transaction. This addresses all of the mentioned painpoints, where the most important part is that it will allow backends to have way more leeway in how exactly they want to handle the initial transaction. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/clone.c | 4 ++-- refs.c | 10 +--------- refs.h | 37 ++++++++++++++++++------------------- refs/debug.c | 13 ------------- refs/files-backend.c | 16 ++++++++-------- refs/packed-backend.c | 8 -------- refs/refs-internal.h | 1 - refs/reftable-backend.c | 8 -------- 8 files changed, 29 insertions(+), 68 deletions(-) (limited to 'refs/files-backend.c') diff --git a/builtin/clone.c b/builtin/clone.c index d963cc6eb5..8427ccec17 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -574,7 +574,7 @@ static void write_remote_refs(const struct ref *local_refs) struct strbuf err = STRBUF_INIT; t = ref_store_transaction_begin(get_main_ref_store(the_repository), - 0, &err); + REF_TRANSACTION_FLAG_INITIAL, &err); if (!t) die("%s", err.buf); @@ -586,7 +586,7 @@ static void write_remote_refs(const struct ref *local_refs) die("%s", err.buf); } - if (initial_ref_transaction_commit(t, &err)) + if (ref_transaction_commit(t, &err)) die("%s", err.buf); strbuf_release(&err); diff --git a/refs.c b/refs.c index 9effeb01eb..8b9dfc6173 100644 --- a/refs.c +++ b/refs.c @@ -2315,7 +2315,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, } ret = refs->be->transaction_finish(refs, transaction, err); - if (!ret) + if (!ret && !(transaction->flags & REF_TRANSACTION_FLAG_INITIAL)) run_transaction_hook(transaction, "committed"); return ret; } @@ -2486,14 +2486,6 @@ int refs_reflog_expire(struct ref_store *refs, cleanup_fn, policy_cb_data); } -int initial_ref_transaction_commit(struct ref_transaction *transaction, - struct strbuf *err) -{ - struct ref_store *refs = transaction->ref_store; - - return refs->be->initial_transaction_commit(refs, transaction, err); -} - void ref_transaction_for_each_queued_update(struct ref_transaction *transaction, ref_transaction_for_each_queued_update_fn cb, void *cb_data) diff --git a/refs.h b/refs.h index 9821f3e80d..024a370554 100644 --- a/refs.h +++ b/refs.h @@ -214,11 +214,9 @@ char *repo_default_branch_name(struct repository *r, int quiet); * * Or * - * - Call `initial_ref_transaction_commit()` if the ref database is - * known to be empty and have no other writers (e.g. during - * clone). This is likely to be much faster than - * `ref_transaction_commit()`. `ref_transaction_prepare()` should - * *not* be called before `initial_ref_transaction_commit()`. + * - Call `ref_transaction_begin()` with REF_TRANSACTION_FLAG_INITIAL if the + * ref database is known to be empty and have no other writers (e.g. during + * clone). This is likely to be much faster than without the flag. * * - Then finally, call `ref_transaction_free()` to free the * `ref_transaction` data structure. @@ -579,6 +577,21 @@ enum action_on_err { UPDATE_REFS_QUIET_ON_ERR }; +enum ref_transaction_flag { + /* + * The ref transaction is part of the initial creation of the ref store + * and can thus assume that the ref store is completely empty. This + * allows the backend to perform the transaction more efficiently by + * skipping certain checks. + * + * It is a bug to set this flag when there might be other processes + * accessing the repository or if there are existing references that + * might conflict with the ones being created. All old_oid values must + * either be absent or null_oid. + */ + REF_TRANSACTION_FLAG_INITIAL = (1 << 0), +}; + /* * Begin a reference transaction. The reference transaction must * be freed by calling ref_transaction_free(). @@ -798,20 +811,6 @@ int ref_transaction_commit(struct ref_transaction *transaction, int ref_transaction_abort(struct ref_transaction *transaction, struct strbuf *err); -/* - * Like ref_transaction_commit(), but optimized for creating - * references when originally initializing a repository (e.g., by "git - * clone"). It writes the new references directly to packed-refs - * without locking the individual references. - * - * It is a bug to call this function when there might be other - * processes accessing the repository or if there are existing - * references that might conflict with the ones being created. All - * old_oid values must either be absent or null_oid. - */ -int initial_ref_transaction_commit(struct ref_transaction *transaction, - struct strbuf *err); - /* * Execute the given callback function for each of the reference updates which * have been queued in the given transaction. `old_oid` and `new_oid` may be diff --git a/refs/debug.c b/refs/debug.c index 45e2e784a0..cbac62c8a4 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -118,18 +118,6 @@ static int debug_transaction_abort(struct ref_store *refs, return res; } -static int debug_initial_transaction_commit(struct ref_store *refs, - struct ref_transaction *transaction, - struct strbuf *err) -{ - struct debug_ref_store *drefs = (struct debug_ref_store *)refs; - int res; - transaction->ref_store = drefs->refs; - res = drefs->refs->be->initial_transaction_commit(drefs->refs, - transaction, err); - return res; -} - static int debug_pack_refs(struct ref_store *ref_store, struct pack_refs_opts *opts) { struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store; @@ -443,7 +431,6 @@ struct ref_storage_be refs_be_debug = { .transaction_prepare = debug_transaction_prepare, .transaction_finish = debug_transaction_finish, .transaction_abort = debug_transaction_abort, - .initial_transaction_commit = debug_initial_transaction_commit, .pack_refs = debug_pack_refs, .rename_ref = debug_rename_ref, diff --git a/refs/files-backend.c b/refs/files-backend.c index f37c805a34..3ed18475a7 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2781,6 +2781,8 @@ static int files_transaction_prepare(struct ref_store *ref_store, assert(err); + if (transaction->flags & REF_TRANSACTION_FLAG_INITIAL) + goto cleanup; if (!transaction->nr) goto cleanup; @@ -2985,13 +2987,10 @@ static int ref_present(const char *refname, const char *referent UNUSED, return string_list_has_string(affected_refnames, refname); } -static int files_initial_transaction_commit(struct ref_store *ref_store, +static int files_transaction_finish_initial(struct files_ref_store *refs, struct ref_transaction *transaction, struct strbuf *err) { - struct files_ref_store *refs = - files_downcast(ref_store, REF_STORE_WRITE, - "initial_ref_transaction_commit"); size_t i; int ret = 0; struct string_list affected_refnames = STRING_LIST_INIT_NODUP; @@ -2999,8 +2998,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store, assert(err); - if (transaction->state != REF_TRANSACTION_OPEN) - BUG("commit called for transaction that is not open"); + if (transaction->state != REF_TRANSACTION_PREPARED) + BUG("commit called for transaction that is not prepared"); /* Fail if a refname appears more than once in the transaction: */ for (i = 0; i < transaction->nr; i++) @@ -3063,7 +3062,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store, goto cleanup; } - if (initial_ref_transaction_commit(packed_transaction, err)) { + if (ref_transaction_commit(packed_transaction, err)) { ret = TRANSACTION_GENERIC_ERROR; } @@ -3091,6 +3090,8 @@ static int files_transaction_finish(struct ref_store *ref_store, assert(err); + if (transaction->flags & REF_TRANSACTION_FLAG_INITIAL) + return files_transaction_finish_initial(refs, transaction, err); if (!transaction->nr) { transaction->state = REF_TRANSACTION_CLOSED; return 0; @@ -3617,7 +3618,6 @@ struct ref_storage_be refs_be_files = { .transaction_prepare = files_transaction_prepare, .transaction_finish = files_transaction_finish, .transaction_abort = files_transaction_abort, - .initial_transaction_commit = files_initial_transaction_commit, .pack_refs = files_pack_refs, .rename_ref = files_rename_ref, diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 07c57fd541..794471de60 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1730,13 +1730,6 @@ cleanup: return ret; } -static int packed_initial_transaction_commit(struct ref_store *ref_store UNUSED, - struct ref_transaction *transaction, - struct strbuf *err) -{ - return ref_transaction_commit(transaction, err); -} - static int packed_pack_refs(struct ref_store *ref_store UNUSED, struct pack_refs_opts *pack_opts UNUSED) { @@ -1769,7 +1762,6 @@ struct ref_storage_be refs_be_packed = { .transaction_prepare = packed_transaction_prepare, .transaction_finish = packed_transaction_finish, .transaction_abort = packed_transaction_abort, - .initial_transaction_commit = packed_initial_transaction_commit, .pack_refs = packed_pack_refs, .rename_ref = NULL, diff --git a/refs/refs-internal.h b/refs/refs-internal.h index dbc6360c5a..33335d54c9 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -666,7 +666,6 @@ struct ref_storage_be { ref_transaction_prepare_fn *transaction_prepare; ref_transaction_finish_fn *transaction_finish; ref_transaction_abort_fn *transaction_abort; - ref_transaction_commit_fn *initial_transaction_commit; pack_refs_fn *pack_refs; rename_ref_fn *rename_ref; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 38eb14d591..8e914afc81 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1490,13 +1490,6 @@ done: return ret; } -static int reftable_be_initial_transaction_commit(struct ref_store *ref_store UNUSED, - struct ref_transaction *transaction, - struct strbuf *err) -{ - return ref_transaction_commit(transaction, err); -} - static int reftable_be_pack_refs(struct ref_store *ref_store, struct pack_refs_opts *opts) { @@ -2490,7 +2483,6 @@ struct ref_storage_be refs_be_reftable = { .transaction_prepare = reftable_be_transaction_prepare, .transaction_finish = reftable_be_transaction_finish, .transaction_abort = reftable_be_transaction_abort, - .initial_transaction_commit = reftable_be_initial_transaction_commit, .pack_refs = reftable_be_pack_refs, .rename_ref = reftable_be_rename_ref, -- cgit v1.2.3 From c0b9cf3b55e9e1d6ea2d848bae1f8efe67b3b61b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 08:51:33 +0100 Subject: refs/files: support symbolic and root refs in initial transaction The "files" backend has implemented special logic when committing the first transactions in an otherwise empty ref store: instead of writing all refs as separate loose files, it instead knows to write them all into a "packed-refs" file directly. This is significantly more efficient than having to write each of the refs as separate "loose" ref. The only user of this optimization is git-clone(1), which only uses this mechanism to write regular refs. Consequently, the implementation does not know how to handle both symbolic and root refs. While fine in the context of git-clone(1), this keeps us from using the mechanism in more cases. Adapt the logic to also support symbolic and root refs by using a second transaction that we use for all of the refs that need to be written as loose refs. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/files-backend.c | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 3ed18475a7..116d425969 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2995,6 +2995,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, int ret = 0; struct string_list affected_refnames = STRING_LIST_INIT_NODUP; struct ref_transaction *packed_transaction = NULL; + struct ref_transaction *loose_transaction = NULL; assert(err); @@ -3040,6 +3041,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, if ((update->flags & REF_HAVE_OLD) && !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, err)) { @@ -3048,26 +3050,48 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, } /* - * Add a reference creation for this reference to the - * packed-refs transaction: + * packed-refs don't support symbolic refs and root refs, so we + * have to queue these references via the loose transaction. */ - ref_transaction_add_update(packed_transaction, update->refname, - update->flags & ~REF_HAVE_OLD, - &update->new_oid, &update->old_oid, - NULL, NULL, NULL); + if (update->new_target || is_root_ref(update->refname)) { + if (!loose_transaction) { + loose_transaction = ref_store_transaction_begin(&refs->base, 0, err); + if (!loose_transaction) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; + } + } + + ref_transaction_add_update(loose_transaction, update->refname, + update->flags & ~REF_HAVE_OLD, + update->new_target ? NULL : &update->new_oid, NULL, + update->new_target, NULL, NULL); + } else { + ref_transaction_add_update(packed_transaction, update->refname, + update->flags & ~REF_HAVE_OLD, + &update->new_oid, &update->old_oid, + NULL, NULL, NULL); + } } - if (packed_refs_lock(refs->packed_ref_store, 0, err)) { + if (packed_refs_lock(refs->packed_ref_store, 0, err) || + ref_transaction_commit(packed_transaction, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } + packed_refs_unlock(refs->packed_ref_store); - if (ref_transaction_commit(packed_transaction, err)) { - ret = TRANSACTION_GENERIC_ERROR; + if (loose_transaction) { + if (ref_transaction_prepare(loose_transaction, err) || + ref_transaction_commit(loose_transaction, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; + } } - packed_refs_unlock(refs->packed_ref_store); cleanup: + if (loose_transaction) + ref_transaction_free(loose_transaction); if (packed_transaction) ref_transaction_free(packed_transaction); transaction->state = REF_TRANSACTION_CLOSED; -- cgit v1.2.3 From e4929cdf797467b5d55a976ee938f1d4f358900e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 08:51:35 +0100 Subject: refs: skip collision checks in initial transactions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reference transactions use `refs_verify_refname_available()` to check for colliding references. This check consists of two parts: - Checks for whether multiple ref updates in the same transaction conflict with each other. - Checks for whether existing refs conflict with any refs part of the transaction. While we generally cannot avoid the first check, the second check is superfluous in cases where the transaction is an initial one in an otherwise empty ref store. The check results in multiple ref reads as well as the creation of a ref iterator for every ref we're checking, which adds up quite fast when performing the check for many refs. Introduce a new flag that allows us to skip this check and wire it up in such that the backends pass it when running an initial transaction. This leads to significant speedups when migrating ref storage backends. From "files" to "reftable": Benchmark 1: migrate files:reftable (refcount = 100000, revision = HEAD~) Time (mean ± σ): 472.4 ms ± 6.7 ms [User: 175.9 ms, System: 285.2 ms] Range (min … max): 463.5 ms … 483.2 ms 10 runs Benchmark 2: migrate files:reftable (refcount = 100000, revision = HEAD) Time (mean ± σ): 86.1 ms ± 1.9 ms [User: 67.9 ms, System: 16.0 ms] Range (min … max): 82.9 ms … 90.9 ms 29 runs Summary migrate files:reftable (refcount = 100000, revision = HEAD) ran 5.48 ± 0.15 times faster than migrate files:reftable (refcount = 100000, revision = HEAD~) And from "reftable" to "files": Benchmark 1: migrate reftable:files (refcount = 100000, revision = HEAD~) Time (mean ± σ): 452.7 ms ± 3.4 ms [User: 209.9 ms, System: 235.4 ms] Range (min … max): 445.9 ms … 457.5 ms 10 runs Benchmark 2: migrate reftable:files (refcount = 100000, revision = HEAD) Time (mean ± σ): 95.2 ms ± 2.2 ms [User: 73.6 ms, System: 20.6 ms] Range (min … max): 91.7 ms … 100.8 ms 28 runs Summary migrate reftable:files (refcount = 100000, revision = HEAD) ran 4.76 ± 0.11 times faster than migrate reftable:files (refcount = 100000, revision = HEAD~) Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 37 +++++++++++++++++++++---------------- refs.h | 5 ++++- refs/files-backend.c | 13 ++++++------- refs/reftable-backend.c | 6 ++++-- t/helper/test-ref-store.c | 2 +- 5 files changed, 36 insertions(+), 27 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index 0f10c565bb..d690eb19b3 100644 --- a/refs.c +++ b/refs.c @@ -2324,6 +2324,7 @@ int refs_verify_refname_available(struct ref_store *refs, const char *refname, const struct string_list *extras, const struct string_list *skip, + int initial_transaction, struct strbuf *err) { const char *slash; @@ -2332,8 +2333,6 @@ int refs_verify_refname_available(struct ref_store *refs, struct strbuf referent = STRBUF_INIT; struct object_id oid; unsigned int type; - struct ref_iterator *iter; - int ok; int ret = -1; /* @@ -2363,7 +2362,8 @@ int refs_verify_refname_available(struct ref_store *refs, if (skip && string_list_has_string(skip, dirname.buf)) continue; - if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent, + if (!initial_transaction && + !refs_read_raw_ref(refs, dirname.buf, &oid, &referent, &type, &ignore_errno)) { strbuf_addf(err, _("'%s' exists; cannot create '%s'"), dirname.buf, refname); @@ -2388,21 +2388,26 @@ int refs_verify_refname_available(struct ref_store *refs, strbuf_addstr(&dirname, refname + dirname.len); strbuf_addch(&dirname, '/'); - iter = refs_ref_iterator_begin(refs, dirname.buf, NULL, 0, - DO_FOR_EACH_INCLUDE_BROKEN); - while ((ok = ref_iterator_advance(iter)) == ITER_OK) { - if (skip && - string_list_has_string(skip, iter->refname)) - continue; + if (!initial_transaction) { + struct ref_iterator *iter; + int ok; - strbuf_addf(err, _("'%s' exists; cannot create '%s'"), - iter->refname, refname); - ref_iterator_abort(iter); - goto cleanup; - } + iter = refs_ref_iterator_begin(refs, dirname.buf, NULL, 0, + DO_FOR_EACH_INCLUDE_BROKEN); + while ((ok = ref_iterator_advance(iter)) == ITER_OK) { + if (skip && + string_list_has_string(skip, iter->refname)) + continue; - if (ok != ITER_DONE) - BUG("error while iterating over references"); + strbuf_addf(err, _("'%s' exists; cannot create '%s'"), + iter->refname, refname); + ref_iterator_abort(iter); + goto cleanup; + } + + if (ok != ITER_DONE) + BUG("error while iterating over references"); + } extra_refname = find_descendant_ref(dirname.buf, extras, skip); if (extra_refname) diff --git a/refs.h b/refs.h index 024a370554..980bd20cf2 100644 --- a/refs.h +++ b/refs.h @@ -101,13 +101,16 @@ int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname, * both "foo" and with "foo/bar/baz" but not with "foo/bar" or * "foo/barbados". * + * If `initial_transaction` is truish, then all collision checks with + * preexisting refs are skipped. + * * extras and skip must be sorted. */ - int refs_verify_refname_available(struct ref_store *refs, const char *refname, const struct string_list *extras, const struct string_list *skip, + int initial_transaction, struct strbuf *err); int refs_ref_exists(struct ref_store *refs, const char *refname); diff --git a/refs/files-backend.c b/refs/files-backend.c index 116d425969..d27806c02c 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -706,7 +706,7 @@ retry: * reason to expect this error to be transitory. */ if (refs_verify_refname_available(&refs->base, refname, - extras, NULL, err)) { + extras, NULL, 0, err)) { if (mustexist) { /* * To the user the relevant error is @@ -813,7 +813,7 @@ retry: REMOVE_DIR_EMPTY_ONLY)) { if (refs_verify_refname_available( &refs->base, refname, - extras, NULL, err)) { + extras, NULL, 0, err)) { /* * The error message set by * verify_refname_available() is OK. @@ -850,7 +850,7 @@ retry: */ if (refs_verify_refname_available( refs->packed_ref_store, refname, - extras, NULL, err)) { + extras, NULL, 0, err)) { ret = TRANSACTION_NAME_CONFLICT; goto error_return; } @@ -1159,7 +1159,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, */ if (is_null_oid(&lock->old_oid) && refs_verify_refname_available(refs->packed_ref_store, refname, - NULL, NULL, err)) + NULL, NULL, 0, err)) goto error_return; lock->ref_name = xstrdup(refname); @@ -1538,7 +1538,7 @@ static int refs_rename_ref_available(struct ref_store *refs, string_list_insert(&skip, old_refname); ok = !refs_verify_refname_available(refs, new_refname, - NULL, &skip, &err); + NULL, &skip, 0, &err); if (!ok) error("%s", err.buf); @@ -3043,8 +3043,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, BUG("initial ref transaction with old_sha1 set"); if (refs_verify_refname_available(&refs->base, update->refname, - &affected_refnames, NULL, - err)) { + &affected_refnames, NULL, 1, err)) { ret = TRANSACTION_NAME_CONFLICT; goto cleanup; } diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 8e914afc81..bbc779ab41 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1097,7 +1097,9 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, * at a later point. */ ret = refs_verify_refname_available(ref_store, u->refname, - &affected_refnames, NULL, err); + &affected_refnames, NULL, + transaction->flags & REF_TRANSACTION_FLAG_INITIAL, + err); if (ret < 0) goto done; @@ -1584,7 +1586,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data) if (arg->delete_old) string_list_insert(&skip, arg->oldname); ret = refs_verify_refname_available(&arg->refs->base, arg->newname, - NULL, &skip, &errbuf); + NULL, &skip, 0, &errbuf); if (ret < 0) { error("%s", errbuf.buf); goto done; diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index 65346dee55..240f6fc29d 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -199,7 +199,7 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv) struct strbuf err = STRBUF_INIT; int ret; - ret = refs_verify_refname_available(refs, refname, NULL, NULL, &err); + ret = refs_verify_refname_available(refs, refname, NULL, NULL, 0, &err); if (err.len) puts(err.buf); return ret; -- cgit v1.2.3