From 05a1834e429c619602a8507d8a2c9b81d467c24d Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 8 Apr 2025 10:51:05 +0200 Subject: refs/files: remove redundant check in split_symref_update() In `split_symref_update()`, there were two checks for duplicate refnames: - At the start, `string_list_has_string()` ensures the refname is not already in `affected_refnames`, preventing duplicates from being added. - After adding the refname, another check verifies whether the newly inserted item has a `util` value. The second check is unnecessary because the first one guarantees that `string_list_insert()` will never encounter a preexisting entry. The `item->util` field is assigned to validate that a rename doesn't already exist in the list. The validation is done after the first check. As this check is removed, clean up the validation and the assignment of this field in `split_head_update()` and `files_transaction_prepare()`. Signed-off-by: Karthik Nayak Acked-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/files-backend.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 5f921e85eb..dab3951ccf 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2382,7 +2382,6 @@ static int split_head_update(struct ref_update *update, struct string_list *affected_refnames, struct strbuf *err) { - struct string_list_item *item; struct ref_update *new_update; if ((update->flags & REF_LOG_ONLY) || @@ -2421,8 +2420,7 @@ static int split_head_update(struct ref_update *update, */ if (strcmp(new_update->refname, "HEAD")) BUG("%s unexpectedly not 'HEAD'", new_update->refname); - item = string_list_insert(affected_refnames, new_update->refname); - item->util = new_update; + string_list_insert(affected_refnames, new_update->refname); return 0; } @@ -2441,7 +2439,6 @@ static int split_symref_update(struct ref_update *update, struct string_list *affected_refnames, struct strbuf *err) { - struct string_list_item *item; struct ref_update *new_update; unsigned int new_flags; @@ -2496,11 +2493,7 @@ static int split_symref_update(struct ref_update *update, * be valid as long as affected_refnames is in use, and NOT * referent, which might soon be freed by our caller. */ - item = string_list_insert(affected_refnames, new_update->refname); - if (item->util) - BUG("%s unexpectedly found in affected_refnames", - new_update->refname); - item->util = new_update; + string_list_insert(affected_refnames, new_update->refname); return 0; } @@ -2834,7 +2827,6 @@ static int files_transaction_prepare(struct ref_store *ref_store, */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; - struct string_list_item *item; if ((update->flags & REF_IS_PRUNING) && !(update->flags & REF_NO_DEREF)) @@ -2843,13 +2835,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, if (update->flags & REF_LOG_ONLY) continue; - item = string_list_append(&affected_refnames, update->refname); - /* - * We store a pointer to update in item->util, but at - * the moment we never use the value of this field - * except to check whether it is non-NULL. - */ - item->util = update; + string_list_append(&affected_refnames, update->refname); } string_list_sort(&affected_refnames); if (ref_update_reject_duplicates(&affected_refnames, err)) { -- cgit v1.2.3 From c3baddf04f8fb20bec590f492f00189fd6c02a35 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 8 Apr 2025 10:51:06 +0200 Subject: refs: move duplicate refname update check to generic layer Move the tracking of refnames in `affected_refnames` from individual backends into the generic layer in 'refs.c'. This centralizes the duplicate refname detection that was previously handled separately by each backend. Make some changes to accommodate this move: - Add a `string_list` field `refnames` to `ref_transaction` to contain all the references in a transaction. This field is updated whenever a new update is added via `ref_transaction_add_update`, so manual additions in reference backends are dropped. - Modify the backends to use this field internally as needed. The backends need to check if an update for refname already exists when splitting symrefs or adding an update for 'HEAD'. - In the reftable backend, within `reftable_be_transaction_prepare()`, move the `string_list_has_string()` check above `ref_transaction_add_update()`. Since `ref_transaction_add_update()` automatically adds the refname to `transaction->refnames`, performing the check after will always return true, so we perform the check before adding the update. This helps reduce duplication of functionality between the backends and makes it easier to make changes in a more centralized manner. Signed-off-by: Karthik Nayak Acked-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 17 +++++++++++++ refs/files-backend.c | 67 +++++++++++-------------------------------------- refs/packed-backend.c | 25 +----------------- refs/refs-internal.h | 2 ++ refs/reftable-backend.c | 54 +++++++++++++-------------------------- 5 files changed, 51 insertions(+), 114 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index 79d5a8b8d4..22000798c7 100644 --- a/refs.c +++ b/refs.c @@ -1175,6 +1175,7 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs, CALLOC_ARRAY(tr, 1); tr->ref_store = refs; tr->flags = flags; + string_list_init_dup(&tr->refnames); return tr; } @@ -1205,6 +1206,7 @@ void ref_transaction_free(struct ref_transaction *transaction) free((char *)transaction->updates[i]->old_target); free(transaction->updates[i]); } + string_list_clear(&transaction->refnames, 0); free(transaction->updates); free(transaction); } @@ -1218,6 +1220,7 @@ struct ref_update *ref_transaction_add_update( const char *committer_info, const char *msg) { + struct string_list_item *item; struct ref_update *update; if (transaction->state != REF_TRANSACTION_OPEN) @@ -1245,6 +1248,16 @@ struct ref_update *ref_transaction_add_update( update->msg = normalize_reflog_message(msg); } + /* + * This list is generally used by the backends to avoid duplicates. + * But we do support multiple log updates for a given refname within + * a single transaction. + */ + if (!(update->flags & REF_LOG_ONLY)) { + item = string_list_append(&transaction->refnames, refname); + item->util = update; + } + return update; } @@ -2405,6 +2418,10 @@ int ref_transaction_prepare(struct ref_transaction *transaction, return -1; } + string_list_sort(&transaction->refnames); + if (ref_update_reject_duplicates(&transaction->refnames, err)) + return TRANSACTION_GENERIC_ERROR; + ret = refs->be->transaction_prepare(refs, transaction, err); if (ret) return ret; diff --git a/refs/files-backend.c b/refs/files-backend.c index dab3951ccf..ecf2df556d 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2378,9 +2378,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st */ static int split_head_update(struct ref_update *update, struct ref_transaction *transaction, - const char *head_ref, - struct string_list *affected_refnames, - struct strbuf *err) + const char *head_ref, struct strbuf *err) { struct ref_update *new_update; @@ -2398,7 +2396,7 @@ static int split_head_update(struct ref_update *update, * transaction. This check is O(lg N) in the transaction * size, but it happens at most once per transaction. */ - if (string_list_has_string(affected_refnames, "HEAD")) { + if (string_list_has_string(&transaction->refnames, "HEAD")) { /* An entry already existed */ strbuf_addf(err, "multiple updates for 'HEAD' (including one " @@ -2420,7 +2418,6 @@ static int split_head_update(struct ref_update *update, */ if (strcmp(new_update->refname, "HEAD")) BUG("%s unexpectedly not 'HEAD'", new_update->refname); - string_list_insert(affected_refnames, new_update->refname); return 0; } @@ -2436,7 +2433,6 @@ static int split_head_update(struct ref_update *update, static int split_symref_update(struct ref_update *update, const char *referent, struct ref_transaction *transaction, - struct string_list *affected_refnames, struct strbuf *err) { struct ref_update *new_update; @@ -2448,7 +2444,7 @@ static int split_symref_update(struct ref_update *update, * size, but it happens at most once per symref in a * transaction. */ - if (string_list_has_string(affected_refnames, referent)) { + if (string_list_has_string(&transaction->refnames, referent)) { /* An entry already exists */ strbuf_addf(err, "multiple updates for '%s' (including one " @@ -2486,15 +2482,6 @@ static int split_symref_update(struct ref_update *update, update->flags |= REF_LOG_ONLY | REF_NO_DEREF; update->flags &= ~REF_HAVE_OLD; - /* - * Add the referent. This insertion is O(N) in the transaction - * size, but it happens at most once per symref in a - * transaction. Make sure to add new_update->refname, which will - * be valid as long as affected_refnames is in use, and NOT - * referent, which might soon be freed by our caller. - */ - string_list_insert(affected_refnames, new_update->refname); - return 0; } @@ -2558,7 +2545,6 @@ static int lock_ref_for_update(struct files_ref_store *refs, struct ref_transaction *transaction, const char *head_ref, struct string_list *refnames_to_check, - struct string_list *affected_refnames, struct strbuf *err) { struct strbuf referent = STRBUF_INIT; @@ -2575,8 +2561,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, update->flags |= REF_DELETING; if (head_ref) { - ret = split_head_update(update, transaction, head_ref, - affected_refnames, err); + ret = split_head_update(update, transaction, head_ref, err); if (ret) goto out; } @@ -2586,9 +2571,8 @@ static int lock_ref_for_update(struct files_ref_store *refs, lock->count++; } else { ret = lock_raw_ref(refs, update->refname, mustexist, - refnames_to_check, affected_refnames, - &lock, &referent, - &update->type, err); + refnames_to_check, &transaction->refnames, + &lock, &referent, &update->type, err); if (ret) { char *reason; @@ -2642,9 +2626,8 @@ static int lock_ref_for_update(struct files_ref_store *refs, * of processing the split-off update, so we * don't have to do it here. */ - ret = split_symref_update(update, - referent.buf, transaction, - affected_refnames, err); + ret = split_symref_update(update, referent.buf, + transaction, err); if (ret) goto out; } @@ -2799,7 +2782,6 @@ static int files_transaction_prepare(struct ref_store *ref_store, "ref_transaction_prepare"); 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; @@ -2818,12 +2800,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, transaction->backend_data = backend_data; /* - * Fail if a refname appears more than once in the - * transaction. (If we end up splitting up any updates using - * split_symref_update() or split_head_update(), those - * functions will check that the new updates don't have the - * same refname as any existing ones.) Also fail if any of the - * updates use REF_IS_PRUNING without REF_NO_DEREF. + * Fail if any of the updates use REF_IS_PRUNING without REF_NO_DEREF. */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; @@ -2831,16 +2808,6 @@ static int files_transaction_prepare(struct ref_store *ref_store, if ((update->flags & REF_IS_PRUNING) && !(update->flags & REF_NO_DEREF)) BUG("REF_IS_PRUNING set without REF_NO_DEREF"); - - if (update->flags & REF_LOG_ONLY) - continue; - - string_list_append(&affected_refnames, update->refname); - } - string_list_sort(&affected_refnames); - if (ref_update_reject_duplicates(&affected_refnames, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; } /* @@ -2882,7 +2849,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, ret = lock_ref_for_update(refs, update, transaction, head_ref, &refnames_to_check, - &affected_refnames, err); + err); if (ret) goto cleanup; @@ -2929,7 +2896,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, * So instead, we accept the race for now. */ if (refs_verify_refnames_available(refs->packed_ref_store, &refnames_to_check, - &affected_refnames, NULL, 0, err)) { + &transaction->refnames, NULL, 0, err)) { ret = TRANSACTION_NAME_CONFLICT; goto cleanup; } @@ -2975,7 +2942,6 @@ static int files_transaction_prepare(struct ref_store *ref_store, cleanup: free(head_ref); - string_list_clear(&affected_refnames, 0); string_list_clear(&refnames_to_check, 0); if (ret) @@ -3050,13 +3016,8 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, 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++) - if (!(transaction->updates[i]->flags & REF_LOG_ONLY)) - string_list_append(&affected_refnames, - transaction->updates[i]->refname); - string_list_sort(&affected_refnames); - if (ref_update_reject_duplicates(&affected_refnames, err)) { + string_list_sort(&transaction->refnames); + if (ref_update_reject_duplicates(&transaction->refnames, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } @@ -3074,7 +3035,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, * that we are creating already exists. */ if (refs_for_each_rawref(&refs->base, ref_present, - &affected_refnames)) + &transaction->refnames)) BUG("initial ref transaction called with existing refs"); packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, diff --git a/refs/packed-backend.c b/refs/packed-backend.c index f4c82ba2c7..19220d2e99 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1622,8 +1622,6 @@ int is_packed_transaction_needed(struct ref_store *ref_store, struct packed_transaction_backend_data { /* True iff the transaction owns the packed-refs lock. */ int own_lock; - - struct string_list updates; }; static void packed_transaction_cleanup(struct packed_ref_store *refs, @@ -1632,8 +1630,6 @@ static void packed_transaction_cleanup(struct packed_ref_store *refs, struct packed_transaction_backend_data *data = transaction->backend_data; if (data) { - string_list_clear(&data->updates, 0); - if (is_tempfile_active(refs->tempfile)) delete_tempfile(&refs->tempfile); @@ -1658,7 +1654,6 @@ static int packed_transaction_prepare(struct ref_store *ref_store, REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB, "ref_transaction_prepare"); struct packed_transaction_backend_data *data; - size_t i; int ret = TRANSACTION_GENERIC_ERROR; /* @@ -1671,34 +1666,16 @@ static int packed_transaction_prepare(struct ref_store *ref_store, */ CALLOC_ARRAY(data, 1); - string_list_init_nodup(&data->updates); transaction->backend_data = data; - /* - * Stick the updates in a string list by refname so that we - * can sort them: - */ - for (i = 0; i < transaction->nr; i++) { - struct ref_update *update = transaction->updates[i]; - struct string_list_item *item = - string_list_append(&data->updates, update->refname); - - /* Store a pointer to update in item->util: */ - item->util = update; - } - string_list_sort(&data->updates); - - if (ref_update_reject_duplicates(&data->updates, err)) - goto failure; - if (!is_lock_file_locked(&refs->lock)) { if (packed_refs_lock(ref_store, 0, err)) goto failure; data->own_lock = 1; } - if (write_with_updates(refs, &data->updates, err)) + if (write_with_updates(refs, &transaction->refnames, err)) goto failure; transaction->state = REF_TRANSACTION_PREPARED; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index e5862757a7..92db793026 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -3,6 +3,7 @@ #include "refs.h" #include "iterator.h" +#include "string-list.h" struct fsck_options; struct ref_transaction; @@ -198,6 +199,7 @@ enum ref_transaction_state { struct ref_transaction { struct ref_store *ref_store; struct ref_update **updates; + struct string_list refnames; size_t alloc; size_t nr; enum ref_transaction_state state; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index c8f86da731..3688ffd683 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1076,7 +1076,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, struct reftable_ref_store *refs = reftable_be_downcast(ref_store, REF_STORE_WRITE|REF_STORE_MAIN, "ref_transaction_prepare"); struct strbuf referent = STRBUF_INIT, head_referent = STRBUF_INIT; - struct string_list affected_refnames = STRING_LIST_INIT_NODUP; struct string_list refnames_to_check = STRING_LIST_INIT_NODUP; struct reftable_transaction_data *tx_data = NULL; struct reftable_backend *be; @@ -1101,10 +1100,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, transaction->updates[i], err); if (ret) goto done; - - if (!(transaction->updates[i]->flags & REF_LOG_ONLY)) - string_list_append(&affected_refnames, - transaction->updates[i]->refname); } /* @@ -1116,17 +1111,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, tx_data->args[i].updates_alloc = tx_data->args[i].updates_expected; } - /* - * Fail if a refname appears more than once in the transaction. - * This code is taken from the files backend and is a good candidate to - * be moved into the generic layer. - */ - string_list_sort(&affected_refnames); - if (ref_update_reject_duplicates(&affected_refnames, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto done; - } - /* * TODO: it's dubious whether we should reload the stack that "HEAD" * belongs to or not. In theory, it may happen that we only modify @@ -1194,14 +1178,12 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, !(u->flags & REF_LOG_ONLY) && !(u->flags & REF_UPDATE_VIA_HEAD) && !strcmp(rewritten_ref, head_referent.buf)) { - struct ref_update *new_update; - /* * First make sure that HEAD is not already in the * transaction. This check is O(lg N) in the transaction * size, but it happens at most once per transaction. */ - if (string_list_has_string(&affected_refnames, "HEAD")) { + if (string_list_has_string(&transaction->refnames, "HEAD")) { /* An entry already existed */ strbuf_addf(err, _("multiple updates for 'HEAD' (including one " @@ -1211,12 +1193,11 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, goto done; } - new_update = ref_transaction_add_update( - transaction, "HEAD", - u->flags | REF_LOG_ONLY | REF_NO_DEREF, - &u->new_oid, &u->old_oid, NULL, NULL, NULL, - u->msg); - string_list_insert(&affected_refnames, new_update->refname); + ref_transaction_add_update( + transaction, "HEAD", + u->flags | REF_LOG_ONLY | REF_NO_DEREF, + &u->new_oid, &u->old_oid, NULL, NULL, NULL, + u->msg); } ret = reftable_backend_read_ref(be, rewritten_ref, @@ -1281,6 +1262,15 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, if (!strcmp(rewritten_ref, "HEAD")) new_flags |= REF_UPDATE_VIA_HEAD; + if (string_list_has_string(&transaction->refnames, referent.buf)) { + strbuf_addf(err, + _("multiple updates for '%s' (including one " + "via symref '%s') are not allowed"), + referent.buf, u->refname); + ret = TRANSACTION_NAME_CONFLICT; + goto done; + } + /* * If we are updating a symref (eg. HEAD), we should also * update the branch that the symref points to. @@ -1305,16 +1295,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, */ u->flags |= REF_LOG_ONLY | REF_NO_DEREF; u->flags &= ~REF_HAVE_OLD; - - if (string_list_has_string(&affected_refnames, new_update->refname)) { - strbuf_addf(err, - _("multiple updates for '%s' (including one " - "via symref '%s') are not allowed"), - referent.buf, u->refname); - ret = TRANSACTION_NAME_CONFLICT; - goto done; - } - string_list_insert(&affected_refnames, new_update->refname); } } @@ -1383,7 +1363,8 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, } } - ret = refs_verify_refnames_available(ref_store, &refnames_to_check, &affected_refnames, NULL, + ret = refs_verify_refnames_available(ref_store, &refnames_to_check, + &transaction->refnames, NULL, transaction->flags & REF_TRANSACTION_FLAG_INITIAL, err); if (ret < 0) @@ -1401,7 +1382,6 @@ done: strbuf_addf(err, _("reftable: transaction prepare: %s"), reftable_error_str(ret)); } - string_list_clear(&affected_refnames, 0); strbuf_release(&referent); strbuf_release(&head_referent); string_list_clear(&refnames_to_check, 0); -- cgit v1.2.3 From 4dfcf18089be03d70dd4a0437bb40af156062738 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 8 Apr 2025 10:51:07 +0200 Subject: refs/files: remove duplicate duplicates check Within the files reference backend's transaction's 'finish' phase, a verification step is currently performed wherein the refnames list is sorted and examined for multiple updates targeting the same refname. It has been observed that this verification is redundant, as an identical check is already executed during the transaction's 'prepare' stage. Since the refnames list remains unmodified following the 'prepare' stage, this secondary verification can be safely eliminated. The duplicate check has been removed accordingly, and the `ref_update_reject_duplicates()` function has been marked as static, as its usage is now confined to 'refs.c'. Signed-off-by: Karthik Nayak Acked-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 9 +++++++-- refs/files-backend.c | 6 ------ refs/refs-internal.h | 8 -------- 3 files changed, 7 insertions(+), 16 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index 22000798c7..b34969c792 100644 --- a/refs.c +++ b/refs.c @@ -2303,8 +2303,13 @@ cleanup: return ret; } -int ref_update_reject_duplicates(struct string_list *refnames, - struct strbuf *err) +/* + * Write an error to `err` and return a nonzero value iff the same + * refname appears multiple times in `refnames`. `refnames` must be + * sorted on entry to this function. + */ +static int ref_update_reject_duplicates(struct string_list *refnames, + struct strbuf *err) { size_t i, n = refnames->nr; diff --git a/refs/files-backend.c b/refs/files-backend.c index ecf2df556d..73da0d70e8 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3016,12 +3016,6 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, if (transaction->state != REF_TRANSACTION_PREPARED) BUG("commit called for transaction that is not prepared"); - string_list_sort(&transaction->refnames); - if (ref_update_reject_duplicates(&transaction->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 diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 92db793026..6d3770d0cc 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -142,14 +142,6 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, struct object_id *oid, struct strbuf *referent, unsigned int *type, int *failure_errno); -/* - * Write an error to `err` and return a nonzero value iff the same - * refname appears multiple times in `refnames`. `refnames` must be - * sorted on entry to this function. - */ -int ref_update_reject_duplicates(struct string_list *refnames, - struct strbuf *err); - /* * Add a ref_update with the specified properties to transaction, and * return a pointer to the new object. This function does not verify -- cgit v1.2.3 From 76e760b99923cb9afb52ef08607f736ff3eeaad7 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 8 Apr 2025 10:51:09 +0200 Subject: refs: introduce enum-based transaction error types Replace preprocessor-defined transaction errors with a strongly-typed enum `ref_transaction_error`. This change: - Improves type safety and function signature clarity. - Makes error handling more explicit and discoverable. - Maintains existing error cases, while adding new error cases for common scenarios. This refactoring paves the way for more comprehensive error handling which we will utilize in the upcoming commits to add batch reference update support. Signed-off-by: Karthik Nayak Acked-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/fetch.c | 2 +- refs.c | 49 ++++++------ refs.h | 48 +++++++----- refs/files-backend.c | 202 ++++++++++++++++++++++++------------------------ refs/packed-backend.c | 23 +++--- refs/refs-internal.h | 5 +- refs/reftable-backend.c | 64 +++++++-------- 7 files changed, 207 insertions(+), 186 deletions(-) (limited to 'refs/files-backend.c') diff --git a/builtin/fetch.c b/builtin/fetch.c index 1c740d5aac..52c913d28a 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -687,7 +687,7 @@ static int s_update_ref(const char *action, switch (ref_transaction_commit(our_transaction, &err)) { case 0: break; - case TRANSACTION_NAME_CONFLICT: + case REF_TRANSACTION_ERROR_NAME_CONFLICT: ret = STORE_REF_ERROR_DF_CONFLICT; goto out; default: diff --git a/refs.c b/refs.c index b34969c792..ca0a6b61b8 100644 --- a/refs.c +++ b/refs.c @@ -2271,7 +2271,7 @@ int refs_update_symref_extended(struct ref_store *refs, const char *ref, REF_NO_DEREF, logmsg, &err)) goto error_return; prepret = ref_transaction_prepare(transaction, &err); - if (prepret && prepret != TRANSACTION_CREATE_EXISTS) + if (prepret && prepret != REF_TRANSACTION_ERROR_CREATE_EXISTS) goto error_return; } else { if (ref_transaction_update(transaction, ref, NULL, NULL, @@ -2289,7 +2289,7 @@ int refs_update_symref_extended(struct ref_store *refs, const char *ref, } } - if (prepret == TRANSACTION_CREATE_EXISTS) + if (prepret == REF_TRANSACTION_ERROR_CREATE_EXISTS) goto cleanup; if (ref_transaction_commit(transaction, &err)) @@ -2425,7 +2425,7 @@ int ref_transaction_prepare(struct ref_transaction *transaction, string_list_sort(&transaction->refnames); if (ref_update_reject_duplicates(&transaction->refnames, err)) - return TRANSACTION_GENERIC_ERROR; + return REF_TRANSACTION_ERROR_GENERIC; ret = refs->be->transaction_prepare(refs, transaction, err); if (ret) @@ -2497,19 +2497,19 @@ int ref_transaction_commit(struct ref_transaction *transaction, return ret; } -int refs_verify_refnames_available(struct ref_store *refs, - const struct string_list *refnames, - const struct string_list *extras, - const struct string_list *skip, - unsigned int initial_transaction, - struct strbuf *err) +enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs, + const struct string_list *refnames, + const struct string_list *extras, + const struct string_list *skip, + unsigned int initial_transaction, + struct strbuf *err) { struct strbuf dirname = STRBUF_INIT; struct strbuf referent = STRBUF_INIT; struct string_list_item *item; struct ref_iterator *iter = NULL; struct strset dirnames; - int ret = -1; + int ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; /* * For the sake of comments in this function, suppose that @@ -2625,12 +2625,13 @@ cleanup: return ret; } -int refs_verify_refname_available(struct ref_store *refs, - const char *refname, - const struct string_list *extras, - const struct string_list *skip, - unsigned int initial_transaction, - struct strbuf *err) +enum ref_transaction_error refs_verify_refname_available( + struct ref_store *refs, + const char *refname, + const struct string_list *extras, + const struct string_list *skip, + unsigned int initial_transaction, + struct strbuf *err) { struct string_list_item item = { .string = (char *) refname }; struct string_list refnames = { @@ -2818,8 +2819,9 @@ int ref_update_has_null_new_value(struct ref_update *update) return !update->new_target && is_null_oid(&update->new_oid); } -int ref_update_check_old_target(const char *referent, struct ref_update *update, - struct strbuf *err) +enum ref_transaction_error ref_update_check_old_target(const char *referent, + struct ref_update *update, + struct strbuf *err) { if (!update->old_target) BUG("called without old_target set"); @@ -2827,17 +2829,18 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update, if (!strcmp(referent, update->old_target)) return 0; - if (!strcmp(referent, "")) + if (!strcmp(referent, "")) { strbuf_addf(err, "verifying symref target: '%s': " "reference is missing but expected %s", ref_update_original_update_refname(update), update->old_target); - else - strbuf_addf(err, "verifying symref target: '%s': " - "is at %s but expected %s", + return REF_TRANSACTION_ERROR_NONEXISTENT_REF; + } + + strbuf_addf(err, "verifying symref target: '%s': is at %s but expected %s", ref_update_original_update_refname(update), referent, update->old_target); - return -1; + return REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE; } struct migration_data { diff --git a/refs.h b/refs.h index b14ba1f9ff..d4af4ceeb2 100644 --- a/refs.h +++ b/refs.h @@ -16,6 +16,23 @@ struct worktree; enum ref_storage_format ref_storage_format_by_name(const char *name); const char *ref_storage_format_to_name(enum ref_storage_format ref_storage_format); +enum ref_transaction_error { + /* Default error code */ + REF_TRANSACTION_ERROR_GENERIC = -1, + /* Ref name conflict like A vs A/B */ + REF_TRANSACTION_ERROR_NAME_CONFLICT = -2, + /* Ref to be created already exists */ + REF_TRANSACTION_ERROR_CREATE_EXISTS = -3, + /* ref expected but doesn't exist */ + REF_TRANSACTION_ERROR_NONEXISTENT_REF = -4, + /* Provided old_oid or old_target of reference doesn't match actual */ + REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE = -5, + /* Provided new_oid or new_target is invalid */ + REF_TRANSACTION_ERROR_INVALID_NEW_VALUE = -6, + /* Expected ref to be symref, but is a regular ref */ + REF_TRANSACTION_ERROR_EXPECTED_SYMREF = -7, +}; + /* * Resolve a reference, recursively following symbolic references. * @@ -117,24 +134,24 @@ int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname, * * 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, - unsigned int initial_transaction, - struct strbuf *err); +enum ref_transaction_error refs_verify_refname_available(struct ref_store *refs, + const char *refname, + const struct string_list *extras, + const struct string_list *skip, + unsigned int initial_transaction, + struct strbuf *err); /* * Same as `refs_verify_refname_available()`, but checking for a list of * refnames instead of only a single item. This is more efficient in the case * where one needs to check multiple refnames. */ -int refs_verify_refnames_available(struct ref_store *refs, - const struct string_list *refnames, - const struct string_list *extras, - const struct string_list *skip, - unsigned int initial_transaction, - struct strbuf *err); +enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs, + const struct string_list *refnames, + const struct string_list *extras, + const struct string_list *skip, + unsigned int initial_transaction, + struct strbuf *err); int refs_ref_exists(struct ref_store *refs, const char *refname); @@ -830,13 +847,6 @@ int ref_transaction_verify(struct ref_transaction *transaction, unsigned int flags, struct strbuf *err); -/* Naming conflict (for example, the ref names A and A/B conflict). */ -#define TRANSACTION_NAME_CONFLICT -1 -/* When only creation was requested, but the ref already exists. */ -#define TRANSACTION_CREATE_EXISTS -2 -/* All other errors. */ -#define TRANSACTION_GENERIC_ERROR -3 - /* * Perform the preparatory stages of committing `transaction`. Acquire * any needed locks, check preconditions, etc.; basically, do as much diff --git a/refs/files-backend.c b/refs/files-backend.c index 73da0d70e8..770acdfa97 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -663,7 +663,7 @@ static void unlock_ref(struct ref_lock *lock) * broken, lock the reference anyway but clear old_oid. * * Return 0 on success. On failure, write an error message to err and - * return TRANSACTION_NAME_CONFLICT or TRANSACTION_GENERIC_ERROR. + * return REF_TRANSACTION_ERROR_NAME_CONFLICT or REF_TRANSACTION_ERROR_GENERIC. * * Implementation note: This function is basically * @@ -676,19 +676,20 @@ static void unlock_ref(struct ref_lock *lock) * avoided, namely if we were successfully able to read the ref * - Generate informative error messages in the case of failure */ -static int lock_raw_ref(struct files_ref_store *refs, - const char *refname, int mustexist, - struct string_list *refnames_to_check, - const struct string_list *extras, - struct ref_lock **lock_p, - struct strbuf *referent, - unsigned int *type, - struct strbuf *err) -{ +static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs, + const char *refname, + int mustexist, + struct string_list *refnames_to_check, + const struct string_list *extras, + struct ref_lock **lock_p, + struct strbuf *referent, + unsigned int *type, + struct strbuf *err) +{ + enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC; struct ref_lock *lock; struct strbuf ref_file = STRBUF_INIT; int attempts_remaining = 3; - int ret = TRANSACTION_GENERIC_ERROR; int failure_errno; assert(err); @@ -728,13 +729,14 @@ retry: strbuf_reset(err); strbuf_addf(err, "unable to resolve reference '%s'", refname); + ret = REF_TRANSACTION_ERROR_NONEXISTENT_REF; } else { /* * The error message set by * refs_verify_refname_available() is * OK. */ - ret = TRANSACTION_NAME_CONFLICT; + ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; } } else { /* @@ -788,6 +790,7 @@ retry: /* Garden variety missing reference. */ strbuf_addf(err, "unable to resolve reference '%s'", refname); + ret = REF_TRANSACTION_ERROR_NONEXISTENT_REF; goto error_return; } else { /* @@ -820,6 +823,7 @@ retry: /* Garden variety missing reference. */ strbuf_addf(err, "unable to resolve reference '%s'", refname); + ret = REF_TRANSACTION_ERROR_NONEXISTENT_REF; goto error_return; } else if (remove_dir_recursively(&ref_file, REMOVE_DIR_EMPTY_ONLY)) { @@ -830,7 +834,7 @@ retry: * The error message set by * verify_refname_available() is OK. */ - ret = TRANSACTION_NAME_CONFLICT; + ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; goto error_return; } else { /* @@ -1517,10 +1521,11 @@ static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname) return ret; } -static int write_ref_to_lockfile(struct files_ref_store *refs, - struct ref_lock *lock, - const struct object_id *oid, - int skip_oid_verification, struct strbuf *err); +static enum ref_transaction_error write_ref_to_lockfile(struct files_ref_store *refs, + struct ref_lock *lock, + const struct object_id *oid, + int skip_oid_verification, + struct strbuf *err); static int commit_ref_update(struct files_ref_store *refs, struct ref_lock *lock, const struct object_id *oid, const char *logmsg, @@ -1926,10 +1931,11 @@ static int files_log_ref_write(struct files_ref_store *refs, * Write oid into the open lockfile, then close the lockfile. On * errors, rollback the lockfile, fill in *err and return -1. */ -static int write_ref_to_lockfile(struct files_ref_store *refs, - struct ref_lock *lock, - const struct object_id *oid, - int skip_oid_verification, struct strbuf *err) +static enum ref_transaction_error write_ref_to_lockfile(struct files_ref_store *refs, + struct ref_lock *lock, + const struct object_id *oid, + int skip_oid_verification, + struct strbuf *err) { static char term = '\n'; struct object *o; @@ -1943,7 +1949,7 @@ static int write_ref_to_lockfile(struct files_ref_store *refs, "trying to write ref '%s' with nonexistent object %s", lock->ref_name, oid_to_hex(oid)); unlock_ref(lock); - return -1; + return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE; } if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) { strbuf_addf( @@ -1951,7 +1957,7 @@ static int write_ref_to_lockfile(struct files_ref_store *refs, "trying to write non-commit object %s to branch '%s'", oid_to_hex(oid), lock->ref_name); unlock_ref(lock); - return -1; + return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE; } } fd = get_lock_file_fd(&lock->lk); @@ -1962,7 +1968,7 @@ static int write_ref_to_lockfile(struct files_ref_store *refs, strbuf_addf(err, "couldn't write '%s'", get_lock_file_path(&lock->lk)); unlock_ref(lock); - return -1; + return REF_TRANSACTION_ERROR_GENERIC; } return 0; } @@ -2376,9 +2382,10 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st * If update is a direct update of head_ref (the reference pointed to * by HEAD), then add an extra REF_LOG_ONLY update for HEAD. */ -static int split_head_update(struct ref_update *update, - struct ref_transaction *transaction, - const char *head_ref, struct strbuf *err) +static enum ref_transaction_error split_head_update(struct ref_update *update, + struct ref_transaction *transaction, + const char *head_ref, + struct strbuf *err) { struct ref_update *new_update; @@ -2402,7 +2409,7 @@ static int split_head_update(struct ref_update *update, "multiple updates for 'HEAD' (including one " "via its referent '%s') are not allowed", update->refname); - return TRANSACTION_NAME_CONFLICT; + return REF_TRANSACTION_ERROR_NAME_CONFLICT; } new_update = ref_transaction_add_update( @@ -2430,10 +2437,10 @@ static int split_head_update(struct ref_update *update, * Note that the new update will itself be subject to splitting when * the iteration gets to it. */ -static int split_symref_update(struct ref_update *update, - const char *referent, - struct ref_transaction *transaction, - struct strbuf *err) +static enum ref_transaction_error split_symref_update(struct ref_update *update, + const char *referent, + struct ref_transaction *transaction, + struct strbuf *err) { struct ref_update *new_update; unsigned int new_flags; @@ -2450,7 +2457,7 @@ static int split_symref_update(struct ref_update *update, "multiple updates for '%s' (including one " "via symref '%s') are not allowed", referent, update->refname); - return TRANSACTION_NAME_CONFLICT; + return REF_TRANSACTION_ERROR_NAME_CONFLICT; } new_flags = update->flags; @@ -2491,11 +2498,10 @@ static int split_symref_update(struct ref_update *update, * everything is OK, return 0; otherwise, write an error message to * err and return -1. */ -static int check_old_oid(struct ref_update *update, struct object_id *oid, - struct strbuf *err) +static enum ref_transaction_error check_old_oid(struct ref_update *update, + struct object_id *oid, + struct strbuf *err) { - int ret = TRANSACTION_GENERIC_ERROR; - if (!(update->flags & REF_HAVE_OLD) || oideq(oid, &update->old_oid)) return 0; @@ -2504,21 +2510,20 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid, strbuf_addf(err, "cannot lock ref '%s': " "reference already exists", ref_update_original_update_refname(update)); - ret = TRANSACTION_CREATE_EXISTS; - } - else if (is_null_oid(oid)) + return REF_TRANSACTION_ERROR_CREATE_EXISTS; + } else if (is_null_oid(oid)) { strbuf_addf(err, "cannot lock ref '%s': " "reference is missing but expected %s", ref_update_original_update_refname(update), oid_to_hex(&update->old_oid)); - else - strbuf_addf(err, "cannot lock ref '%s': " - "is at %s but expected %s", - ref_update_original_update_refname(update), - oid_to_hex(oid), - oid_to_hex(&update->old_oid)); + return REF_TRANSACTION_ERROR_NONEXISTENT_REF; + } - return ret; + strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s", + ref_update_original_update_refname(update), oid_to_hex(oid), + oid_to_hex(&update->old_oid)); + + return REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE; } struct files_transaction_backend_data { @@ -2540,17 +2545,17 @@ struct files_transaction_backend_data { * - If it is an update of head_ref, add a corresponding REF_LOG_ONLY * update of HEAD. */ -static int lock_ref_for_update(struct files_ref_store *refs, - struct ref_update *update, - struct ref_transaction *transaction, - const char *head_ref, - struct string_list *refnames_to_check, - struct strbuf *err) +static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *refs, + struct ref_update *update, + struct ref_transaction *transaction, + const char *head_ref, + struct string_list *refnames_to_check, + struct strbuf *err) { struct strbuf referent = STRBUF_INIT; int mustexist = ref_update_expects_existing_old_ref(update); struct files_transaction_backend_data *backend_data; - int ret = 0; + enum ref_transaction_error ret = 0; struct ref_lock *lock; files_assert_main_repository(refs, "lock_ref_for_update"); @@ -2602,22 +2607,17 @@ static int lock_ref_for_update(struct files_ref_store *refs, strbuf_addf(err, "cannot lock ref '%s': " "error reading reference", ref_update_original_update_refname(update)); - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto out; } } - if (update->old_target) { - if (ref_update_check_old_target(referent.buf, update, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto out; - } - } else { + if (update->old_target) + ret = ref_update_check_old_target(referent.buf, update, err); + else ret = check_old_oid(update, &lock->old_oid, err); - if (ret) { - goto out; - } - } + if (ret) + goto out; } else { /* * Create a new update for the reference this @@ -2644,7 +2644,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, "but is a regular ref"), ref_update_original_update_refname(update), update->old_target); - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_EXPECTED_SYMREF; goto out; } else { ret = check_old_oid(update, &lock->old_oid, err); @@ -2668,14 +2668,14 @@ static int lock_ref_for_update(struct files_ref_store *refs, if (update->new_target && !(update->flags & REF_LOG_ONLY)) { if (create_symref_lock(lock, update->new_target, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto out; } if (close_ref_gently(lock)) { strbuf_addf(err, "couldn't close '%s.lock'", update->refname); - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto out; } @@ -2693,25 +2693,27 @@ static int lock_ref_for_update(struct files_ref_store *refs, * The reference already has the desired * value, so we don't need to write it. */ - } else if (write_ref_to_lockfile( - refs, lock, &update->new_oid, - update->flags & REF_SKIP_OID_VERIFICATION, - err)) { - char *write_err = strbuf_detach(err, NULL); - - /* - * The lock was freed upon failure of - * write_ref_to_lockfile(): - */ - update->backend_data = NULL; - strbuf_addf(err, - "cannot update ref '%s': %s", - update->refname, write_err); - free(write_err); - ret = TRANSACTION_GENERIC_ERROR; - goto out; } else { - update->flags |= REF_NEEDS_COMMIT; + ret = write_ref_to_lockfile( + refs, lock, &update->new_oid, + update->flags & REF_SKIP_OID_VERIFICATION, + err); + if (ret) { + char *write_err = strbuf_detach(err, NULL); + + /* + * The lock was freed upon failure of + * write_ref_to_lockfile(): + */ + update->backend_data = NULL; + strbuf_addf(err, + "cannot update ref '%s': %s", + update->refname, write_err); + free(write_err); + goto out; + } else { + update->flags |= REF_NEEDS_COMMIT; + } } } if (!(update->flags & REF_NEEDS_COMMIT)) { @@ -2723,7 +2725,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, if (close_ref_gently(lock)) { strbuf_addf(err, "couldn't close '%s.lock'", update->refname); - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto out; } } @@ -2865,7 +2867,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, refs->packed_ref_store, transaction->flags, err); if (!packed_transaction) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } @@ -2897,13 +2899,13 @@ static int files_transaction_prepare(struct ref_store *ref_store, */ if (refs_verify_refnames_available(refs->packed_ref_store, &refnames_to_check, &transaction->refnames, NULL, 0, err)) { - ret = TRANSACTION_NAME_CONFLICT; + ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; goto cleanup; } if (packed_transaction) { if (packed_refs_lock(refs->packed_ref_store, 0, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } backend_data->packed_refs_locked = 1; @@ -2934,7 +2936,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, */ backend_data->packed_transaction = NULL; if (ref_transaction_abort(packed_transaction, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } } @@ -3035,7 +3037,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, transaction->flags, err); if (!packed_transaction) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } @@ -3058,7 +3060,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, if (!loose_transaction) { loose_transaction = ref_store_transaction_begin(&refs->base, 0, err); if (!loose_transaction) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } } @@ -3083,19 +3085,19 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, } if (packed_refs_lock(refs->packed_ref_store, 0, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } 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; + ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; goto cleanup; } if (ref_transaction_commit(packed_transaction, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } packed_refs_unlock(refs->packed_ref_store); @@ -3103,7 +3105,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, if (loose_transaction) { if (ref_transaction_prepare(loose_transaction, err) || ref_transaction_commit(loose_transaction, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } } @@ -3152,7 +3154,7 @@ static int files_transaction_finish(struct ref_store *ref_store, if (update->flags & REF_NEEDS_COMMIT || update->flags & REF_LOG_ONLY) { if (parse_and_write_reflog(refs, update, lock, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } } @@ -3171,7 +3173,7 @@ static int files_transaction_finish(struct ref_store *ref_store, strbuf_addf(err, "couldn't set '%s'", lock->ref_name); unlock_ref(lock); update->backend_data = NULL; - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } } @@ -3227,7 +3229,7 @@ static int files_transaction_finish(struct ref_store *ref_store, strbuf_reset(&sb); files_ref_path(refs, &sb, lock->ref_name); if (unlink_or_msg(sb.buf, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } } diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 19220d2e99..d90bd815a3 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1326,10 +1326,11 @@ static int packed_ref_store_remove_on_disk(struct ref_store *ref_store, * The packfile must be locked before calling this function and will * remain locked when it is done. */ -static int write_with_updates(struct packed_ref_store *refs, - struct string_list *updates, - struct strbuf *err) +static enum ref_transaction_error write_with_updates(struct packed_ref_store *refs, + struct string_list *updates, + struct strbuf *err) { + enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC; struct ref_iterator *iter = NULL; size_t i; int ok; @@ -1353,7 +1354,7 @@ static int write_with_updates(struct packed_ref_store *refs, strbuf_addf(err, "unable to create file %s: %s", sb.buf, strerror(errno)); strbuf_release(&sb); - return -1; + return REF_TRANSACTION_ERROR_GENERIC; } strbuf_release(&sb); @@ -1409,6 +1410,7 @@ static int write_with_updates(struct packed_ref_store *refs, strbuf_addf(err, "cannot update ref '%s': " "reference already exists", update->refname); + ret = REF_TRANSACTION_ERROR_CREATE_EXISTS; goto error; } else if (!oideq(&update->old_oid, iter->oid)) { strbuf_addf(err, "cannot update ref '%s': " @@ -1416,6 +1418,7 @@ static int write_with_updates(struct packed_ref_store *refs, update->refname, oid_to_hex(iter->oid), oid_to_hex(&update->old_oid)); + ret = REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE; goto error; } } @@ -1452,6 +1455,7 @@ static int write_with_updates(struct packed_ref_store *refs, "reference is missing but expected %s", update->refname, oid_to_hex(&update->old_oid)); + ret = REF_TRANSACTION_ERROR_NONEXISTENT_REF; goto error; } } @@ -1509,7 +1513,7 @@ static int write_with_updates(struct packed_ref_store *refs, strerror(errno)); strbuf_release(&sb); delete_tempfile(&refs->tempfile); - return -1; + return REF_TRANSACTION_ERROR_GENERIC; } return 0; @@ -1521,7 +1525,7 @@ write_error: error: ref_iterator_free(iter); delete_tempfile(&refs->tempfile); - return -1; + return ret; } int is_packed_transaction_needed(struct ref_store *ref_store, @@ -1654,7 +1658,7 @@ static int packed_transaction_prepare(struct ref_store *ref_store, REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB, "ref_transaction_prepare"); struct packed_transaction_backend_data *data; - int ret = TRANSACTION_GENERIC_ERROR; + enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC; /* * Note that we *don't* skip transactions with zero updates, @@ -1675,7 +1679,8 @@ static int packed_transaction_prepare(struct ref_store *ref_store, data->own_lock = 1; } - if (write_with_updates(refs, &transaction->refnames, err)) + ret = write_with_updates(refs, &transaction->refnames, err); + if (ret) goto failure; transaction->state = REF_TRANSACTION_PREPARED; @@ -1707,7 +1712,7 @@ static int packed_transaction_finish(struct ref_store *ref_store, ref_store, REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB, "ref_transaction_finish"); - int ret = TRANSACTION_GENERIC_ERROR; + int ret = REF_TRANSACTION_ERROR_GENERIC; char *packed_refs_path; clear_snapshot(refs); diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 6d3770d0cc..3f1d19abd9 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -770,8 +770,9 @@ int ref_update_has_null_new_value(struct ref_update *update); * If everything is OK, return 0; otherwise, write an error message to * err and return -1. */ -int ref_update_check_old_target(const char *referent, struct ref_update *update, - struct strbuf *err); +enum ref_transaction_error ref_update_check_old_target(const char *referent, + struct ref_update *update, + struct strbuf *err); /* * Check if the ref must exist, this means that the old_oid or diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index b942d5eaf4..e318e6270e 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1069,20 +1069,20 @@ static int queue_transaction_update(struct reftable_ref_store *refs, return 0; } -static int prepare_single_update(struct reftable_ref_store *refs, - struct reftable_transaction_data *tx_data, - struct ref_transaction *transaction, - struct reftable_backend *be, - struct ref_update *u, - struct string_list *refnames_to_check, - unsigned int head_type, - struct strbuf *head_referent, - struct strbuf *referent, - struct strbuf *err) +static enum ref_transaction_error prepare_single_update(struct reftable_ref_store *refs, + struct reftable_transaction_data *tx_data, + struct ref_transaction *transaction, + struct reftable_backend *be, + struct ref_update *u, + struct string_list *refnames_to_check, + unsigned int head_type, + struct strbuf *head_referent, + struct strbuf *referent, + struct strbuf *err) { + enum ref_transaction_error ret = 0; struct object_id current_oid = {0}; const char *rewritten_ref; - int ret = 0; /* * There is no need to reload the respective backends here as @@ -1093,7 +1093,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, */ ret = backend_for(&be, refs, u->refname, &rewritten_ref, 0); if (ret) - return ret; + return REF_TRANSACTION_ERROR_GENERIC; /* Verify that the new object ID is valid. */ if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) && @@ -1104,13 +1104,13 @@ static int prepare_single_update(struct reftable_ref_store *refs, strbuf_addf(err, _("trying to write ref '%s' with nonexistent object %s"), u->refname, oid_to_hex(&u->new_oid)); - return -1; + return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE; } if (o->type != OBJ_COMMIT && is_branch(u->refname)) { strbuf_addf(err, _("trying to write non-commit object %s to branch '%s'"), oid_to_hex(&u->new_oid), u->refname); - return -1; + return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE; } } @@ -1134,7 +1134,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, _("multiple updates for 'HEAD' (including one " "via its referent '%s') are not allowed"), u->refname); - return TRANSACTION_NAME_CONFLICT; + return REF_TRANSACTION_ERROR_NAME_CONFLICT; } ref_transaction_add_update( @@ -1147,7 +1147,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, ret = reftable_backend_read_ref(be, rewritten_ref, ¤t_oid, referent, &u->type); if (ret < 0) - return ret; + return REF_TRANSACTION_ERROR_GENERIC; if (ret > 0 && !ref_update_expects_existing_old_ref(u)) { /* * The reference does not exist, and we either have no @@ -1168,7 +1168,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, ret = queue_transaction_update(refs, tx_data, u, ¤t_oid, err); if (ret) - return ret; + return REF_TRANSACTION_ERROR_GENERIC; } return 0; @@ -1180,7 +1180,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, "unable to resolve reference '%s'"), ref_update_original_update_refname(u), u->refname); - return -1; + return REF_TRANSACTION_ERROR_NONEXISTENT_REF; } if (u->type & REF_ISSYMREF) { @@ -1196,7 +1196,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, if (u->flags & REF_HAVE_OLD && !resolved) { strbuf_addf(err, _("cannot lock ref '%s': " "error reading reference"), u->refname); - return -1; + return REF_TRANSACTION_ERROR_GENERIC; } } else { struct ref_update *new_update; @@ -1211,7 +1211,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, _("multiple updates for '%s' (including one " "via symref '%s') are not allowed"), referent->buf, u->refname); - return TRANSACTION_NAME_CONFLICT; + return REF_TRANSACTION_ERROR_NAME_CONFLICT; } /* @@ -1255,31 +1255,32 @@ static int prepare_single_update(struct reftable_ref_store *refs, "but is a regular ref"), ref_update_original_update_refname(u), u->old_target); - return -1; + return REF_TRANSACTION_ERROR_EXPECTED_SYMREF; } - if (ref_update_check_old_target(referent->buf, u, err)) { - return -1; - } + ret = ref_update_check_old_target(referent->buf, u, err); + if (ret) + return ret; } else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) { if (is_null_oid(&u->old_oid)) { strbuf_addf(err, _("cannot lock ref '%s': " "reference already exists"), ref_update_original_update_refname(u)); - return TRANSACTION_CREATE_EXISTS; - } - else if (is_null_oid(¤t_oid)) + return REF_TRANSACTION_ERROR_CREATE_EXISTS; + } else if (is_null_oid(¤t_oid)) { strbuf_addf(err, _("cannot lock ref '%s': " "reference is missing but expected %s"), ref_update_original_update_refname(u), oid_to_hex(&u->old_oid)); - else + return REF_TRANSACTION_ERROR_NONEXISTENT_REF; + } else { strbuf_addf(err, _("cannot lock ref '%s': " "is at %s but expected %s"), ref_update_original_update_refname(u), oid_to_hex(¤t_oid), oid_to_hex(&u->old_oid)); - return TRANSACTION_NAME_CONFLICT; + return REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE; + } } /* @@ -1296,8 +1297,8 @@ static int prepare_single_update(struct reftable_ref_store *refs, if ((u->type & REF_ISSYMREF) || (u->flags & REF_LOG_ONLY) || (u->flags & REF_HAVE_NEW && !oideq(¤t_oid, &u->new_oid))) - return queue_transaction_update(refs, tx_data, u, - ¤t_oid, err); + if (queue_transaction_update(refs, tx_data, u, ¤t_oid, err)) + return REF_TRANSACTION_ERROR_GENERIC; return 0; } @@ -1385,7 +1386,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, transaction->state = REF_TRANSACTION_PREPARED; done: - assert(ret != REFTABLE_API_ERROR); if (ret < 0) { free_transaction_data(tx_data); transaction->state = REF_TRANSACTION_CLOSED; -- cgit v1.2.3 From 23fc8e4f613179900ce28da959757a387543b468 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 8 Apr 2025 10:51:10 +0200 Subject: refs: implement batch reference update support Git supports making reference updates with or without transactions. Updates with transactions are generally better optimized. But transactions are all or nothing. This means, if a user wants to batch updates to take advantage of the optimizations without the hard requirement that all updates must succeed, there is no way currently to do so. Particularly with the reftable backend where batching multiple reference updates is more efficient than performing them sequentially. Introduce batched update support with a new flag, 'REF_TRANSACTION_ALLOW_FAILURE'. Batched updates while different from transactions, use the transaction infrastructure under the hood. When enabled, this flag allows individual reference updates that would typically cause the entire transaction to fail due to non-system-related errors to be marked as rejected while permitting other updates to proceed. System errors referred by 'REF_TRANSACTION_ERROR_GENERIC' continue to result in the entire transaction failing. This approach enhances flexibility while preserving transactional integrity where necessary. The implementation introduces several key components: - Add 'rejection_err' field to struct `ref_update` to track failed updates with failure reason. - Add a new struct `ref_transaction_rejections` and a field within `ref_transaction` to this struct to allow quick iteration over rejected updates. - Modify reference backends (files, packed, reftable) to handle partial transactions by using `ref_transaction_set_rejected()` instead of failing the entire transaction when `REF_TRANSACTION_ALLOW_FAILURE` is set. - Add `ref_transaction_for_each_rejected_update()` to let callers examine which updates were rejected and why. This foundational change enables batched update support throughout the reference subsystem. A following commit will expose this capability to users by adding a `--batch-updates` flag to 'git-update-ref(1)', providing both a user-facing feature and a testable implementation. Signed-off-by: Karthik Nayak Acked-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++ refs.h | 22 ++++++++++++++++++ refs/files-backend.c | 12 +++++++++- refs/packed-backend.c | 27 ++++++++++++++++++++-- refs/refs-internal.h | 26 +++++++++++++++++++++ refs/reftable-backend.c | 12 +++++++++- 6 files changed, 156 insertions(+), 4 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index ca0a6b61b8..6edc79262a 100644 --- a/refs.c +++ b/refs.c @@ -1176,6 +1176,10 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs, tr->ref_store = refs; tr->flags = flags; string_list_init_dup(&tr->refnames); + + if (flags & REF_TRANSACTION_ALLOW_FAILURE) + CALLOC_ARRAY(tr->rejections, 1); + return tr; } @@ -1206,11 +1210,45 @@ void ref_transaction_free(struct ref_transaction *transaction) free((char *)transaction->updates[i]->old_target); free(transaction->updates[i]); } + + if (transaction->rejections) + free(transaction->rejections->update_indices); + free(transaction->rejections); + string_list_clear(&transaction->refnames, 0); free(transaction->updates); free(transaction); } +int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, + size_t update_idx, + enum ref_transaction_error err) +{ + if (update_idx >= transaction->nr) + BUG("trying to set rejection on invalid update index"); + + if (!(transaction->flags & REF_TRANSACTION_ALLOW_FAILURE)) + return 0; + + if (!transaction->rejections) + BUG("transaction not inititalized with failure support"); + + /* + * Don't accept generic errors, since these errors are not user + * input related. + */ + if (err == REF_TRANSACTION_ERROR_GENERIC) + return 0; + + transaction->updates[update_idx]->rejection_err = err; + ALLOC_GROW(transaction->rejections->update_indices, + transaction->rejections->nr + 1, + transaction->rejections->alloc); + transaction->rejections->update_indices[transaction->rejections->nr++] = update_idx; + + return 1; +} + struct ref_update *ref_transaction_add_update( struct ref_transaction *transaction, const char *refname, unsigned int flags, @@ -1236,6 +1274,7 @@ struct ref_update *ref_transaction_add_update( transaction->updates[transaction->nr++] = update; update->flags = flags; + update->rejection_err = 0; update->new_target = xstrdup_or_null(new_target); update->old_target = xstrdup_or_null(old_target); @@ -2728,6 +2767,28 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction, } } +void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction, + ref_transaction_for_each_rejected_update_fn cb, + void *cb_data) +{ + if (!transaction->rejections) + return; + + for (size_t i = 0; i < transaction->rejections->nr; i++) { + size_t update_index = transaction->rejections->update_indices[i]; + struct ref_update *update = transaction->updates[update_index]; + + if (!update->rejection_err) + continue; + + cb(update->refname, + (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL, + (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL, + update->old_target, update->new_target, + update->rejection_err, cb_data); + } +} + int refs_delete_refs(struct ref_store *refs, const char *logmsg, struct string_list *refnames, unsigned int flags) { diff --git a/refs.h b/refs.h index d4af4ceeb2..43f2041edf 100644 --- a/refs.h +++ b/refs.h @@ -667,6 +667,13 @@ enum ref_transaction_flag { * either be absent or null_oid. */ REF_TRANSACTION_FLAG_INITIAL = (1 << 0), + + /* + * The transaction mechanism by default fails all updates if any conflict + * is detected. This flag allows transactions to partially apply updates + * while rejecting updates which do not match the expected state. + */ + REF_TRANSACTION_ALLOW_FAILURE = (1 << 1), }; /* @@ -897,6 +904,21 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction, ref_transaction_for_each_queued_update_fn cb, void *cb_data); +/* + * Execute the given callback function for each of the reference updates which + * have been rejected in the given transaction. + */ +typedef void ref_transaction_for_each_rejected_update_fn(const char *refname, + const struct object_id *old_oid, + const struct object_id *new_oid, + const char *old_target, + const char *new_target, + enum ref_transaction_error err, + void *cb_data); +void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction, + ref_transaction_for_each_rejected_update_fn cb, + void *cb_data); + /* * Free `*transaction` and all associated data. */ diff --git a/refs/files-backend.c b/refs/files-backend.c index 770acdfa97..9620dd86fb 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2852,8 +2852,15 @@ static int files_transaction_prepare(struct ref_store *ref_store, ret = lock_ref_for_update(refs, update, transaction, head_ref, &refnames_to_check, err); - if (ret) + if (ret) { + if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + strbuf_reset(err); + ret = 0; + + continue; + } goto cleanup; + } if (update->flags & REF_DELETING && !(update->flags & REF_LOG_ONLY) && @@ -3151,6 +3158,9 @@ static int files_transaction_finish(struct ref_store *ref_store, struct ref_update *update = transaction->updates[i]; struct ref_lock *lock = update->backend_data; + if (update->rejection_err) + continue; + if (update->flags & REF_NEEDS_COMMIT || update->flags & REF_LOG_ONLY) { if (parse_and_write_reflog(refs, update, lock, err)) { diff --git a/refs/packed-backend.c b/refs/packed-backend.c index d90bd815a3..debca86a2b 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1327,10 +1327,11 @@ static int packed_ref_store_remove_on_disk(struct ref_store *ref_store, * remain locked when it is done. */ static enum ref_transaction_error write_with_updates(struct packed_ref_store *refs, - struct string_list *updates, + struct ref_transaction *transaction, struct strbuf *err) { enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC; + struct string_list *updates = &transaction->refnames; struct ref_iterator *iter = NULL; size_t i; int ok; @@ -1411,6 +1412,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re "reference already exists", update->refname); ret = REF_TRANSACTION_ERROR_CREATE_EXISTS; + + if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + strbuf_reset(err); + ret = 0; + continue; + } + goto error; } else if (!oideq(&update->old_oid, iter->oid)) { strbuf_addf(err, "cannot update ref '%s': " @@ -1419,6 +1427,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re oid_to_hex(iter->oid), oid_to_hex(&update->old_oid)); ret = REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE; + + if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + strbuf_reset(err); + ret = 0; + continue; + } + goto error; } } @@ -1456,6 +1471,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re update->refname, oid_to_hex(&update->old_oid)); ret = REF_TRANSACTION_ERROR_NONEXISTENT_REF; + + if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + strbuf_reset(err); + ret = 0; + continue; + } + goto error; } } @@ -1521,6 +1543,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re write_error: strbuf_addf(err, "error writing to %s: %s", get_tempfile_path(refs->tempfile), strerror(errno)); + ret = REF_TRANSACTION_ERROR_GENERIC; error: ref_iterator_free(iter); @@ -1679,7 +1702,7 @@ static int packed_transaction_prepare(struct ref_store *ref_store, data->own_lock = 1; } - ret = write_with_updates(refs, &transaction->refnames, err); + ret = write_with_updates(refs, transaction, err); if (ret) goto failure; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 3f1d19abd9..73a5379b73 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -123,6 +123,12 @@ struct ref_update { */ uint64_t index; + /* + * Used in batched reference updates to mark if a given update + * was rejected. + */ + enum ref_transaction_error rejection_err; + /* * If this ref_update was split off of a symref update via * split_symref_update(), then this member points at that @@ -142,6 +148,13 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, struct object_id *oid, struct strbuf *referent, unsigned int *type, int *failure_errno); +/* + * Mark a given update as rejected with a given reason. + */ +int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, + size_t update_idx, + enum ref_transaction_error err); + /* * Add a ref_update with the specified properties to transaction, and * return a pointer to the new object. This function does not verify @@ -183,6 +196,18 @@ enum ref_transaction_state { REF_TRANSACTION_CLOSED = 2 }; +/* + * Data structure to hold indices of updates which were rejected, for batched + * reference updates. While the updates themselves hold the rejection error, + * this structure allows a transaction to iterate only over the rejected + * updates. + */ +struct ref_transaction_rejections { + size_t *update_indices; + size_t alloc; + size_t nr; +}; + /* * Data structure for holding a reference transaction, which can * consist of checks and updates to multiple references, carried out @@ -195,6 +220,7 @@ struct ref_transaction { size_t alloc; size_t nr; enum ref_transaction_state state; + struct ref_transaction_rejections *rejections; void *backend_data; unsigned int flags; uint64_t max_index; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index e318e6270e..8fb7d6cc71 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1371,8 +1371,15 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, transaction->updates[i], &refnames_to_check, head_type, &head_referent, &referent, err); - if (ret) + if (ret) { + if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + strbuf_reset(err); + ret = 0; + + continue; + } goto done; + } } ret = refs_verify_refnames_available(ref_store, &refnames_to_check, @@ -1454,6 +1461,9 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data struct reftable_transaction_update *tx_update = &arg->updates[i]; struct ref_update *u = tx_update->update; + if (u->rejection_err) + continue; + /* * Write a reflog entry when updating a ref to point to * something new in either of the following cases: -- cgit v1.2.3 From 31726bb90d70236f7afaa345bf45195e2ef62d22 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 8 Apr 2025 10:51:11 +0200 Subject: refs: support rejection in batch updates during F/D checks The `refs_verify_refnames_available()` is used to batch check refnames for F/D conflicts. While this is the more performant alternative than its individual version, it does not provide rejection capabilities on a single update level. For batched updates, this would mean a rejection of the entire transaction whenever one reference has a F/D conflict. Modify the function to call `ref_transaction_maybe_set_rejected()` to check if a single update can be rejected. Since this function is only internally used within 'refs/' and we want to pass in a `struct ref_transaction *` as a variable. We also move and mark `refs_verify_refnames_available()` to 'refs-internal.h' to be an internal function. Signed-off-by: Karthik Nayak Acked-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 37 ++++++++++++++++++++++++++++++++++--- refs.h | 12 ------------ refs/files-backend.c | 27 ++++++++++++++++++--------- refs/refs-internal.h | 16 ++++++++++++++++ refs/reftable-backend.c | 11 ++++++++--- 5 files changed, 76 insertions(+), 27 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index 6edc79262a..498aec3fc0 100644 --- a/refs.c +++ b/refs.c @@ -2540,6 +2540,7 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs const struct string_list *refnames, const struct string_list *extras, const struct string_list *skip, + struct ref_transaction *transaction, unsigned int initial_transaction, struct strbuf *err) { @@ -2547,6 +2548,7 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs struct strbuf referent = STRBUF_INIT; struct string_list_item *item; struct ref_iterator *iter = NULL; + struct strset conflicting_dirnames; struct strset dirnames; int ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; @@ -2557,9 +2559,11 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs assert(err); + strset_init(&conflicting_dirnames); strset_init(&dirnames); for_each_string_list_item(item, refnames) { + const size_t *update_idx = (size_t *)item->util; const char *refname = item->string; const char *extra_refname; struct object_id oid; @@ -2597,14 +2601,30 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs continue; if (!initial_transaction && - !refs_read_raw_ref(refs, dirname.buf, &oid, &referent, - &type, &ignore_errno)) { + (strset_contains(&conflicting_dirnames, dirname.buf) || + !refs_read_raw_ref(refs, dirname.buf, &oid, &referent, + &type, &ignore_errno))) { + if (transaction && ref_transaction_maybe_set_rejected( + transaction, *update_idx, + REF_TRANSACTION_ERROR_NAME_CONFLICT)) { + strset_remove(&dirnames, dirname.buf); + strset_add(&conflicting_dirnames, dirname.buf); + continue; + } + strbuf_addf(err, _("'%s' exists; cannot create '%s'"), dirname.buf, refname); goto cleanup; } if (extras && string_list_has_string(extras, dirname.buf)) { + if (transaction && ref_transaction_maybe_set_rejected( + transaction, *update_idx, + REF_TRANSACTION_ERROR_NAME_CONFLICT)) { + strset_remove(&dirnames, dirname.buf); + continue; + } + strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), refname, dirname.buf); goto cleanup; @@ -2637,6 +2657,11 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs string_list_has_string(skip, iter->refname)) continue; + if (transaction && ref_transaction_maybe_set_rejected( + transaction, *update_idx, + REF_TRANSACTION_ERROR_NAME_CONFLICT)) + continue; + strbuf_addf(err, _("'%s' exists; cannot create '%s'"), iter->refname, refname); goto cleanup; @@ -2648,6 +2673,11 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs extra_refname = find_descendant_ref(dirname.buf, extras, skip); if (extra_refname) { + if (transaction && ref_transaction_maybe_set_rejected( + transaction, *update_idx, + REF_TRANSACTION_ERROR_NAME_CONFLICT)) + continue; + strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), refname, extra_refname); goto cleanup; @@ -2659,6 +2689,7 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs cleanup: strbuf_release(&referent); strbuf_release(&dirname); + strset_clear(&conflicting_dirnames); strset_clear(&dirnames); ref_iterator_free(iter); return ret; @@ -2679,7 +2710,7 @@ enum ref_transaction_error refs_verify_refname_available( }; return refs_verify_refnames_available(refs, &refnames, extras, skip, - initial_transaction, err); + NULL, initial_transaction, err); } struct do_for_each_reflog_help { diff --git a/refs.h b/refs.h index 43f2041edf..67a9b2c454 100644 --- a/refs.h +++ b/refs.h @@ -141,18 +141,6 @@ enum ref_transaction_error refs_verify_refname_available(struct ref_store *refs, unsigned int initial_transaction, struct strbuf *err); -/* - * Same as `refs_verify_refname_available()`, but checking for a list of - * refnames instead of only a single item. This is more efficient in the case - * where one needs to check multiple refnames. - */ -enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs, - const struct string_list *refnames, - const struct string_list *extras, - const struct string_list *skip, - unsigned int initial_transaction, - struct strbuf *err); - int refs_ref_exists(struct ref_store *refs, const char *refname); int should_autocreate_reflog(enum log_refs_config log_all_ref_updates, diff --git a/refs/files-backend.c b/refs/files-backend.c index 9620dd86fb..8b20e40401 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -677,16 +677,18 @@ static void unlock_ref(struct ref_lock *lock) * - Generate informative error messages in the case of failure */ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs, - const char *refname, + struct ref_update *update, + size_t update_idx, int mustexist, struct string_list *refnames_to_check, const struct string_list *extras, struct ref_lock **lock_p, struct strbuf *referent, - unsigned int *type, struct strbuf *err) { enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC; + const char *refname = update->refname; + unsigned int *type = &update->type; struct ref_lock *lock; struct strbuf ref_file = STRBUF_INIT; int attempts_remaining = 3; @@ -785,6 +787,8 @@ retry: if (files_read_raw_ref(&refs->base, refname, &lock->old_oid, referent, type, &failure_errno)) { + struct string_list_item *item; + if (failure_errno == ENOENT) { if (mustexist) { /* Garden variety missing reference. */ @@ -864,7 +868,9 @@ retry: * make sure there is no existing packed ref that conflicts * with refname. This check is deferred so that we can batch it. */ - string_list_append(refnames_to_check, refname); + item = string_list_append(refnames_to_check, refname); + item->util = xmalloc(sizeof(update_idx)); + memcpy(item->util, &update_idx, sizeof(update_idx)); } ret = 0; @@ -2547,6 +2553,7 @@ struct files_transaction_backend_data { */ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *refs, struct ref_update *update, + size_t update_idx, struct ref_transaction *transaction, const char *head_ref, struct string_list *refnames_to_check, @@ -2575,9 +2582,9 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re if (lock) { lock->count++; } else { - ret = lock_raw_ref(refs, update->refname, mustexist, + ret = lock_raw_ref(refs, update, update_idx, mustexist, refnames_to_check, &transaction->refnames, - &lock, &referent, &update->type, err); + &lock, &referent, err); if (ret) { char *reason; @@ -2849,7 +2856,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; - ret = lock_ref_for_update(refs, update, transaction, + ret = lock_ref_for_update(refs, update, i, transaction, head_ref, &refnames_to_check, err); if (ret) { @@ -2905,7 +2912,8 @@ static int files_transaction_prepare(struct ref_store *ref_store, * So instead, we accept the race for now. */ if (refs_verify_refnames_available(refs->packed_ref_store, &refnames_to_check, - &transaction->refnames, NULL, 0, err)) { + &transaction->refnames, NULL, transaction, + 0, err)) { ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; goto cleanup; } @@ -2951,7 +2959,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, cleanup: free(head_ref); - string_list_clear(&refnames_to_check, 0); + string_list_clear(&refnames_to_check, 1); if (ret) files_transaction_cleanup(refs, transaction); @@ -3097,7 +3105,8 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, } if (refs_verify_refnames_available(&refs->base, &refnames_to_check, - &affected_refnames, NULL, 1, err)) { + &affected_refnames, NULL, transaction, + 1, err)) { packed_refs_unlock(refs->packed_ref_store); ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; goto cleanup; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 73a5379b73..f868870851 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -806,4 +806,20 @@ enum ref_transaction_error ref_update_check_old_target(const char *referent, */ int ref_update_expects_existing_old_ref(struct ref_update *update); +/* + * Same as `refs_verify_refname_available()`, but checking for a list of + * refnames instead of only a single item. This is more efficient in the case + * where one needs to check multiple refnames. + * + * If using batched updates, then individual updates are marked rejected, + * reference backends are then in charge of not committing those updates. + */ +enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs, + const struct string_list *refnames, + const struct string_list *extras, + const struct string_list *skip, + struct ref_transaction *transaction, + unsigned int initial_transaction, + struct strbuf *err); + #endif /* REFS_REFS_INTERNAL_H */ diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 8fb7d6cc71..a461d1b8e0 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1074,6 +1074,7 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor struct ref_transaction *transaction, struct reftable_backend *be, struct ref_update *u, + size_t update_idx, struct string_list *refnames_to_check, unsigned int head_type, struct strbuf *head_referent, @@ -1149,6 +1150,7 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor if (ret < 0) return REF_TRANSACTION_ERROR_GENERIC; if (ret > 0 && !ref_update_expects_existing_old_ref(u)) { + struct string_list_item *item; /* * The reference does not exist, and we either have no * old object ID or expect the reference to not exist. @@ -1158,7 +1160,9 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor * can output a proper error message instead of failing * at a later point. */ - string_list_append(refnames_to_check, u->refname); + item = string_list_append(refnames_to_check, u->refname); + item->util = xmalloc(sizeof(update_idx)); + memcpy(item->util, &update_idx, sizeof(update_idx)); /* * There is no need to write the reference deletion @@ -1368,7 +1372,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, for (i = 0; i < transaction->nr; i++) { ret = prepare_single_update(refs, tx_data, transaction, be, - transaction->updates[i], + transaction->updates[i], i, &refnames_to_check, head_type, &head_referent, &referent, err); if (ret) { @@ -1384,6 +1388,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, ret = refs_verify_refnames_available(ref_store, &refnames_to_check, &transaction->refnames, NULL, + transaction, transaction->flags & REF_TRANSACTION_FLAG_INITIAL, err); if (ret < 0) @@ -1402,7 +1407,7 @@ done: } strbuf_release(&referent); strbuf_release(&head_referent); - string_list_clear(&refnames_to_check, 0); + string_list_clear(&refnames_to_check, 1); return ret; } -- cgit v1.2.3