From be8b2457721e1ec154ecb6e037e797b37578ea62 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 13 Jul 2022 07:36:13 +0200 Subject: Makefile: Update Git to v2.37.1 Update our bundled Git version to v2.37.1. This both updates our major version to include the latest changes from v2.37, but also updates our minor version to include fixes for CVE-2022-29187, which is another variant of opening repositories owned by a different user leading to privilege escalation. To the best of my knowledge, Gitaly is not impacted by this specific vulnerability. It does not perform repository discovery by walking up the filesystem hierarchy and thus wouldn't pick up repositories in any of the parent directories of the storage root. And if an adversary is in a posititon to change the owner of repositories contained in Gitaly's storage root, they would already have other ways to attack the host. Also note that we're upgrading the bundled Git version v2.36.1 in-place. This can be done because its feature flag is not yet default-enabled and hasn't been rolled out anywhere due to a set of incompatibilities. Changelog: changed --- Makefile | 16 +- ...t-packed_refs_delete_refs-to-allow-contro.patch | 156 ------------------ ...passing-flags-when-beginning-transactions.patch | 183 --------------------- ...w-skipping-the-reference-transaction-hook.patch | 60 ------- ...trate-excessive-execution-of-the-referenc.patch | 97 ----------- ...-execute-reference-transaction-hook-on-pa.patch | 81 --------- ...hooks-when-deleting-uncovered-packed-refs.patch | 99 ----------- ...t-packed_refs_delete_refs-to-allow-contro.patch | 156 ++++++++++++++++++ ...passing-flags-when-beginning-transactions.patch | 183 +++++++++++++++++++++ ...w-skipping-the-reference-transaction-hook.patch | 60 +++++++ ...trate-excessive-execution-of-the-referenc.patch | 97 +++++++++++ ...-execute-reference-transaction-hook-on-pa.patch | 81 +++++++++ ...hooks-when-deleting-uncovered-packed-refs.patch | 99 +++++++++++ internal/git/command_factory_test.go | 4 +- internal/git/execution_environment.go | 4 +- internal/metadata/featureflag/ff_git_v2361.go | 9 - internal/metadata/featureflag/ff_git_v2371.go | 9 + internal/testhelper/testhelper.go | 2 +- 18 files changed, 698 insertions(+), 698 deletions(-) delete mode 100644 _support/git-patches/v2.36.0.gl1/0001-refs-extract-packed_refs_delete_refs-to-allow-contro.patch delete mode 100644 _support/git-patches/v2.36.0.gl1/0002-refs-allow-passing-flags-when-beginning-transactions.patch delete mode 100644 _support/git-patches/v2.36.0.gl1/0003-refs-allow-skipping-the-reference-transaction-hook.patch delete mode 100644 _support/git-patches/v2.36.0.gl1/0004-refs-demonstrate-excessive-execution-of-the-referenc.patch delete mode 100644 _support/git-patches/v2.36.0.gl1/0005-refs-do-not-execute-reference-transaction-hook-on-pa.patch delete mode 100644 _support/git-patches/v2.36.0.gl1/0006-refs-skip-hooks-when-deleting-uncovered-packed-refs.patch create mode 100644 _support/git-patches/v2.37.1.gl1/0001-refs-extract-packed_refs_delete_refs-to-allow-contro.patch create mode 100644 _support/git-patches/v2.37.1.gl1/0002-refs-allow-passing-flags-when-beginning-transactions.patch create mode 100644 _support/git-patches/v2.37.1.gl1/0003-refs-allow-skipping-the-reference-transaction-hook.patch create mode 100644 _support/git-patches/v2.37.1.gl1/0004-refs-demonstrate-excessive-execution-of-the-referenc.patch create mode 100644 _support/git-patches/v2.37.1.gl1/0005-refs-do-not-execute-reference-transaction-hook-on-pa.patch create mode 100644 _support/git-patches/v2.37.1.gl1/0006-refs-skip-hooks-when-deleting-uncovered-packed-refs.patch delete mode 100644 internal/metadata/featureflag/ff_git_v2361.go create mode 100644 internal/metadata/featureflag/ff_git_v2371.go diff --git a/Makefile b/Makefile index 1d80763af..a5efefe7a 100644 --- a/Makefile +++ b/Makefile @@ -325,17 +325,17 @@ install: build .PHONY: build-bundled-git ## Build bundled Git binaries. build-bundled-git: build-bundled-git-v2.35.1.gl1 -build-bundled-git: build-bundled-git-v2.36.1.gl1 +build-bundled-git: build-bundled-git-v2.37.1.gl1 build-bundled-git-v2.35.1.gl1: $(patsubst %,${BUILD_DIR}/bin/gitaly-%-v2.35.1.gl1,${GIT_EXECUTABLES}) -build-bundled-git-v2.36.1.gl1: $(patsubst %,${BUILD_DIR}/bin/gitaly-%-v2.36.1.gl1,${GIT_EXECUTABLES}) +build-bundled-git-v2.37.1.gl1: $(patsubst %,${BUILD_DIR}/bin/gitaly-%-v2.37.1.gl1,${GIT_EXECUTABLES}) .PHONY: install-bundled-git ## Install bundled Git binaries. The target directory can be modified by ## setting PREFIX and DESTDIR. install-bundled-git: install-bundled-git-v2.35.1.gl1 -install-bundled-git: install-bundled-git-v2.36.1.gl1 +install-bundled-git: install-bundled-git-v2.37.1.gl1 install-bundled-git-v2.35.1.gl1: $(patsubst %,${INSTALL_DEST_DIR}/gitaly-%-v2.35.1.gl1,${GIT_EXECUTABLES}) -install-bundled-git-v2.36.1.gl1: $(patsubst %,${INSTALL_DEST_DIR}/gitaly-%-v2.36.1.gl1,${GIT_EXECUTABLES}) +install-bundled-git-v2.37.1.gl1: $(patsubst %,${INSTALL_DEST_DIR}/gitaly-%-v2.37.1.gl1,${GIT_EXECUTABLES}) ifdef WITH_BUNDLED_GIT build: build-bundled-git @@ -560,10 +560,10 @@ ${BUILD_DIR}/bin/gitaly-%-v2.35.1.gl1: override GIT_EXTRA_VERSION = gl1 ${BUILD_DIR}/bin/gitaly-%-v2.35.1.gl1: ${DEPENDENCY_DIR}/git-v2.35.1.gl1/% | ${BUILD_DIR}/bin ${Q}install $< $@ -${BUILD_DIR}/bin/gitaly-%-v2.36.1.gl1: override GIT_PATCHES := $(sort $(wildcard ${SOURCE_DIR}/_support/git-patches/v2.36.1.gl1/*)) -${BUILD_DIR}/bin/gitaly-%-v2.36.1.gl1: override GIT_VERSION = v2.36.1 -${BUILD_DIR}/bin/gitaly-%-v2.36.1.gl1: override GIT_EXTRA_VERSION = gl1 -${BUILD_DIR}/bin/gitaly-%-v2.36.1.gl1: ${DEPENDENCY_DIR}/git-v2.36.1.gl1/% | ${BUILD_DIR}/bin +${BUILD_DIR}/bin/gitaly-%-v2.37.1.gl1: override GIT_PATCHES := $(sort $(wildcard ${SOURCE_DIR}/_support/git-patches/v2.37.1.gl1/*)) +${BUILD_DIR}/bin/gitaly-%-v2.37.1.gl1: override GIT_VERSION = v2.37.1 +${BUILD_DIR}/bin/gitaly-%-v2.37.1.gl1: override GIT_EXTRA_VERSION = gl1 +${BUILD_DIR}/bin/gitaly-%-v2.37.1.gl1: ${DEPENDENCY_DIR}/git-v2.37.1.gl1/% | ${BUILD_DIR}/bin ${Q}install $< $@ ${BUILD_DIR}/bin/%: ${BUILD_DIR}/intermediate/% | ${BUILD_DIR}/bin diff --git a/_support/git-patches/v2.36.0.gl1/0001-refs-extract-packed_refs_delete_refs-to-allow-contro.patch b/_support/git-patches/v2.36.0.gl1/0001-refs-extract-packed_refs_delete_refs-to-allow-contro.patch deleted file mode 100644 index 47dd3e41c..000000000 --- a/_support/git-patches/v2.36.0.gl1/0001-refs-extract-packed_refs_delete_refs-to-allow-contro.patch +++ /dev/null @@ -1,156 +0,0 @@ -From c74f385fb46855ac0db222b6845ddb95e6a36264 Mon Sep 17 00:00:00 2001 -Message-Id: -From: Patrick Steinhardt -Date: Mon, 17 Jan 2022 09:12:31 +0100 -Subject: [PATCH 21/34] refs: extract packed_refs_delete_refs() to allow - control of transaction - -When deleting loose refs, then we also have to delete the refs in the -packed backend. This is done by calling `refs_delete_refs()`, which -then uses the packed-backend's logic to delete refs. This doesn't allow -us to exercise any control over the reference transaction which is being -created in the packed backend, which is required in a subsequent commit. - -Extract a new function `packed_refs_delete_refs()`, which hosts most of -the logic to delete refs except for creating the transaction itself. -Like this, we can easily create the transaction in the files backend -and thus exert more control over it. - -Signed-off-by: Patrick Steinhardt -Signed-off-by: Junio C Hamano -(cherry picked from commit 69840cc0f7b4f3352903bd2b8f3de7077916c26b) ---- - refs/files-backend.c | 13 ++++++++++--- - refs/packed-backend.c | 26 ++++++++++++++++++++------ - refs/packed-backend.h | 7 +++++++ - 3 files changed, 37 insertions(+), 9 deletions(-) - -diff --git a/refs/files-backend.c b/refs/files-backend.c -index 43a3b882d7..18baea4c6a 100644 ---- a/refs/files-backend.c -+++ b/refs/files-backend.c -@@ -1244,6 +1244,7 @@ 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; - -@@ -1253,10 +1254,14 @@ 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; - -- if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) { -- packed_refs_unlock(refs->packed_ref_store); -+ transaction = ref_store_transaction_begin(refs->packed_ref_store, &err); -+ if (!transaction) -+ goto error; -+ -+ result = packed_refs_delete_refs(refs->packed_ref_store, -+ transaction, msg, refnames, flags); -+ if (result) - goto error; -- } - - packed_refs_unlock(refs->packed_ref_store); - -@@ -1267,6 +1272,7 @@ 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; - -@@ -1283,6 +1289,7 @@ 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; - } -diff --git a/refs/packed-backend.c b/refs/packed-backend.c -index d91a2018f6..960b43ff76 100644 ---- a/refs/packed-backend.c -+++ b/refs/packed-backend.c -@@ -1521,15 +1521,10 @@ 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; - -@@ -1543,6 +1538,26 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg, - 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)) { -@@ -1562,7 +1577,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg, - 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 9dd8a344c3..52e0490753 100644 ---- a/refs/packed-backend.h -+++ b/refs/packed-backend.h -@@ -3,6 +3,7 @@ - - struct repository; - struct ref_transaction; -+struct string_list; - - /* - * Support for storing references in a `packed-refs` file. -@@ -27,6 +28,12 @@ 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 --- -2.35.1 - diff --git a/_support/git-patches/v2.36.0.gl1/0002-refs-allow-passing-flags-when-beginning-transactions.patch b/_support/git-patches/v2.36.0.gl1/0002-refs-allow-passing-flags-when-beginning-transactions.patch deleted file mode 100644 index 8038daca2..000000000 --- a/_support/git-patches/v2.36.0.gl1/0002-refs-allow-passing-flags-when-beginning-transactions.patch +++ /dev/null @@ -1,183 +0,0 @@ -From 60d0682e02db5c197c09bc4cd5a643dc2269cca2 Mon Sep 17 00:00:00 2001 -Message-Id: <60d0682e02db5c197c09bc4cd5a643dc2269cca2.1646206541.git.ps@pks.im> -In-Reply-To: -References: -From: Patrick Steinhardt -Date: Mon, 17 Jan 2022 09:12:35 +0100 -Subject: [PATCH 22/34] refs: allow passing flags when beginning transactions - -We do not currently have any flags when creating reference transactions, -but we'll add one to disable execution of the reference transaction hook -in some cases. - -Allow passing flags to `ref_store_transaction_begin()` to prepare for -this change. - -Signed-off-by: Patrick Steinhardt -Signed-off-by: Junio C Hamano -(cherry picked from commit fbe73f61cbc29f6c4a85478cf792c37dbe5aa26c) ---- - refs.c | 8 +++++--- - refs.h | 3 ++- - refs/files-backend.c | 10 +++++----- - refs/packed-backend.c | 2 +- - refs/refs-internal.h | 1 + - sequencer.c | 2 +- - 6 files changed, 15 insertions(+), 11 deletions(-) - -diff --git a/refs.c b/refs.c -index addb26293b..f498d232e5 100644 ---- a/refs.c -+++ b/refs.c -@@ -800,7 +800,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, &err); -+ transaction = ref_store_transaction_begin(refs, 0, &err); - if (!transaction || - ref_transaction_delete(transaction, refname, old_oid, - flags, msg, &err) || -@@ -1005,6 +1005,7 @@ 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; -@@ -1012,12 +1013,13 @@ 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), err); -+ return ref_store_transaction_begin(get_main_ref_store(the_repository), 0, err); - } - - void ref_transaction_free(struct ref_transaction *transaction) -@@ -1156,7 +1158,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, &err); -+ t = ref_store_transaction_begin(refs, 0, &err); - if (!t || - ref_transaction_update(t, refname, new_oid, old_oid, flags, msg, - &err) || -diff --git a/refs.h b/refs.h -index 8f91a7f9ff..3a141d7066 100644 ---- a/refs.h -+++ b/refs.h -@@ -231,7 +231,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, &err); -+ * transaction = ref_store_transaction_begin(refs, 0, &err); - * if (!transaction || - * ref_transaction_update(...) || - * ref_transaction_create(...) || -@@ -573,6 +573,7 @@ enum action_on_err { - * 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 18baea4c6a..758d12a0fa 100644 ---- a/refs/files-backend.c -+++ b/refs/files-backend.c -@@ -1116,7 +1116,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, &err); -+ transaction = ref_store_transaction_begin(&refs->base, 0, &err); - if (!transaction) - goto cleanup; - ref_transaction_add_update( -@@ -1187,7 +1187,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, &err); -+ transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err); - if (!transaction) - return -1; - -@@ -1254,7 +1254,7 @@ 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, &err); -+ transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err); - if (!transaction) - goto error; - -@@ -2769,7 +2769,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, - */ - if (!packed_transaction) { - packed_transaction = ref_store_transaction_begin( -- refs->packed_ref_store, err); -+ refs->packed_ref_store, 0, err); - if (!packed_transaction) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; -@@ -3040,7 +3040,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, err); -+ packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, err); - if (!packed_transaction) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; -diff --git a/refs/packed-backend.c b/refs/packed-backend.c -index 960b43ff76..27dd8c3922 100644 ---- a/refs/packed-backend.c -+++ b/refs/packed-backend.c -@@ -1534,7 +1534,7 @@ 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, &err); -+ transaction = ref_store_transaction_begin(ref_store, 0, &err); - if (!transaction) - return -1; - -diff --git a/refs/refs-internal.h b/refs/refs-internal.h -index 7ff6fba4f0..6e15db3ca4 100644 ---- a/refs/refs-internal.h -+++ b/refs/refs-internal.h -@@ -213,6 +213,7 @@ 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 5213d16e97..5a2b609557 100644 ---- a/sequencer.c -+++ b/sequencer.c -@@ -3588,7 +3588,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, &err); -+ transaction = ref_store_transaction_begin(refs, 0, &err); - if (!transaction) { - error("%s", err.buf); - ret = -1; --- -2.35.1 - diff --git a/_support/git-patches/v2.36.0.gl1/0003-refs-allow-skipping-the-reference-transaction-hook.patch b/_support/git-patches/v2.36.0.gl1/0003-refs-allow-skipping-the-reference-transaction-hook.patch deleted file mode 100644 index cd194f8aa..000000000 --- a/_support/git-patches/v2.36.0.gl1/0003-refs-allow-skipping-the-reference-transaction-hook.patch +++ /dev/null @@ -1,60 +0,0 @@ -From 4eccd16b45516df5ab02288d0c50c16e03cc44e4 Mon Sep 17 00:00:00 2001 -Message-Id: <4eccd16b45516df5ab02288d0c50c16e03cc44e4.1646206541.git.ps@pks.im> -In-Reply-To: -References: -From: Patrick Steinhardt -Date: Mon, 17 Jan 2022 09:12:39 +0100 -Subject: [PATCH 23/34] refs: allow skipping the reference-transaction hook - -The reference-transaction hook is executing whenever we prepare, commit -or abort a reference transaction. While this is mostly intentional, in -case of the files backend we're leaking the implementation detail that -the store is in fact a composite store with one loose and one packed -backend to the caller. So while we want to execute the hook for all -logical updates, executing it for such implementation details is -unexpected. - -Prepare for a fix by adding a new flag which allows to skip execution of -the hook. - -Signed-off-by: Patrick Steinhardt -Signed-off-by: Junio C Hamano -(cherry picked from commit 958fbc74e3d0fcc88b2065190e23db556a963644) ---- - refs.c | 3 +++ - refs.h | 5 +++++ - 2 files changed, 8 insertions(+) - -diff --git a/refs.c b/refs.c -index f498d232e5..435b90b1ec 100644 ---- a/refs.c -+++ b/refs.c -@@ -2084,6 +2084,9 @@ 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 3a141d7066..a015354fd6 100644 ---- a/refs.h -+++ b/refs.h -@@ -568,6 +568,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(). --- -2.35.1 - diff --git a/_support/git-patches/v2.36.0.gl1/0004-refs-demonstrate-excessive-execution-of-the-referenc.patch b/_support/git-patches/v2.36.0.gl1/0004-refs-demonstrate-excessive-execution-of-the-referenc.patch deleted file mode 100644 index aa6d96a3e..000000000 --- a/_support/git-patches/v2.36.0.gl1/0004-refs-demonstrate-excessive-execution-of-the-referenc.patch +++ /dev/null @@ -1,97 +0,0 @@ -From 079f96ed16bb70f99fbe810b1646b1709bb82871 Mon Sep 17 00:00:00 2001 -Message-Id: <079f96ed16bb70f99fbe810b1646b1709bb82871.1646206541.git.ps@pks.im> -In-Reply-To: -References: -From: Patrick Steinhardt -Date: Mon, 17 Jan 2022 09:12:44 +0100 -Subject: [PATCH 24/34] refs: demonstrate excessive execution of the - reference-transaction hook - -Add tests which demonstate that we're executing the -reference-transaction hook too often in some cases, which thus leaks -implementation details about the reference store's implementation -itself. Behaviour will be fixed in follow-up commits. - -Signed-off-by: Patrick Steinhardt -Signed-off-by: Junio C Hamano -(cherry picked from commit 2ce825436268d6409bee8ebb5f5500b7ff172b1e) ---- - t/t1416-ref-transaction-hooks.sh | 64 ++++++++++++++++++++++++++++++++ - 1 file changed, 64 insertions(+) - -diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh -index 6c941027a8..0567fbdf0b 100755 ---- a/t/t1416-ref-transaction-hooks.sh -+++ b/t/t1416-ref-transaction-hooks.sh -@@ -136,4 +136,68 @@ 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 && -+ -+ write_script .git/hooks/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). But currently, packing refs will also trigger the -+ # hook. -+ cat >expect <<-EOF && -+ prepared -+ $ZERO_OID $POST_OID refs/heads/unpacked-ref -+ committed -+ $ZERO_OID $POST_OID refs/heads/unpacked-ref -+ prepared -+ $ZERO_OID $POST_OID refs/heads/unpacked-ref -+ committed -+ $ZERO_OID $POST_OID refs/heads/unpacked-ref -+ prepared -+ $POST_OID $ZERO_OID refs/heads/unpacked-ref -+ committed -+ $POST_OID $ZERO_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 && -+ -+ write_script .git/hooks/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. But currently, we see two interleaving transactions, once -+ # for deleting the loose refs and once for deleting the packed ref. -+ cat >expect <<-EOF && -+ prepared -+ $ZERO_OID $ZERO_OID refs/heads/to-be-deleted -+ prepared -+ $POST_OID $ZERO_OID refs/heads/to-be-deleted -+ committed -+ $ZERO_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 --- -2.35.1 - diff --git a/_support/git-patches/v2.36.0.gl1/0005-refs-do-not-execute-reference-transaction-hook-on-pa.patch b/_support/git-patches/v2.36.0.gl1/0005-refs-do-not-execute-reference-transaction-hook-on-pa.patch deleted file mode 100644 index f2e7c06ed..000000000 --- a/_support/git-patches/v2.36.0.gl1/0005-refs-do-not-execute-reference-transaction-hook-on-pa.patch +++ /dev/null @@ -1,81 +0,0 @@ -From ef35ab926309bf406d3871679c576c718708a93b Mon Sep 17 00:00:00 2001 -Message-Id: -In-Reply-To: -References: -From: Patrick Steinhardt -Date: Mon, 17 Jan 2022 09:12:48 +0100 -Subject: [PATCH 25/34] refs: do not execute reference-transaction hook on - packing refs - -The reference-transaction hook is supposed to track logical changes to -references, but it currently also gets executed when packing refs in a -repository. This is unexpected and ultimately not all that useful: -packing refs is not supposed to result in any user-visible change to the -refs' state, and it ultimately is an implementation detail of how refs -stores work. - -Fix this excessive execution of the hook when packing refs. - -Reported-by: Waleed Khan -Signed-off-by: Patrick Steinhardt -Signed-off-by: Junio C Hamano -(cherry picked from commit ffad9941383465553bf26d88050f3243726f30df) ---- - refs/files-backend.c | 6 ++++-- - t/t1416-ref-transaction-hooks.sh | 11 +---------- - 2 files changed, 5 insertions(+), 12 deletions(-) - -diff --git a/refs/files-backend.c b/refs/files-backend.c -index 758d12a0fa..19f43e8d29 100644 ---- a/refs/files-backend.c -+++ b/refs/files-backend.c -@@ -1116,7 +1116,8 @@ 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, 0, &err); -+ transaction = ref_store_transaction_begin(&refs->base, -+ REF_TRANSACTION_SKIP_HOOK, &err); - if (!transaction) - goto cleanup; - ref_transaction_add_update( -@@ -1187,7 +1188,8 @@ 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, 0, &err); -+ transaction = ref_store_transaction_begin(refs->packed_ref_store, -+ REF_TRANSACTION_SKIP_HOOK, &err); - if (!transaction) - return -1; - -diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh -index 0567fbdf0b..f9d3d5213f 100755 ---- a/t/t1416-ref-transaction-hooks.sh -+++ b/t/t1416-ref-transaction-hooks.sh -@@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' ' - git pack-refs --all && - - # We only expect a single hook invocation, which is the call to -- # git-update-ref(1). But currently, packing refs will also trigger the -- # hook. -+ # 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 -- prepared -- $ZERO_OID $POST_OID refs/heads/unpacked-ref -- committed -- $ZERO_OID $POST_OID refs/heads/unpacked-ref -- prepared -- $POST_OID $ZERO_OID refs/heads/unpacked-ref -- committed -- $POST_OID $ZERO_OID refs/heads/unpacked-ref - EOF - - test_cmp expect actual --- -2.35.1 - diff --git a/_support/git-patches/v2.36.0.gl1/0006-refs-skip-hooks-when-deleting-uncovered-packed-refs.patch b/_support/git-patches/v2.36.0.gl1/0006-refs-skip-hooks-when-deleting-uncovered-packed-refs.patch deleted file mode 100644 index 3b21bf489..000000000 --- a/_support/git-patches/v2.36.0.gl1/0006-refs-skip-hooks-when-deleting-uncovered-packed-refs.patch +++ /dev/null @@ -1,99 +0,0 @@ -From d56f2a0e1eed4d0a39b99638117e0c8259e5078d Mon Sep 17 00:00:00 2001 -Message-Id: -In-Reply-To: -References: -From: Patrick Steinhardt -Date: Mon, 17 Jan 2022 09:12:53 +0100 -Subject: [PATCH 26/34] refs: skip hooks when deleting uncovered packed refs - -When deleting refs from the loose-files refs backend, then we need to be -careful to also delete the same ref from the packed refs backend, if it -exists. If we don't, then deleting the loose ref would "uncover" the -packed ref. We thus always have to queue up deletions of refs for both -the loose and the packed refs backend. This is done in two separate -transactions, where the end result is that the reference-transaction -hook is executed twice for the deleted refs. - -This behaviour is quite misleading: it's exposing implementation details -of how the files backend works to the user, in contrast to the logical -updates that we'd really want to expose via the hook. Worse yet, whether -the hook gets executed once or twice depends on how well-packed the -repository is: if the ref only exists as a loose ref, then we execute it -once, otherwise if it is also packed then we execute it twice. - -Fix this behaviour and don't execute the reference-transaction hook at -all when refs in the packed-refs backend if it's driven by the files -backend. This works as expected even in case the refs to be deleted only -exist in the packed-refs backend because the loose-backend always queues -refs in its own transaction even if they don't exist such that they can -be locked for concurrent creation. And it also does the right thing in -case neither of the backends has the ref because that would cause the -transaction to fail completely. - -Signed-off-by: Patrick Steinhardt -Signed-off-by: Junio C Hamano -(cherry picked from commit 2ed1b64ebdeefc7f9473ae159fb45ff0c6cf121a) ---- - refs/files-backend.c | 9 ++++++--- - t/t1416-ref-transaction-hooks.sh | 7 +------ - 2 files changed, 7 insertions(+), 9 deletions(-) - -diff --git a/refs/files-backend.c b/refs/files-backend.c -index 19f43e8d29..c931dd479c 100644 ---- a/refs/files-backend.c -+++ b/refs/files-backend.c -@@ -1256,7 +1256,8 @@ 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, 0, &err); -+ transaction = ref_store_transaction_begin(refs->packed_ref_store, -+ REF_TRANSACTION_SKIP_HOOK, &err); - if (!transaction) - goto error; - -@@ -2771,7 +2772,8 @@ static int files_transaction_prepare(struct ref_store *ref_store, - */ - if (!packed_transaction) { - packed_transaction = ref_store_transaction_begin( -- refs->packed_ref_store, 0, err); -+ refs->packed_ref_store, -+ REF_TRANSACTION_SKIP_HOOK, err); - if (!packed_transaction) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; -@@ -3042,7 +3044,8 @@ 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, 0, err); -+ packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, -+ REF_TRANSACTION_SKIP_HOOK, err); - if (!packed_transaction) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; -diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh -index f9d3d5213f..4e1e84a91f 100755 ---- a/t/t1416-ref-transaction-hooks.sh -+++ b/t/t1416-ref-transaction-hooks.sh -@@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' ' - git update-ref -d refs/heads/to-be-deleted $POST_OID && - - # We only expect a single hook invocation, which is the logical -- # deletion. But currently, we see two interleaving transactions, once -- # for deleting the loose refs and once for deleting the packed ref. -+ # deletion. - cat >expect <<-EOF && -- prepared -- $ZERO_OID $ZERO_OID refs/heads/to-be-deleted - prepared - $POST_OID $ZERO_OID refs/heads/to-be-deleted - committed -- $ZERO_OID $ZERO_OID refs/heads/to-be-deleted -- committed - $POST_OID $ZERO_OID refs/heads/to-be-deleted - EOF - --- -2.35.1 - diff --git a/_support/git-patches/v2.37.1.gl1/0001-refs-extract-packed_refs_delete_refs-to-allow-contro.patch b/_support/git-patches/v2.37.1.gl1/0001-refs-extract-packed_refs_delete_refs-to-allow-contro.patch new file mode 100644 index 000000000..47dd3e41c --- /dev/null +++ b/_support/git-patches/v2.37.1.gl1/0001-refs-extract-packed_refs_delete_refs-to-allow-contro.patch @@ -0,0 +1,156 @@ +From c74f385fb46855ac0db222b6845ddb95e6a36264 Mon Sep 17 00:00:00 2001 +Message-Id: +From: Patrick Steinhardt +Date: Mon, 17 Jan 2022 09:12:31 +0100 +Subject: [PATCH 21/34] refs: extract packed_refs_delete_refs() to allow + control of transaction + +When deleting loose refs, then we also have to delete the refs in the +packed backend. This is done by calling `refs_delete_refs()`, which +then uses the packed-backend's logic to delete refs. This doesn't allow +us to exercise any control over the reference transaction which is being +created in the packed backend, which is required in a subsequent commit. + +Extract a new function `packed_refs_delete_refs()`, which hosts most of +the logic to delete refs except for creating the transaction itself. +Like this, we can easily create the transaction in the files backend +and thus exert more control over it. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +(cherry picked from commit 69840cc0f7b4f3352903bd2b8f3de7077916c26b) +--- + refs/files-backend.c | 13 ++++++++++--- + refs/packed-backend.c | 26 ++++++++++++++++++++------ + refs/packed-backend.h | 7 +++++++ + 3 files changed, 37 insertions(+), 9 deletions(-) + +diff --git a/refs/files-backend.c b/refs/files-backend.c +index 43a3b882d7..18baea4c6a 100644 +--- a/refs/files-backend.c ++++ b/refs/files-backend.c +@@ -1244,6 +1244,7 @@ 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; + +@@ -1253,10 +1254,14 @@ 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; + +- if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) { +- packed_refs_unlock(refs->packed_ref_store); ++ transaction = ref_store_transaction_begin(refs->packed_ref_store, &err); ++ if (!transaction) ++ goto error; ++ ++ result = packed_refs_delete_refs(refs->packed_ref_store, ++ transaction, msg, refnames, flags); ++ if (result) + goto error; +- } + + packed_refs_unlock(refs->packed_ref_store); + +@@ -1267,6 +1272,7 @@ 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; + +@@ -1283,6 +1289,7 @@ 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; + } +diff --git a/refs/packed-backend.c b/refs/packed-backend.c +index d91a2018f6..960b43ff76 100644 +--- a/refs/packed-backend.c ++++ b/refs/packed-backend.c +@@ -1521,15 +1521,10 @@ 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; + +@@ -1543,6 +1538,26 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg, + 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)) { +@@ -1562,7 +1577,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg, + 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 9dd8a344c3..52e0490753 100644 +--- a/refs/packed-backend.h ++++ b/refs/packed-backend.h +@@ -3,6 +3,7 @@ + + struct repository; + struct ref_transaction; ++struct string_list; + + /* + * Support for storing references in a `packed-refs` file. +@@ -27,6 +28,12 @@ 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 +-- +2.35.1 + diff --git a/_support/git-patches/v2.37.1.gl1/0002-refs-allow-passing-flags-when-beginning-transactions.patch b/_support/git-patches/v2.37.1.gl1/0002-refs-allow-passing-flags-when-beginning-transactions.patch new file mode 100644 index 000000000..8038daca2 --- /dev/null +++ b/_support/git-patches/v2.37.1.gl1/0002-refs-allow-passing-flags-when-beginning-transactions.patch @@ -0,0 +1,183 @@ +From 60d0682e02db5c197c09bc4cd5a643dc2269cca2 Mon Sep 17 00:00:00 2001 +Message-Id: <60d0682e02db5c197c09bc4cd5a643dc2269cca2.1646206541.git.ps@pks.im> +In-Reply-To: +References: +From: Patrick Steinhardt +Date: Mon, 17 Jan 2022 09:12:35 +0100 +Subject: [PATCH 22/34] refs: allow passing flags when beginning transactions + +We do not currently have any flags when creating reference transactions, +but we'll add one to disable execution of the reference transaction hook +in some cases. + +Allow passing flags to `ref_store_transaction_begin()` to prepare for +this change. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +(cherry picked from commit fbe73f61cbc29f6c4a85478cf792c37dbe5aa26c) +--- + refs.c | 8 +++++--- + refs.h | 3 ++- + refs/files-backend.c | 10 +++++----- + refs/packed-backend.c | 2 +- + refs/refs-internal.h | 1 + + sequencer.c | 2 +- + 6 files changed, 15 insertions(+), 11 deletions(-) + +diff --git a/refs.c b/refs.c +index addb26293b..f498d232e5 100644 +--- a/refs.c ++++ b/refs.c +@@ -800,7 +800,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, &err); ++ transaction = ref_store_transaction_begin(refs, 0, &err); + if (!transaction || + ref_transaction_delete(transaction, refname, old_oid, + flags, msg, &err) || +@@ -1005,6 +1005,7 @@ 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; +@@ -1012,12 +1013,13 @@ 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), err); ++ return ref_store_transaction_begin(get_main_ref_store(the_repository), 0, err); + } + + void ref_transaction_free(struct ref_transaction *transaction) +@@ -1156,7 +1158,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, &err); ++ t = ref_store_transaction_begin(refs, 0, &err); + if (!t || + ref_transaction_update(t, refname, new_oid, old_oid, flags, msg, + &err) || +diff --git a/refs.h b/refs.h +index 8f91a7f9ff..3a141d7066 100644 +--- a/refs.h ++++ b/refs.h +@@ -231,7 +231,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, &err); ++ * transaction = ref_store_transaction_begin(refs, 0, &err); + * if (!transaction || + * ref_transaction_update(...) || + * ref_transaction_create(...) || +@@ -573,6 +573,7 @@ enum action_on_err { + * 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 18baea4c6a..758d12a0fa 100644 +--- a/refs/files-backend.c ++++ b/refs/files-backend.c +@@ -1116,7 +1116,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, &err); ++ transaction = ref_store_transaction_begin(&refs->base, 0, &err); + if (!transaction) + goto cleanup; + ref_transaction_add_update( +@@ -1187,7 +1187,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, &err); ++ transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err); + if (!transaction) + return -1; + +@@ -1254,7 +1254,7 @@ 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, &err); ++ transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err); + if (!transaction) + goto error; + +@@ -2769,7 +2769,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, + */ + if (!packed_transaction) { + packed_transaction = ref_store_transaction_begin( +- refs->packed_ref_store, err); ++ refs->packed_ref_store, 0, err); + if (!packed_transaction) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; +@@ -3040,7 +3040,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, err); ++ packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, err); + if (!packed_transaction) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; +diff --git a/refs/packed-backend.c b/refs/packed-backend.c +index 960b43ff76..27dd8c3922 100644 +--- a/refs/packed-backend.c ++++ b/refs/packed-backend.c +@@ -1534,7 +1534,7 @@ 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, &err); ++ transaction = ref_store_transaction_begin(ref_store, 0, &err); + if (!transaction) + return -1; + +diff --git a/refs/refs-internal.h b/refs/refs-internal.h +index 7ff6fba4f0..6e15db3ca4 100644 +--- a/refs/refs-internal.h ++++ b/refs/refs-internal.h +@@ -213,6 +213,7 @@ 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 5213d16e97..5a2b609557 100644 +--- a/sequencer.c ++++ b/sequencer.c +@@ -3588,7 +3588,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, &err); ++ transaction = ref_store_transaction_begin(refs, 0, &err); + if (!transaction) { + error("%s", err.buf); + ret = -1; +-- +2.35.1 + diff --git a/_support/git-patches/v2.37.1.gl1/0003-refs-allow-skipping-the-reference-transaction-hook.patch b/_support/git-patches/v2.37.1.gl1/0003-refs-allow-skipping-the-reference-transaction-hook.patch new file mode 100644 index 000000000..cd194f8aa --- /dev/null +++ b/_support/git-patches/v2.37.1.gl1/0003-refs-allow-skipping-the-reference-transaction-hook.patch @@ -0,0 +1,60 @@ +From 4eccd16b45516df5ab02288d0c50c16e03cc44e4 Mon Sep 17 00:00:00 2001 +Message-Id: <4eccd16b45516df5ab02288d0c50c16e03cc44e4.1646206541.git.ps@pks.im> +In-Reply-To: +References: +From: Patrick Steinhardt +Date: Mon, 17 Jan 2022 09:12:39 +0100 +Subject: [PATCH 23/34] refs: allow skipping the reference-transaction hook + +The reference-transaction hook is executing whenever we prepare, commit +or abort a reference transaction. While this is mostly intentional, in +case of the files backend we're leaking the implementation detail that +the store is in fact a composite store with one loose and one packed +backend to the caller. So while we want to execute the hook for all +logical updates, executing it for such implementation details is +unexpected. + +Prepare for a fix by adding a new flag which allows to skip execution of +the hook. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +(cherry picked from commit 958fbc74e3d0fcc88b2065190e23db556a963644) +--- + refs.c | 3 +++ + refs.h | 5 +++++ + 2 files changed, 8 insertions(+) + +diff --git a/refs.c b/refs.c +index f498d232e5..435b90b1ec 100644 +--- a/refs.c ++++ b/refs.c +@@ -2084,6 +2084,9 @@ 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 3a141d7066..a015354fd6 100644 +--- a/refs.h ++++ b/refs.h +@@ -568,6 +568,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(). +-- +2.35.1 + diff --git a/_support/git-patches/v2.37.1.gl1/0004-refs-demonstrate-excessive-execution-of-the-referenc.patch b/_support/git-patches/v2.37.1.gl1/0004-refs-demonstrate-excessive-execution-of-the-referenc.patch new file mode 100644 index 000000000..aa6d96a3e --- /dev/null +++ b/_support/git-patches/v2.37.1.gl1/0004-refs-demonstrate-excessive-execution-of-the-referenc.patch @@ -0,0 +1,97 @@ +From 079f96ed16bb70f99fbe810b1646b1709bb82871 Mon Sep 17 00:00:00 2001 +Message-Id: <079f96ed16bb70f99fbe810b1646b1709bb82871.1646206541.git.ps@pks.im> +In-Reply-To: +References: +From: Patrick Steinhardt +Date: Mon, 17 Jan 2022 09:12:44 +0100 +Subject: [PATCH 24/34] refs: demonstrate excessive execution of the + reference-transaction hook + +Add tests which demonstate that we're executing the +reference-transaction hook too often in some cases, which thus leaks +implementation details about the reference store's implementation +itself. Behaviour will be fixed in follow-up commits. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +(cherry picked from commit 2ce825436268d6409bee8ebb5f5500b7ff172b1e) +--- + t/t1416-ref-transaction-hooks.sh | 64 ++++++++++++++++++++++++++++++++ + 1 file changed, 64 insertions(+) + +diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh +index 6c941027a8..0567fbdf0b 100755 +--- a/t/t1416-ref-transaction-hooks.sh ++++ b/t/t1416-ref-transaction-hooks.sh +@@ -136,4 +136,68 @@ 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 && ++ ++ write_script .git/hooks/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). But currently, packing refs will also trigger the ++ # hook. ++ cat >expect <<-EOF && ++ prepared ++ $ZERO_OID $POST_OID refs/heads/unpacked-ref ++ committed ++ $ZERO_OID $POST_OID refs/heads/unpacked-ref ++ prepared ++ $ZERO_OID $POST_OID refs/heads/unpacked-ref ++ committed ++ $ZERO_OID $POST_OID refs/heads/unpacked-ref ++ prepared ++ $POST_OID $ZERO_OID refs/heads/unpacked-ref ++ committed ++ $POST_OID $ZERO_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 && ++ ++ write_script .git/hooks/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. But currently, we see two interleaving transactions, once ++ # for deleting the loose refs and once for deleting the packed ref. ++ cat >expect <<-EOF && ++ prepared ++ $ZERO_OID $ZERO_OID refs/heads/to-be-deleted ++ prepared ++ $POST_OID $ZERO_OID refs/heads/to-be-deleted ++ committed ++ $ZERO_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 +-- +2.35.1 + diff --git a/_support/git-patches/v2.37.1.gl1/0005-refs-do-not-execute-reference-transaction-hook-on-pa.patch b/_support/git-patches/v2.37.1.gl1/0005-refs-do-not-execute-reference-transaction-hook-on-pa.patch new file mode 100644 index 000000000..f2e7c06ed --- /dev/null +++ b/_support/git-patches/v2.37.1.gl1/0005-refs-do-not-execute-reference-transaction-hook-on-pa.patch @@ -0,0 +1,81 @@ +From ef35ab926309bf406d3871679c576c718708a93b Mon Sep 17 00:00:00 2001 +Message-Id: +In-Reply-To: +References: +From: Patrick Steinhardt +Date: Mon, 17 Jan 2022 09:12:48 +0100 +Subject: [PATCH 25/34] refs: do not execute reference-transaction hook on + packing refs + +The reference-transaction hook is supposed to track logical changes to +references, but it currently also gets executed when packing refs in a +repository. This is unexpected and ultimately not all that useful: +packing refs is not supposed to result in any user-visible change to the +refs' state, and it ultimately is an implementation detail of how refs +stores work. + +Fix this excessive execution of the hook when packing refs. + +Reported-by: Waleed Khan +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +(cherry picked from commit ffad9941383465553bf26d88050f3243726f30df) +--- + refs/files-backend.c | 6 ++++-- + t/t1416-ref-transaction-hooks.sh | 11 +---------- + 2 files changed, 5 insertions(+), 12 deletions(-) + +diff --git a/refs/files-backend.c b/refs/files-backend.c +index 758d12a0fa..19f43e8d29 100644 +--- a/refs/files-backend.c ++++ b/refs/files-backend.c +@@ -1116,7 +1116,8 @@ 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, 0, &err); ++ transaction = ref_store_transaction_begin(&refs->base, ++ REF_TRANSACTION_SKIP_HOOK, &err); + if (!transaction) + goto cleanup; + ref_transaction_add_update( +@@ -1187,7 +1188,8 @@ 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, 0, &err); ++ transaction = ref_store_transaction_begin(refs->packed_ref_store, ++ REF_TRANSACTION_SKIP_HOOK, &err); + if (!transaction) + return -1; + +diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh +index 0567fbdf0b..f9d3d5213f 100755 +--- a/t/t1416-ref-transaction-hooks.sh ++++ b/t/t1416-ref-transaction-hooks.sh +@@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' ' + git pack-refs --all && + + # We only expect a single hook invocation, which is the call to +- # git-update-ref(1). But currently, packing refs will also trigger the +- # hook. ++ # 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 +- prepared +- $ZERO_OID $POST_OID refs/heads/unpacked-ref +- committed +- $ZERO_OID $POST_OID refs/heads/unpacked-ref +- prepared +- $POST_OID $ZERO_OID refs/heads/unpacked-ref +- committed +- $POST_OID $ZERO_OID refs/heads/unpacked-ref + EOF + + test_cmp expect actual +-- +2.35.1 + diff --git a/_support/git-patches/v2.37.1.gl1/0006-refs-skip-hooks-when-deleting-uncovered-packed-refs.patch b/_support/git-patches/v2.37.1.gl1/0006-refs-skip-hooks-when-deleting-uncovered-packed-refs.patch new file mode 100644 index 000000000..3b21bf489 --- /dev/null +++ b/_support/git-patches/v2.37.1.gl1/0006-refs-skip-hooks-when-deleting-uncovered-packed-refs.patch @@ -0,0 +1,99 @@ +From d56f2a0e1eed4d0a39b99638117e0c8259e5078d Mon Sep 17 00:00:00 2001 +Message-Id: +In-Reply-To: +References: +From: Patrick Steinhardt +Date: Mon, 17 Jan 2022 09:12:53 +0100 +Subject: [PATCH 26/34] refs: skip hooks when deleting uncovered packed refs + +When deleting refs from the loose-files refs backend, then we need to be +careful to also delete the same ref from the packed refs backend, if it +exists. If we don't, then deleting the loose ref would "uncover" the +packed ref. We thus always have to queue up deletions of refs for both +the loose and the packed refs backend. This is done in two separate +transactions, where the end result is that the reference-transaction +hook is executed twice for the deleted refs. + +This behaviour is quite misleading: it's exposing implementation details +of how the files backend works to the user, in contrast to the logical +updates that we'd really want to expose via the hook. Worse yet, whether +the hook gets executed once or twice depends on how well-packed the +repository is: if the ref only exists as a loose ref, then we execute it +once, otherwise if it is also packed then we execute it twice. + +Fix this behaviour and don't execute the reference-transaction hook at +all when refs in the packed-refs backend if it's driven by the files +backend. This works as expected even in case the refs to be deleted only +exist in the packed-refs backend because the loose-backend always queues +refs in its own transaction even if they don't exist such that they can +be locked for concurrent creation. And it also does the right thing in +case neither of the backends has the ref because that would cause the +transaction to fail completely. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +(cherry picked from commit 2ed1b64ebdeefc7f9473ae159fb45ff0c6cf121a) +--- + refs/files-backend.c | 9 ++++++--- + t/t1416-ref-transaction-hooks.sh | 7 +------ + 2 files changed, 7 insertions(+), 9 deletions(-) + +diff --git a/refs/files-backend.c b/refs/files-backend.c +index 19f43e8d29..c931dd479c 100644 +--- a/refs/files-backend.c ++++ b/refs/files-backend.c +@@ -1256,7 +1256,8 @@ 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, 0, &err); ++ transaction = ref_store_transaction_begin(refs->packed_ref_store, ++ REF_TRANSACTION_SKIP_HOOK, &err); + if (!transaction) + goto error; + +@@ -2771,7 +2772,8 @@ static int files_transaction_prepare(struct ref_store *ref_store, + */ + if (!packed_transaction) { + packed_transaction = ref_store_transaction_begin( +- refs->packed_ref_store, 0, err); ++ refs->packed_ref_store, ++ REF_TRANSACTION_SKIP_HOOK, err); + if (!packed_transaction) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; +@@ -3042,7 +3044,8 @@ 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, 0, err); ++ packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, ++ REF_TRANSACTION_SKIP_HOOK, err); + if (!packed_transaction) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; +diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh +index f9d3d5213f..4e1e84a91f 100755 +--- a/t/t1416-ref-transaction-hooks.sh ++++ b/t/t1416-ref-transaction-hooks.sh +@@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' ' + git update-ref -d refs/heads/to-be-deleted $POST_OID && + + # We only expect a single hook invocation, which is the logical +- # deletion. But currently, we see two interleaving transactions, once +- # for deleting the loose refs and once for deleting the packed ref. ++ # deletion. + cat >expect <<-EOF && +- prepared +- $ZERO_OID $ZERO_OID refs/heads/to-be-deleted + prepared + $POST_OID $ZERO_OID refs/heads/to-be-deleted + committed +- $ZERO_OID $ZERO_OID refs/heads/to-be-deleted +- committed + $POST_OID $ZERO_OID refs/heads/to-be-deleted + EOF + +-- +2.35.1 + diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go index b47119a71..9a60a78c4 100644 --- a/internal/git/command_factory_test.go +++ b/internal/git/command_factory_test.go @@ -217,8 +217,8 @@ func TestCommandFactory_ExecutionEnvironment(t *testing.T) { }) t.Run("set using GITALY_TESTING_BUNDLED_GIT_PATH", func(t *testing.T) { - ctx := featureflag.ContextWithFeatureFlag(ctx, featureflag.GitV2361Gl1, true) - suffix := "-v2.36.1.gl1" + ctx := featureflag.ContextWithFeatureFlag(ctx, featureflag.GitV2371Gl1, true) + suffix := "-v2.37.1.gl1" bundledGitDir := testhelper.TempDir(t) diff --git a/internal/git/execution_environment.go b/internal/git/execution_environment.go index 9e2232a05..c35d5e108 100644 --- a/internal/git/execution_environment.go +++ b/internal/git/execution_environment.go @@ -26,9 +26,9 @@ var ( // case `IsEnabled()` returns `false` though. ExecutionEnvironmentConstructors = []ExecutionEnvironmentConstructor{ BundledGitEnvironmentConstructor{ - Suffix: "-v2.36.1.gl1", + Suffix: "-v2.37.1.gl1", FeatureFlags: []featureflag.FeatureFlag{ - featureflag.GitV2361Gl1, + featureflag.GitV2371Gl1, }, }, BundledGitEnvironmentConstructor{ diff --git a/internal/metadata/featureflag/ff_git_v2361.go b/internal/metadata/featureflag/ff_git_v2361.go deleted file mode 100644 index 869f3d8e6..000000000 --- a/internal/metadata/featureflag/ff_git_v2361.go +++ /dev/null @@ -1,9 +0,0 @@ -package featureflag - -// GitV2361Gl1 will enable use of Git v2.36.1.gl1. -var GitV2361Gl1 = NewFeatureFlag( - "git_v2361gl1", - "v15.0.0", - "https://gitlab.com/gitlab-org/gitaly/-/issues/4194", - false, -) diff --git a/internal/metadata/featureflag/ff_git_v2371.go b/internal/metadata/featureflag/ff_git_v2371.go new file mode 100644 index 000000000..b68423548 --- /dev/null +++ b/internal/metadata/featureflag/ff_git_v2371.go @@ -0,0 +1,9 @@ +package featureflag + +// GitV2371Gl1 will enable use of Git v2.37.1.gl1. +var GitV2371Gl1 = NewFeatureFlag( + "git_v2371gl1", + "v15.0.0", + "https://gitlab.com/gitlab-org/gitaly/-/issues/4194", + false, +) diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index e917a7b79..8e5977401 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -175,7 +175,7 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context { ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.RunCommandsInCGroup, true) // Randomly inject the Git flag so that we have coverage of tests with both old and new Git // version by pure chance. - ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.GitV2361Gl1, rnd.Int()%2 == 0) + ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.GitV2371Gl1, rnd.Int()%2 == 0) // PraefectGeneratedReplicaPaths affects many tests as it changes the repository creation logic. // Randomly enable the flag to exercise both paths to some extent. ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.PraefectGeneratedReplicaPaths, rnd.Int()%2 == 0) -- cgit v1.2.3