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 <karthik.188@gmail.com>
Acked-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs.c b/refs.c
index 6edc79262a40b9f58156df6140b66e1f2a503feb..498aec3fc0194eff47c704e7f0ac7199d343b6a9 100644 (file)
--- 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 43f2041edfe9aaceb581ac0635d50f3bc4d4ea1e..67a9b2c454867087aef7102a591bb194571c94c9 100644 (file)
--- 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 9620dd86fb3a1fdbddca08a46abab1b14c7f8520..8b20e4040193d8a39f6a925ec5ec96d47898528e 100644 (file)
--- 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 73a5379b73ec93226c529731b2f758107737e50c..f86887085191e8320900b11976a5dc80a75c560c 100644 (file)
--- 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 8fb7d6cc713449ecd8420b7d88f10b58a0de4e17..a461d1b8e0ea8dcccc3ac901b1d96d135f8a5a07 100644 (file)
--- 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;
 }