{"thread":{"id":"53600","subject":"[PATCH] refs: implement reference transaction hooks","startedAt":"2020-06-02T08:25:00Z","lastAt":"2020-10-28T21:51:03Z","messageCount":26,"participants":["Patrick Steinhardt","Junio C Hamano","SZEDER Gábor","Taylor Blau","Han-Wen Nienhuys","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"399030","messageId":"1d1a94426f95d842e0e3ea6a1569c0c45239229c.1591086316.git.ps@pks.im","threadId":"53600","inReplyTo":null,"subject":"[PATCH] refs: implement reference transaction hooks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-06-02T08:25:44Z","receivedAt":"2020-06-02T08:25:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The low-level reference transactions used to update references are\ncurrently completely opaque to the user. While certainly desirable in\nmost usecases, there are some which might want to hook into the\ntransaction to observe all queued reference updates as well as observing\nthe abortion or commit of a prepared transaction.\n\nOne such usecase would be to have a set of replicas of a given Git\nrepository, where we perform Git operations on all of the repositories\nat once and expect the outcome to be the same in all of them. While\nthere exist hooks already for a certain subset of Git commands that\ncould be used to implement a voting mechanism for this, many others\ncurrently don't have any mechanism for this.\n\nThe above scenario is the motivation for a set of three new hooks that\nreach directly into Git's reference transaction. Each of the following\nnew hooks (currently) doesn't accept any parameters and receives the set\nof queued reference updates via stdin:\n\n - ref-transaction-prepared gets called when all reference updates\n have been queued. At this stage, the hook may decide to abort the\n transaction prematurely by returning a non-zero status code.\n\n - ref-transaction-committed gets called when a reference transaction\n was transmitted and all queued updates have been persisted.\n\n - ref-transaction-aborted gets called when a reference transaction\n was aborted and all queued updates have been rolled back.\n\nGiven the usecase described above, a voting mechanism can now be\nimplemented as a \"ref-transaction-prepared\" hook: as soon as it gets\ncalled, it will take all of stdin and use it to cast a vote to a central\nservice. When all replicas of the repository agree, the hook will exit\nwith zero, otherwise it will abort the transaction by returning\nnon-zero. The most important upside is that this will catch _all_\ncommands writing references at once, allowing to implement strong\nconsistency for reference updates via a single mechanism.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/githooks.txt | 51 ++++++++++++++++++\n refs.c | 67 +++++++++++++++++++++++-\n t/t1416-ref-transaction-hooks.sh | 88 ++++++++++++++++++++++++++++++++\n 3 files changed, 204 insertions(+), 2 deletions(-)\n create mode 100755 t/t1416-ref-transaction-hooks.sh\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 81f2a87e88..48f8446943 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -404,6 +404,57 @@ Both standard output and standard error output are forwarded to\n `git send-pack` on the other end, so you can simply `echo` messages\n for the user.\n \n+ref-transaction-prepared\n+~~~~~~~~~~~~~~~~~~~~~~~~\n+\n+This hook is invoked by any Git command that performs reference\n+updates. It executes as soon as all reference updates were queued to\n+the transaction and locked on disk. This hook executes for every\n+reference transaction that is being prepared and may thus get called\n+multiple times.\n+\n+It takes no arguments, but for each ref to be updated it receives on\n+standard input a line of the format:\n+\n+ SP SP LF\n+\n+If the hook exits with a non-zero status, the transaction is aborted\n+and the command exits immediately. The\n+<> hook is not\n+executed in that case.\n+\n+[[ref-transaction-aborted]]\n+ref-transaction-aborted\n+~~~~~~~~~~~~~~~~~~~~~~~\n+\n+This hook is invoked by any Git command that performs reference\n+updates. It executes as soon as a reference transaction is aborted and\n+after all reference locks were released and any changes made to\n+references were rolled back. The hook may get called multiple times or\n+never in case no transaction was aborted.\n+\n+The hook takes no arguments, but for each ref to be updated it\n+receives on standard input a line of the format:\n+\n+ SP SP LF\n+\n+The hook's exit code is discarded by Git.\n+\n+ref-transaction-committed\n+~~~~~~~~~~~~~~~~~~~~~~~~~\n+\n+This hook is invoked by any Git command that performs reference\n+updates. It executes as soon as a reference transaction is committed,\n+persisting all changes to disk and releasing any locks. The hook may\n+get called multiple times or never in case no transaction was aborted.\n+\n+The hook takes no arguments, but for each ref to be updated it\n+receives on standard input a line of the format:\n+\n+ SP SP LF\n+\n+The hook's exit code is discarded by Git.\n+\n push-to-checkout\n ~~~~~~~~~~~~~~~~\n \ndiff --git a/refs.c b/refs.c\nindex 224ff66c7b..e41fa7ea55 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -9,6 +9,7 @@\n #include \"iterator.h\"\n #include \"refs.h\"\n #include \"refs/refs-internal.h\"\n+#include \"run-command.h\"\n #include \"object-store.h\"\n #include \"object.h\"\n #include \"tag.h\"\n@@ -16,6 +17,7 @@\n #include \"worktree.h\"\n #include \"argv-array.h\"\n #include \"repository.h\"\n+#include \"sigchain.h\"\n \n /*\n * List of all available backends\n@@ -1986,10 +1988,56 @@ int ref_update_reject_duplicates(struct string_list *refnames,\n \treturn 0;\n }\n \n+static int run_transaction_hook(struct ref_transaction *transaction,\n+\t\t\t\tconst char *hook_name)\n+{\n+\tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *argv[2];\n+\tint code, i;\n+\n+\targv[0] = find_hook(hook_name);\n+\tif (!argv[0])\n+\t\treturn 0;\n+\n+\targv[1] = NULL;\n+\n+\tproc.argv = argv;\n+\tproc.in = -1;\n+\tproc.stdout_to_stderr = 1;\n+\tproc.trace2_hook_name = hook_name;\n+\n+\tcode = start_command(&proc);\n+\tif (code)\n+\t\treturn code;\n+\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addf(&buf, \"%s %s %s\\n\",\n+\t\t\t oid_to_hex(&update->old_oid),\n+\t\t\t oid_to_hex(&update->new_oid),\n+\t\t\t update->refname);\n+\n+\t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0)\n+\t\t\tbreak;\n+\t}\n+\n+\tclose(proc.in);\n+\tsigchain_pop(SIGPIPE);\n+\n+\tstrbuf_release(&buf);\n+\treturn finish_command(&proc);\n+}\n+\n int ref_transaction_prepare(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_store *refs = transaction->ref_store;\n+\tint ret;\n \n \tswitch (transaction->state) {\n \tcase REF_TRANSACTION_OPEN:\n@@ -2012,7 +2060,17 @@ int ref_transaction_prepare(struct ref_transaction *transaction,\n \t\treturn -1;\n \t}\n \n-\treturn refs->be->transaction_prepare(refs, transaction, err);\n+\tret = refs->be->transaction_prepare(refs, transaction, err);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\tret = run_transaction_hook(transaction, \"ref-transaction-prepared\");\n+\tif (ret) {\n+\t\tref_transaction_abort(transaction, err);\n+\t\tdie(_(\"ref updates aborted by hook\"));\n+\t}\n+\n+\treturn 0;\n }\n \n int ref_transaction_abort(struct ref_transaction *transaction,\n@@ -2036,6 +2094,8 @@ int ref_transaction_abort(struct ref_transaction *transaction,\n \t\tbreak;\n \t}\n \n+\trun_transaction_hook(transaction, \"ref-transaction-aborted\");\n+\n \tref_transaction_free(transaction);\n \treturn ret;\n }\n@@ -2064,7 +2124,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tbreak;\n \t}\n \n-\treturn refs->be->transaction_finish(refs, transaction, err);\n+\tret = refs->be->transaction_finish(refs, transaction, err);\n+\tif (!ret)\n+\t\trun_transaction_hook(transaction, \"ref-transaction-committed\");\n+\treturn ret;\n }\n \n int refs_verify_refname_available(struct ref_store *refs,\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nnew file mode 100755\nindex 0000000000..b6df5fc883\n--- /dev/null\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -0,0 +1,88 @@\n+#!/bin/sh\n+\n+test_description='reference transaction hooks'\n+\n+. ./test-lib.sh\n+\n+create_commit ()\n+{\n+\ttest_tick &&\n+\tT=$(git write-tree) &&\n+\tsha1=$(echo message | git commit-tree $T) &&\n+\techo $sha1\n+}\n+\n+test_expect_success setup '\n+\tmkdir -p .git/hooks\n+'\n+\n+test_expect_success 'prepared hook allows updating ref' '\n+\ttest_when_finished \"rm .git/hooks/ref-transaction-prepared\" &&\n+\twrite_script .git/hooks/ref-transaction-prepared <<-\\EOF &&\n+\t\texit 0\n+\tEOF\n+\tC=$(create_commit) &&\n+\tgit update-ref HEAD $C\n+'\n+\n+test_expect_success 'prepared hook aborts updating ref' '\n+\ttest_when_finished \"rm .git/hooks/ref-transaction-prepared\" &&\n+\twrite_script .git/hooks/ref-transaction-prepared <<-\\EOF &&\n+\t\texit 1\n+\tEOF\n+\tC=$(create_commit) &&\n+\ttest_must_fail git update-ref HEAD $C 2>err &&\n+\tgrep \"ref updates aborted by hook\" err\n+'\n+\n+test_expect_success 'prepared hook gets all queued updates' '\n+\ttest_when_finished \"rm .git/hooks/ref-transaction-prepared actual\" &&\n+\twrite_script .git/hooks/ref-transaction-prepared <<-\\EOF &&\n+\t\twhile read -r line; do printf \"%s\\n\" \"$line\"; done >actual\n+\tEOF\n+\tC=$(create_commit) &&\n+\tcat >expect <<-EOF &&\n+\t\t$ZERO_OID $C HEAD\n+\t\t$ZERO_OID $C refs/heads/master\n+\tEOF\n+\tgit update-ref HEAD $C <<-EOF &&\n+\t\tupdate HEAD $ZERO_OID $C\n+\t\tupdate refs/heads/master $ZERO_OID $C\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'committed hook gets all queued updates' '\n+\ttest_when_finished \"rm .git/hooks/ref-transaction-committed actual\" &&\n+\twrite_script .git/hooks/ref-transaction-committed <<-\\EOF &&\n+\t\twhile read -r line; do printf \"%s\\n\" \"$line\"; done >actual\n+\tEOF\n+\tC=$(create_commit) &&\n+\tcat >expect <<-EOF &&\n+\t\t$ZERO_OID $C HEAD\n+\t\t$ZERO_OID $C refs/heads/master\n+\tEOF\n+\tgit update-ref HEAD $C &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'aborted hook gets all queued updates' '\n+\ttest_when_finished \"rm .git/hooks/ref-transaction-aborted actual\" &&\n+\twrite_script .git/hooks/ref-transaction-aborted <<-\\EOF &&\n+\t\twhile read -r line; do printf \"%s\\n\" \"$line\"; done >actual\n+\tEOF\n+\tC=$(create_commit) &&\n+\tcat >expect <<-EOF &&\n+\t\t$ZERO_OID $C HEAD\n+\t\t$ZERO_OID $C refs/heads/master\n+\tEOF\n+\tgit update-ref --stdin <<-EOF &&\n+\t\tstart\n+\t\tupdate HEAD $C $ZERO_OID\n+\t\tupdate refs/heads/master $C $ZERO_OID\n+\t\tabort\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.26.2\n\n"},{"id":"399038","messageId":"xmqq4krttl4k.fsf@gitster.c.googlers.com","threadId":"53600","inReplyTo":"1d1a94426f95d842e0e3ea6a1569c0c45239229c.1591086316.git.ps@pks.im","subject":"Re: [PATCH] refs: implement reference transaction hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-02T17:47:55Z","receivedAt":"2020-06-02T17:48:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The above scenario is the motivation for a set of three new hooks that\n> reach directly into Git's reference transaction. Each of the following\n> new hooks (currently) doesn't accept any parameters and receives the set\n> of queued reference updates via stdin:\n\nDo we have something (e.g. performance measurement) to convince\nourselves that this won't incur unacceptable levels of overhead in\nnull cases where there is no hook installed in the repository?\n\n> +static int run_transaction_hook(struct ref_transaction *transaction,\n> +\t\t\t\tconst char *hook_name)\n> +{\n> +\tstruct child_process proc = CHILD_PROCESS_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tconst char *argv[2];\n> +\tint code, i;\n> +\n> +\targv[0] = find_hook(hook_name);\n> +\tif (!argv[0])\n> +\t\treturn 0;\n> +\n> +\targv[1] = NULL;\n> +\n> +\tproc.argv = argv;\n\nLet's use proc.args and argv_push() API in newly introduced code\ninstead.\n\n> +\tproc.in = -1;\n> +\tproc.stdout_to_stderr = 1;\n> +\tproc.trace2_hook_name = hook_name;\n> +\n> +\tcode = start_command(&proc);\n> +\tif (code)\n> +\t\treturn code;\n> +\n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\n> +\tfor (i = 0; i < transaction->nr; i++) {\n> +\t\tstruct ref_update *update = transaction->updates[i];\n> +\n> +\t\tstrbuf_reset(&buf);\n> +\t\tstrbuf_addf(&buf, \"%s %s %s\\n\",\n> +\t\t\t oid_to_hex(&update->old_oid),\n> +\t\t\t oid_to_hex(&update->new_oid),\n> +\t\t\t update->refname);\n> +\n> +\t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0)\n> +\t\t\tbreak;\n\nWe leave the loop early when we detect a write failure here...\n\n> +\t}\n> +\n> +\tclose(proc.in);\n> +\tsigchain_pop(SIGPIPE);\n> +\n> +\tstrbuf_release(&buf);\n> +\treturn finish_command(&proc);\n\n... but the caller does not get notified. Intended?\n\n> +}\n> +\n> int ref_transaction_prepare(struct ref_transaction *transaction,\n> \t\t\t struct strbuf *err)\n> {\n> \tstruct ref_store *refs = transaction->ref_store;\n> +\tint ret;\n> \n> \tswitch (transaction->state) {\n> \tcase REF_TRANSACTION_OPEN:\n> @@ -2012,7 +2060,17 @@ int ref_transaction_prepare(struct ref_transaction *transaction,\n> \t\treturn -1;\n> \t}\n> \n> -\treturn refs->be->transaction_prepare(refs, transaction, err);\n> +\tret = refs->be->transaction_prepare(refs, transaction, err);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = run_transaction_hook(transaction, \"ref-transaction-prepared\");\n\nThis caller does care about it, no?\n\n> +\tif (ret) {\n> +\t\tref_transaction_abort(transaction, err);\n> +\t\tdie(_(\"ref updates aborted by hook\"));\n> +\t}\n> +\n> +\treturn 0;\n> }\n> \n> int ref_transaction_abort(struct ref_transaction *transaction,\n> @@ -2036,6 +2094,8 @@ int ref_transaction_abort(struct ref_transaction *transaction,\n> \t\tbreak;\n> \t}\n> \n> +\trun_transaction_hook(transaction, \"ref-transaction-aborted\");\n\nAnd I presume that the callers of \"ref_xn_abort()\" would, too, but\nthe value returned here does not get folded into 'ret'.\n\n> \tref_transaction_free(transaction);\n> \treturn ret;\n> }\n> @@ -2064,7 +2124,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \t\tbreak;\n> \t}\n> \n> -\treturn refs->be->transaction_finish(refs, transaction, err);\n> +\tret = refs->be->transaction_finish(refs, transaction, err);\n> +\tif (!ret)\n> +\t\trun_transaction_hook(transaction, \"ref-transaction-committed\");\n> +\treturn ret;\n> }\n> \n> int refs_verify_refname_available(struct ref_store *refs,\n> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> new file mode 100755\n> index 0000000000..b6df5fc883\n> --- /dev/null\n> +++ b/t/t1416-ref-transaction-hooks.sh\n> @@ -0,0 +1,88 @@\n> +#!/bin/sh\n> +\n> +test_description='reference transaction hooks'\n> +\n> +. ./test-lib.sh\n> +\n> +create_commit ()\n> +{\n\nStyle (Documentation/CodingGuidelines):\n\n - We prefer a space between the function name and the parentheses,\n and no space inside the parentheses. The opening \"{\" should also\n be on the same line.\n\n> +\ttest_tick &&\n> +\tT=$(git write-tree) &&\n> +\tsha1=$(echo message | git commit-tree $T) &&\n> +\techo $sha1\n\nCalling this helper in a subprocess does not have the intended\neffect of calling test_tick, which increments the committer\ntimestamp by 1 second to prepare for the next call...\n\n> +}\n> +\n> +test_expect_success setup '\n> +\tmkdir -p .git/hooks\n> +'\n> +\n> +test_expect_success 'prepared hook allows updating ref' '\n> +\ttest_when_finished \"rm .git/hooks/ref-transaction-prepared\" &&\n> +\twrite_script .git/hooks/ref-transaction-prepared <<-\\EOF &&\n> +\t\texit 0\n> +\tEOF\n> +\tC=$(create_commit) &&\n\n... like here.\n\nInstead of creating test commits left and right at each test, how\nabout preparing a pair of them (PRE, POST) upfront in the \"setup\"\nstep, reset the refs involved to PRE at the very beginning of each\ntest, and use POST to operations that would update the refs? We can\nuse a couple of calls to test_commit helper to do so, without having\nto bother with the low level porcelain calls if we go that route.\n\n> +\tgit update-ref HEAD $C\n> +'\n> +\n> +test_expect_success 'prepared hook aborts updating ref' '\n> +\ttest_when_finished \"rm .git/hooks/ref-transaction-prepared\" &&\n> +\twrite_script .git/hooks/ref-transaction-prepared <<-\\EOF &&\n> +\t\texit 1\n> +\tEOF\n> +\tC=$(create_commit) &&\n> +\ttest_must_fail git update-ref HEAD $C 2>err &&\n> +\tgrep \"ref updates aborted by hook\" err\n\nRunning \"make GIT_TEST_GETTEXT_POISON=Yes test\" would probably break\nthis line. Use test_i18ngrep instead.\n\n> +'\n> +\n> +test_expect_success 'prepared hook gets all queued updates' '\n> +\ttest_when_finished \"rm .git/hooks/ref-transaction-prepared actual\" &&\n> +\twrite_script .git/hooks/ref-transaction-prepared <<-\\EOF &&\n> +\t\twhile read -r line; do printf \"%s\\n\" \"$line\"; done >actual\n\nStyle (used in the generated script)?\n\n> +\tEOF\n> +\tC=$(create_commit) &&\n> +\tcat >expect <<-EOF &&\n> +\t\t$ZERO_OID $C HEAD\n> +\t\t$ZERO_OID $C refs/heads/master\n> +\tEOF\n> +\tgit update-ref HEAD $C <<-EOF &&\n> +\t\tupdate HEAD $ZERO_OID $C\n> +\t\tupdate refs/heads/master $ZERO_OID $C\n> +\tEOF\n> +\ttest_cmp expect actual\n\nOK, good futureproofing for the hash algorithm update ;-).\n"},{"id":"399042","messageId":"20200602180900.GA11344@szeder.dev","threadId":"53600","inReplyTo":"1d1a94426f95d842e0e3ea6a1569c0c45239229c.1591086316.git.ps@pks.im","subject":"Re: [PATCH] refs: implement reference transaction hooks","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-06-02T18:09:00Z","receivedAt":"2020-06-02T18:09:11Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jun 02, 2020 at 10:25:44AM +0200, Patrick Steinhardt wrote:\n> The low-level reference transactions used to update references are\n> currently completely opaque to the user. While certainly desirable in\n> most usecases, there are some which might want to hook into the\n> transaction to observe all queued reference updates as well as observing\n> the abortion or commit of a prepared transaction.\n> \n> One such usecase would be to have a set of replicas of a given Git\n> repository, where we perform Git operations on all of the repositories\n> at once and expect the outcome to be the same in all of them. While\n> there exist hooks already for a certain subset of Git commands that\n> could be used to implement a voting mechanism for this, many others\n> currently don't have any mechanism for this.\n> \n> The above scenario is the motivation for a set of three new hooks that\n> reach directly into Git's reference transaction. Each of the following\n> new hooks (currently) doesn't accept any parameters and receives the set\n> of queued reference updates via stdin:\n> \n> - ref-transaction-prepared gets called when all reference updates\n> have been queued. At this stage, the hook may decide to abort the\n> transaction prematurely by returning a non-zero status code.\n> \n> - ref-transaction-committed gets called when a reference transaction\n> was transmitted and all queued updates have been persisted.\n> \n> - ref-transaction-aborted gets called when a reference transaction\n> was aborted and all queued updates have been rolled back.\n\nThe point of reference transactions is that they are atomic, and these\nhooks must work together to ensure that. This raises the question how\nthese hooks can be updated in an actively used repository.\n\nHaving multiple hooks means that they can't be updated atomically, and\ngit might invoke the new abort hook after the transaction was prepared\nwith the old hook. Now, if there were a single 'ref-transaction' hook\n(which gets the phase of the transaction ('prepared', 'committed' or\n'aborted') as a parameter), then it could be updated atomically by\nmv-ing it to place, but even that update can happen in between git\ninvokes 'ref-transaction prepared' and 'ref-transaction aborted'.\n\nI suppose this issue could be addressed by a single hook which runs\nduring the whole transaction and some back-and-forth communication\nthrough stdin/out between git and the hook. However, this would, I'm\nafraid, complicate both Git's handling of this hook and the hook as\nwell, so let's take a step back first: is this something we should\nworry about in the first place?\n\n> Given the usecase described above, a voting mechanism can now be\n> implemented as a \"ref-transaction-prepared\" hook: as soon as it gets\n> called, it will take all of stdin and use it to cast a vote to a central\n> service. When all replicas of the repository agree, the hook will exit\n> with zero, otherwise it will abort the transaction by returning\n> non-zero. The most important upside is that this will catch _all_\n> commands writing references at once, allowing to implement strong\n> consistency for reference updates via a single mechanism.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> Documentation/githooks.txt | 51 ++++++++++++++++++\n> refs.c | 67 +++++++++++++++++++++++-\n> t/t1416-ref-transaction-hooks.sh | 88 ++++++++++++++++++++++++++++++++\n> 3 files changed, 204 insertions(+), 2 deletions(-)\n> create mode 100755 t/t1416-ref-transaction-hooks.sh\n> \n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index 81f2a87e88..48f8446943 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -404,6 +404,57 @@ Both standard output and standard error output are forwarded to\n> `git send-pack` on the other end, so you can simply `echo` messages\n> for the user.\n> \n> +ref-transaction-prepared\n> +~~~~~~~~~~~~~~~~~~~~~~~~\n> +\n> +This hook is invoked by any Git command that performs reference\n> +updates. It executes as soon as all reference updates were queued to\n> +the transaction and locked on disk. This hook executes for every\n> +reference transaction that is being prepared and may thus get called\n> +multiple times.\n> +\n> +It takes no arguments, but for each ref to be updated it receives on\n> +standard input a line of the format:\n> +\n> + SP SP LF\n> +\n> +If the hook exits with a non-zero status, the transaction is aborted\n> +and the command exits immediately. The\n> +<> hook is not\n> +executed in that case.\n> +\n> +[[ref-transaction-aborted]]\n> +ref-transaction-aborted\n> +~~~~~~~~~~~~~~~~~~~~~~~\n> +\n> +This hook is invoked by any Git command that performs reference\n> +updates. It executes as soon as a reference transaction is aborted and\n> +after all reference locks were released and any changes made to\n> +references were rolled back. The hook may get called multiple times or\n> +never in case no transaction was aborted.\n> +\n> +The hook takes no arguments, but for each ref to be updated it\n\nNit: I found it a bit surprising to read about refs \"to be updated\" in\nthe description of the 'aborted' hook, because by the time this hook\nis called the update has already been refused. The same applies to\nthe 'committed' hook below as well.\n\n> +receives on standard input a line of the format:\n> +\n> + SP SP LF\n> +\n> +The hook's exit code is discarded by Git.\n> +\n> +ref-transaction-committed\n> +~~~~~~~~~~~~~~~~~~~~~~~~~\n> +\n> +This hook is invoked by any Git command that performs reference\n> +updates. It executes as soon as a reference transaction is committed,\n> +persisting all changes to disk and releasing any locks. The hook may\n> +get called multiple times or never in case no transaction was aborted.\n> +\n> +The hook takes no arguments, but for each ref to be updated it\n> +receives on standard input a line of the format:\n> +\n> + SP SP LF\n> +\n> +The hook's exit code is discarded by Git.\n> +\n> push-to-checkout\n> ~~~~~~~~~~~~~~~~\n> \n"},{"id":"399070","messageId":"20200603094618.GA4194@tanuki.pks.im","threadId":"53600","inReplyTo":"20200602180900.GA11344@szeder.dev","subject":"Re: [PATCH] refs: implement reference transaction hooks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-06-03T09:46:18Z","receivedAt":"2020-06-03T09:45:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jun 02, 2020 at 08:09:00PM +0200, SZEDER Gábor wrote:\n> On Tue, Jun 02, 2020 at 10:25:44AM +0200, Patrick Steinhardt wrote:\n> > The low-level reference transactions used to update references are\n> > currently completely opaque to the user. While certainly desirable in\n> > most usecases, there are some which might want to hook into the\n> > transaction to observe all queued reference updates as well as observing\n> > the abortion or commit of a prepared transaction.\n> > \n> > One such usecase would be to have a set of replicas of a given Git\n> > repository, where we perform Git operations on all of the repositories\n> > at once and expect the outcome to be the same in all of them. While\n> > there exist hooks already for a certain subset of Git commands that\n> > could be used to implement a voting mechanism for this, many others\n> > currently don't have any mechanism for this.\n> > \n> > The above scenario is the motivation for a set of three new hooks that\n> > reach directly into Git's reference transaction. Each of the following\n> > new hooks (currently) doesn't accept any parameters and receives the set\n> > of queued reference updates via stdin:\n> > \n> > - ref-transaction-prepared gets called when all reference updates\n> > have been queued. At this stage, the hook may decide to abort the\n> > transaction prematurely by returning a non-zero status code.\n> > \n> > - ref-transaction-committed gets called when a reference transaction\n> > was transmitted and all queued updates have been persisted.\n> > \n> > - ref-transaction-aborted gets called when a reference transaction\n> > was aborted and all queued updates have been rolled back.\n> \n> The point of reference transactions is that they are atomic, and these\n> hooks must work together to ensure that. This raises the question how\n> these hooks can be updated in an actively used repository.\n> \n> Having multiple hooks means that they can't be updated atomically, and\n> git might invoke the new abort hook after the transaction was prepared\n> with the old hook. Now, if there were a single 'ref-transaction' hook\n> (which gets the phase of the transaction ('prepared', 'committed' or\n> 'aborted') as a parameter), then it could be updated atomically by\n> mv-ing it to place, but even that update can happen in between git\n> invokes 'ref-transaction prepared' and 'ref-transaction aborted'.\n> \n> I suppose this issue could be addressed by a single hook which runs\n> during the whole transaction and some back-and-forth communication\n> through stdin/out between git and the hook. However, this would, I'm\n> afraid, complicate both Git's handling of this hook and the hook as\n> well, so let's take a step back first: is this something we should\n> worry about in the first place?\n\nVery good point about which I didn't previously think, thanks a lot for\nraising it!\n\nI agree that using a single long-lived hook would complicate the logic\nby quite a bit. Given that the ref-transaction mechanism is such a\ncentral piece to Git, I'd be wary of introducing such complexity. But\nmerging the current three hooks into a single hook accepting a parameter\nsounds like a fair compromise to me that would at least allow users to\nreplace them atomically, even though it doesn't mean all stages were run\nwith the same version of the hook.\n\nIf we want to go further and also ensure the same script's run across\nall hook invocations, we could also open the hook's file descriptor on\nfirst invocation and then use `fexecve` on all subsequent invocations.\nAs long as the user doesn't do inline rewrites of the file, we'd thus\nalways use the same file. I'm not sure how portable that syscall is,\nthough, but it's at least part of POSIX-2008.\n\n> > Given the usecase described above, a voting mechanism can now be\n> > implemented as a \"ref-transaction-prepared\" hook: as soon as it gets\n> > called, it will take all of stdin and use it to cast a vote to a central\n> > service. When all replicas of the repository agree, the hook will exit\n> > with zero, otherwise it will abort the transaction by returning\n> > non-zero. The most important upside is that this will catch _all_\n> > commands writing references at once, allowing to implement strong\n> > consistency for reference updates via a single mechanism.\n> > \n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > Documentation/githooks.txt | 51 ++++++++++++++++++\n> > refs.c | 67 +++++++++++++++++++++++-\n> > t/t1416-ref-transaction-hooks.sh | 88 ++++++++++++++++++++++++++++++++\n> > 3 files changed, 204 insertions(+), 2 deletions(-)\n> > create mode 100755 t/t1416-ref-transaction-hooks.sh\n> > \n> > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> > index 81f2a87e88..48f8446943 100644\n> > --- a/Documentation/githooks.txt\n> > +++ b/Documentation/githooks.txt\n> > @@ -404,6 +404,57 @@ Both standard output and standard error output are forwarded to\n> > `git send-pack` on the other end, so you can simply `echo` messages\n> > for the user.\n> > \n> > +ref-transaction-prepared\n> > +~~~~~~~~~~~~~~~~~~~~~~~~\n> > +\n> > +This hook is invoked by any Git command that performs reference\n> > +updates. It executes as soon as all reference updates were queued to\n> > +the transaction and locked on disk. This hook executes for every\n> > +reference transaction that is being prepared and may thus get called\n> > +multiple times.\n> > +\n> > +It takes no arguments, but for each ref to be updated it receives on\n> > +standard input a line of the format:\n> > +\n> > + SP SP LF\n> > +\n> > +If the hook exits with a non-zero status, the transaction is aborted\n> > +and the command exits immediately. The\n> > +<> hook is not\n> > +executed in that case.\n> > +\n> > +[[ref-transaction-aborted]]\n> > +ref-transaction-aborted\n> > +~~~~~~~~~~~~~~~~~~~~~~~\n> > +\n> > +This hook is invoked by any Git command that performs reference\n> > +updates. It executes as soon as a reference transaction is aborted and\n> > +after all reference locks were released and any changes made to\n> > +references were rolled back. The hook may get called multiple times or\n> > +never in case no transaction was aborted.\n> > +\n> > +The hook takes no arguments, but for each ref to be updated it\n> \n> Nit: I found it a bit surprising to read about refs \"to be updated\" in\n> the description of the 'aborted' hook, because by the time this hook\n> is called the update has already been refused. The same applies to\n> the 'committed' hook below as well.\n\nFair. This should rather read \"that would have been updated\" and \"that\nwere updated\", respectively.\n\nPatrick\n"},{"id":"399071","messageId":"20200603112604.GA25644@tanuki.pks.im","threadId":"53600","inReplyTo":"xmqq4krttl4k.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] refs: implement reference transaction hooks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-06-03T11:26:04Z","receivedAt":"2020-06-03T11:25:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jun 02, 2020 at 10:47:55AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > The above scenario is the motivation for a set of three new hooks that\n> > reach directly into Git's reference transaction. Each of the following\n> > new hooks (currently) doesn't accept any parameters and receives the set\n> > of queued reference updates via stdin:\n> \n> Do we have something (e.g. performance measurement) to convince\n> ourselves that this won't incur unacceptable levels of overhead in\n> null cases where there is no hook installed in the repository?\n\nNot yet, but I'll try to come up with a benchmark in the next iteration.\nI guess the best way to test is to directly exercise git-update-refs, as\nit's nearly a direct wrapper around reference transactions.\n\n> > +\tproc.in = -1;\n> > +\tproc.stdout_to_stderr = 1;\n> > +\tproc.trace2_hook_name = hook_name;\n> > +\n> > +\tcode = start_command(&proc);\n> > +\tif (code)\n> > +\t\treturn code;\n> > +\n> > +\tsigchain_push(SIGPIPE, SIG_IGN);\n> > +\n> > +\tfor (i = 0; i < transaction->nr; i++) {\n> > +\t\tstruct ref_update *update = transaction->updates[i];\n> > +\n> > +\t\tstrbuf_reset(&buf);\n> > +\t\tstrbuf_addf(&buf, \"%s %s %s\\n\",\n> > +\t\t\t oid_to_hex(&update->old_oid),\n> > +\t\t\t oid_to_hex(&update->new_oid),\n> > +\t\t\t update->refname);\n> > +\n> > +\t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0)\n> > +\t\t\tbreak;\n> \n> We leave the loop early when we detect a write failure here...\n> \n> > +\t}\n> > +\n> > +\tclose(proc.in);\n> > +\tsigchain_pop(SIGPIPE);\n> > +\n> > +\tstrbuf_release(&buf);\n> > +\treturn finish_command(&proc);\n> \n> ... but the caller does not get notified. Intended?\n\nThis is semi-intended. In case the hook doesn't fully consume stdin and\nexits early, writing to its stdin would fail as we ignore SIGPIPE. We\ndon't want to force the hook to care about consuming all of stdin,\nthough.\n\nWe could improve error handling here by ignoring EPIPE, but making every\nother write error fatal. If there's any other abnormal error condition\nthen we certainly don't want the hook to act on incomplete data and\npretend everything's fine.\n\n> > +}\n> > +\n> > int ref_transaction_prepare(struct ref_transaction *transaction,\n> > \t\t\t struct strbuf *err)\n> > {\n> > \tstruct ref_store *refs = transaction->ref_store;\n> > +\tint ret;\n> > \n> > \tswitch (transaction->state) {\n> > \tcase REF_TRANSACTION_OPEN:\n> > @@ -2012,7 +2060,17 @@ int ref_transaction_prepare(struct ref_transaction *transaction,\n> > \t\treturn -1;\n> > \t}\n> > \n> > -\treturn refs->be->transaction_prepare(refs, transaction, err);\n> > +\tret = refs->be->transaction_prepare(refs, transaction, err);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = run_transaction_hook(transaction, \"ref-transaction-prepared\");\n> \n> This caller does care about it, no?\n\nThis caller does as it may abort the transaction, but...\n\n> > +\tif (ret) {\n> > +\t\tref_transaction_abort(transaction, err);\n> > +\t\tdie(_(\"ref updates aborted by hook\"));\n> > +\t}\n> > +\n> > +\treturn 0;\n> > }\n> > \n> > int ref_transaction_abort(struct ref_transaction *transaction,\n> > @@ -2036,6 +2094,8 @@ int ref_transaction_abort(struct ref_transaction *transaction,\n> > \t\tbreak;\n> > \t}\n> > \n> > +\trun_transaction_hook(transaction, \"ref-transaction-aborted\");\n> \n> And I presume that the callers of \"ref_xn_abort()\" would, too, but\n> the value returned here does not get folded into 'ret'.\n\n... this one doesn't. The thing is that the reference transaction hook\nfor the \"aborted\" case can't do anything about an aborted transaction\nafter the fact anyway. That's why I chose to ignore errors here, same\nfor the \"committed\" case.\n\nPatrick\n"},{"id":"399072","messageId":"04116cc57ab37eeb50bd51a065a7c06503493bf3.1591186875.git.ps@pks.im","threadId":"53600","inReplyTo":"1d1a94426f95d842e0e3ea6a1569c0c45239229c.1591086316.git.ps@pks.im","subject":"[PATCH v2] refs: implement reference transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-06-03T12:27:50Z","receivedAt":"2020-06-03T12:27:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The low-level reference transactions used to update references are\ncurrently completely opaque to the user. While certainly desirable in\nmost usecases, there are some which might want to hook into the\ntransaction to observe all queued reference updates as well as observing\nthe abortion or commit of a prepared transaction.\n\nOne such usecase would be to have a set of replicas of a given Git\nrepository, where we perform Git operations on all of the repositories\nat once and expect the outcome to be the same in all of them. While\nthere exist hooks already for a certain subset of Git commands that\ncould be used to implement a voting mechanism for this, many others\ncurrently don't have any mechanism for this.\n\nThe above scenario is the motivation for the new \"reference-transaction\"\nhook that reaches directly into Git's reference transaction mechanism.\nThe hook receives as parameter the current state the transaction was\nmoved to (\"prepared\", \"committed\" or \"aborted\") and gets via its\nstandard input all queued reference updates. While the exit code gets\nignored in the \"committed\" and \"aborted\" states, a non-zero exit code in\nthe \"prepared\" state will cause the transaction to be aborted\nprematurely.\n\nGiven the usecase described above, a voting mechanism can now be\nimplemented via this hook: as soon as it gets called, it will take all\nof stdin and use it to cast a vote to a central service. When all\nreplicas of the repository agree, the hook will exit with zero,\notherwise it will abort the transaction by returning non-zero. The most\nimportant upside is that this will catch _all_ commands writing\nreferences at once, allowing to implement strong consistency for\nreference updates via a single mechanism.\n\nIn order to test the impact on the case where we don't have any\n\"reference-transaction\" hook installed in the repository, this commit\nintroduces a new performance test for git-update-refs(1). Run against an\nempty repository, it produces the following results:\n\n Test HEAD~ HEAD\n ------------------------------------------------------------------------------\n 1400.2: update existing reference 2.05(1.58+0.54) 2.08(1.58+0.57) +1.5%\n 1400.3: create and destroy reference 1.79(1.38+0.49) 1.82(1.39+0.51) +1.7%\n\nSo the overhead is around ~1.5%. Given that git-update-refs(1) is a\nnear-direct wrapper around reference transactions, there likely is no\nother command that is impacted much worse than this.\n\nSigned-off-by: Patrick Steinhardt \n---\n\nThe main adjustment in this second version is that I merged the\npreviously three hooks into a single one that gets the transaction state\nas its first parameter, as proposed by Gábor. This is mainly to enable\natomic replacement of all three scripts, even though it could still be\nthat the hook gets replaced during a session. But I think it makes sense\nregardless to merge these hooks into a single one.\n\nI've also made changes based on Junio's feedback and added a benchmark\nthat exercises git-update-refs(1) as a proxy for this change. I guess\nthe ~1.5% penalty isn't too bad. It might be improved by caching hook\nexistence, but I don't think it necessary right now.\n\nThanks to both of you for your feedback!\n\nPatrick\n\n Documentation/githooks.txt | 29 ++++++++\n refs.c | 72 +++++++++++++++++++-\n t/perf/p1400-update-ref.sh | 31 +++++++++\n t/t1416-ref-transaction-hooks.sh | 109 +++++++++++++++++++++++++++++++\n 4 files changed, 239 insertions(+), 2 deletions(-)\n create mode 100755 t/perf/p1400-update-ref.sh\n create mode 100755 t/t1416-ref-transaction-hooks.sh\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 81f2a87e88..642471109f 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -404,6 +404,35 @@ Both standard output and standard error output are forwarded to\n `git send-pack` on the other end, so you can simply `echo` messages\n for the user.\n \n+ref-transaction\n+~~~~~~~~~~~~~~~\n+\n+This hook is invoked by any Git command that performs reference\n+updates. It executes whenever a reference transaction is prepared,\n+committed or aborted and may thus get called multiple times.\n+\n+The hook takes exactly one argument, which is the current state the\n+given reference transaction is in:\n+\n+ - \"prepared\": All reference updates have been queued to the\n+ transaction and references were locked on disk.\n+\n+ - \"committed\": The reference transaction was committed and all\n+ references now have their respective new value.\n+\n+ - \"aborted\": The reference transaction was aborted, no changes\n+ were performed and the locks have been released.\n+\n+For each reference update that was added to the transaction, the hook\n+receives on standard input a line of the format:\n+\n+ SP SP LF\n+\n+The exit status of the hook is ignored for any state except for the\n+\"prepared\" state. In the \"prepared\" state, a non-zero exit status will\n+cause the transaction to be aborted. The hook will not be called with\n+\"aborted\" state in that case.\n+\n push-to-checkout\n ~~~~~~~~~~~~~~~~\n \ndiff --git a/refs.c b/refs.c\nindex 224ff66c7b..af752e1759 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -9,6 +9,7 @@\n #include \"iterator.h\"\n #include \"refs.h\"\n #include \"refs/refs-internal.h\"\n+#include \"run-command.h\"\n #include \"object-store.h\"\n #include \"object.h\"\n #include \"tag.h\"\n@@ -16,6 +17,7 @@\n #include \"worktree.h\"\n #include \"argv-array.h\"\n #include \"repository.h\"\n+#include \"sigchain.h\"\n \n /*\n * List of all available backends\n@@ -1986,10 +1988,61 @@ int ref_update_reject_duplicates(struct string_list *refnames,\n \treturn 0;\n }\n \n+static int run_transaction_hook(struct ref_transaction *transaction,\n+\t\t\t\tconst char *state)\n+{\n+\tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint saved_errno = 0, ret, i;\n+\tconst char *hook;\n+\n+\thook = find_hook(\"reference-transaction\");\n+\tif (!hook)\n+\t\treturn 0;\n+\n+\targv_array_pushl(&proc.args, hook, state, NULL);\n+\tproc.in = -1;\n+\tproc.stdout_to_stderr = 1;\n+\tproc.trace2_hook_name = hook;\n+\n+\tret = start_command(&proc);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addf(&buf, \"%s %s %s\\n\",\n+\t\t\t oid_to_hex(&update->old_oid),\n+\t\t\t oid_to_hex(&update->new_oid),\n+\t\t\t update->refname);\n+\n+\t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0) {\n+\t\t\tif (errno != EPIPE)\n+\t\t\t\tsaved_errno = errno;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tclose(proc.in);\n+\tsigchain_pop(SIGPIPE);\n+\tstrbuf_release(&buf);\n+\n+\tret = finish_command(&proc);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\treturn saved_errno;\n+}\n+\n int ref_transaction_prepare(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_store *refs = transaction->ref_store;\n+\tint ret;\n \n \tswitch (transaction->state) {\n \tcase REF_TRANSACTION_OPEN:\n@@ -2012,7 +2065,17 @@ int ref_transaction_prepare(struct ref_transaction *transaction,\n \t\treturn -1;\n \t}\n \n-\treturn refs->be->transaction_prepare(refs, transaction, err);\n+\tret = refs->be->transaction_prepare(refs, transaction, err);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\tret = run_transaction_hook(transaction, \"prepared\");\n+\tif (ret) {\n+\t\tref_transaction_abort(transaction, err);\n+\t\tdie(_(\"ref updates aborted by hook\"));\n+\t}\n+\n+\treturn 0;\n }\n \n int ref_transaction_abort(struct ref_transaction *transaction,\n@@ -2036,6 +2099,8 @@ int ref_transaction_abort(struct ref_transaction *transaction,\n \t\tbreak;\n \t}\n \n+\trun_transaction_hook(transaction, \"aborted\");\n+\n \tref_transaction_free(transaction);\n \treturn ret;\n }\n@@ -2064,7 +2129,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tbreak;\n \t}\n \n-\treturn refs->be->transaction_finish(refs, transaction, err);\n+\tret = refs->be->transaction_finish(refs, transaction, err);\n+\tif (!ret)\n+\t\trun_transaction_hook(transaction, \"committed\");\n+\treturn ret;\n }\n \n int refs_verify_refname_available(struct ref_store *refs,\ndiff --git a/t/perf/p1400-update-ref.sh b/t/perf/p1400-update-ref.sh\nnew file mode 100755\nindex 0000000000..4f4519529e\n--- /dev/null\n+++ b/t/perf/p1400-update-ref.sh\n@@ -0,0 +1,31 @@\n+#!/bin/sh\n+\n+test_description=\"Tests performance of update-ref\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_fresh_repo\n+\n+test_expect_success \"setup\" '\n+\ttest_commit PRE &&\n+\ttest_commit POST &&\n+\tgit branch update-branch\n+'\n+\n+test_perf \"update existing reference\" '\n+\tfor i in $(test_seq 1000)\n+\tdo\n+\t\tgit update-ref refs/heads/update-branch PRE POST &&\n+\t\tgit update-ref refs/heads/update-branch POST PRE\n+\tdone\n+'\n+\n+test_perf \"create and destroy reference\" '\n+\tfor i in $(test_seq 1000)\n+\tdo\n+\t\tgit update-ref refs/heads/new POST\n+\t\tgit update-ref -d refs/heads/new\n+\tdone\n+'\n+\n+test_done\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nnew file mode 100755\nindex 0000000000..da58d867a5\n--- /dev/null\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -0,0 +1,109 @@\n+#!/bin/sh\n+\n+test_description='reference transaction hooks'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tmkdir -p .git/hooks &&\n+\ttest_commit PRE &&\n+\ttest_commit POST &&\n+\tPOST_OID=$(git rev-parse POST)\n+'\n+\n+test_expect_success 'hook allows updating ref if successful' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\techo \"$*\" >>actual\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\tcommitted\n+\tEOF\n+\tgit update-ref HEAD POST &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'hook aborts updating ref in prepared state' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\tif test \"$1\" = prepared\n+\t\tthen\n+\t\t\texit 1\n+\t\tfi\n+\tEOF\n+\ttest_must_fail git update-ref HEAD POST 2>err &&\n+\ttest_i18ngrep \"ref updates aborted by hook\" err\n+'\n+\n+test_expect_success 'hook gets all queued updates in prepared state' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction actual\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\tif test \"$1\" = prepared\n+\t\tthen\n+\t\t\twhile read -r line\n+\t\t\tdo\n+\t\t\t\tprintf \"%s\\n\" \"$line\"\n+\t\t\tdone >actual\n+\t\tfi\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\t$ZERO_OID $POST_OID HEAD\n+\t\t$ZERO_OID $POST_OID refs/heads/master\n+\tEOF\n+\tgit update-ref HEAD POST <<-EOF &&\n+\t\tupdate HEAD $ZERO_OID $POST_OID\n+\t\tupdate refs/heads/master $ZERO_OID $POST_OID\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'hook gets all queued updates in committed state' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction actual\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\tif test \"$1\" = committed\n+\t\tthen\n+\t\t\twhile read -r line\n+\t\t\tdo\n+\t\t\t\tprintf \"%s\\n\" \"$line\"\n+\t\t\tdone >actual\n+\t\tfi\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\t$ZERO_OID $POST_OID HEAD\n+\t\t$ZERO_OID $POST_OID refs/heads/master\n+\tEOF\n+\tgit update-ref HEAD POST &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'hook gets all queued updates in aborted state' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction actual\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\tif test \"$1\" = aborted\n+\t\tthen\n+\t\t\twhile read -r line\n+\t\t\tdo\n+\t\t\t\tprintf \"%s\\n\" \"$line\"\n+\t\t\tdone >actual\n+\t\tfi\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\t$ZERO_OID $POST_OID HEAD\n+\t\t$ZERO_OID $POST_OID refs/heads/master\n+\tEOF\n+\tgit update-ref --stdin <<-EOF &&\n+\t\tstart\n+\t\tupdate HEAD POST $ZERO_OID\n+\t\tupdate refs/heads/master POST $ZERO_OID\n+\t\tabort\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.27.0\n\n"},{"id":"399080","messageId":"20200603165142.GA24049@syl.local","threadId":"53600","inReplyTo":"04116cc57ab37eeb50bd51a065a7c06503493bf3.1591186875.git.ps@pks.im","subject":"Re: [PATCH v2] refs: implement reference transaction hook","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-03T16:51:42Z","receivedAt":"2020-06-03T16:51:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Patrick,\n\nOn Wed, Jun 03, 2020 at 02:27:50PM +0200, Patrick Steinhardt wrote:\n> In order to test the impact on the case where we don't have any\n> \"reference-transaction\" hook installed in the repository, this commit\n> introduces a new performance test for git-update-refs(1). Run against an\n> empty repository, it produces the following results:\n>\n> Test HEAD~ HEAD\n> ------------------------------------------------------------------------------\n> 1400.2: update existing reference 2.05(1.58+0.54) 2.08(1.58+0.57) +1.5%\n> 1400.3: create and destroy reference 1.79(1.38+0.49) 1.82(1.39+0.51) +1.7%\n>\n> So the overhead is around ~1.5%. Given that git-update-refs(1) is a\n> near-direct wrapper around reference transactions, there likely is no\n> other command that is impacted much worse than this.\n\nThis is a serious performance regression that is worth considering. For\nexample, a 1.5% slow-down on average in reference transactions would\ncause be enough for me to bisect the regression down to see what\nchanged.\n\nWhat are ways that this could be avoided? I bet that this would work\nquite well with Emily's work on hooks, where we could check in the\nconfiguration first whether a hook is even configured.\n\nCould this be integrated with that? If not, could you cache the result\nof whether or not the hook exists, and/or implement some mechanism to\nsay something like, for eg., \"only run reference transaction hooks\ncore.blah = 1\" is true?\n\nI haven't myself looked seriously at your patch (although I do plan on\ndoing so, but haven't yet had time), but I think that this should be\nconsidered thoroughly before moving forward.\n\nThanks,\nTaylor\n"},{"id":"399082","messageId":"CAFQ2z_OSfsKKkOu_wKEAWi-Zihp4g265jtmXzteaU=BQZohEAQ@mail.gmail.com","threadId":"53600","inReplyTo":"04116cc57ab37eeb50bd51a065a7c06503493bf3.1591186875.git.ps@pks.im","subject":"Re: [PATCH v2] refs: implement reference transaction hook","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-06-03T17:44:05Z","receivedAt":"2020-06-03T17:44:20Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Jun 3, 2020 at 2:27 PM Patrick Steinhardt wrote:\n> The low-level reference transactions used to update references are\n> currently completely opaque to the user. While certainly desirable in\n> most usecases, there are some which might want to hook into the\n> transaction to observe all queued reference updates as well as observing\n> the abortion or commit of a prepared transaction.\n\nI have an alternate suggestion, but it would depend on having\nreftable. In reftable, the transaction is\n\n 0) create tables.list.lock\n 1) write out new reftable(s),\n 2) renaming tables.list.lock to tables.list.\n\nIf you insert a hook between 1) and 2), the hook would not need to be\nsupplied with any data, so it would have minimal performance impact.\nIf the hook runs, it can compare tables.list and tables.list.lock,\nread out the new reftables, and make decisions based on that. Or\nalternatively, the transaction could be passed a list of reftables,\nthat the hook could then process at its leisure.\n\n> +For each reference update that was added to the transaction, the hook\n> +receives on standard input a line of the format:\n> +\n> + SP SP LF\n\nDoes this work for symrefs as well? Should it?\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"399083","messageId":"xmqq7dworpqy.fsf@gitster.c.googlers.com","threadId":"53600","inReplyTo":"CAFQ2z_OSfsKKkOu_wKEAWi-Zihp4g265jtmXzteaU=BQZohEAQ@mail.gmail.com","subject":"Re: [PATCH v2] refs: implement reference transaction hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-03T18:03:17Z","receivedAt":"2020-06-03T18:03:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n>> +For each reference update that was added to the transaction, the hook\n>> +receives on standard input a line of the format:\n>> +\n>> + SP SP LF\n>\n> Does this work for symrefs as well? Should it?\n\nThat's a good question.\n\nI am assuming that you are asking about updating the value of the\nunderlying ref the symbolic ref points at (i.e. would be\n\"HEAD\" and old- and new-value are commits at the tip of the branch\nthat is currently checked out), and not about repointing the\nsymbolic ref to a different underlying ref. I suspect we do not\nwant to. In such a case, we'd have a separate \"refs/heads/master\nmoved from old to new\" record anyway, so we probably should not even\ntalk about symbolic refs when we report \"a ref got updated to point\na different object\" in the above format.\n\nAnd obviously the above format cannot talk about repointing HEAD to\na different branch (well, it can in a sense that old and new branch\nboth would have some object name that can be filled in the old- and\nnew-value fields, but the record misses the most interesting part of\nthe \"branch switching\" without the actual old and new refnames), so\nthere must be another format that allows us to report such an event.\n\n \"Sym\" SP SP SP LF\n\nor something like that, so that\n\n\t$ git checkout next\n\nmay yield\n\n \"Sym\" SP \"refs/heads/master\" SP \"refs/heads/next\" SP \"HEAD\"\n\nif we originally were on the 'master' branch. Obviously we need a\nway to represent different kinds of transition, like (1) a new\nsymbolic ref has been created, pointing at some underlying ref\n(which may or may not exist), (2) an existing ref that pointed\ndirectly at an object now has become a symbolic ref, (3) a symbolic\nref stopped pointing at a ref and now points directly at an object,\nand (4) a symbolic ref has been removed.\n\nBoth (1) and (4) would be a trivial variation of the above format, I\nguess, using some special token to denote void (say, we'd use an\nempty string for that):\n\n \"Sym\" \"\" SP SP LF\n \"Sym\" SP \"\" SP LF\n\n(2) and (3) are interesting. We could represent them as two\nseparate operations inside the same transaction, e.g.\n\n \"Sym\" \"refs/heads/master\" SP \"\" SP \"HEAD\" LF\n \"0{40}\" SP \"b3d7a52fac39193503a0b6728771d1bf6a161464\" SP \"HEADD\" LF\n\nor we'd need to invent different type of record to represent such\n\"detaching\" and \"reattaching\" events.\n\n\n \n\n\n\n"},{"id":"399104","messageId":"20200604073632.GA498923@tanuki.pks.im","threadId":"53600","inReplyTo":"20200603165142.GA24049@syl.local","subject":"Re: [PATCH v2] refs: implement reference transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-06-04T07:36:32Z","receivedAt":"2020-06-04T07:35:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jun 03, 2020 at 10:51:42AM -0600, Taylor Blau wrote:\n> Hi Patrick,\n> \n> On Wed, Jun 03, 2020 at 02:27:50PM +0200, Patrick Steinhardt wrote:\n> > In order to test the impact on the case where we don't have any\n> > \"reference-transaction\" hook installed in the repository, this commit\n> > introduces a new performance test for git-update-refs(1). Run against an\n> > empty repository, it produces the following results:\n> >\n> > Test HEAD~ HEAD\n> > ------------------------------------------------------------------------------\n> > 1400.2: update existing reference 2.05(1.58+0.54) 2.08(1.58+0.57) +1.5%\n> > 1400.3: create and destroy reference 1.79(1.38+0.49) 1.82(1.39+0.51) +1.7%\n> >\n> > So the overhead is around ~1.5%. Given that git-update-refs(1) is a\n> > near-direct wrapper around reference transactions, there likely is no\n> > other command that is impacted much worse than this.\n> \n> This is a serious performance regression that is worth considering. For\n> example, a 1.5% slow-down on average in reference transactions would\n> cause be enough for me to bisect the regression down to see what\n> changed.\n> \n> What are ways that this could be avoided? I bet that this would work\n> quite well with Emily's work on hooks, where we could check in the\n> configuration first whether a hook is even configured.\n> \n> Could this be integrated with that? If not, could you cache the result\n> of whether or not the hook exists, and/or implement some mechanism to\n> say something like, for eg., \"only run reference transaction hooks\n> core.blah = 1\" is true?\n\nI wasn't aware of her work until now, so I'll take a look.\n\nMeanwhile, I toyed around with performance and tried out two different\ncaching mechanisms:\n\n - map-cache: `find_hook()` uses a map of hook names mapped to their\n resolved hook path (or `NULL` if none was found). This is a\n generic mechanism working across all hooks, but has some overhead\n in maintaining the map.\n\n - reftx-cache: `run_transaction_hook()` caches the path returned by\n `find_hook()`. It's got less overhead as it only caches the path,\n but obviously only applies to the reference-transaction hook.\n\nIn theory, we could go even further and cache the hook's file\ndescriptor, executing it via fexecve(3P) whenever it's required, but I\ndidn't go down that route yet. This would also solve the atomicity\nissue, though, if the administrator replaces the reference-transactions\nhook halfway through the transaction.\n\nBenchmarking results are mixed, mostly in the sense that I can choose\nwhich run of the benchmarks I take in order to have my own work look\nbetter or worse, as timings vary quite a lot between runs. Which\nprobably hints at the fact that the benchmarks themselves aren't really\nreliable. The issue is that a single git-update-ref(1) run finishes so\nquick that it's hard to measure with our benchmarks, but spawning\nthousands of them to get something different than 0.0s very much depends\non the operating system and thus fluctuates. On the other hand, if we\nonly spawn a single git-update-refs and have it perform a few thousand\nref updates, overhead of the hook will not show at all as it is only\nexecuted once per prepare/commit of the transaction.\n\nThe following timings are for the case where we execute\n\n git update-ref refs/heads/update-branch PRE POST &&\n git update-ref refs/heads/update-branch POST PRE\n\nrespectively\n\n git update-ref refs/heads/new POST &&\n git update-ref -d refs/heads/new\n\na thousand times:\n\nTest master ref-hook-no-cache ref-hook-map-cache ref-hook-reftx-cache\n------------------------------------------------------------------------------------------------------------------------------\n1400.2: update existing reference 1.96(1.50+0.53) 2.00(1.54+0.53) +2.0% 2.02(1.54+0.55) +3.1% 1.98(1.52+0.52) +1.0%\n1400.3: create and destroy reference 1.74(1.33+0.49) 1.77(1.38+0.47) +1.7% 1.77(1.36+0.48) +1.7% 1.76(1.35+0.49) +1.1%\n\nFor such a short-lived program like git-update-refs(1), one can see that\nthe overhead of using a map negatively impacts performance compared to\nthe no-cache case. But using the reftx-cache roughly cuts the overhead\nin half as expected, as we only need to look up the hook once instead of\ntwice.\n\nAnd here's the results if we use a single `git update-ref --stdin` with a\nthousand reference updates at once:\n\nTest master ref-hook-no-cache ref-hook-map-cache ref-hook-reftx-cache\n------------------------------------------------------------------------------------------------------------------------\n1400.2: git update-ref --stdin 0.21(0.09+0.12) 0.21(0.07+0.14) +0.0% 0.21(0.07+0.13) +0.0% 0.21(0.07+0.13) +0.0%\n\nAs expected, there's nothing much to be seen here because there's only a\nsingle transaction for all ref updates and, as a result, at most two\ncalls to `access(refhook_path, X_OK)`.\n\nPatrick\n"},{"id":"399262","messageId":"20200607201233.GB8232@szeder.dev","threadId":"53600","inReplyTo":"20200603112604.GA25644@tanuki.pks.im","subject":"Re: [PATCH] refs: implement reference transaction hooks","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-06-07T20:12:33Z","receivedAt":"2020-06-07T20:12:42Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jun 03, 2020 at 01:26:04PM +0200, Patrick Steinhardt wrote:\n> On Tue, Jun 02, 2020 at 10:47:55AM -0700, Junio C Hamano wrote:\n> > Patrick Steinhardt writes:\n> > \n> > > The above scenario is the motivation for a set of three new hooks that\n> > > reach directly into Git's reference transaction. Each of the following\n> > > new hooks (currently) doesn't accept any parameters and receives the set\n> > > of queued reference updates via stdin:\n> > \n> > Do we have something (e.g. performance measurement) to convince\n> > ourselves that this won't incur unacceptable levels of overhead in\n> > null cases where there is no hook installed in the repository?\n> \n> Not yet, but I'll try to come up with a benchmark in the next iteration.\n> I guess the best way to test is to directly exercise git-update-refs, as\n> it's nearly a direct wrapper around reference transactions.\n> \n> > > +\tproc.in = -1;\n> > > +\tproc.stdout_to_stderr = 1;\n> > > +\tproc.trace2_hook_name = hook_name;\n> > > +\n> > > +\tcode = start_command(&proc);\n> > > +\tif (code)\n> > > +\t\treturn code;\n> > > +\n> > > +\tsigchain_push(SIGPIPE, SIG_IGN);\n> > > +\n> > > +\tfor (i = 0; i < transaction->nr; i++) {\n> > > +\t\tstruct ref_update *update = transaction->updates[i];\n> > > +\n> > > +\t\tstrbuf_reset(&buf);\n> > > +\t\tstrbuf_addf(&buf, \"%s %s %s\\n\",\n> > > +\t\t\t oid_to_hex(&update->old_oid),\n> > > +\t\t\t oid_to_hex(&update->new_oid),\n> > > +\t\t\t update->refname);\n> > > +\n> > > +\t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0)\n> > > +\t\t\tbreak;\n> > \n> > We leave the loop early when we detect a write failure here...\n> > \n> > > +\t}\n> > > +\n> > > +\tclose(proc.in);\n> > > +\tsigchain_pop(SIGPIPE);\n> > > +\n> > > +\tstrbuf_release(&buf);\n> > > +\treturn finish_command(&proc);\n> > \n> > ... but the caller does not get notified. Intended?\n> \n> This is semi-intended. In case the hook doesn't fully consume stdin and\n> exits early, writing to its stdin would fail as we ignore SIGPIPE. We\n> don't want to force the hook to care about consuming all of stdin,\n> though.\n\nWhy? How could the prepared hook properly initialize the voting\nmechanism for the transaction without reading all the refs to be\nupdated?\n\n> We could improve error handling here by ignoring EPIPE, but making every\n> other write error fatal. If there's any other abnormal error condition\n> then we certainly don't want the hook to act on incomplete data and\n> pretend everything's fine.\n\nAs I read v2 of this patch, a prepared hook can exit(0) early without\nreading all the refs to be updated, cause EPIPE in the git process\ninvoking the hook, and that process would interpret that as success.\nI haven't though it through how such a voting mechanism would work,\nbut I have a gut feeling that this can't be good.\n\n"},{"id":"399270","messageId":"20200608053605.GA851@tanuki","threadId":"53600","inReplyTo":"20200607201233.GB8232@szeder.dev","subject":"Re: [PATCH] refs: implement reference transaction hooks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-06-08T05:36:05Z","receivedAt":"2020-06-08T05:35:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jun 07, 2020 at 10:12:33PM +0200, SZEDER Gábor wrote:\n> On Wed, Jun 03, 2020 at 01:26:04PM +0200, Patrick Steinhardt wrote:\n> > On Tue, Jun 02, 2020 at 10:47:55AM -0700, Junio C Hamano wrote:\n> > > Patrick Steinhardt writes:\n> > > \n> > > > The above scenario is the motivation for a set of three new hooks that\n> > > > reach directly into Git's reference transaction. Each of the following\n> > > > new hooks (currently) doesn't accept any parameters and receives the set\n> > > > of queued reference updates via stdin:\n> > > \n> > > Do we have something (e.g. performance measurement) to convince\n> > > ourselves that this won't incur unacceptable levels of overhead in\n> > > null cases where there is no hook installed in the repository?\n> > \n> > Not yet, but I'll try to come up with a benchmark in the next iteration.\n> > I guess the best way to test is to directly exercise git-update-refs, as\n> > it's nearly a direct wrapper around reference transactions.\n> > \n> > > > +\tproc.in = -1;\n> > > > +\tproc.stdout_to_stderr = 1;\n> > > > +\tproc.trace2_hook_name = hook_name;\n> > > > +\n> > > > +\tcode = start_command(&proc);\n> > > > +\tif (code)\n> > > > +\t\treturn code;\n> > > > +\n> > > > +\tsigchain_push(SIGPIPE, SIG_IGN);\n> > > > +\n> > > > +\tfor (i = 0; i < transaction->nr; i++) {\n> > > > +\t\tstruct ref_update *update = transaction->updates[i];\n> > > > +\n> > > > +\t\tstrbuf_reset(&buf);\n> > > > +\t\tstrbuf_addf(&buf, \"%s %s %s\\n\",\n> > > > +\t\t\t oid_to_hex(&update->old_oid),\n> > > > +\t\t\t oid_to_hex(&update->new_oid),\n> > > > +\t\t\t update->refname);\n> > > > +\n> > > > +\t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0)\n> > > > +\t\t\tbreak;\n> > > \n> > > We leave the loop early when we detect a write failure here...\n> > > \n> > > > +\t}\n> > > > +\n> > > > +\tclose(proc.in);\n> > > > +\tsigchain_pop(SIGPIPE);\n> > > > +\n> > > > +\tstrbuf_release(&buf);\n> > > > +\treturn finish_command(&proc);\n> > > \n> > > ... but the caller does not get notified. Intended?\n> > \n> > This is semi-intended. In case the hook doesn't fully consume stdin and\n> > exits early, writing to its stdin would fail as we ignore SIGPIPE. We\n> > don't want to force the hook to care about consuming all of stdin,\n> > though.\n> \n> Why? How could the prepared hook properly initialize the voting\n> mechanism for the transaction without reading all the refs to be\n> updated?\n\nBecause the hook might not want to implement a voting mechanism after\nall but something entirely different which we're currently not\nforeseeing as a valid usecase. We don't enforce this anywhere else\neither, like e.g. for the pre-receive hook. If that one exits early\nwithout consuming its stdin then that's totally fine.\n\n> > We could improve error handling here by ignoring EPIPE, but making every\n> > other write error fatal. If there's any other abnormal error condition\n> > then we certainly don't want the hook to act on incomplete data and\n> > pretend everything's fine.\n> \n> As I read v2 of this patch, a prepared hook can exit(0) early without\n> reading all the refs to be updated, cause EPIPE in the git process\n> invoking the hook, and that process would interpret that as success.\n> I haven't though it through how such a voting mechanism would work,\n> but I have a gut feeling that this can't be good.\n\nAs said, I lean towards allowing more flexibility for the hook\nimplementation to also cater for other usecases. But I agree that in a\nvoting implementation, not reading all of stdin is a bad thing and may\npoint to a buggy hook implementation. Aborting the transaction if the\nhook didn't read all of stdin would be a nice safeguard in that case.\n\nWith the current implementation of using a single hook for \"prepared\",\n\"committed\" and \"aborted\", it'd also force the hook implementation to do\nsomething in cases it doesn't care about. E.g.\n\n #!/bin/sh\n case \"$1\" in\n prepared)\n VOTE=$(sha1sum <&0)\n cast $VOTE\n ;;\n aborted|committed)\n cat <&0 >/dev/null\n ;;\n esac\n\nThat being said, I'm not opposed to enforce this and not treat EPIPE\ndifferently.\n\nPatrick\n"},{"id":"399794","messageId":"20200615164639.GD71506@syl.local","threadId":"53600","inReplyTo":"20200604073632.GA498923@tanuki.pks.im","subject":"Re: [PATCH v2] refs: implement reference transaction hook","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-15T16:46:39Z","receivedAt":"2020-06-15T16:46:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Patrick,\n\nSorry for the slow response. I was out of the office last week and am\nonly just now getting a chance to catch up on the backlog of emails that\nI missed.\n\nOn Thu, Jun 04, 2020 at 09:36:32AM +0200, Patrick Steinhardt wrote:\n> On Wed, Jun 03, 2020 at 10:51:42AM -0600, Taylor Blau wrote:\n> > Hi Patrick,\n> >\n> > On Wed, Jun 03, 2020 at 02:27:50PM +0200, Patrick Steinhardt wrote:\n> > > In order to test the impact on the case where we don't have any\n> > > \"reference-transaction\" hook installed in the repository, this commit\n> > > introduces a new performance test for git-update-refs(1). Run against an\n> > > empty repository, it produces the following results:\n> > >\n> > > Test HEAD~ HEAD\n> > > ------------------------------------------------------------------------------\n> > > 1400.2: update existing reference 2.05(1.58+0.54) 2.08(1.58+0.57) +1.5%\n> > > 1400.3: create and destroy reference 1.79(1.38+0.49) 1.82(1.39+0.51) +1.7%\n> > >\n> > > So the overhead is around ~1.5%. Given that git-update-refs(1) is a\n> > > near-direct wrapper around reference transactions, there likely is no\n> > > other command that is impacted much worse than this.\n> >\n> > This is a serious performance regression that is worth considering. For\n> > example, a 1.5% slow-down on average in reference transactions would\n> > cause be enough for me to bisect the regression down to see what\n> > changed.\n> >\n> > What are ways that this could be avoided? I bet that this would work\n> > quite well with Emily's work on hooks, where we could check in the\n> > configuration first whether a hook is even configured.\n> >\n> > Could this be integrated with that? If not, could you cache the result\n> > of whether or not the hook exists, and/or implement some mechanism to\n> > say something like, for eg., \"only run reference transaction hooks\n> > core.blah = 1\" is true?\n>\n> I wasn't aware of her work until now, so I'll take a look.\n>\n> Meanwhile, I toyed around with performance and tried out two different\n> caching mechanisms:\n>\n> - map-cache: `find_hook()` uses a map of hook names mapped to their\n> resolved hook path (or `NULL` if none was found). This is a\n> generic mechanism working across all hooks, but has some overhead\n> in maintaining the map.\n>\n> - reftx-cache: `run_transaction_hook()` caches the path returned by\n> `find_hook()`. It's got less overhead as it only caches the path,\n> but obviously only applies to the reference-transaction hook.\n>\n> In theory, we could go even further and cache the hook's file\n> descriptor, executing it via fexecve(3P) whenever it's required, but I\n> didn't go down that route yet. This would also solve the atomicity\n> issue, though, if the administrator replaces the reference-transactions\n> hook halfway through the transaction.\n>\n> Benchmarking results are mixed, mostly in the sense that I can choose\n> which run of the benchmarks I take in order to have my own work look\n> better or worse, as timings vary quite a lot between runs. Which\n> probably hints at the fact that the benchmarks themselves aren't really\n> reliable. The issue is that a single git-update-ref(1) run finishes so\n> quick that it's hard to measure with our benchmarks, but spawning\n> thousands of them to get something different than 0.0s very much depends\n> on the operating system and thus fluctuates. On the other hand, if we\n> only spawn a single git-update-refs and have it perform a few thousand\n> ref updates, overhead of the hook will not show at all as it is only\n> executed once per prepare/commit of the transaction.\n>\n> The following timings are for the case where we execute\n>\n> git update-ref refs/heads/update-branch PRE POST &&\n> git update-ref refs/heads/update-branch POST PRE\n>\n> respectively\n>\n> git update-ref refs/heads/new POST &&\n> git update-ref -d refs/heads/new\n>\n> a thousand times:\n>\n> Test master ref-hook-no-cache ref-hook-map-cache ref-hook-reftx-cache\n> ------------------------------------------------------------------------------------------------------------------------------\n> 1400.2: update existing reference 1.96(1.50+0.53) 2.00(1.54+0.53) +2.0% 2.02(1.54+0.55) +3.1% 1.98(1.52+0.52) +1.0%\n> 1400.3: create and destroy reference 1.74(1.33+0.49) 1.77(1.38+0.47) +1.7% 1.77(1.36+0.48) +1.7% 1.76(1.35+0.49) +1.1%\n\nHuh. It is super interesting (to me, at least) that caching the set of\nhooks that are on disk and should be run makes this *slower* than\nwithout the cache. What's going on there? In p1400.2, I'd expect to see\n'ref-hook-map-cache' attain at most a 2.0% slow-down, plus a little bit\nto process the hook when it is there, but not much more than that.\n\nI think that this just seems a little contrived to me. I can understand\nwhy server administrators may want this feature, but the general\nuser-base of Git doesn't seem to stand to benefit much from this change\n(in my own mind, at least).\n\nSo... I'm not sure where this leaves us. Maybe a 2.0% slow-down on an\nalready fast 'git update-ref' invocation for the average user won't be\nnoticeable. It certainly *will* be noticeable to administrators who\nprocesses a much higher volume of such transactions.\n\n> For such a short-lived program like git-update-refs(1), one can see that\n> the overhead of using a map negatively impacts performance compared to\n> the no-cache case. But using the reftx-cache roughly cuts the overhead\n> in half as expected, as we only need to look up the hook once instead of\n> twice.\n>\n> And here's the results if we use a single `git update-ref --stdin` with a\n> thousand reference updates at once:\n>\n> Test master ref-hook-no-cache ref-hook-map-cache ref-hook-reftx-cache\n> ------------------------------------------------------------------------------------------------------------------------\n> 1400.2: git update-ref --stdin 0.21(0.09+0.12) 0.21(0.07+0.14) +0.0% 0.21(0.07+0.13) +0.0% 0.21(0.07+0.13) +0.0%\n>\n> As expected, there's nothing much to be seen here because there's only a\n> single transaction for all ref updates and, as a result, at most two\n> calls to `access(refhook_path, X_OK)`.\n\nBetter, but I have to imagine that real-world usage will look much more\nlike a thousand tiny transactions than one large one.\n\n> Patrick\n\nThanks,\nTaylor\n"},{"id":"399845","messageId":"20200616054521.GA846@tanuki","threadId":"53600","inReplyTo":"20200615164639.GD71506@syl.local","subject":"Re: [PATCH v2] refs: implement reference transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-06-16T05:45:21Z","receivedAt":"2020-06-16T05:44:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jun 15, 2020 at 10:46:39AM -0600, Taylor Blau wrote:\n> Hi Patrick,\n> \n> Sorry for the slow response. I was out of the office last week and am\n> only just now getting a chance to catch up on the backlog of emails that\n> I missed.\n\nNo worries!\n\n> On Thu, Jun 04, 2020 at 09:36:32AM +0200, Patrick Steinhardt wrote:\n> > On Wed, Jun 03, 2020 at 10:51:42AM -0600, Taylor Blau wrote:\n> > > Hi Patrick,\n> > >\n> > > On Wed, Jun 03, 2020 at 02:27:50PM +0200, Patrick Steinhardt wrote:\n> > > > In order to test the impact on the case where we don't have any\n> > > > \"reference-transaction\" hook installed in the repository, this commit\n> > > > introduces a new performance test for git-update-refs(1). Run against an\n> > > > empty repository, it produces the following results:\n> > > >\n> > > > Test HEAD~ HEAD\n> > > > ------------------------------------------------------------------------------\n> > > > 1400.2: update existing reference 2.05(1.58+0.54) 2.08(1.58+0.57) +1.5%\n> > > > 1400.3: create and destroy reference 1.79(1.38+0.49) 1.82(1.39+0.51) +1.7%\n> > > >\n> > > > So the overhead is around ~1.5%. Given that git-update-refs(1) is a\n> > > > near-direct wrapper around reference transactions, there likely is no\n> > > > other command that is impacted much worse than this.\n> > >\n> > > This is a serious performance regression that is worth considering. For\n> > > example, a 1.5% slow-down on average in reference transactions would\n> > > cause be enough for me to bisect the regression down to see what\n> > > changed.\n> > >\n> > > What are ways that this could be avoided? I bet that this would work\n> > > quite well with Emily's work on hooks, where we could check in the\n> > > configuration first whether a hook is even configured.\n> > >\n> > > Could this be integrated with that? If not, could you cache the result\n> > > of whether or not the hook exists, and/or implement some mechanism to\n> > > say something like, for eg., \"only run reference transaction hooks\n> > > core.blah = 1\" is true?\n> >\n> > I wasn't aware of her work until now, so I'll take a look.\n> >\n> > Meanwhile, I toyed around with performance and tried out two different\n> > caching mechanisms:\n> >\n> > - map-cache: `find_hook()` uses a map of hook names mapped to their\n> > resolved hook path (or `NULL` if none was found). This is a\n> > generic mechanism working across all hooks, but has some overhead\n> > in maintaining the map.\n> >\n> > - reftx-cache: `run_transaction_hook()` caches the path returned by\n> > `find_hook()`. It's got less overhead as it only caches the path,\n> > but obviously only applies to the reference-transaction hook.\n> >\n> > In theory, we could go even further and cache the hook's file\n> > descriptor, executing it via fexecve(3P) whenever it's required, but I\n> > didn't go down that route yet. This would also solve the atomicity\n> > issue, though, if the administrator replaces the reference-transactions\n> > hook halfway through the transaction.\n> >\n> > Benchmarking results are mixed, mostly in the sense that I can choose\n> > which run of the benchmarks I take in order to have my own work look\n> > better or worse, as timings vary quite a lot between runs. Which\n> > probably hints at the fact that the benchmarks themselves aren't really\n> > reliable. The issue is that a single git-update-ref(1) run finishes so\n> > quick that it's hard to measure with our benchmarks, but spawning\n> > thousands of them to get something different than 0.0s very much depends\n> > on the operating system and thus fluctuates. On the other hand, if we\n> > only spawn a single git-update-refs and have it perform a few thousand\n> > ref updates, overhead of the hook will not show at all as it is only\n> > executed once per prepare/commit of the transaction.\n> >\n> > The following timings are for the case where we execute\n> >\n> > git update-ref refs/heads/update-branch PRE POST &&\n> > git update-ref refs/heads/update-branch POST PRE\n> >\n> > respectively\n> >\n> > git update-ref refs/heads/new POST &&\n> > git update-ref -d refs/heads/new\n> >\n> > a thousand times:\n> >\n> > Test master ref-hook-no-cache ref-hook-map-cache ref-hook-reftx-cache\n> > ------------------------------------------------------------------------------------------------------------------------------\n> > 1400.2: update existing reference 1.96(1.50+0.53) 2.00(1.54+0.53) +2.0% 2.02(1.54+0.55) +3.1% 1.98(1.52+0.52) +1.0%\n> > 1400.3: create and destroy reference 1.74(1.33+0.49) 1.77(1.38+0.47) +1.7% 1.77(1.36+0.48) +1.7% 1.76(1.35+0.49) +1.1%\n> \n> Huh. It is super interesting (to me, at least) that caching the set of\n> hooks that are on disk and should be run makes this *slower* than\n> without the cache. What's going on there? In p1400.2, I'd expect to see\n> 'ref-hook-map-cache' attain at most a 2.0% slow-down, plus a little bit\n> to process the hook when it is there, but not much more than that.\n\nI think the issue is that a single git-update-ref(1) invocation does so\nlittle work that allocating the hashmap and inserting the hook already\nhas noticeable impact on the program's runtime. E.g. in above\nbenchmark, a single call to git-update-ref in p1400.2 takes roughly\n0.002s on my machine. You also see this by the fact that doing a single\nstat(3P) call as introduced by my patch adds a 1% performance penalty\nalready, and with the map cache we still have to do this single stat(3P)\ncall in addition to the dynamic memory allocations for the map and\ninsertion of the hook.\n\n> I think that this just seems a little contrived to me. I can understand\n> why server administrators may want this feature, but the general\n> user-base of Git doesn't seem to stand to benefit much from this change\n> (in my own mind, at least).\n\nThat's true for several hooks we have, though.\n\n> So... I'm not sure where this leaves us. Maybe a 2.0% slow-down on an\n> already fast 'git update-ref' invocation for the average user won't be\n> noticeable. It certainly *will* be noticeable to administrators who\n> processes a much higher volume of such transactions.\n\nI think we should keep in mind that it's git-update-ref(1) we're talking\nabout, which is a nearly direct wrapper around reference transactions.\nThe 1% perfomance hit is thus the worst case that can happen, as there\nis no other tool that does as little work around the reftx as this one.\nFor any other tool, I imagine the performance hit to be at worst the\nsame (e.g. git-branch(1)) or not noticeable at all because a single\nstat(3P) call will be drowned out by other things (e.g. git-clone(1)).\n\nThat's not to say that nobody will be impacted by this change, I bet\nthere are setups that make heavy use of git-update-ref(1).\n\n> > For such a short-lived program like git-update-refs(1), one can see that\n> > the overhead of using a map negatively impacts performance compared to\n> > the no-cache case. But using the reftx-cache roughly cuts the overhead\n> > in half as expected, as we only need to look up the hook once instead of\n> > twice.\n> >\n> > And here's the results if we use a single `git update-ref --stdin` with a\n> > thousand reference updates at once:\n> >\n> > Test master ref-hook-no-cache ref-hook-map-cache ref-hook-reftx-cache\n> > ------------------------------------------------------------------------------------------------------------------------\n> > 1400.2: git update-ref --stdin 0.21(0.09+0.12) 0.21(0.07+0.14) +0.0% 0.21(0.07+0.13) +0.0% 0.21(0.07+0.13) +0.0%\n> >\n> > As expected, there's nothing much to be seen here because there's only a\n> > single transaction for all ref updates and, as a result, at most two\n> > calls to `access(refhook_path, X_OK)`.\n> \n> Better, but I have to imagine that real-world usage will look much more\n> like a thousand tiny transactions than one large one.\n\nMost likely, yes. Doesn't happen too often that one updates multiple\nreferences at once.\n\nPatrick\n"},{"id":"400052","messageId":"1de96b96e3448c8f7e7974f7c082fd08d2d14e96.1592475610.git.ps@pks.im","threadId":"53600","inReplyTo":"1d1a94426f95d842e0e3ea6a1569c0c45239229c.1591086316.git.ps@pks.im","subject":"[PATCH v3] refs: implement reference transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-06-18T10:27:43Z","receivedAt":"2020-06-18T10:27:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The low-level reference transactions used to update references are\ncurrently completely opaque to the user. While certainly desirable in\nmost usecases, there are some which might want to hook into the\ntransaction to observe all queued reference updates as well as observing\nthe abortion or commit of a prepared transaction.\n\nOne such usecase would be to have a set of replicas of a given Git\nrepository, where we perform Git operations on all of the repositories\nat once and expect the outcome to be the same in all of them. While\nthere exist hooks already for a certain subset of Git commands that\ncould be used to implement a voting mechanism for this, many others\ncurrently don't have any mechanism for this.\n\nThe above scenario is the motivation for the new \"reference-transaction\"\nhook that reaches directly into Git's reference transaction mechanism.\nThe hook receives as parameter the current state the transaction was\nmoved to (\"prepared\", \"committed\" or \"aborted\") and gets via its\nstandard input all queued reference updates. While the exit code gets\nignored in the \"committed\" and \"aborted\" states, a non-zero exit code in\nthe \"prepared\" state will cause the transaction to be aborted\nprematurely.\n\nGiven the usecase described above, a voting mechanism can now be\nimplemented via this hook: as soon as it gets called, it will take all\nof stdin and use it to cast a vote to a central service. When all\nreplicas of the repository agree, the hook will exit with zero,\notherwise it will abort the transaction by returning non-zero. The most\nimportant upside is that this will catch _all_ commands writing\nreferences at once, allowing to implement strong consistency for\nreference updates via a single mechanism.\n\nIn order to test the impact on the case where we don't have any\n\"reference-transaction\" hook installed in the repository, this commit\nintroduce two new performance tests for git-update-refs(1). Run against\nan empty repository, it produces the following results:\n\n Test origin/master HEAD\n --------------------------------------------------------------------\n 1400.2: update-ref 2.70(2.10+0.71) 2.71(2.10+0.73) +0.4%\n 1400.3: update-ref --stdin 0.21(0.09+0.11) 0.21(0.07+0.14) +0.0%\n\nThe performance test p1400.2 creates, updates and deletes a branch a\nthousand times, thus averaging runtime of git-update-refs over 3000\ninvocations. p1400.3 instead calls `git-update-refs --stdin` three times\nand queues a thousand creations, updates and deletes respectively.\n\nAs expected, p1400.3 consistently shows no noticeable impact, as for\neach batch of updates there's a single call to access(3P) for the\nnegative hook lookup. On the other hand, for p1400.2, one can see an\nimpact caused by this patchset. But doing five runs of the performance\ntests where each one was run with GIT_PERF_REPEAT_COUNT=10, the overhead\nranged from -1.5% to +1.1%. These inconsistent performance numbers can\nbe explained by the overhead of spawning 3000 processes. This shows that\nthe overhead of assembling the hook path and executing access(3P) once\nto check if it's there is mostly outweighed by the operating system's\noverhead.\n\nSigned-off-by: Patrick Steinhardt \n---\n\nThere's two changes compared to v2:\n\n 1. I've added lookup cache for the hook that both caches the\n positive as well as the negative lookup of this hook. It's\n specific to the reference-transaction hook, only, and will simply\n store the path to the found hook or a sentinel value in case it\n wasn't found. Like this, we avoid one of the two calls to\n access(3P) when executing git-update-ref(1). This improved\n performance into the range where variance between runs drowns out\n the impact of the single access(3P) call.\n\n 2. I've amend a second benchmark to p1400, where one tests single\n invocations of git-update-ref and the second one uses batched\n invocations via its --stdin switch. The latter doesn't show any\n impact, while the former one ranged from an overhead of -1.5% to\n +1.1%.\n\nI did have a look at integrating this work with Emily's work, but I\ndon't really think it necessary given that benchmarks show that the\noverhead of the access(3P) call is drowned out by the OS anyway. If you\nfeel differently, I may revisit it and perform some benchmarks on top of\nher work.\n\nPatrick\n\n Documentation/githooks.txt | 29 ++++++++\n refs.c | 79 +++++++++++++++++++++-\n t/perf/p1400-update-ref.sh | 32 +++++++++\n t/t1416-ref-transaction-hooks.sh | 109 +++++++++++++++++++++++++++++++\n 4 files changed, 247 insertions(+), 2 deletions(-)\n create mode 100755 t/perf/p1400-update-ref.sh\n create mode 100755 t/t1416-ref-transaction-hooks.sh\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 81f2a87e88..642471109f 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -404,6 +404,35 @@ Both standard output and standard error output are forwarded to\n `git send-pack` on the other end, so you can simply `echo` messages\n for the user.\n \n+ref-transaction\n+~~~~~~~~~~~~~~~\n+\n+This hook is invoked by any Git command that performs reference\n+updates. It executes whenever a reference transaction is prepared,\n+committed or aborted and may thus get called multiple times.\n+\n+The hook takes exactly one argument, which is the current state the\n+given reference transaction is in:\n+\n+ - \"prepared\": All reference updates have been queued to the\n+ transaction and references were locked on disk.\n+\n+ - \"committed\": The reference transaction was committed and all\n+ references now have their respective new value.\n+\n+ - \"aborted\": The reference transaction was aborted, no changes\n+ were performed and the locks have been released.\n+\n+For each reference update that was added to the transaction, the hook\n+receives on standard input a line of the format:\n+\n+ SP SP LF\n+\n+The exit status of the hook is ignored for any state except for the\n+\"prepared\" state. In the \"prepared\" state, a non-zero exit status will\n+cause the transaction to be aborted. The hook will not be called with\n+\"aborted\" state in that case.\n+\n push-to-checkout\n ~~~~~~~~~~~~~~~~\n \ndiff --git a/refs.c b/refs.c\nindex 224ff66c7b..c4961f4ad9 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -9,6 +9,7 @@\n #include \"iterator.h\"\n #include \"refs.h\"\n #include \"refs/refs-internal.h\"\n+#include \"run-command.h\"\n #include \"object-store.h\"\n #include \"object.h\"\n #include \"tag.h\"\n@@ -16,6 +17,7 @@\n #include \"worktree.h\"\n #include \"argv-array.h\"\n #include \"repository.h\"\n+#include \"sigchain.h\"\n \n /*\n * List of all available backends\n@@ -1986,10 +1988,68 @@ int ref_update_reject_duplicates(struct string_list *refnames,\n \treturn 0;\n }\n \n+static const char hook_not_found;\n+static const char *hook;\n+\n+static int run_transaction_hook(struct ref_transaction *transaction,\n+\t\t\t\tconst char *state)\n+{\n+\tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint saved_errno = 0, ret, i;\n+\n+\tif (hook == &hook_not_found)\n+\t\treturn 0;\n+\tif (!hook)\n+\t\thook = find_hook(\"reference-transaction\");\n+\tif (!hook) {\n+\t\thook = &hook_not_found;\n+\t\treturn 0;\n+\t}\n+\n+\targv_array_pushl(&proc.args, hook, state, NULL);\n+\tproc.in = -1;\n+\tproc.stdout_to_stderr = 1;\n+\tproc.trace2_hook_name = \"reference-transaction\";\n+\n+\tret = start_command(&proc);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addf(&buf, \"%s %s %s\\n\",\n+\t\t\t oid_to_hex(&update->old_oid),\n+\t\t\t oid_to_hex(&update->new_oid),\n+\t\t\t update->refname);\n+\n+\t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0) {\n+\t\t\tif (errno != EPIPE)\n+\t\t\t\tsaved_errno = errno;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tclose(proc.in);\n+\tsigchain_pop(SIGPIPE);\n+\tstrbuf_release(&buf);\n+\n+\tret = finish_command(&proc);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\treturn saved_errno;\n+}\n+\n int ref_transaction_prepare(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_store *refs = transaction->ref_store;\n+\tint ret;\n \n \tswitch (transaction->state) {\n \tcase REF_TRANSACTION_OPEN:\n@@ -2012,7 +2072,17 @@ int ref_transaction_prepare(struct ref_transaction *transaction,\n \t\treturn -1;\n \t}\n \n-\treturn refs->be->transaction_prepare(refs, transaction, err);\n+\tret = refs->be->transaction_prepare(refs, transaction, err);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\tret = run_transaction_hook(transaction, \"prepared\");\n+\tif (ret) {\n+\t\tref_transaction_abort(transaction, err);\n+\t\tdie(_(\"ref updates aborted by hook\"));\n+\t}\n+\n+\treturn 0;\n }\n \n int ref_transaction_abort(struct ref_transaction *transaction,\n@@ -2036,6 +2106,8 @@ int ref_transaction_abort(struct ref_transaction *transaction,\n \t\tbreak;\n \t}\n \n+\trun_transaction_hook(transaction, \"aborted\");\n+\n \tref_transaction_free(transaction);\n \treturn ret;\n }\n@@ -2064,7 +2136,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tbreak;\n \t}\n \n-\treturn refs->be->transaction_finish(refs, transaction, err);\n+\tret = refs->be->transaction_finish(refs, transaction, err);\n+\tif (!ret)\n+\t\trun_transaction_hook(transaction, \"committed\");\n+\treturn ret;\n }\n \n int refs_verify_refname_available(struct ref_store *refs,\ndiff --git a/t/perf/p1400-update-ref.sh b/t/perf/p1400-update-ref.sh\nnew file mode 100755\nindex 0000000000..d275a81248\n--- /dev/null\n+++ b/t/perf/p1400-update-ref.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description=\"Tests performance of update-ref\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_fresh_repo\n+\n+test_expect_success \"setup\" '\n+\ttest_commit PRE &&\n+\ttest_commit POST &&\n+\tprintf \"create refs/heads/%d PRE\\n\" $(test_seq 1000) >create &&\n+\tprintf \"update refs/heads/%d POST PRE\\n\" $(test_seq 1000) >update &&\n+\tprintf \"delete refs/heads/%d POST\\n\" $(test_seq 1000) >delete\n+'\n+\n+test_perf \"update-ref\" '\n+\tfor i in $(test_seq 1000)\n+\tdo\n+\t\tgit update-ref refs/heads/branch PRE &&\n+\t\tgit update-ref refs/heads/branch POST PRE &&\n+\t\tgit update-ref -d refs/heads/branch\n+\tdone\n+'\n+\n+test_perf \"update-ref --stdin\" '\n+\tgit update-ref --stdin >actual\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\tcommitted\n+\tEOF\n+\tgit update-ref HEAD POST &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'hook aborts updating ref in prepared state' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\tif test \"$1\" = prepared\n+\t\tthen\n+\t\t\texit 1\n+\t\tfi\n+\tEOF\n+\ttest_must_fail git update-ref HEAD POST 2>err &&\n+\ttest_i18ngrep \"ref updates aborted by hook\" err\n+'\n+\n+test_expect_success 'hook gets all queued updates in prepared state' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction actual\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\tif test \"$1\" = prepared\n+\t\tthen\n+\t\t\twhile read -r line\n+\t\t\tdo\n+\t\t\t\tprintf \"%s\\n\" \"$line\"\n+\t\t\tdone >actual\n+\t\tfi\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\t$ZERO_OID $POST_OID HEAD\n+\t\t$ZERO_OID $POST_OID refs/heads/master\n+\tEOF\n+\tgit update-ref HEAD POST <<-EOF &&\n+\t\tupdate HEAD $ZERO_OID $POST_OID\n+\t\tupdate refs/heads/master $ZERO_OID $POST_OID\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'hook gets all queued updates in committed state' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction actual\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\tif test \"$1\" = committed\n+\t\tthen\n+\t\t\twhile read -r line\n+\t\t\tdo\n+\t\t\t\tprintf \"%s\\n\" \"$line\"\n+\t\t\tdone >actual\n+\t\tfi\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\t$ZERO_OID $POST_OID HEAD\n+\t\t$ZERO_OID $POST_OID refs/heads/master\n+\tEOF\n+\tgit update-ref HEAD POST &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'hook gets all queued updates in aborted state' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction actual\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\tif test \"$1\" = aborted\n+\t\tthen\n+\t\t\twhile read -r line\n+\t\t\tdo\n+\t\t\t\tprintf \"%s\\n\" \"$line\"\n+\t\t\tdone >actual\n+\t\tfi\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\t$ZERO_OID $POST_OID HEAD\n+\t\t$ZERO_OID $POST_OID refs/heads/master\n+\tEOF\n+\tgit update-ref --stdin <<-EOF &&\n+\t\tstart\n+\t\tupdate HEAD POST $ZERO_OID\n+\t\tupdate refs/heads/master POST $ZERO_OID\n+\t\tabort\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.27.0\n\n"},{"id":"400094","messageId":"xmqqo8pgko8k.fsf@gitster.c.googlers.com","threadId":"53600","inReplyTo":"1de96b96e3448c8f7e7974f7c082fd08d2d14e96.1592475610.git.ps@pks.im","subject":"Re: [PATCH v3] refs: implement reference transaction hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-18T22:23:07Z","receivedAt":"2020-06-18T22:23:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> +static const char hook_not_found;\n> +static const char *hook;\n\n;-) Nice.\n\n> +static int run_transaction_hook(struct ref_transaction *transaction,\n> +\t\t\t\tconst char *state)\n> +{\n> +\tstruct child_process proc = CHILD_PROCESS_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tint saved_errno = 0, ret, i;\n> +...\n> +\tret = start_command(&proc);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\n> +\tfor (i = 0; i < transaction->nr; i++) {\n> +\t\tstruct ref_update *update = transaction->updates[i];\n> + ...\n> +\t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0) {\n> +\t\t\tif (errno != EPIPE)\n> +\t\t\t\tsaved_errno = errno;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tclose(proc.in);\n> +\tsigchain_pop(SIGPIPE);\n> +\tstrbuf_release(&buf);\n> +\n> +\tret = finish_command(&proc);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn saved_errno;\n> +}\n\nOK, the only thing that looked a bit tricky was the \"saved_errno\"\nthat is used in an unusual (relative to its name) way. The use\nisn't incorrect per-se, but it rubs readers' expectation the wrong\nway to use the variable named saved_errno for any purpose other than\nthe established pattern:\n\n\tsaved_errno = errno;\n\tif (some_libcall_that_may_update_errno()) {\n\t\t... deal with an error and perform\n\t\t... some fallback action\n\t}\n\terrno = saved_errno;\n\nthat allows the caller to be oblivious to the library call that is\nmade as a mere implementation detail whose failure does not matter\nto the caller.\n\nIn any case, the idea of the code in the patch is to make sure we\nremember the fact that we failed to write (or caught any other\nerror, if we added more calls in the future) in a variable, and make\nsure we return an error even if we manage to cleanly call\n\"finish_command()\". For that purpose, in order to avoid overwriting\nthe \"ret\" variable with the returned value from finish_command(), a\nseparate variable is needed, and \"saved_errno\" was picked for the\nname of the variable.\n\nBut I do not think it is a good idea to return the errno in one\ncodepath when the function can return an error status that is not an\nerrno that is received from start_command() and finish_command().\nThe caller of this function cannot (and probably do not want to)\ntell what the failed syscall was and would be checking if the return\nvalue was success (=0) or failure (<0).\n\nSo I'd rather simplify the error handling to\n\n - Remove \"saved_errno\"; instead initialize ret to 0 at the beginning;\n\n - Return \"ret\" even if we return hardcoded 0 in the earlier part\n for consistency;\n\n - Update \"ret\" in the loop to -1 (the same error return status that\n is returned by start_command() and finish_command()) when we\n found a write error that we are not ignoring before breaking out\n of the loop.\n\n - We need to call finish_command() even if we earlier saw an error\n once we successfully called start_command(). So do something\n like this:\n\n\tret |= finish_command(&proc);\n\treturn ret;\n\n to make sure we retain an earlier error in \"ret\", we\n unconditionally call finish_command() when the control reaches\n there, and we mark the result a failure when finish_command()\n fails.\n\nif I were writing this function.\n\nThanks.\n"},{"id":"400101","messageId":"55c58e9b09691dc11dea911f26424594fb3922c9.1592549570.git.ps@pks.im","threadId":"53600","inReplyTo":"1d1a94426f95d842e0e3ea6a1569c0c45239229c.1591086316.git.ps@pks.im","subject":"[PATCH v4] refs: implement reference transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-06-19T06:56:14Z","receivedAt":"2020-06-19T06:56:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The low-level reference transactions used to update references are\ncurrently completely opaque to the user. While certainly desirable in\nmost usecases, there are some which might want to hook into the\ntransaction to observe all queued reference updates as well as observing\nthe abortion or commit of a prepared transaction.\n\nOne such usecase would be to have a set of replicas of a given Git\nrepository, where we perform Git operations on all of the repositories\nat once and expect the outcome to be the same in all of them. While\nthere exist hooks already for a certain subset of Git commands that\ncould be used to implement a voting mechanism for this, many others\ncurrently don't have any mechanism for this.\n\nThe above scenario is the motivation for the new \"reference-transaction\"\nhook that reaches directly into Git's reference transaction mechanism.\nThe hook receives as parameter the current state the transaction was\nmoved to (\"prepared\", \"committed\" or \"aborted\") and gets via its\nstandard input all queued reference updates. While the exit code gets\nignored in the \"committed\" and \"aborted\" states, a non-zero exit code in\nthe \"prepared\" state will cause the transaction to be aborted\nprematurely.\n\nGiven the usecase described above, a voting mechanism can now be\nimplemented via this hook: as soon as it gets called, it will take all\nof stdin and use it to cast a vote to a central service. When all\nreplicas of the repository agree, the hook will exit with zero,\notherwise it will abort the transaction by returning non-zero. The most\nimportant upside is that this will catch _all_ commands writing\nreferences at once, allowing to implement strong consistency for\nreference updates via a single mechanism.\n\nIn order to test the impact on the case where we don't have any\n\"reference-transaction\" hook installed in the repository, this commit\nintroduce two new performance tests for git-update-refs(1). Run against\nan empty repository, it produces the following results:\n\n Test origin/master HEAD\n --------------------------------------------------------------------\n 1400.2: update-ref 2.70(2.10+0.71) 2.71(2.10+0.73) +0.4%\n 1400.3: update-ref --stdin 0.21(0.09+0.11) 0.21(0.07+0.14) +0.0%\n\nThe performance test p1400.2 creates, updates and deletes a branch a\nthousand times, thus averaging runtime of git-update-refs over 3000\ninvocations. p1400.3 instead calls `git-update-refs --stdin` three times\nand queues a thousand creations, updates and deletes respectively.\n\nAs expected, p1400.3 consistently shows no noticeable impact, as for\neach batch of updates there's a single call to access(3P) for the\nnegative hook lookup. On the other hand, for p1400.2, one can see an\nimpact caused by this patchset. But doing five runs of the performance\ntests where each one was run with GIT_PERF_REPEAT_COUNT=10, the overhead\nranged from -1.5% to +1.1%. These inconsistent performance numbers can\nbe explained by the overhead of spawning 3000 processes. This shows that\nthe overhead of assembling the hook path and executing access(3P) once\nto check if it's there is mostly outweighed by the operating system's\noverhead.\n\nSigned-off-by: Patrick Steinhardt \n---\n\nThe only change was simplified error handling as proposed by Junio.\nThanks for your feedback, the result definitely looks easier to read to\nme.\n\nRange-diff against v3:\n1: 1de96b96e3 ! 1: 55c58e9b09 refs: implement reference transaction hook\n @@ refs.c: int ref_update_reject_duplicates(struct string_list *refnames,\n +{\n +\tstruct child_process proc = CHILD_PROCESS_INIT;\n +\tstruct strbuf buf = STRBUF_INIT;\n -+\tint saved_errno = 0, ret, i;\n ++\tint ret = 0, i;\n +\n +\tif (hook == &hook_not_found)\n -+\t\treturn 0;\n ++\t\treturn ret;\n +\tif (!hook)\n +\t\thook = find_hook(\"reference-transaction\");\n +\tif (!hook) {\n +\t\thook = &hook_not_found;\n -+\t\treturn 0;\n ++\t\treturn ret;\n +\t}\n +\n +\targv_array_pushl(&proc.args, hook, state, NULL);\n @@ refs.c: int ref_update_reject_duplicates(struct string_list *refnames,\n +\n +\t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0) {\n +\t\t\tif (errno != EPIPE)\n -+\t\t\t\tsaved_errno = errno;\n ++\t\t\t\tret = -1;\n +\t\t\tbreak;\n +\t\t}\n +\t}\n @@ refs.c: int ref_update_reject_duplicates(struct string_list *refnames,\n +\tsigchain_pop(SIGPIPE);\n +\tstrbuf_release(&buf);\n +\n -+\tret = finish_command(&proc);\n -+\tif (ret)\n -+\t\treturn ret;\n -+\n -+\treturn saved_errno;\n ++\tret |= finish_command(&proc);\n ++\treturn ret;\n +}\n +\n int ref_transaction_prepare(struct ref_transaction *transaction,\n\n Documentation/githooks.txt | 29 ++++++++\n refs.c | 76 ++++++++++++++++++++-\n t/perf/p1400-update-ref.sh | 32 +++++++++\n t/t1416-ref-transaction-hooks.sh | 109 +++++++++++++++++++++++++++++++\n 4 files changed, 244 insertions(+), 2 deletions(-)\n create mode 100755 t/perf/p1400-update-ref.sh\n create mode 100755 t/t1416-ref-transaction-hooks.sh\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 81f2a87e88..642471109f 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -404,6 +404,35 @@ Both standard output and standard error output are forwarded to\n `git send-pack` on the other end, so you can simply `echo` messages\n for the user.\n \n+ref-transaction\n+~~~~~~~~~~~~~~~\n+\n+This hook is invoked by any Git command that performs reference\n+updates. It executes whenever a reference transaction is prepared,\n+committed or aborted and may thus get called multiple times.\n+\n+The hook takes exactly one argument, which is the current state the\n+given reference transaction is in:\n+\n+ - \"prepared\": All reference updates have been queued to the\n+ transaction and references were locked on disk.\n+\n+ - \"committed\": The reference transaction was committed and all\n+ references now have their respective new value.\n+\n+ - \"aborted\": The reference transaction was aborted, no changes\n+ were performed and the locks have been released.\n+\n+For each reference update that was added to the transaction, the hook\n+receives on standard input a line of the format:\n+\n+ SP SP LF\n+\n+The exit status of the hook is ignored for any state except for the\n+\"prepared\" state. In the \"prepared\" state, a non-zero exit status will\n+cause the transaction to be aborted. The hook will not be called with\n+\"aborted\" state in that case.\n+\n push-to-checkout\n ~~~~~~~~~~~~~~~~\n \ndiff --git a/refs.c b/refs.c\nindex 224ff66c7b..f05295f503 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -9,6 +9,7 @@\n #include \"iterator.h\"\n #include \"refs.h\"\n #include \"refs/refs-internal.h\"\n+#include \"run-command.h\"\n #include \"object-store.h\"\n #include \"object.h\"\n #include \"tag.h\"\n@@ -16,6 +17,7 @@\n #include \"worktree.h\"\n #include \"argv-array.h\"\n #include \"repository.h\"\n+#include \"sigchain.h\"\n \n /*\n * List of all available backends\n@@ -1986,10 +1988,65 @@ int ref_update_reject_duplicates(struct string_list *refnames,\n \treturn 0;\n }\n \n+static const char hook_not_found;\n+static const char *hook;\n+\n+static int run_transaction_hook(struct ref_transaction *transaction,\n+\t\t\t\tconst char *state)\n+{\n+\tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint ret = 0, i;\n+\n+\tif (hook == &hook_not_found)\n+\t\treturn ret;\n+\tif (!hook)\n+\t\thook = find_hook(\"reference-transaction\");\n+\tif (!hook) {\n+\t\thook = &hook_not_found;\n+\t\treturn ret;\n+\t}\n+\n+\targv_array_pushl(&proc.args, hook, state, NULL);\n+\tproc.in = -1;\n+\tproc.stdout_to_stderr = 1;\n+\tproc.trace2_hook_name = \"reference-transaction\";\n+\n+\tret = start_command(&proc);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addf(&buf, \"%s %s %s\\n\",\n+\t\t\t oid_to_hex(&update->old_oid),\n+\t\t\t oid_to_hex(&update->new_oid),\n+\t\t\t update->refname);\n+\n+\t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0) {\n+\t\t\tif (errno != EPIPE)\n+\t\t\t\tret = -1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tclose(proc.in);\n+\tsigchain_pop(SIGPIPE);\n+\tstrbuf_release(&buf);\n+\n+\tret |= finish_command(&proc);\n+\treturn ret;\n+}\n+\n int ref_transaction_prepare(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_store *refs = transaction->ref_store;\n+\tint ret;\n \n \tswitch (transaction->state) {\n \tcase REF_TRANSACTION_OPEN:\n@@ -2012,7 +2069,17 @@ int ref_transaction_prepare(struct ref_transaction *transaction,\n \t\treturn -1;\n \t}\n \n-\treturn refs->be->transaction_prepare(refs, transaction, err);\n+\tret = refs->be->transaction_prepare(refs, transaction, err);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\tret = run_transaction_hook(transaction, \"prepared\");\n+\tif (ret) {\n+\t\tref_transaction_abort(transaction, err);\n+\t\tdie(_(\"ref updates aborted by hook\"));\n+\t}\n+\n+\treturn 0;\n }\n \n int ref_transaction_abort(struct ref_transaction *transaction,\n@@ -2036,6 +2103,8 @@ int ref_transaction_abort(struct ref_transaction *transaction,\n \t\tbreak;\n \t}\n \n+\trun_transaction_hook(transaction, \"aborted\");\n+\n \tref_transaction_free(transaction);\n \treturn ret;\n }\n@@ -2064,7 +2133,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tbreak;\n \t}\n \n-\treturn refs->be->transaction_finish(refs, transaction, err);\n+\tret = refs->be->transaction_finish(refs, transaction, err);\n+\tif (!ret)\n+\t\trun_transaction_hook(transaction, \"committed\");\n+\treturn ret;\n }\n \n int refs_verify_refname_available(struct ref_store *refs,\ndiff --git a/t/perf/p1400-update-ref.sh b/t/perf/p1400-update-ref.sh\nnew file mode 100755\nindex 0000000000..d275a81248\n--- /dev/null\n+++ b/t/perf/p1400-update-ref.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description=\"Tests performance of update-ref\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_fresh_repo\n+\n+test_expect_success \"setup\" '\n+\ttest_commit PRE &&\n+\ttest_commit POST &&\n+\tprintf \"create refs/heads/%d PRE\\n\" $(test_seq 1000) >create &&\n+\tprintf \"update refs/heads/%d POST PRE\\n\" $(test_seq 1000) >update &&\n+\tprintf \"delete refs/heads/%d POST\\n\" $(test_seq 1000) >delete\n+'\n+\n+test_perf \"update-ref\" '\n+\tfor i in $(test_seq 1000)\n+\tdo\n+\t\tgit update-ref refs/heads/branch PRE &&\n+\t\tgit update-ref refs/heads/branch POST PRE &&\n+\t\tgit update-ref -d refs/heads/branch\n+\tdone\n+'\n+\n+test_perf \"update-ref --stdin\" '\n+\tgit update-ref --stdin >actual\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\tprepared\n+\t\tcommitted\n+\tEOF\n+\tgit update-ref HEAD POST &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'hook aborts updating ref in prepared state' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\tif test \"$1\" = prepared\n+\t\tthen\n+\t\t\texit 1\n+\t\tfi\n+\tEOF\n+\ttest_must_fail git update-ref HEAD POST 2>err &&\n+\ttest_i18ngrep \"ref updates aborted by hook\" err\n+'\n+\n+test_expect_success 'hook gets all queued updates in prepared state' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction actual\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\tif test \"$1\" = prepared\n+\t\tthen\n+\t\t\twhile read -r line\n+\t\t\tdo\n+\t\t\t\tprintf \"%s\\n\" \"$line\"\n+\t\t\tdone >actual\n+\t\tfi\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\t$ZERO_OID $POST_OID HEAD\n+\t\t$ZERO_OID $POST_OID refs/heads/master\n+\tEOF\n+\tgit update-ref HEAD POST <<-EOF &&\n+\t\tupdate HEAD $ZERO_OID $POST_OID\n+\t\tupdate refs/heads/master $ZERO_OID $POST_OID\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'hook gets all queued updates in committed state' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction actual\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\tif test \"$1\" = committed\n+\t\tthen\n+\t\t\twhile read -r line\n+\t\t\tdo\n+\t\t\t\tprintf \"%s\\n\" \"$line\"\n+\t\t\tdone >actual\n+\t\tfi\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\t$ZERO_OID $POST_OID HEAD\n+\t\t$ZERO_OID $POST_OID refs/heads/master\n+\tEOF\n+\tgit update-ref HEAD POST &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'hook gets all queued updates in aborted state' '\n+\ttest_when_finished \"rm .git/hooks/reference-transaction actual\" &&\n+\tgit reset --hard PRE &&\n+\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n+\t\tif test \"$1\" = aborted\n+\t\tthen\n+\t\t\twhile read -r line\n+\t\t\tdo\n+\t\t\t\tprintf \"%s\\n\" \"$line\"\n+\t\t\tdone >actual\n+\t\tfi\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\t$ZERO_OID $POST_OID HEAD\n+\t\t$ZERO_OID $POST_OID refs/heads/master\n+\tEOF\n+\tgit update-ref --stdin <<-EOF &&\n+\t\tstart\n+\t\tupdate HEAD POST $ZERO_OID\n+\t\tupdate refs/heads/master POST $ZERO_OID\n+\t\tabort\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.27.0\n\n"},{"id":"408196","messageId":"20201023010315.GA1542721@coredump.intra.peff.net","threadId":"53600","inReplyTo":"55c58e9b09691dc11dea911f26424594fb3922c9.1592549570.git.ps@pks.im","subject":"Re: [PATCH v4] refs: implement reference transaction hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-23T01:03:15Z","receivedAt":"2020-10-23T01:03:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 19, 2020 at 08:56:14AM +0200, Patrick Steinhardt wrote:\n\n> The above scenario is the motivation for the new \"reference-transaction\"\n> hook that reaches directly into Git's reference transaction mechanism.\n> The hook receives as parameter the current state the transaction was\n> moved to (\"prepared\", \"committed\" or \"aborted\") and gets via its\n> standard input all queued reference updates. While the exit code gets\n> ignored in the \"committed\" and \"aborted\" states, a non-zero exit code in\n> the \"prepared\" state will cause the transaction to be aborted\n> prematurely.\n\nI know this has been merged for a while, but Taylor and I were looking\nat it today and came across a question. The docs say:\n\n> +For each reference update that was added to the transaction, the hook\n> +receives on standard input a line of the format:\n> +\n> + SP SP LF\n\nbut that doesn't define . I take it to mean the current value\nof the ref before the proposed update. But in the tests:\n\n> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> new file mode 100755\n> index 0000000000..da58d867a5\n> --- /dev/null\n> +++ b/t/t1416-ref-transaction-hooks.sh\n> [...]\n> +test_expect_success 'hook gets all queued updates in prepared state' '\n> +\ttest_when_finished \"rm .git/hooks/reference-transaction actual\" &&\n> +\tgit reset --hard PRE &&\n> +\twrite_script .git/hooks/reference-transaction <<-\\EOF &&\n> +\t\tif test \"$1\" = prepared\n> +\t\tthen\n> +\t\t\twhile read -r line\n> +\t\t\tdo\n> +\t\t\t\tprintf \"%s\\n\" \"$line\"\n> +\t\t\tdone >actual\n> +\t\tfi\n> +\tEOF\n> +\tcat >expect <<-EOF &&\n> +\t\t$ZERO_OID $POST_OID HEAD\n> +\t\t$ZERO_OID $POST_OID refs/heads/master\n> +\tEOF\n\nWe are expecting to see $ZERO_OID in that slot, even though the current\nvalue of the ref is \"PRE\" due to our reset above.\n\nShould this be $PRE_OID (we don't have that variable, but it would be\nthe result of \"git rev-parse PRE\")?\n\nI could alternatively see an argument that is the old-value\nthat the caller asked for. So seeing $ZERO_OID is saying that the caller\nwants to move from _anything_ to $POST_OID, and we're not willing to\ntell the hook what the current value actually is.\n\nWe could actually fill in the current value for zero cost. The reason we\nfound this is that we have a custom patch at GitHub that fills in these\nvalues when we open the ref after locking.\n\nIn real usage, I'm not sure how much the distinction would matter,\nbecause any careful caller would provide a non-zero \"old\" value. And if\nthat doesn't match the current value, we'd reject the transaction before\nwe even hit the hook, I think. It's only the fact that the update-ref\ncalls are sloppy and do not provide an expected old value that it even\nmatters.\n\nSo I wonder if:\n\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex f6e741c6c0..8155522a1a 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -9,6 +9,7 @@ test_expect_success setup '\n \ttest_commit PRE &&\n \tPRE_OID=$(git rev-parse PRE) &&\n \ttest_commit POST &&\n+\tPRE_OID=$(git rev-parse PRE) &&\n \tPOST_OID=$(git rev-parse POST)\n '\n \n@@ -52,10 +53,10 @@ test_expect_success 'hook gets all queued updates in prepared state' '\n \t\tfi\n \tEOF\n \tcat >expect <<-EOF &&\n-\t\t$ZERO_OID $POST_OID HEAD\n-\t\t$ZERO_OID $POST_OID refs/heads/master\n+\t\t$PRE_OID $POST_OID HEAD\n+\t\t$PRE_OID $POST_OID refs/heads/master\n \tEOF\n-\tgit update-ref HEAD POST <<-EOF &&\n+\tgit update-ref HEAD POST POST <<-EOF &&\n \t\tupdate HEAD $ZERO_OID $POST_OID\n \t\tupdate refs/heads/master $ZERO_OID $POST_OID\n \tEOF\n@@ -75,10 +76,10 @@ test_expect_success 'hook gets all queued updates in committed state' '\n \t\tfi\n \tEOF\n \tcat >expect <<-EOF &&\n-\t\t$ZERO_OID $POST_OID HEAD\n-\t\t$ZERO_OID $POST_OID refs/heads/master\n+\t\t$PRE_OID $POST_OID HEAD\n+\t\t$PRE_OID $POST_OID refs/heads/master\n \tEOF\n-\tgit update-ref HEAD POST &&\n+\tgit update-ref HEAD POST PRE &&\n \ttest_cmp expect actual\n '\n \n\nwould be a step forward. This isn't changing the actual behavior,\nobviously. It's just tweaking the test so that it tests the more likely\nreal-world case. But we'd possibly want to actually change the behavior\nto always send the actual ref value to the hook.\n\n-Peff\n"},{"id":"408200","messageId":"xmqqpn59k2xw.fsf@gitster.c.googlers.com","threadId":"53600","inReplyTo":"20201023010315.GA1542721@coredump.intra.peff.net","subject":"Re: [PATCH v4] refs: implement reference transaction hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-23T03:59:23Z","receivedAt":"2020-10-23T03:59:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I wonder if:\n>\n> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> index f6e741c6c0..8155522a1a 100755\n> --- a/t/t1416-ref-transaction-hooks.sh\n> +++ b/t/t1416-ref-transaction-hooks.sh\n> @@ -9,6 +9,7 @@ test_expect_success setup '\n> \ttest_commit PRE &&\n> \tPRE_OID=$(git rev-parse PRE) &&\n> \ttest_commit POST &&\n> +\tPRE_OID=$(git rev-parse PRE) &&\n> \tPOST_OID=$(git rev-parse POST)\n> '\n> \n> @@ -52,10 +53,10 @@ test_expect_success 'hook gets all queued updates in prepared state' '\n> \t\tfi\n> \tEOF\n> \tcat >expect <<-EOF &&\n> -\t\t$ZERO_OID $POST_OID HEAD\n> -\t\t$ZERO_OID $POST_OID refs/heads/master\n> +\t\t$PRE_OID $POST_OID HEAD\n> +\t\t$PRE_OID $POST_OID refs/heads/master\n> \tEOF\n> -\tgit update-ref HEAD POST <<-EOF &&\n> +\tgit update-ref HEAD POST POST <<-EOF &&\n> \t\tupdate HEAD $ZERO_OID $POST_OID\n> \t\tupdate refs/heads/master $ZERO_OID $POST_OID\n> \tEOF\n> @@ -75,10 +76,10 @@ test_expect_success 'hook gets all queued updates in committed state' '\n> \t\tfi\n> \tEOF\n> \tcat >expect <<-EOF &&\n> -\t\t$ZERO_OID $POST_OID HEAD\n> -\t\t$ZERO_OID $POST_OID refs/heads/master\n> +\t\t$PRE_OID $POST_OID HEAD\n> +\t\t$PRE_OID $POST_OID refs/heads/master\n> \tEOF\n> -\tgit update-ref HEAD POST &&\n> +\tgit update-ref HEAD POST PRE &&\n> \ttest_cmp expect actual\n> '\n\nI think that makes a lot of sense. In addition, \n\n> ... But we'd possibly want to actually change the behavior\n> to always send the actual ref value to the hook.\n\nI offhand do not see a reason why we shouldn't do that.\n\nThanks for carefully thinking things through.\n\n\n"},{"id":"408278","messageId":"X5M1oe4lfkUy9lAh@nand.local","threadId":"53600","inReplyTo":"xmqqpn59k2xw.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4] refs: implement reference transaction hook","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-23T19:57:21Z","receivedAt":"2020-10-23T19:57:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 22, 2020 at 08:59:23PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> [...]\n> I think that makes a lot of sense. In addition,\n>\n> > ... But we'd possibly want to actually change the behavior\n> > to always send the actual ref value to the hook.\n>\n> I offhand do not see a reason why we shouldn't do that.\n\nI can't see a reason either, but teaching the new ref transaction hook\nabout these updates gets a little tricky. In particular, we update the\nold_oid for ref updates that were split off with something like:\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 04e85e7002..9c105a376b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2484,9 +2484,20 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t parent_update = parent_update->parent_update) {\n \t\t\tstruct ref_lock *parent_lock = parent_update->backend_data;\n \t\t\toidcpy(&parent_lock->old_oid, &lock->old_oid);\n+\t\t\t/*\n+\t\t\t * Now the parent knows its old OID too;\n+\t\t\t * record that fact for logging.\n+\t\t\t */\n+\t\t\tparent_update->flags |= REF_HAVE_OLD;\n \t\t}\n \t}\n\n+\t/* Now we know the old value. Record that fact for logging. */\n+\tif (!(update->flags & REF_HAVE_OLD)) {\n+\t\toidcpy(&update->old_oid, &lock->old_oid);\n+\t\tupdate->flags |= REF_HAVE_OLD;\n+\t}\n+\n \tif ((update->flags & REF_HAVE_NEW) &&\n \t !(update->flags & REF_DELETING) &&\n \t !(update->flags & REF_LOG_ONLY)) {\n\n...but by that time we have already recorded via an oidcpy the old and\nnew state of HEAD. So, Peff's patch passes in isolation, but applying\nthis on top produces failures in t1416 like:\n\n--- expect\t2020-10-23 19:56:15.649101024 +0000\n+++ actual\t2020-10-23 19:56:15.657100941 +0000\n@@ -1,2 +1,2 @@\n-63ac8e7bcdb882293465435909f54a96de17d4f7 99d53161c3a0a903b6561b9f6c0c665b3a476401 HEAD\n+0000000000000000000000000000000000000000 99d53161c3a0a903b6561b9f6c0c665b3a476401 HEAD\n 63ac8e7bcdb882293465435909f54a96de17d4f7 99d53161c3a0a903b6561b9f6c0c665b3a476401 refs/heads/master\n\nIt should be possible to keep track of the old and new OIDs via a\nnon-copied pointer, but I have to untangle this code a little bit more\nbefore I can be sure.\n\n> Thanks for carefully thinking things through.\n\nThanks,\nTaylor\n"},{"id":"408279","messageId":"X5M2Yd7cxVRN1nlA@nand.local","threadId":"53600","inReplyTo":"20201023010315.GA1542721@coredump.intra.peff.net","subject":"Re: [PATCH v4] refs: implement reference transaction hook","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-23T20:00:33Z","receivedAt":"2020-10-23T20:00:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 22, 2020 at 09:03:15PM -0400, Jeff King wrote:\n> We are expecting to see $ZERO_OID in that slot, even though the current\n> value of the ref is \"PRE\" due to our reset above.\n>\n> Should this be $PRE_OID (we don't have that variable, but it would be\n> the result of \"git rev-parse PRE\")?\n\nI also skimmed past it, but we do already have $PRE_OID, which is\nconvenient.\n\n> I could alternatively see an argument that is the old-value\n> that the caller asked for. So seeing $ZERO_OID is saying that the caller\n> wants to move from _anything_ to $POST_OID, and we're not willing to\n> tell the hook what the current value actually is.\n>\n> We could actually fill in the current value for zero cost. The reason we\n> found this is that we have a custom patch at GitHub that fills in these\n> values when we open the ref after locking.\n\nYup, modulo being easy for symrefs (which I talk about in [1]), but it\nshouldn't be impossible.\n\n[1]: https://lore.kernel.org/git/X5M1oe4lfkUy9lAh@nand.local\n\n> In real usage, I'm not sure how much the distinction would matter,\n> because any careful caller would provide a non-zero \"old\" value. And if\n> that doesn't match the current value, we'd reject the transaction before\n> we even hit the hook, I think. It's only the fact that the update-ref\n> calls are sloppy and do not provide an expected old value that it even\n> matters.\n>\n> So I wonder if:\n>\n> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> index f6e741c6c0..8155522a1a 100755\n> --- a/t/t1416-ref-transaction-hooks.sh\n> +++ b/t/t1416-ref-transaction-hooks.sh\n> @@ -9,6 +9,7 @@ test_expect_success setup '\n> \ttest_commit PRE &&\n> \tPRE_OID=$(git rev-parse PRE) &&\n> \ttest_commit POST &&\n> +\tPRE_OID=$(git rev-parse PRE) &&\n> \tPOST_OID=$(git rev-parse POST)\n> '\n>\n> @@ -52,10 +53,10 @@ test_expect_success 'hook gets all queued updates in prepared state' '\n> \t\tfi\n> \tEOF\n> \tcat >expect <<-EOF &&\n> -\t\t$ZERO_OID $POST_OID HEAD\n> -\t\t$ZERO_OID $POST_OID refs/heads/master\n> +\t\t$PRE_OID $POST_OID HEAD\n> +\t\t$PRE_OID $POST_OID refs/heads/master\n> \tEOF\n> -\tgit update-ref HEAD POST <<-EOF &&\n> +\tgit update-ref HEAD POST POST <<-EOF &&\n\nThis should be \"git update-ref HEAD POST PRE\", since PRE is the before\nstate.\n\n> would be a step forward. This isn't changing the actual behavior,\n> obviously. It's just tweaking the test so that it tests the more likely\n> real-world case. But we'd possibly want to actually change the behavior\n> to always send the actual ref value to the hook.\n\nI have to look at the issue in [1] a little bit more to determine\nwhether or not it requires major surgery. If it does, then I'd be fine\ngoing forward with just your patch to the tests. If it doesn't, then\nupdating the refs machinery to invoke the hook with the before OIDs\ncorrectly even for symrefs seems sensible to me.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"408286","messageId":"X5NUEtS0S30iZY0A@nand.local","threadId":"53600","inReplyTo":"X5M1oe4lfkUy9lAh@nand.local","subject":"Re: [PATCH v4] refs: implement reference transaction hook","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-23T22:07:14Z","receivedAt":"2020-10-23T22:07:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 23, 2020 at 03:57:21PM -0400, Taylor Blau wrote:\n> It should be possible to keep track of the old and new OIDs via a\n> non-copied pointer, but I have to untangle this code a little bit more\n> before I can be sure.\n\nThis may be both easier and harder than I was imagining ;-). In the\neasier direction, the following patch is sufficient to get the tests\npassing again:\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 04e85e7002..744c93b7ff 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2311,12 +2311,11 @@ static int split_symref_update(struct ref_update *update,\n \tnew_update->parent_update = update;\n\n \t/*\n-\t * Change the symbolic ref update to log only. Also, it\n-\t * doesn't need to check its old OID value, as that will be\n-\t * done when new_update is processed.\n+\t * Change the symbolic ref update to log only. Though we don't\n+\t * need to check its old OID value, leave REF_HAVE_OLD alone so\n+\t * we can propagate it to the ref transaction hook.\n \t */\n \tupdate->flags |= REF_LOG_ONLY | REF_NO_DEREF;\n-\tupdate->flags &= ~REF_HAVE_OLD;\n\n \t/*\n \t * Add the referent. This insertion is O(N) in the transaction\n\nBut, it also means that we're now needlessly re-verifying the before\nstate of symrefs (or so I think, I haven't yet proved it one way or the\nother).\n\nSo, I need to look into that before deciding if this is a good direction\nto go.\n\nThanks,\nTaylor\n"},{"id":"408382","messageId":"20201026074303.GA972@ncase","threadId":"53600","inReplyTo":"xmqqpn59k2xw.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4] refs: implement reference transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-10-26T07:43:03Z","receivedAt":"2020-10-26T07:43:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 22, 2020 at 08:59:23PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > So I wonder if:\n> >\n> > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> > index f6e741c6c0..8155522a1a 100755\n> > --- a/t/t1416-ref-transaction-hooks.sh\n> > +++ b/t/t1416-ref-transaction-hooks.sh\n> > @@ -9,6 +9,7 @@ test_expect_success setup '\n> > \ttest_commit PRE &&\n> > \tPRE_OID=$(git rev-parse PRE) &&\n> > \ttest_commit POST &&\n> > +\tPRE_OID=$(git rev-parse PRE) &&\n> > \tPOST_OID=$(git rev-parse POST)\n> > '\n> > \n> > @@ -52,10 +53,10 @@ test_expect_success 'hook gets all queued updates in prepared state' '\n> > \t\tfi\n> > \tEOF\n> > \tcat >expect <<-EOF &&\n> > -\t\t$ZERO_OID $POST_OID HEAD\n> > -\t\t$ZERO_OID $POST_OID refs/heads/master\n> > +\t\t$PRE_OID $POST_OID HEAD\n> > +\t\t$PRE_OID $POST_OID refs/heads/master\n> > \tEOF\n> > -\tgit update-ref HEAD POST <<-EOF &&\n> > +\tgit update-ref HEAD POST POST <<-EOF &&\n> > \t\tupdate HEAD $ZERO_OID $POST_OID\n> > \t\tupdate refs/heads/master $ZERO_OID $POST_OID\n> > \tEOF\n> > @@ -75,10 +76,10 @@ test_expect_success 'hook gets all queued updates in committed state' '\n> > \t\tfi\n> > \tEOF\n> > \tcat >expect <<-EOF &&\n> > -\t\t$ZERO_OID $POST_OID HEAD\n> > -\t\t$ZERO_OID $POST_OID refs/heads/master\n> > +\t\t$PRE_OID $POST_OID HEAD\n> > +\t\t$PRE_OID $POST_OID refs/heads/master\n> > \tEOF\n> > -\tgit update-ref HEAD POST &&\n> > +\tgit update-ref HEAD POST PRE &&\n> > \ttest_cmp expect actual\n> > '\n> \n> I think that makes a lot of sense. In addition, \n> \n> > ... But we'd possibly want to actually change the behavior\n> > to always send the actual ref value to the hook.\n> \n> I offhand do not see a reason why we shouldn't do that.\n> \n> Thanks for carefully thinking things through.\n\nThanks for having a look! I agree, changing this seems sensible to me.\nIn the end, the intention of the hook is to have a single script which\nis able to verify all reference updates, no matter where they come from.\nAnd behaving differently based on whether the user passed the zero OID\nor not doesn't seem useful to me in that context.\n\nWe should also a better job than I did and describe what the old OID\nactually is in the documentation.\n\n@Taylor, given that you've already dug into the code: do you already\nhave plans to post a patch for this?\n\nPatrick\n"},{"id":"408470","messageId":"X5dhN+dsLXlKfukF@nand.local","threadId":"53600","inReplyTo":"20201026074303.GA972@ncase","subject":"Re: [PATCH v4] refs: implement reference transaction hook","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-26T23:52:23Z","receivedAt":"2020-10-26T23:52:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 26, 2020 at 08:43:03AM +0100, Patrick Steinhardt wrote:\n> @Taylor, given that you've already dug into the code: do you already\n> have plans to post a patch for this?\n\nYou are likely in a better position to do that than I am. I am\nunfamiliar enough with the refs.c code to feel confident that my change\nis correct, let alone working. The combination of REF_HAVE_OLD, the lock\nOID, the update OID, and so on is very puzzling.\n\nOrdinarily, I'd be happy to post a patch after familiarizing myself, but\nright now I don't have the time. So, I may come back to this in six\nmonths, but I certainly won't feel bad if you beat me to it ;-).\n\nIn the meantime, I'd be fine to apply Peff's patch with some fix-ups,\nmaybe something like what's below the scissors line.\n\nTaylor\n\n--- >8 ---\n\nSubject: [PATCH] t1416: specify pre-state for ref-updates\n\nThe ref-transaction hook documentation says that the expected format for\neach line is:\n\n SP SP LF\n\nwithout defining what is. It could either be the current\nstate of the refs (after locking, but before committing the\ntransaction), or the optional caller-provided pre-state.\n\nIf is to mean the caller-provided pre-state, than $ZERO_OID\ncould be taken to mean that the update is allowed to take place without\nrequiring the ref to be at some state. On the other hand, if \nis taken to mean \"the current value of the reference\", then that\nrequires a behavior change.\n\nBut that may only be semi-realistic, since any careful callers are\nlikely to pass a pre-state around anyway, and failing to meet that\npre-state means the hook will not even be invoked.\n\nSo, tweak the tests to more closely match how callers will actually\ninvoke this hook by providing a pre-state explicitly and then asserting\nthat it made its way down to the ref-transaction hook.\n\nIf we do decide to go further and implement a behavior change, it would\nmake sense to modify the tests to instead look something like:\n\n for before in \"$PRE\" \"\"\n do\n cat >expect <<-EOF &&\n $ZERO_OID $POST_OID HEAD\n $ZERO_OID $POST_OID refs/heads/master\n $PRE_OID $POST_OID HEAD\n $PRE_OID $POST_OID refs/heads/master\n EOF\n git reset --hard $PRE &&\n git update-ref HEAD POST $before &&\n test_cmp expect actual\n done\n\nCo-authored-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n t/t1416-ref-transaction-hooks.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex f6e741c6c0..74f94e293c 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -52,10 +52,10 @@ test_expect_success 'hook gets all queued updates in prepared state' '\n \t\tfi\n \tEOF\n \tcat >expect <<-EOF &&\n-\t\t$ZERO_OID $POST_OID HEAD\n-\t\t$ZERO_OID $POST_OID refs/heads/master\n+\t\t$PRE_OID $POST_OID HEAD\n+\t\t$PRE_OID $POST_OID refs/heads/master\n \tEOF\n-\tgit update-ref HEAD POST <<-EOF &&\n+\tgit update-ref HEAD POST PRE <<-EOF &&\n \t\tupdate HEAD $ZERO_OID $POST_OID\n \t\tupdate refs/heads/master $ZERO_OID $POST_OID\n \tEOF\n@@ -75,10 +75,10 @@ test_expect_success 'hook gets all queued updates in committed state' '\n \t\tfi\n \tEOF\n \tcat >expect <<-EOF &&\n-\t\t$ZERO_OID $POST_OID HEAD\n-\t\t$ZERO_OID $POST_OID refs/heads/master\n+\t\t$PRE_OID $POST_OID HEAD\n+\t\t$PRE_OID $POST_OID refs/heads/master\n \tEOF\n-\tgit update-ref HEAD POST &&\n+\tgit update-ref HEAD POST PRE &&\n \ttest_cmp expect actual\n '\n\n--\n2.29.1.9.g605042ee00\n\n"},{"id":"408483","messageId":"20201027053753.GA3005508@coredump.intra.peff.net","threadId":"53600","inReplyTo":"X5dhN+dsLXlKfukF@nand.local","subject":"Re: [PATCH v4] refs: implement reference transaction hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-27T05:37:53Z","receivedAt":"2020-10-27T05:37:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 26, 2020 at 07:52:23PM -0400, Taylor Blau wrote:\n\n> On Mon, Oct 26, 2020 at 08:43:03AM +0100, Patrick Steinhardt wrote:\n> > @Taylor, given that you've already dug into the code: do you already\n> > have plans to post a patch for this?\n> \n> You are likely in a better position to do that than I am. I am\n> unfamiliar enough with the refs.c code to feel confident that my change\n> is correct, let alone working. The combination of REF_HAVE_OLD, the lock\n> OID, the update OID, and so on is very puzzling.\n> \n> Ordinarily, I'd be happy to post a patch after familiarizing myself, but\n> right now I don't have the time. So, I may come back to this in six\n> months, but I certainly won't feel bad if you beat me to it ;-).\n> \n> In the meantime, I'd be fine to apply Peff's patch with some fix-ups,\n> maybe something like what's below the scissors line.\n\nThanks for moving this forward. I'm definitely OK with leaving the code\nfor now and just tightening the test. But there is one curiosity, still.\nYour patch tweaks two tests:\n\n> @@ -52,10 +52,10 @@ test_expect_success 'hook gets all queued updates in prepared state' '\n> [...]\n> @@ -75,10 +75,10 @@ test_expect_success 'hook gets all queued updates in committed state' '\n\nbut there's a third one, which checks the hook's view of the \"aborted\"\nstate. And there we _do_ pass in an expected state which is not\n$PRE_OID. But it's $ZERO_OID, so we can't tell if if the hook is getting\nwhat we passed in, or some sensible value.\n\nIf we instead do this:\n\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex 74f94e293c..a7ed983d3c 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -94,14 +94,15 @@ test_expect_success 'hook gets all queued updates in aborted state' '\n \t\t\tdone >actual\n \t\tfi\n \tEOF\n+\tnonsense_oid=$(echo $ZERO_OID | tr 0 a) &&\n \tcat >expect <<-EOF &&\n-\t\t$ZERO_OID $POST_OID HEAD\n-\t\t$ZERO_OID $POST_OID refs/heads/master\n+\t\t$nonsense_oid $POST_OID HEAD\n+\t\t$nonsense_oid $POST_OID refs/heads/master\n \tEOF\n \tgit update-ref --stdin <<-EOF &&\n \t\tstart\n-\t\tupdate HEAD POST $ZERO_OID\n-\t\tupdate refs/heads/master POST $ZERO_OID\n+\t\tupdate HEAD POST $nonsense_oid\n+\t\tupdate refs/heads/master POST $nonsense_oid\n \t\tabort\n \tEOF\n \ttest_cmp expect actual\n\nThen test still passes, because it's passing along the value we\nprovided. And I think that's the only sensible thing we can do: show the\nhook the proposed update. There's little point in telling it what the\nactual ref values were, I would think.\n\nSo I think it's worth adding in to the test, but:\n\n - probably that meaning of old-value needs to be discussed in more\n detail in the hook documentation, because it feels like it differs a\n bit in the \"aborted\" case versus the \"committed\" case\n\n - we'd want to make sure this keeps passing if further changes to the\n code are made to support the case without an expected state\n specified (and not, say, accidentally passing $PRE_OID to the hook)\n\n - we'd likewise want eventually a test for the case without an\n expected state (which I guess would actually pass $ZERO_OID to the\n hook). Of course we're using a mismatch of the expected value as the\n reason to abort, so we'd have to find another reliable way to make\n the transaction abort. :) Perhaps a refname with illegal characters,\n or trying to create \"foo/bar\" when \"foo\" exists (or vice versa).\n\nMost of that can be bumped to later, but I think it's worth squashing\nsomething like the hunk above into the patch you posted.\n\n-Peff\n"},{"id":"408557","messageId":"20201028182216.GB807@tanuki","threadId":"53600","inReplyTo":"X5dhN+dsLXlKfukF@nand.local","subject":"Re: [PATCH v4] refs: implement reference transaction hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-10-28T18:22:16Z","receivedAt":"2020-10-28T21:51:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 26, 2020 at 07:52:23PM -0400, Taylor Blau wrote:\n> On Mon, Oct 26, 2020 at 08:43:03AM +0100, Patrick Steinhardt wrote:\n> > @Taylor, given that you've already dug into the code: do you already\n> > have plans to post a patch for this?\n> \n> You are likely in a better position to do that than I am. I am\n> unfamiliar enough with the refs.c code to feel confident that my change\n> is correct, let alone working. The combination of REF_HAVE_OLD, the lock\n> OID, the update OID, and so on is very puzzling.\n> \n> Ordinarily, I'd be happy to post a patch after familiarizing myself, but\n> right now I don't have the time. So, I may come back to this in six\n> months, but I certainly won't feel bad if you beat me to it ;-).\n> \n> In the meantime, I'd be fine to apply Peff's patch with some fix-ups,\n> maybe something like what's below the scissors line.\n\nFair enough, let's do it like this and submit the test change first.\nI'll try to squeeze in doing the hook change soonish, but I'm currently\nlacking time myself. So no promise I'll get to it soonish,\nunfortunately.\n\nPatrick\n"}]}