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 <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs.c b/refs.c
index 965b232a06e9b9f46d31c2c19ebf5d24592588d4..3459d0e4e5edf23b7fdad0c8fb7b5e30ae111d91 100644 (file)
--- 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 6f6f76a8d86dc47bc7488df18374971f69306619..6790d8bf535e5a02ddde2f060f25bdb20c9d4fc8 100644 (file)
--- 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 4ea0c1229946bdd6554fb20894ecf7bbe2c20317..59b3ecb9d647161e4afc0fa90fa723a5805fdd43 100644 (file)
--- 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 c7d2a6e50b7696cc3eba626a7538dcbc0dba1880..191a25683fdf318f98edf5bc8af16781f03dd83d 100644 (file)
--- 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 4319a4eacbafc433533b04c62b5cf8a625cac8c9..0e2648e36cc8bd60b26802bc2b8c6977a9485580 100644 (file)
--- 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;