From 3f5f409159db7d3246452dbe2bbb5bc2c436ea00 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Fri, 16 Jan 2026 22:27:07 +0100 Subject: [PATCH] refs: skip to next ref when current ref is rejected In `refs_verify_refnames_available()` we have two nested loops: the outer loop iterates over all references to check, while the inner loop checks for filesystem conflicts for a given ref by breaking down its path. With batched updates, when we detect a filesystem conflict, we mark the update as rejected and execute 'continue'. However, this only skips to the next iteration of the inner loop, not the outer loop as intended. This causes the same reference to be repeatedly rejected. Fix this by using a goto statement to skip to the next reference in the outer loop. Signed-off-by: Karthik Nayak Signed-off-by: Junio C Hamano --- refs.c | 44 ++++++++++++++++++++++++----------------- refs/files-backend.c | 5 ++--- refs/packed-backend.c | 12 +++++------ refs/refs-internal.h | 4 +++- refs/reftable-backend.c | 5 ++--- 5 files changed, 39 insertions(+), 31 deletions(-) diff --git a/refs.c b/refs.c index 965b232a06..3459d0e4e5 100644 --- a/refs.c +++ b/refs.c @@ -1222,6 +1222,7 @@ void ref_transaction_free(struct ref_transaction *transaction) free(transaction->updates[i]->committer_info); free((char *)transaction->updates[i]->new_target); free((char *)transaction->updates[i]->old_target); + free((char *)transaction->updates[i]->rejection_details); free(transaction->updates[i]); } @@ -1236,7 +1237,8 @@ void ref_transaction_free(struct ref_transaction *transaction) int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, size_t update_idx, - enum ref_transaction_error err) + enum ref_transaction_error err, + struct strbuf *details) { if (update_idx >= transaction->nr) BUG("trying to set rejection on invalid update index"); @@ -1262,6 +1264,7 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, transaction->updates[update_idx]->refname, 0); transaction->updates[update_idx]->rejection_err = err; + transaction->updates[update_idx]->rejection_details = strbuf_detach(details, NULL); ALLOC_GROW(transaction->rejections->update_indices, transaction->rejections->nr + 1, transaction->rejections->alloc); @@ -2657,30 +2660,33 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs if (!initial_transaction && (strset_contains(&conflicting_dirnames, dirname.buf) || !refs_read_raw_ref(refs, dirname.buf, &oid, &referent, - &type, &ignore_errno))) { + &type, &ignore_errno))) { + + strbuf_addf(err, _("'%s' exists; cannot create '%s'"), + dirname.buf, refname); + if (transaction && ref_transaction_maybe_set_rejected( transaction, *update_idx, - REF_TRANSACTION_ERROR_NAME_CONFLICT)) { + REF_TRANSACTION_ERROR_NAME_CONFLICT, err)) { strset_remove(&dirnames, dirname.buf); strset_add(&conflicting_dirnames, dirname.buf); - continue; + goto next_ref; } - strbuf_addf(err, _("'%s' exists; cannot create '%s'"), - dirname.buf, refname); goto cleanup; } if (extras && string_list_has_string(extras, dirname.buf)) { + strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), + refname, dirname.buf); + if (transaction && ref_transaction_maybe_set_rejected( transaction, *update_idx, - REF_TRANSACTION_ERROR_NAME_CONFLICT)) { + REF_TRANSACTION_ERROR_NAME_CONFLICT, err)) { strset_remove(&dirnames, dirname.buf); - continue; + goto next_ref; } - strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), - refname, dirname.buf); goto cleanup; } } @@ -2710,14 +2716,14 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs if (skip && string_list_has_string(skip, iter->ref.name)) continue; + strbuf_addf(err, _("'%s' exists; cannot create '%s'"), + iter->ref.name, refname); if (transaction && ref_transaction_maybe_set_rejected( transaction, *update_idx, - REF_TRANSACTION_ERROR_NAME_CONFLICT)) - continue; + REF_TRANSACTION_ERROR_NAME_CONFLICT, err)) + goto next_ref; - strbuf_addf(err, _("'%s' exists; cannot create '%s'"), - iter->ref.name, refname); goto cleanup; } @@ -2727,15 +2733,17 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs extra_refname = find_descendant_ref(dirname.buf, extras, skip); if (extra_refname) { + strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), + refname, extra_refname); + if (transaction && ref_transaction_maybe_set_rejected( transaction, *update_idx, - REF_TRANSACTION_ERROR_NAME_CONFLICT)) - continue; + REF_TRANSACTION_ERROR_NAME_CONFLICT, err)) + goto next_ref; - strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), - refname, extra_refname); goto cleanup; } +next_ref:; } ret = 0; diff --git a/refs/files-backend.c b/refs/files-backend.c index 6f6f76a8d8..6790d8bf53 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2983,10 +2983,9 @@ static int files_transaction_prepare(struct ref_store *ref_store, head_ref, &refnames_to_check, err); if (ret) { - if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { - strbuf_reset(err); + if (ref_transaction_maybe_set_rejected(transaction, i, + ret, err)) { ret = 0; - continue; } goto cleanup; diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 4ea0c12299..59b3ecb9d6 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1437,8 +1437,8 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re update->refname); ret = REF_TRANSACTION_ERROR_CREATE_EXISTS; - if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { - strbuf_reset(err); + if (ref_transaction_maybe_set_rejected(transaction, i, + ret, err)) { ret = 0; continue; } @@ -1452,8 +1452,8 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re 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); + if (ref_transaction_maybe_set_rejected(transaction, i, + ret, err)) { ret = 0; continue; } @@ -1496,8 +1496,8 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re oid_to_hex(&update->old_oid)); ret = REF_TRANSACTION_ERROR_NONEXISTENT_REF; - if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { - strbuf_reset(err); + if (ref_transaction_maybe_set_rejected(transaction, i, + ret, err)) { ret = 0; continue; } diff --git a/refs/refs-internal.h b/refs/refs-internal.h index c7d2a6e50b..191a25683f 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -128,6 +128,7 @@ struct ref_update { * was rejected. */ enum ref_transaction_error rejection_err; + const char *rejection_details; /* * If this ref_update was split off of a symref update via @@ -153,7 +154,8 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, */ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, size_t update_idx, - enum ref_transaction_error err); + enum ref_transaction_error err, + struct strbuf *details); /* * Add a ref_update with the specified properties to transaction, and diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 4319a4eacb..0e2648e36c 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1401,10 +1401,9 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, &refnames_to_check, head_type, &head_referent, &referent, err); if (ret) { - if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { - strbuf_reset(err); + if (ref_transaction_maybe_set_rejected(transaction, i, + ret, err)) { ret = 0; - continue; } goto done; -- 2.47.3