From c6da34a610e58f7e58042b5ed24a19bd2c18e928 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 13 Apr 2022 15:51:33 -0700 Subject: [PATCH] Revert "Merge branch 'ps/avoid-unnecessary-hook-invocation-with-packed-refs'" This reverts commit 991b4d47f0accd3955d05927d5ce434e03ffbdb6, reversing changes made to bcd020f88e1e22f38422ac3f73ab06b34ec4bef1. --- refs.c | 11 ++----- refs.h | 8 +---- refs/files-backend.c | 26 +++++------------ refs/packed-backend.c | 28 +++++------------- refs/packed-backend.h | 7 ----- refs/refs-internal.h | 1 - sequencer.c | 2 +- t/t1416-ref-transaction-hooks.sh | 50 -------------------------------- 8 files changed, 19 insertions(+), 114 deletions(-) diff --git a/refs.c b/refs.c index 1a964505f92..9db66e99551 100644 --- a/refs.c +++ b/refs.c @@ -794,7 +794,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; - transaction = ref_store_transaction_begin(refs, 0, &err); + transaction = ref_store_transaction_begin(refs, &err); if (!transaction || ref_transaction_delete(transaction, refname, old_oid, flags, msg, &err) || @@ -999,7 +999,6 @@ int read_ref_at(struct ref_store *refs, const char *refname, } struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs, - unsigned int flags, struct strbuf *err) { struct ref_transaction *tr; @@ -1007,13 +1006,12 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs, CALLOC_ARRAY(tr, 1); tr->ref_store = refs; - tr->flags = flags; return tr; } struct ref_transaction *ref_transaction_begin(struct strbuf *err) { - return ref_store_transaction_begin(get_main_ref_store(the_repository), 0, err); + return ref_store_transaction_begin(get_main_ref_store(the_repository), err); } void ref_transaction_free(struct ref_transaction *transaction) @@ -1152,7 +1150,7 @@ int refs_update_ref(struct ref_store *refs, const char *msg, struct strbuf err = STRBUF_INIT; int ret = 0; - t = ref_store_transaction_begin(refs, 0, &err); + t = ref_store_transaction_begin(refs, &err); if (!t || ref_transaction_update(t, refname, new_oid, old_oid, flags, msg, &err) || @@ -2074,9 +2072,6 @@ static int run_transaction_hook(struct ref_transaction *transaction, const char *hook; int ret = 0, i; - if (transaction->flags & REF_TRANSACTION_SKIP_HOOK) - return 0; - hook = find_hook("reference-transaction"); if (!hook) return ret; diff --git a/refs.h b/refs.h index 23479c7ee09..47cb9edbaa8 100644 --- a/refs.h +++ b/refs.h @@ -229,7 +229,7 @@ char *repo_default_branch_name(struct repository *r, int quiet); * struct strbuf err = STRBUF_INIT; * int ret = 0; * - * transaction = ref_store_transaction_begin(refs, 0, &err); + * transaction = ref_store_transaction_begin(refs, &err); * if (!transaction || * ref_transaction_update(...) || * ref_transaction_create(...) || @@ -566,17 +566,11 @@ enum action_on_err { UPDATE_REFS_QUIET_ON_ERR }; -/* - * Skip executing the reference-transaction hook. - */ -#define REF_TRANSACTION_SKIP_HOOK (1 << 0) - /* * Begin a reference transaction. The reference transaction must * be freed by calling ref_transaction_free(). */ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs, - unsigned int flags, struct strbuf *err); struct ref_transaction *ref_transaction_begin(struct strbuf *err); diff --git a/refs/files-backend.c b/refs/files-backend.c index 95acab78eef..8db7882aacb 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1136,8 +1136,7 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r) if (check_refname_format(r->name, 0)) return; - transaction = ref_store_transaction_begin(&refs->base, - REF_TRANSACTION_SKIP_HOOK, &err); + transaction = ref_store_transaction_begin(&refs->base, &err); if (!transaction) goto cleanup; ref_transaction_add_update( @@ -1208,8 +1207,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags) struct strbuf err = STRBUF_INIT; struct ref_transaction *transaction; - transaction = ref_store_transaction_begin(refs->packed_ref_store, - REF_TRANSACTION_SKIP_HOOK, &err); + transaction = ref_store_transaction_begin(refs->packed_ref_store, &err); if (!transaction) return -1; @@ -1266,7 +1264,6 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg, { struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_WRITE, "delete_refs"); - struct ref_transaction *transaction = NULL; struct strbuf err = STRBUF_INIT; int i, result = 0; @@ -1276,15 +1273,10 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg, if (packed_refs_lock(refs->packed_ref_store, 0, &err)) goto error; - transaction = ref_store_transaction_begin(refs->packed_ref_store, - REF_TRANSACTION_SKIP_HOOK, &err); - if (!transaction) - goto error; - - result = packed_refs_delete_refs(refs->packed_ref_store, - transaction, msg, refnames, flags); - if (result) + if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) { + packed_refs_unlock(refs->packed_ref_store); goto error; + } packed_refs_unlock(refs->packed_ref_store); @@ -1295,7 +1287,6 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg, result |= error(_("could not remove reference %s"), refname); } - ref_transaction_free(transaction); strbuf_release(&err); return result; @@ -1312,7 +1303,6 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg, else error(_("could not delete references: %s"), err.buf); - ref_transaction_free(transaction); strbuf_release(&err); return -1; } @@ -2784,8 +2774,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, */ if (!packed_transaction) { packed_transaction = ref_store_transaction_begin( - refs->packed_ref_store, - REF_TRANSACTION_SKIP_HOOK, err); + refs->packed_ref_store, err); if (!packed_transaction) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; @@ -3056,8 +3045,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store, &affected_refnames)) BUG("initial ref transaction called with existing refs"); - packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, - REF_TRANSACTION_SKIP_HOOK, err); + packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, err); if (!packed_transaction) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 66c4574c99d..97b68377673 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1522,10 +1522,15 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store, static int packed_delete_refs(struct ref_store *ref_store, const char *msg, struct string_list *refnames, unsigned int flags) { + struct packed_ref_store *refs = + packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs"); struct strbuf err = STRBUF_INIT; struct ref_transaction *transaction; + struct string_list_item *item; int ret; + (void)refs; /* We need the check above, but don't use the variable */ + if (!refnames->nr) return 0; @@ -1535,30 +1540,10 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg, * updates into a single transaction. */ - transaction = ref_store_transaction_begin(ref_store, 0, &err); + transaction = ref_store_transaction_begin(ref_store, &err); if (!transaction) return -1; - ret = packed_refs_delete_refs(ref_store, transaction, - msg, refnames, flags); - - ref_transaction_free(transaction); - return ret; -} - -int packed_refs_delete_refs(struct ref_store *ref_store, - struct ref_transaction *transaction, - const char *msg, - struct string_list *refnames, - unsigned int flags) -{ - struct strbuf err = STRBUF_INIT; - struct string_list_item *item; - int ret; - - /* Assert that the ref store refers to a packed backend. */ - packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs"); - for_each_string_list_item(item, refnames) { if (ref_transaction_delete(transaction, item->string, NULL, flags, msg, &err)) { @@ -1578,6 +1563,7 @@ int packed_refs_delete_refs(struct ref_store *ref_store, error(_("could not delete references: %s"), err.buf); } + ref_transaction_free(transaction); strbuf_release(&err); return ret; } diff --git a/refs/packed-backend.h b/refs/packed-backend.h index 52e0490753c..9dd8a344c34 100644 --- a/refs/packed-backend.h +++ b/refs/packed-backend.h @@ -3,7 +3,6 @@ struct repository; struct ref_transaction; -struct string_list; /* * Support for storing references in a `packed-refs` file. @@ -28,12 +27,6 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err) void packed_refs_unlock(struct ref_store *ref_store); int packed_refs_is_locked(struct ref_store *ref_store); -int packed_refs_delete_refs(struct ref_store *ref_store, - struct ref_transaction *transaction, - const char *msg, - struct string_list *refnames, - unsigned int flags); - /* * Return true if `transaction` really needs to be carried out against * the specified packed_ref_store, or false if it can be skipped diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 001ef158354..69f93b0e2ac 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -213,7 +213,6 @@ struct ref_transaction { size_t nr; enum ref_transaction_state state; void *backend_data; - unsigned int flags; }; /* diff --git a/sequencer.c b/sequencer.c index a1bb39383db..a5f678f4529 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3584,7 +3584,7 @@ static int do_label(struct repository *r, const char *name, int len) strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name); strbuf_addf(&msg, "rebase (label) '%.*s'", len, name); - transaction = ref_store_transaction_begin(refs, 0, &err); + transaction = ref_store_transaction_begin(refs, &err); if (!transaction) { error("%s", err.buf); ret = -1; diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 085a7a46f21..27731722a5b 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -133,54 +133,4 @@ test_expect_success 'interleaving hook calls succeed' ' test_cmp expect target-repo.git/actual ' -test_expect_success 'hook does not get called on packing refs' ' - # Pack references first such that we are in a known state. - git pack-refs --all && - - test_hook reference-transaction <<-\EOF && - echo "$@" >>actual - cat >>actual - EOF - rm -f actual && - - git update-ref refs/heads/unpacked-ref $POST_OID && - git pack-refs --all && - - # We only expect a single hook invocation, which is the call to - # git-update-ref(1). - cat >expect <<-EOF && - prepared - $ZERO_OID $POST_OID refs/heads/unpacked-ref - committed - $ZERO_OID $POST_OID refs/heads/unpacked-ref - EOF - - test_cmp expect actual -' - -test_expect_success 'deleting packed ref calls hook once' ' - # Create a reference and pack it. - git update-ref refs/heads/to-be-deleted $POST_OID && - git pack-refs --all && - - test_hook reference-transaction <<-\EOF && - echo "$@" >>actual - cat >>actual - EOF - rm -f actual && - - git update-ref -d refs/heads/to-be-deleted $POST_OID && - - # We only expect a single hook invocation, which is the logical - # deletion. - cat >expect <<-EOF && - prepared - $POST_OID $ZERO_OID refs/heads/to-be-deleted - committed - $POST_OID $ZERO_OID refs/heads/to-be-deleted - EOF - - test_cmp expect actual -' - test_done