{"thread":{"id":"55946","subject":"[RFC PATCH 0/2] MVP implementation of remote-suggested hooks","startedAt":"2021-06-16T23:31:55Z","lastAt":"2021-07-27T22:40:27Z","messageCount":36,"participants":["Jonathan Tan","Junio C Hamano","Emily Shaffer","Randall S. Becker","Matt Rogers","Ævar Arnfjörð Bjarmason","brian m. carlson","Phil Hord"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"427640","messageId":"cover.1623881977.git.jonathantanmy@google.com","threadId":"55946","inReplyTo":null,"subject":"[RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-16T23:31:47Z","receivedAt":"2021-06-16T23:31:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is a continuation of the work from [1]. That work described the\nreasons, possible features, and possible workflows, but not the\nimplementation in detail. This patch set has an MVP implementation, and\nmy hope is that having a concrete implementation to look at makes it\neasier to discuss matters of implementation.\n\nThis but does not use any features from es/config-based-hooks but is\nimplemented on that branch anyway because firstly, I need an existing\ncommand to attach the \"autoupdate\" subcommand (and \"git hook\" works),\nand secondly, when we test this at $DAYJOB, we will be testing it\ntogether with the aforementioned branch.\n\nI have had to make several design choices (which I will discuss later),\nbut now with this implementation, the following workflow is possible:\n\n 1. The remote repo administrator creates a new branch\n \"refs/heads/suggested-hooks\" pointing to a commit that has all the\n hooks that the administrator wants to suggest. The hooks are\n directly referenced by the commit tree (i.e. they are in the \"/\"\n directory).\n\n 2. When a user clones, Git notices that\n \"refs/remotes/origin/suggested-hooks\" is present and prints out a\n message about a command that can be run.\n\n 3. If the user runs that command, Git will install the hooks pointed to\n by that ref, and set hook.autoupdate to true. This config variable\n is checked whenever \"git fetch\" is run: whenever it notices that\n \"refs/remotes/origin/suggested-hooks\" changes, it will reinstall the\n hooks.\n\n 4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks\n will remain.\n\nDesign choices:\n\n 1. Where should the suggested hooks be in the remote repo? A branch,\n a non-branch ref, a config? I think that a branch is best - it is\n relatively well-understood and any hooks there can be\n version-controlled (and its history is independent of the other\n branches).\n\n 2. When and how should the local repo update its record of the remote's\n suggested hooks? If we go with storing the hooks in a branch of a\n remote side, this would automatically mean (with the default\n refspec) that it would be in refs/remotes//. This\n incurs network and hard disk cost even if the local repo does not\n want to use the suggested hooks, but I think that typically they\n would want to use it if they're going to do any work on the repo\n (they would either have to trust or inspect Makefiles etc. anyway,\n so they can do the same for the hooks), and even if they don't want\n to use the remote's hooks, they probably still want to know what the\n remote suggests.\n\n So using a branch provides a well-understood way of storing the\n hooks on the remote, transmitting it to the local repo, and storing\n the hooks in the local repo.\n\n So: what should be the default name of this branch? Presumably, \"git\n clone\" would need to be able to override this.\n\n 3. How should the local repo detect when the remote has updated its\n suggested hooks? I'm thinking when a certain local ref is updated.\n Right now it's hardcoded, but perhaps \"git clone\" could detect what\n \"refs/heads/suggested-hooks\" would correspond to, and then set it in\n a config accordingly. Other options include remembering what the\n remote's \"refs/heads/suggested-hooks\" used to be and always\n comparing it upon every \"ls-refs\" call, but I think that the local\n ref method is more straightforward.\n\n 4. Should the local repo try to notice if the hooks have been changed\n locally before overwriting upon autoupdate? This would be nice to\n have, but I don't know how practical it would be. In particular, I\n don't know if we can trust that\n \"refs/remotes/origin/suggested-hooks\" has not been clobbered.\n\n 5. Should we have a command that manually updates the hooks with what's\n in \"refs/heads/suggested-hooks\"? This is not in this patch set, but\n it sounds like a good idea.\n\nThere are perhaps other points that I haven't thought of, of course.\n\n[1] https://lore.kernel.org/git/pull.908.git.1616105016055.gitgitgadget@gmail.com/\n\nJonathan Tan (2):\n hook: move list of hooks\n clone,fetch: remote-suggested auto-updating hooks\n\n builtin/bugreport.c | 38 ++------------\n builtin/clone.c | 10 ++++\n builtin/fetch.c | 21 ++++++++\n builtin/hook.c | 13 +++--\n hook.c | 118 ++++++++++++++++++++++++++++++++++++++++++++\n hook.h | 5 ++\n t/t5601-clone.sh | 36 ++++++++++++++\n 7 files changed, 202 insertions(+), 39 deletions(-)\n\n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427641","messageId":"74ed668e9bbf56bb7898bcdfaaf77db4f3205fe5.1623881977.git.jonathantanmy@google.com","threadId":"55946","inReplyTo":"cover.1623881977.git.jonathantanmy@google.com","subject":"[RFC PATCH 1/2] hook: move list of hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-16T23:31:48Z","receivedAt":"2021-06-16T23:31:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The list of hooks will need to be used outside bugreport, so move it to\na central location.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/bugreport.c | 38 +++-----------------------------------\n hook.c | 34 ++++++++++++++++++++++++++++++++++\n hook.h | 3 +++\n 3 files changed, 40 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 190272ba70..4e0806dff3 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -41,38 +41,6 @@ static void get_system_info(struct strbuf *sys_info)\n \n static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n {\n-\t/*\n-\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n-\t * so below is a transcription of `git help hooks`. Later, this should\n-\t * be replaced with some programmatically generated list (generated from\n-\t * doc or else taken from some library which tells us about all the\n-\t * hooks)\n-\t */\n-\tstatic const char *hook[] = {\n-\t\t\"applypatch-msg\",\n-\t\t\"pre-applypatch\",\n-\t\t\"post-applypatch\",\n-\t\t\"pre-commit\",\n-\t\t\"pre-merge-commit\",\n-\t\t\"prepare-commit-msg\",\n-\t\t\"commit-msg\",\n-\t\t\"post-commit\",\n-\t\t\"pre-rebase\",\n-\t\t\"post-checkout\",\n-\t\t\"post-merge\",\n-\t\t\"pre-push\",\n-\t\t\"pre-receive\",\n-\t\t\"update\",\n-\t\t\"post-receive\",\n-\t\t\"post-update\",\n-\t\t\"push-to-checkout\",\n-\t\t\"pre-auto-gc\",\n-\t\t\"post-rewrite\",\n-\t\t\"sendemail-validate\",\n-\t\t\"fsmonitor-watchman\",\n-\t\t\"p4-pre-submit\",\n-\t\t\"post-index-change\",\n-\t};\n \tint i;\n \n \tif (nongit) {\n@@ -81,9 +49,9 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n \t\treturn;\n \t}\n \n-\tfor (i = 0; i < ARRAY_SIZE(hook); i++)\n-\t\tif (hook_exists(hook[i], HOOKDIR_USE_CONFIG))\n-\t\t\tstrbuf_addf(hook_info, \"%s\\n\", hook[i]);\n+\tfor (i = 0; i < hook_name_count; i++)\n+\t\tif (hook_exists(hook_name[i], HOOKDIR_USE_CONFIG))\n+\t\t\tstrbuf_addf(hook_info, \"%s\\n\", hook_name[i]);\n }\n \n static const char * const bugreport_usage[] = {\ndiff --git a/hook.c b/hook.c\nindex ff80e52edd..3ccacb72fa 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -5,6 +5,40 @@\n #include \"run-command.h\"\n #include \"prompt.h\"\n \n+/*\n+ * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n+ * so below is a transcription of `git help hooks`. Later, this should\n+ * be replaced with some programmatically generated list (generated from\n+ * doc or else taken from some library which tells us about all the\n+ * hooks)\n+ */\n+const char *hook_name[] = {\n+\t\"applypatch-msg\",\n+\t\"pre-applypatch\",\n+\t\"post-applypatch\",\n+\t\"pre-commit\",\n+\t\"pre-merge-commit\",\n+\t\"prepare-commit-msg\",\n+\t\"commit-msg\",\n+\t\"post-commit\",\n+\t\"pre-rebase\",\n+\t\"post-checkout\",\n+\t\"post-merge\",\n+\t\"pre-push\",\n+\t\"pre-receive\",\n+\t\"update\",\n+\t\"post-receive\",\n+\t\"post-update\",\n+\t\"push-to-checkout\",\n+\t\"pre-auto-gc\",\n+\t\"post-rewrite\",\n+\t\"sendemail-validate\",\n+\t\"fsmonitor-watchman\",\n+\t\"p4-pre-submit\",\n+\t\"post-index-change\",\n+};\n+int hook_name_count = ARRAY_SIZE(hook_name);\n+\n void free_hook(struct hook *ptr)\n {\n \tif (ptr) {\ndiff --git a/hook.h b/hook.h\nindex f32189380a..d902166408 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -4,6 +4,9 @@\n #include \"strvec.h\"\n #include \"run-command.h\"\n \n+extern const char *hook_name[];\n+extern int hook_name_count;\n+\n struct hook {\n \tstruct list_head list;\n \t/*\n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427642","messageId":"0f03e956c183d827c186b6fb85b92bfd31d2dc47.1623881977.git.jonathantanmy@google.com","threadId":"55946","inReplyTo":"cover.1623881977.git.jonathantanmy@google.com","subject":"[RFC PATCH 2/2] clone,fetch: remote-suggested auto-updating hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-16T23:31:49Z","receivedAt":"2021-06-16T23:32:01Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Upon cloning, if a ref refs/remotes/origin/suggested-hooks is present,\nGit will inform the user that they can run a command to install those\nhooks and keep them auto-updated upon eacn fetch. If that command is\nrun, hooks will be installed (overriding existing ones) and every fetch\nwill check if that ref would be updated, and if yes, reinstall the hooks\nfrom the new commit that the ref points to.\n\nNEEDSWORK: Write a more detailed commit message once the design is\nfinalized.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/clone.c | 10 ++++++\n builtin/fetch.c | 21 ++++++++++++\n builtin/hook.c | 13 +++++---\n hook.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++\n hook.h | 2 ++\n t/t5601-clone.sh | 36 +++++++++++++++++++++\n 6 files changed, 162 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 2a2a03bf76..0ab4494bcd 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1393,6 +1393,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t branch_top.buf, reflog_msg.buf, transport,\n \t\t\t !is_local);\n \n+\tfor (ref = mapped_refs; ref; ref = ref->next) {\n+\t\tif (ref->peer_ref &&\n+\t\t !strcmp(ref->peer_ref->name,\n+\t\t\t \"refs/remotes/origin/suggested-hooks\")) {\n+\t\t\tfprintf(stderr, _(\"The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\\n\"\n+\t\t\t\t\t \"Run `git hook autoupdate` to download them, install them, and turn on automatic update of hooks.\\n\"));\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n \n \t/*\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 769af53ca4..0618d1f091 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -28,6 +28,7 @@\n #include \"promisor-remote.h\"\n #include \"commit-graph.h\"\n #include \"shallow.h\"\n+#include \"hook.h\"\n \n #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n \n@@ -84,6 +85,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n static int fetch_write_commit_graph = -1;\n static int stdin_refspecs = 0;\n static int negotiate_only;\n+static int hook_autoupdate;\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n {\n@@ -123,6 +125,11 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(k, \"hook.autoupdate\")) {\n+\t\thook_autoupdate = git_config_bool(k, v);\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(k, v, cb);\n }\n \n@@ -1313,6 +1320,20 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)\n \t\t\t\t ref_map);\n \ttransport_unlock_pack(transport);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n+\n+\tif (hook_autoupdate) {\n+\t\tstruct ref *ref;\n+\n+\t\tfor (ref = ref_map; ref; ref = ref->next) {\n+\t\t\tif (ref->peer_ref &&\n+\t\t\t !strcmp(ref->peer_ref->name,\n+\t\t\t\t \"refs/remotes/origin/suggested-hooks\")) {\n+\t\t\t\thook_update_suggested();\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \treturn ret;\n }\n \ndiff --git a/builtin/hook.c b/builtin/hook.c\nindex c79a961e80..26d657a48a 100644\n--- a/builtin/hook.c\n+++ b/builtin/hook.c\n@@ -139,6 +139,13 @@ static int run(int argc, const char **argv, const char *prefix)\n \treturn rc;\n }\n \n+static int autoupdate(int argc, const char **argv, const char *prefix)\n+{\n+\tgit_config_set(\"hook.autoupdate\", \"true\");\n+\thook_update_suggested();\n+\treturn 0;\n+}\n+\n int cmd_hook(int argc, const char **argv, const char *prefix)\n {\n \tconst char *run_hookdir = NULL;\n@@ -152,10 +159,6 @@ int cmd_hook(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_hook_options,\n \t\t\t builtin_hook_usage, PARSE_OPT_KEEP_UNKNOWN);\n \n-\t/* after the parse, we should have \" \" */\n-\tif (argc < 2)\n-\t\tusage_with_options(builtin_hook_usage, builtin_hook_options);\n-\n \tgit_config(git_default_config, NULL);\n \n \n@@ -185,6 +188,8 @@ int cmd_hook(int argc, const char **argv, const char *prefix)\n \t\treturn list(argc, argv, prefix);\n \tif (!strcmp(argv[0], \"run\"))\n \t\treturn run(argc, argv, prefix);\n+\tif (!strcmp(argv[0], \"autoupdate\"))\n+\t\treturn autoupdate(argc, argv, prefix);\n \n \tusage_with_options(builtin_hook_usage, builtin_hook_options);\n }\ndiff --git a/hook.c b/hook.c\nindex 3ccacb72fa..5730e6e501 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -4,6 +4,12 @@\n #include \"config.h\"\n #include \"run-command.h\"\n #include \"prompt.h\"\n+#include \"commit.h\"\n+#include \"object.h\"\n+#include \"refs.h\"\n+#include \"tree-walk.h\"\n+#include \"tree.h\"\n+#include \"streaming.h\"\n \n /*\n * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n@@ -515,3 +521,81 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options)\n \n \treturn cb_data.rc;\n }\n+\n+static int is_hook(const char *filename)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < hook_name_count; i++) {\n+\t\tif (!strcmp(filename, hook_name[i]))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+void hook_update_suggested(void)\n+{\n+\tstruct object_id oid;\n+\tstruct object *obj;\n+\tstruct tree *tree;\n+\tstruct tree_desc desc;\n+\tstruct name_entry entry;\n+\tstruct strbuf path = STRBUF_INIT;\n+\n+\tif (read_ref(\"refs/remotes/origin/suggested-hooks\", &oid)) {\n+\t\twarning(_(\"no such ref refs/remotes/origin/suggested-hooks, not updating hooks\"));\n+\t\treturn;\n+\t}\n+\n+\tobj = parse_object(the_repository, &oid);\n+\tif (obj == NULL) {\n+\t\twarning(_(\"object pointed to by refs/remotes/origin/suggested-hooks '%s' does not exist\"),\n+\t\t\toid_to_hex(&oid));\n+\t\treturn;\n+\t}\n+\tif (obj->type != OBJ_COMMIT) {\n+\t\twarning(_(\"object pointed to by refs/remotes/origin/suggested-hooks '%s' is not a commit\"),\n+\t\t\toid_to_hex(&oid));\n+\t\treturn;\n+\t}\n+\n+\ttree = get_commit_tree((struct commit *) obj);\n+\tif (parse_tree(tree)) {\n+\t\twarning(_(\"could not parse tree\"));\n+\t\treturn;\n+\t}\n+\n+\tinit_tree_desc(&desc, tree->buffer, tree->size);\n+\twhile (tree_entry(&desc, &entry)) {\n+\t\tint fd;\n+\n+\t\tif (!is_hook(entry.path)) {\n+\t\t\twarning(_(\"file '%s' is not a hook; ignoring\"),\n+\t\t\t\tentry.path);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (S_ISDIR(entry.mode) || S_ISGITLINK(entry.mode)) {\n+\t\t\twarning(_(\"file '%s' is not an ordinary file; ignoring\"),\n+\t\t\t\tentry.path);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_reset(&path);\n+\t\tstrbuf_git_path(&path, \"hooks/%s\", entry.path);\n+\t\tfd = open(path.buf, O_WRONLY | O_CREAT, 0755);\n+\t\tif (fd < 0) {\n+\t\t\twarning_errno(_(\"could not create file '%s'; skipping this hook\"),\n+\t\t\t\t path.buf);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (stream_blob_to_fd(fd, &entry.oid, NULL, 1)) {\n+\t\t\twarning(_(\"could not write to file '%s'; skipping this hook\"),\n+\t\t\t\tpath.buf);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tclose(fd);\n+\t}\n+\tstrbuf_release(&path);\n+\n+\treturn;\n+}\ndiff --git a/hook.h b/hook.h\nindex d902166408..b0bfd3719b 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -140,3 +140,5 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options);\n void free_hook(struct hook *ptr);\n /* Empties the list at 'head', calling 'free_hook()' on each entry */\n void clear_hook_list(struct list_head *head);\n+\n+void hook_update_suggested(void);\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex c0688467e7..21b96a4999 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -752,6 +752,42 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe\n \tgit clone --filter=blob:limit=0 \"file://$(pwd)/server\" client\n '\n \n+test_expect_success 'detect suggested hook branch during clone' '\n+\trm -rf server client &&\n+\n+\ttest_create_repo server &&\n+\techo \"echo foo\" >server/pre-commit &&\n+\tgit -C server add pre-commit &&\n+\ttest_commit -C server hook-and-not-hook &&\n+\tgit -C server checkout -b suggested-hooks &&\n+\tgit clone server client 2>err &&\n+\ttest_i18ngrep \"The remote has suggested hooks\" err\n+'\n+\n+test_expect_success 'autoupdate' '\n+\tgit -C client hook autoupdate &&\n+\n+\t# Check correct config written\n+\tgit -C client config --local hook.autoupdate >actual &&\n+\techo \"true\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\t# Check hook written\n+\tcat client/.git/hooks/pre-commit >actual &&\n+\techo \"echo foo\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\t# Install new hook and fetch it\n+\techo \"echo bar\" >server/pre-commit &&\n+\tgit -C server commit -am \"new hook\" &&\n+\tgit -C client fetch &&\n+\n+\t# Check new hook written\n+\tcat client/.git/hooks/pre-commit >actual &&\n+\techo \"echo bar\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427649","messageId":"xmqq35thnuqp.fsf@gitster.g","threadId":"55946","inReplyTo":"cover.1623881977.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-17T01:30:06Z","receivedAt":"2021-06-17T01:30:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> 1. The remote repo administrator creates a new branch\n> \"refs/heads/suggested-hooks\" pointing to a commit that has all the\n> hooks that the administrator wants to suggest. The hooks are\n> directly referenced by the commit tree (i.e. they are in the \"/\"\n> directory).\n\nwants to suggest? They simply suggest ;-)\n\n> 2. When a user clones, Git notices that\n> \"refs/remotes/origin/suggested-hooks\" is present and prints out a\n> message about a command that can be run.\n\nCan be run to install? Or can be run to first inspect? Or both?\n\n> 3. If the user runs that command, Git will install the hooks pointed to\n> by that ref, and set hook.autoupdate to true. This config variable\n> is checked whenever \"git fetch\" is run: whenever it notices that\n> \"refs/remotes/origin/suggested-hooks\" changes, it will reinstall the\n> hooks.\n>\n> 4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks\n> will remain.\n\nOK, so \"verify even if you implicitly trust\" is actively discouraged.\n\n> Design choices:\n>\n> 1. Where should the suggested hooks be in the remote repo? A branch,\n> a non-branch ref, a config? I think that a branch is best - it is\n> relatively well-understood and any hooks there can be\n> version-controlled (and its history is independent of the other\n> branches).\n\nAs people mentioned in the previous discussions, \"independent of the\nother branches\" has advantages and disadvantages. The most recent\nset of hooks may have some that would not work well with older\ncodebase, so care must be taken to ensure any hook works on across\nversions of the main codebase. Which may not be a huge downside,\nbut something users must be aware of.\n\n> 2. When and how should the local repo update its record of the remote's\n> suggested hooks? If we go with storing the hooks in a branch of a\n> remote side, this would automatically mean (with the default\n> refspec) that it would be in refs/remotes//. This\n> incurs network and hard disk cost even if the local repo does not\n> want to use the suggested hooks, but I think that typically they\n> would want to use it if they're going to do any work on the repo\n> (they would either have to trust or inspect Makefiles etc. anyway,\n> so they can do the same for the hooks), and even if they don't want\n> to use the remote's hooks, they probably still want to know what the\n> remote suggests.\n\nA way to see what changes are made to recommendation would be\nuseful, and a branch that mostly linearly progresses is a good way\nto give it to the users.\n\nOf course, that can be done with suggested hooks inline with the\nrest of the main codebase, too.\n\n> 4. Should the local repo try to notice if the hooks have been changed\n> locally before overwriting upon autoupdate? This would be nice to\n> have, but I don't know how practical it would be. In particular, I\n> don't know if we can trust that\n> \"refs/remotes/origin/suggested-hooks\" has not been clobbered.\n\nMeaning clobbered by malicious parties? Then the whole thing is a\nno-go from security point of view. Presumably you trust the main\ncontent enough to work on top of it, so as long as you can trust\nthat refs/remotes/origin/hooks to the same degree that you would\ntrust refs/remotes/origin/master, I do not think it is a problem.\n\nWhatever mechanism you use to materialize an updated version of the\nhooks can and should record which commit on the suggested-hooks\nbranch the .git/hooks/* file is based on. Then when the user wants\nto update to a different version of suggested-hooks (either because\nyou auto-detected, or the user issued a command to update), you have\n\n - The current version in .git/hooks/*\n\n - The old pristine version (you recorded the commit when you\n updated the .git/hooks/* copy the last time)\n\n - The new pristine version (you have a remote-tracking branch).\n\nand it is not a brain surgery to perform three-way merge to update\nthe first using the difference between the second and the third.\n\n> 5. Should we have a command that manually updates the hooks with what's\n> in \"refs/heads/suggested-hooks\"? This is not in this patch set, but\n> it sounds like a good idea.\n\nI wonder if having it bound as a submodule to a known location in\nthe main contents tree makes it easier to manage and more flexible.\nJust like you can update the working tree of a submodule to a\nversion that is bound to the superproject tree, or to a more recent\nversion of the \"branch\" in the submodule, you can support workflows\nthat allow suggested hooks to advance independent of the main\ncontents version and that uses a specific version of suggested hooks\ntied to the main contents.\n\n\n\n> There are perhaps other points that I haven't thought of, of course.\n>\n> [1] https://lore.kernel.org/git/pull.908.git.1616105016055.gitgitgadget@gmail.com/\n>\n> Jonathan Tan (2):\n> hook: move list of hooks\n> clone,fetch: remote-suggested auto-updating hooks\n>\n> builtin/bugreport.c | 38 ++------------\n> builtin/clone.c | 10 ++++\n> builtin/fetch.c | 21 ++++++++\n> builtin/hook.c | 13 +++--\n> hook.c | 118 ++++++++++++++++++++++++++++++++++++++++++++\n> hook.h | 5 ++\n> t/t5601-clone.sh | 36 ++++++++++++++\n> 7 files changed, 202 insertions(+), 39 deletions(-)\n"},{"id":"427904","messageId":"YM0IpOFH4Sy9wWaE@google.com","threadId":"55946","inReplyTo":"cover.1623881977.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-18T20:57:08Z","receivedAt":"2021-06-18T20:57:17Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jun 16, 2021 at 04:31:47PM -0700, Jonathan Tan wrote:\n> \n> I have had to make several design choices (which I will discuss later),\n> but now with this implementation, the following workflow is possible:\n> \n> 1. The remote repo administrator creates a new branch\n> \"refs/heads/suggested-hooks\" pointing to a commit that has all the\n> hooks that the administrator wants to suggest. The hooks are\n> directly referenced by the commit tree (i.e. they are in the \"/\"\n> directory).\n\nI don't really like that this is in the same namespace as branches users\ncould create themselves. Hm, I think for 'git maintenance' prefetching\nwe put those refs in some special namespace, right? Can we do something\nsimilar in this case? Would that prevent us from treating that ref like\na normal branch?\n\n> \n> 2. When a user clones, Git notices that\n> \"refs/remotes/origin/suggested-hooks\" is present and prints out a\n> message about a command that can be run.\n> \n> 3. If the user runs that command, Git will install the hooks pointed to\n> by that ref, and set hook.autoupdate to true. This config variable\n> is checked whenever \"git fetch\" is run: whenever it notices that\n> \"refs/remotes/origin/suggested-hooks\" changes, it will reinstall the\n> hooks.\n\nI think this is where most people will have a lot to say from security\nperspective. It will be important to make sure that:\n - \"git fetch\" only automatically updates these scripts when fetching\n from the same remote they were initially fetched from, over some\n secure protocol\n - hook.autoupdate setting maybe should not be default, or should at\n least be able to override during the install command. (One suggestion\n was to also set some experimental.suggestedHooks setting, or\n something, to turn on \"automatically set up autoupdate\" behavior?)\n - we should notify the user that their hooks were updated/reinstalled\n when that happens later on. (Maybe you did this, I didn't look at the\n diff quite yet)\n\n> 4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks\n> will remain.\n> \n> Design choices:\n> \n> 1. Where should the suggested hooks be in the remote repo? A branch,\n> a non-branch ref, a config? I think that a branch is best - it is\n> relatively well-understood and any hooks there can be\n> version-controlled (and its history is independent of the other\n> branches).\n\nI agree - a branch without any prior parent sounds ideal to me, that is,\nthe repo owner setting up the hooks can say 'git checkout --orphan\nsuggested-hooks'. I guess if they want the branch to have history\nshared with the rest of the project there's no reason not to do that,\neither.\n\nDo we want to provide helpful tooling for the administrator to create\nthis magic branch? At the very least I guess we want some documentation,\nbut I wonder if it's possible to make a helper (maybe a 'git hook'\nsubcommand) without being more prescriptive than Git project usually is.\n\n> 3. How should the local repo detect when the remote has updated its\n> suggested hooks? I'm thinking when a certain local ref is updated.\n> Right now it's hardcoded, but perhaps \"git clone\" could detect what\n> \"refs/heads/suggested-hooks\" would correspond to, and then set it in\n> a config accordingly. Other options include remembering what the\n> remote's \"refs/heads/suggested-hooks\" used to be and always\n> comparing it upon every \"ls-refs\" call, but I think that the local\n> ref method is more straightforward.\n\nHm, I like that idea as it's analogous to remote tracking branch + local\ntracking branch (which is a pretty common pattern for people to use).\n\n> \n> 4. Should the local repo try to notice if the hooks have been changed\n> locally before overwriting upon autoupdate? This would be nice to\n> have, but I don't know how practical it would be. In particular, I\n> don't know if we can trust that\n> \"refs/remotes/origin/suggested-hooks\" has not been clobbered.\n\nI think it would be nice to have, yeah. It seems important to notify the\nusers that they are executing something different from yesterday, so\nnoticing when we're making changes to the hook sounds useful.\n\n> \n> 5. Should we have a command that manually updates the hooks with what's\n> in \"refs/heads/suggested-hooks\"? This is not in this patch set, but\n> it sounds like a good idea.\n\nYeah, I think so - that lets a user say \"no, I don't need those hooks\n(or this update)\" at first, but later change their mind.\n\nOne thing that sounds useful to me, even at this RFC stage, is\ndocumentation showing what this feature looks like to use - for both\nthe administrator setting up the magic hook branch, and the developer\ncloning and using hooks. I think we want that in some Git manpage\neventually anyways, and it would help to see the workflow you're trying\nto achieve.\n\nThanks for sending - will review the diffs today too.\n\n - Emily\n\n> \n> There are perhaps other points that I haven't thought of, of course.\n> \n> [1] https://lore.kernel.org/git/pull.908.git.1616105016055.gitgitgadget@gmail.com/\n> \n> Jonathan Tan (2):\n> hook: move list of hooks\n> clone,fetch: remote-suggested auto-updating hooks\n> \n> builtin/bugreport.c | 38 ++------------\n> builtin/clone.c | 10 ++++\n> builtin/fetch.c | 21 ++++++++\n> builtin/hook.c | 13 +++--\n> hook.c | 118 ++++++++++++++++++++++++++++++++++++++++++++\n> hook.h | 5 ++\n> t/t5601-clone.sh | 36 ++++++++++++++\n> 7 files changed, 202 insertions(+), 39 deletions(-)\n> \n> -- \n> 2.32.0.272.g935e593368-goog\n> \n"},{"id":"427906","messageId":"YM0JOyxPGYmzynoc@google.com","threadId":"55946","inReplyTo":"74ed668e9bbf56bb7898bcdfaaf77db4f3205fe5.1623881977.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH 1/2] hook: move list of hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-18T20:59:39Z","receivedAt":"2021-06-18T20:59:49Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jun 16, 2021 at 04:31:48PM -0700, Jonathan Tan wrote:\n> \n> The list of hooks will need to be used outside bugreport, so move it to\n> a central location.\n\nHm, does this fight with\nhttps://lore.kernel.org/git/cover-0.3-0000000000-20210617T100239Z-avarab@gmail.com\n?\n\nThis patch itself looks pretty straightforward, but I think it is a good\nidea to take a look at avarab's change first.\n\n - Emily\n\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/bugreport.c | 38 +++-----------------------------------\n> hook.c | 34 ++++++++++++++++++++++++++++++++++\n> hook.h | 3 +++\n> 3 files changed, 40 insertions(+), 35 deletions(-)\n> \n> diff --git a/builtin/bugreport.c b/builtin/bugreport.c\n> index 190272ba70..4e0806dff3 100644\n> --- a/builtin/bugreport.c\n> +++ b/builtin/bugreport.c\n> @@ -41,38 +41,6 @@ static void get_system_info(struct strbuf *sys_info)\n> \n> static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n> {\n> -\t/*\n> -\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n> -\t * so below is a transcription of `git help hooks`. Later, this should\n> -\t * be replaced with some programmatically generated list (generated from\n> -\t * doc or else taken from some library which tells us about all the\n> -\t * hooks)\n> -\t */\n> -\tstatic const char *hook[] = {\n> -\t\t\"applypatch-msg\",\n> -\t\t\"pre-applypatch\",\n> -\t\t\"post-applypatch\",\n> -\t\t\"pre-commit\",\n> -\t\t\"pre-merge-commit\",\n> -\t\t\"prepare-commit-msg\",\n> -\t\t\"commit-msg\",\n> -\t\t\"post-commit\",\n> -\t\t\"pre-rebase\",\n> -\t\t\"post-checkout\",\n> -\t\t\"post-merge\",\n> -\t\t\"pre-push\",\n> -\t\t\"pre-receive\",\n> -\t\t\"update\",\n> -\t\t\"post-receive\",\n> -\t\t\"post-update\",\n> -\t\t\"push-to-checkout\",\n> -\t\t\"pre-auto-gc\",\n> -\t\t\"post-rewrite\",\n> -\t\t\"sendemail-validate\",\n> -\t\t\"fsmonitor-watchman\",\n> -\t\t\"p4-pre-submit\",\n> -\t\t\"post-index-change\",\n> -\t};\n> \tint i;\n> \n> \tif (nongit) {\n> @@ -81,9 +49,9 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n> \t\treturn;\n> \t}\n> \n> -\tfor (i = 0; i < ARRAY_SIZE(hook); i++)\n> -\t\tif (hook_exists(hook[i], HOOKDIR_USE_CONFIG))\n> -\t\t\tstrbuf_addf(hook_info, \"%s\\n\", hook[i]);\n> +\tfor (i = 0; i < hook_name_count; i++)\n> +\t\tif (hook_exists(hook_name[i], HOOKDIR_USE_CONFIG))\n> +\t\t\tstrbuf_addf(hook_info, \"%s\\n\", hook_name[i]);\n> }\n> \n> static const char * const bugreport_usage[] = {\n> diff --git a/hook.c b/hook.c\n> index ff80e52edd..3ccacb72fa 100644\n> --- a/hook.c\n> +++ b/hook.c\n> @@ -5,6 +5,40 @@\n> #include \"run-command.h\"\n> #include \"prompt.h\"\n> \n> +/*\n> + * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n> + * so below is a transcription of `git help hooks`. Later, this should\n> + * be replaced with some programmatically generated list (generated from\n> + * doc or else taken from some library which tells us about all the\n> + * hooks)\n> + */\n> +const char *hook_name[] = {\n> +\t\"applypatch-msg\",\n> +\t\"pre-applypatch\",\n> +\t\"post-applypatch\",\n> +\t\"pre-commit\",\n> +\t\"pre-merge-commit\",\n> +\t\"prepare-commit-msg\",\n> +\t\"commit-msg\",\n> +\t\"post-commit\",\n> +\t\"pre-rebase\",\n> +\t\"post-checkout\",\n> +\t\"post-merge\",\n> +\t\"pre-push\",\n> +\t\"pre-receive\",\n> +\t\"update\",\n> +\t\"post-receive\",\n> +\t\"post-update\",\n> +\t\"push-to-checkout\",\n> +\t\"pre-auto-gc\",\n> +\t\"post-rewrite\",\n> +\t\"sendemail-validate\",\n> +\t\"fsmonitor-watchman\",\n> +\t\"p4-pre-submit\",\n> +\t\"post-index-change\",\n> +};\n> +int hook_name_count = ARRAY_SIZE(hook_name);\n> +\n> void free_hook(struct hook *ptr)\n> {\n> \tif (ptr) {\n> diff --git a/hook.h b/hook.h\n> index f32189380a..d902166408 100644\n> --- a/hook.h\n> +++ b/hook.h\n> @@ -4,6 +4,9 @@\n> #include \"strvec.h\"\n> #include \"run-command.h\"\n> \n> +extern const char *hook_name[];\n> +extern int hook_name_count;\n> +\n> struct hook {\n> \tstruct list_head list;\n> \t/*\n> -- \n> 2.32.0.272.g935e593368-goog\n> \n"},{"id":"427909","messageId":"YM0Q6AynHC675ndd@google.com","threadId":"55946","inReplyTo":"0f03e956c183d827c186b6fb85b92bfd31d2dc47.1623881977.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH 2/2] clone,fetch: remote-suggested auto-updating hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-18T21:32:24Z","receivedAt":"2021-06-18T21:32:36Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jun 16, 2021 at 04:31:49PM -0700, Jonathan Tan wrote:\n> \n> Upon cloning, if a ref refs/remotes/origin/suggested-hooks is present,\n> Git will inform the user that they can run a command to install those\n> hooks and keep them auto-updated upon eacn fetch. If that command is\n> run, hooks will be installed (overriding existing ones) and every fetch\n> will check if that ref would be updated, and if yes, reinstall the hooks\n> from the new commit that the ref points to.\n> \n> NEEDSWORK: Write a more detailed commit message once the design is\n> finalized.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/clone.c | 10 ++++++\n> builtin/fetch.c | 21 ++++++++++++\n> builtin/hook.c | 13 +++++---\n> hook.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++\n> hook.h | 2 ++\n> t/t5601-clone.sh | 36 +++++++++++++++++++++\n> 6 files changed, 162 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 2a2a03bf76..0ab4494bcd 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1393,6 +1393,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \t\t\t branch_top.buf, reflog_msg.buf, transport,\n> \t\t\t !is_local);\n> \n> +\tfor (ref = mapped_refs; ref; ref = ref->next) {\n> +\t\tif (ref->peer_ref &&\n> +\t\t !strcmp(ref->peer_ref->name,\n> +\t\t\t \"refs/remotes/origin/suggested-hooks\")) {\n> +\t\t\tfprintf(stderr, _(\"The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\\n\"\n> +\t\t\t\t\t \"Run `git hook autoupdate` to download them, install them, and turn on automatic update of hooks.\\n\"));\n\nIt is possible to clone but to name your origin something besides\n'origin', isn't it? *looks..* I guess 'origin' is added by default, but\ncouldn't a user rename their origin remote later on, or want hooks from\nsomewhere else? For example:\n\n - User creates a fork with GitHub web UI\n - User clones their own fork (origin = nasamuffin/someproject)\n - User decides they should probably get latest and greatest code from\n upstream instead, so they add new remote (upstream =\n someproject/someproject)\n - The hooks are in someproject/someproject, and maybe only showed up\n after user forked from web UI. Oops.\n\nAnyway, it might be nice to have this generalized to any remote, not\njust 'origin' - maybe I even want to pull in hooks from upstream as well\nas hooks from downstream fork with confidential patches added. (I had a\nremote setup like this when working on OpenBMC, so it is not beyond\nrealm of possibility.)\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 769af53ca4..0618d1f091 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -28,6 +28,7 @@\n> #include \"promisor-remote.h\"\n> #include \"commit-graph.h\"\n> #include \"shallow.h\"\n> +#include \"hook.h\"\n> \n> #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n> \n> @@ -84,6 +85,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n> static int fetch_write_commit_graph = -1;\n> static int stdin_refspecs = 0;\n> static int negotiate_only;\n> +static int hook_autoupdate;\n> \n> static int git_fetch_config(const char *k, const char *v, void *cb)\n> {\n> @@ -123,6 +125,11 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n> \t\treturn 0;\n> \t}\n> \n> +\tif (!strcmp(k, \"hook.autoupdate\")) {\n> +\t\thook_autoupdate = git_config_bool(k, v);\n> +\t\treturn 0;\n> +\t}\n> +\n\nChecking if we should look for hook changes every time we fetch. Ok.\n\n> @@ -1313,6 +1320,20 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)\n> \t\t\t\t ref_map);\n> \ttransport_unlock_pack(transport);\n> \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n> +\n> +\tif (hook_autoupdate) {\n> +\t\tstruct ref *ref;\n> +\n> +\t\tfor (ref = ref_map; ref; ref = ref->next) {\n> +\t\t\tif (ref->peer_ref &&\n> +\t\t\t !strcmp(ref->peer_ref->name,\n> +\t\t\t\t \"refs/remotes/origin/suggested-hooks\")) {\n> +\t\t\t\thook_update_suggested();\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n\nAnd if fetch gave us a ref like suggested-hooks, then we should go do\nhook update operation. Ok.\n\n> +static int autoupdate(int argc, const char **argv, const char *prefix)\n> +{\n> +\tgit_config_set(\"hook.autoupdate\", \"true\");\n> +\thook_update_suggested();\n> +\treturn 0;\n> +}\n\nThe name is a little bit vague to me - this function turns on autoupdate\nand performs one hook update, so I think the name at least should cover\nthat it's related to hooks, and that it's doing setup.\n\nIf we wanted to gate \"automagically turn on autoupdate\" behind a\nsystem-level config like I mentioned in my review of the cover letter,\nwe could do it here, for example by setting \"hook.autoupdate\" to the\nvalue of \"experimental.autoupdatehooks\" (or whatever name is least\npainful).\n\n> +\n> int cmd_hook(int argc, const char **argv, const char *prefix)\n> {\n> \tconst char *run_hookdir = NULL;\n> @@ -152,10 +159,6 @@ int cmd_hook(int argc, const char **argv, const char *prefix)\n> \targc = parse_options(argc, argv, prefix, builtin_hook_options,\n> \t\t\t builtin_hook_usage, PARSE_OPT_KEEP_UNKNOWN);\n> \n> -\t/* after the parse, we should have \" \" */\n> -\tif (argc < 2)\n> -\t\tusage_with_options(builtin_hook_usage, builtin_hook_options);\n> -\n> \tgit_config(git_default_config, NULL);\n> \n> \n> @@ -185,6 +188,8 @@ int cmd_hook(int argc, const char **argv, const char *prefix)\n> \t\treturn list(argc, argv, prefix);\n> \tif (!strcmp(argv[0], \"run\"))\n> \t\treturn run(argc, argv, prefix);\n> +\tif (!strcmp(argv[0], \"autoupdate\"))\n> +\t\treturn autoupdate(argc, argv, prefix);\n> \n> \tusage_with_options(builtin_hook_usage, builtin_hook_options);\n> }\n> diff --git a/hook.c b/hook.c\n> index 3ccacb72fa..5730e6e501 100644\n> --- a/hook.c\n> +++ b/hook.c\n> @@ -4,6 +4,12 @@\n> #include \"config.h\"\n> #include \"run-command.h\"\n> #include \"prompt.h\"\n> +#include \"commit.h\"\n> +#include \"object.h\"\n> +#include \"refs.h\"\n> +#include \"tree-walk.h\"\n> +#include \"tree.h\"\n> +#include \"streaming.h\"\n> \n> /*\n> * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n> @@ -515,3 +521,81 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options)\n> \n> \treturn cb_data.rc;\n> }\n> +\n> +static int is_hook(const char *filename)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < hook_name_count; i++) {\n> +\t\tif (!strcmp(filename, hook_name[i]))\n> +\t\t\treturn 1;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +void hook_update_suggested(void)\n> +{\n> +\tstruct object_id oid;\n> +\tstruct object *obj;\n> +\tstruct tree *tree;\n> +\tstruct tree_desc desc;\n> +\tstruct name_entry entry;\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\n> +\tif (read_ref(\"refs/remotes/origin/suggested-hooks\", &oid)) {\n> +\t\twarning(_(\"no such ref refs/remotes/origin/suggested-hooks, not updating hooks\"));\n> +\t\treturn;\n> +\t}\n> +\n> +\tobj = parse_object(the_repository, &oid);\n> +\tif (obj == NULL) {\n> +\t\twarning(_(\"object pointed to by refs/remotes/origin/suggested-hooks '%s' does not exist\"),\n> +\t\t\toid_to_hex(&oid));\n> +\t\treturn;\n> +\t}\n> +\tif (obj->type != OBJ_COMMIT) {\n> +\t\twarning(_(\"object pointed to by refs/remotes/origin/suggested-hooks '%s' is not a commit\"),\n> +\t\t\toid_to_hex(&oid));\n> +\t\treturn;\n> +\t}\n> +\n> +\ttree = get_commit_tree((struct commit *) obj);\n> +\tif (parse_tree(tree)) {\n> +\t\twarning(_(\"could not parse tree\"));\n> +\t\treturn;\n> +\t}\n> +\n> +\tinit_tree_desc(&desc, tree->buffer, tree->size);\n> +\twhile (tree_entry(&desc, &entry)) {\n> +\t\tint fd;\n> +\n> +\t\tif (!is_hook(entry.path)) {\n> +\t\t\twarning(_(\"file '%s' is not a hook; ignoring\"),\n> +\t\t\t\tentry.path);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tif (S_ISDIR(entry.mode) || S_ISGITLINK(entry.mode)) {\n> +\t\t\twarning(_(\"file '%s' is not an ordinary file; ignoring\"),\n> +\t\t\t\tentry.path);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tstrbuf_reset(&path);\n> +\t\tstrbuf_git_path(&path, \"hooks/%s\", entry.path);\n> +\t\tfd = open(path.buf, O_WRONLY | O_CREAT, 0755);\n> +\t\tif (fd < 0) {\n> +\t\t\twarning_errno(_(\"could not create file '%s'; skipping this hook\"),\n> +\t\t\t\t path.buf);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tif (stream_blob_to_fd(fd, &entry.oid, NULL, 1)) {\n> +\t\t\twarning(_(\"could not write to file '%s'; skipping this hook\"),\n> +\t\t\t\tpath.buf);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tclose(fd);\n> +\t}\n> +\tstrbuf_release(&path);\n> +\n> +\treturn;\n> +}\n\nI saw that you wanted to reduce reliance on es/config-based-hooks, which\nmakes sense; but if you wanted to do it the configgy way, I'd instead\nadd a local config-based hook with a command something like `exec -\n<<<$(git show refs/suggested-hooks:presubmit)` (that's not quite\nperfect, but I hope gets the point across - materialize the file from\nthe local ref and execute it, rather than copying it into .git/hooks).\n\nOne bonus of doing it that way is that user can commit to their local\nsuggested-hooks branch and try out their new hook (and even send a\nreview from there with the change they liked). Hrm.\n\n> diff --git a/hook.h b/hook.h\n> index d902166408..b0bfd3719b 100644\n> --- a/hook.h\n> +++ b/hook.h\n> @@ -140,3 +140,5 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options);\n> void free_hook(struct hook *ptr);\n> /* Empties the list at 'head', calling 'free_hook()' on each entry */\n> void clear_hook_list(struct list_head *head);\n> +\n> +void hook_update_suggested(void);\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index c0688467e7..21b96a4999 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -752,6 +752,42 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe\n> \tgit clone --filter=blob:limit=0 \"file://$(pwd)/server\" client\n> '\n> \n> +test_expect_success 'detect suggested hook branch during clone' '\n> +\trm -rf server client &&\n> +\n> +\ttest_create_repo server &&\n> +\techo \"echo foo\" >server/pre-commit &&\n> +\tgit -C server add pre-commit &&\n> +\ttest_commit -C server hook-and-not-hook &&\n> +\tgit -C server checkout -b suggested-hooks &&\n> +\tgit clone server client 2>err &&\n> +\ttest_i18ngrep \"The remote has suggested hooks\" err\n> +'\n> +\n> +test_expect_success 'autoupdate' '\n> +\tgit -C client hook autoupdate &&\n> +\n> +\t# Check correct config written\n> +\tgit -C client config --local hook.autoupdate >actual &&\n> +\techo \"true\" >expect &&\n> +\ttest_cmp expect actual &&\n> +\n> +\t# Check hook written\n> +\tcat client/.git/hooks/pre-commit >actual &&\n> +\techo \"echo foo\" >expect &&\n> +\ttest_cmp expect actual &&\n> +\n> +\t# Install new hook and fetch it\n> +\techo \"echo bar\" >server/pre-commit &&\n> +\tgit -C server commit -am \"new hook\" &&\n> +\tgit -C client fetch &&\n> +\n> +\t# Check new hook written\n> +\tcat client/.git/hooks/pre-commit >actual &&\n> +\techo \"echo bar\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> start_httpd\n> \n> -- \n> 2.32.0.272.g935e593368-goog\n> \n"},{"id":"427911","messageId":"20210618214650.792661-1-jonathantanmy@google.com","threadId":"55946","inReplyTo":"xmqq35thnuqp.fsf@gitster.g","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-18T21:46:50Z","receivedAt":"2021-06-18T21:46:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > 1. The remote repo administrator creates a new branch\n> > \"refs/heads/suggested-hooks\" pointing to a commit that has all the\n> > hooks that the administrator wants to suggest. The hooks are\n> > directly referenced by the commit tree (i.e. they are in the \"/\"\n> > directory).\n> \n> wants to suggest? They simply suggest ;-)\n\nAh yes :-)\n\n> \n> > 2. When a user clones, Git notices that\n> > \"refs/remotes/origin/suggested-hooks\" is present and prints out a\n> > message about a command that can be run.\n> \n> Can be run to install? Or can be run to first inspect? Or both?\n\nRight now I only have a command that installs, but I can provide the\nappropriate \"cat-file\" invocations to inspect them as well.\n\n> > 3. If the user runs that command, Git will install the hooks pointed to\n> > by that ref, and set hook.autoupdate to true. This config variable\n> > is checked whenever \"git fetch\" is run: whenever it notices that\n> > \"refs/remotes/origin/suggested-hooks\" changes, it will reinstall the\n> > hooks.\n> >\n> > 4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks\n> > will remain.\n> \n> OK, so \"verify even if you implicitly trust\" is actively discouraged.\n\nYes I was thinking of the model in which we already trust upstream, but\nI agree that verification can be useful. I think we can print the\n\"cat-file\" commands needed to verify before installing, and add a mode\nin which we tell the user that the hooks have been updated (but not\nautomatically install them).\n\n> > Design choices:\n> >\n> > 1. Where should the suggested hooks be in the remote repo? A branch,\n> > a non-branch ref, a config? I think that a branch is best - it is\n> > relatively well-understood and any hooks there can be\n> > version-controlled (and its history is independent of the other\n> > branches).\n> \n> As people mentioned in the previous discussions, \"independent of the\n> other branches\" has advantages and disadvantages. The most recent\n> set of hooks may have some that would not work well with older\n> codebase, so care must be taken to ensure any hook works on across\n> versions of the main codebase. Which may not be a huge downside,\n> but something users must be aware of.\n\nThat's true - and on the flip side, I would presume that the\nhook-introducing admin would usually want those hooks to apply\nretroactively too (say, to someone updating a \"maint\" branch). I think\nit's more flexible if hooks are in an independent branch, though - (if\nindependent branch) a hook can be written to tolerate an old codebase,\nbut if embedded in the code branch, such a hook cannot apply\nretroactively to old code.\n\n> > 2. When and how should the local repo update its record of the remote's\n> > suggested hooks? If we go with storing the hooks in a branch of a\n> > remote side, this would automatically mean (with the default\n> > refspec) that it would be in refs/remotes//. This\n> > incurs network and hard disk cost even if the local repo does not\n> > want to use the suggested hooks, but I think that typically they\n> > would want to use it if they're going to do any work on the repo\n> > (they would either have to trust or inspect Makefiles etc. anyway,\n> > so they can do the same for the hooks), and even if they don't want\n> > to use the remote's hooks, they probably still want to know what the\n> > remote suggests.\n> \n> A way to see what changes are made to recommendation would be\n> useful, and a branch that mostly linearly progresses is a good way\n> to give it to the users.\n> \n> Of course, that can be done with suggested hooks inline with the\n> rest of the main codebase, too.\n\nThat's true.\n\n> > 4. Should the local repo try to notice if the hooks have been changed\n> > locally before overwriting upon autoupdate? This would be nice to\n> > have, but I don't know how practical it would be. In particular, I\n> > don't know if we can trust that\n> > \"refs/remotes/origin/suggested-hooks\" has not been clobbered.\n> \n> Meaning clobbered by malicious parties? Then the whole thing is a\n> no-go from security point of view. Presumably you trust the main\n> content enough to work on top of it, so as long as you can trust\n> that refs/remotes/origin/hooks to the same degree that you would\n> trust refs/remotes/origin/master, I do not think it is a problem.\n\nI meant clobbered by the user - should have made that more clear. To\nelaborate...\n\n> Whatever mechanism you use to materialize an updated version of the\n> hooks can and should record which commit on the suggested-hooks\n> branch the .git/hooks/* file is based on. Then when the user wants\n> to update to a different version of suggested-hooks (either because\n> you auto-detected, or the user issued a command to update), you have\n> \n> - The current version in .git/hooks/*\n> \n> - The old pristine version (you recorded the commit when you\n> updated the .git/hooks/* copy the last time)\n> \n> - The new pristine version (you have a remote-tracking branch).\n> \n> and it is not a brain surgery to perform three-way merge to update\n> the first using the difference between the second and the third.\n\n...I was having a difficult time figuring out where to store such\ninformation (ref? config? I wouldn't be surprised if a user saw a value\nthere and thought that they could change it). Perhaps it could be a\nseparate file like .git/shallow.\n\nI'm not fully convinced that maintaining the ability to retain local\nhook modifications by supporting three-way merges is important, but if\nit is, it might be brain surgery to figure out the UX when merge\nconflicts occur. Normally these conflicts can be written to the worktree\nand index, but we don't have those in the case of hooks.\n\n> > 5. Should we have a command that manually updates the hooks with what's\n> > in \"refs/heads/suggested-hooks\"? This is not in this patch set, but\n> > it sounds like a good idea.\n> \n> I wonder if having it bound as a submodule to a known location in\n> the main contents tree makes it easier to manage and more flexible.\n> Just like you can update the working tree of a submodule to a\n> version that is bound to the superproject tree, or to a more recent\n> version of the \"branch\" in the submodule, you can support workflows\n> that allow suggested hooks to advance independent of the main\n> contents version and that uses a specific version of suggested hooks\n> tied to the main contents.\n\nThis would make the hooks dependent on the commit checked out (with\nperhaps a bit more leeway in that our implementation could be flexible\nand use a commit later than what's in the gitlink), with its own pros\nand cons (as you said earlier).\n"},{"id":"427912","messageId":"20210618214816.793345-1-jonathantanmy@google.com","threadId":"55946","inReplyTo":"YM0JOyxPGYmzynoc@google.com","subject":"Re: [RFC PATCH 1/2] hook: move list of hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-18T21:48:16Z","receivedAt":"2021-06-18T21:48:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Wed, Jun 16, 2021 at 04:31:48PM -0700, Jonathan Tan wrote:\n> > \n> > The list of hooks will need to be used outside bugreport, so move it to\n> > a central location.\n> \n> Hm, does this fight with\n> https://lore.kernel.org/git/cover-0.3-0000000000-20210617T100239Z-avarab@gmail.com\n> ?\n> \n> This patch itself looks pretty straightforward, but I think it is a good\n> idea to take a look at avarab's change first.\n> \n> - Emily\n\nHmm...you might be right. In any case, this is just an RFC patch set so\nI presume avarab's will be merged first (and then I can use his hook\nlist in a subsequent version of my patch set).\n"},{"id":"427918","messageId":"20210618215848.794617-1-jonathantanmy@google.com","threadId":"55946","inReplyTo":"YM0IpOFH4Sy9wWaE@google.com","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-18T21:58:48Z","receivedAt":"2021-06-18T21:58:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Wed, Jun 16, 2021 at 04:31:47PM -0700, Jonathan Tan wrote:\n> > \n> > I have had to make several design choices (which I will discuss later),\n> > but now with this implementation, the following workflow is possible:\n> > \n> > 1. The remote repo administrator creates a new branch\n> > \"refs/heads/suggested-hooks\" pointing to a commit that has all the\n> > hooks that the administrator wants to suggest. The hooks are\n> > directly referenced by the commit tree (i.e. they are in the \"/\"\n> > directory).\n> \n> I don't really like that this is in the same namespace as branches users\n> could create themselves. Hm, I think for 'git maintenance' prefetching\n> we put those refs in some special namespace, right? Can we do something\n> similar in this case? Would that prevent us from treating that ref like\n> a normal branch?\n\nDo you mean that the server should put it in a different place, the\nclient should put it in a different place, or both?\n\n> > 2. When a user clones, Git notices that\n> > \"refs/remotes/origin/suggested-hooks\" is present and prints out a\n> > message about a command that can be run.\n> > \n> > 3. If the user runs that command, Git will install the hooks pointed to\n> > by that ref, and set hook.autoupdate to true. This config variable\n> > is checked whenever \"git fetch\" is run: whenever it notices that\n> > \"refs/remotes/origin/suggested-hooks\" changes, it will reinstall the\n> > hooks.\n> \n> I think this is where most people will have a lot to say from security\n> perspective. It will be important to make sure that:\n> - \"git fetch\" only automatically updates these scripts when fetching\n> from the same remote they were initially fetched from, over some\n> secure protocol\n\nPutting the hooks in a branch does mean that \"git fetch\" gets these\nscripts from the same remote (as opposed to, say, putting them in a\nsubmodule or referring to them by URL), and I think it's easier to\nunderstand (from the user point of view) that branch equals same remote\n(rather than say that a submodule or URL is considered to be from the\nsame remote if $CRITERIA).\n\n> - hook.autoupdate setting maybe should not be default, or should at\n> least be able to override during the install command. (One suggestion\n> was to also set some experimental.suggestedHooks setting, or\n> something, to turn on \"automatically set up autoupdate\" behavior?)\n\nOK, this makes sense.\n\n> - we should notify the user that their hooks were updated/reinstalled\n> when that happens later on. (Maybe you did this, I didn't look at the\n> diff quite yet)\n\nI didn't do this, but this is a good idea.\n\n> > Design choices:\n> > \n> > 1. Where should the suggested hooks be in the remote repo? A branch,\n> > a non-branch ref, a config? I think that a branch is best - it is\n> > relatively well-understood and any hooks there can be\n> > version-controlled (and its history is independent of the other\n> > branches).\n> \n> I agree - a branch without any prior parent sounds ideal to me, that is,\n> the repo owner setting up the hooks can say 'git checkout --orphan\n> suggested-hooks'. I guess if they want the branch to have history\n> shared with the rest of the project there's no reason not to do that,\n> either.\n\nHow would the branch share history with the rest of the project? If you\nmean that we should allow the users to store hooks as part of the main\ncodebase (within a configurable path, say, /suggested_hooks) and then\npoint both \"main\" (or whatever the default branch is) and\n\"refs/remotes/origin/suggested-hooks\" to the same place, then that\nmakes sense (although I would say that it still sounds better to have\nseparate histories).\n\n> Do we want to provide helpful tooling for the administrator to create\n> this magic branch? At the very least I guess we want some documentation,\n> but I wonder if it's possible to make a helper (maybe a 'git hook'\n> subcommand) without being more prescriptive than Git project usually is.\n\nThis sounds like the equivalent of \"git checkout --orphan\", as you\nsuggested above. We could just write it out in the documentation. I'm\nnot completely opposed to a special command, though.\n\n> > 3. How should the local repo detect when the remote has updated its\n> > suggested hooks? I'm thinking when a certain local ref is updated.\n> > Right now it's hardcoded, but perhaps \"git clone\" could detect what\n> > \"refs/heads/suggested-hooks\" would correspond to, and then set it in\n> > a config accordingly. Other options include remembering what the\n> > remote's \"refs/heads/suggested-hooks\" used to be and always\n> > comparing it upon every \"ls-refs\" call, but I think that the local\n> > ref method is more straightforward.\n> \n> Hm, I like that idea as it's analogous to remote tracking branch + local\n> tracking branch (which is a pretty common pattern for people to use).\n\nYeah - that is partly why I like the branch idea (something already\ngenerally understood by people).\n\n> > 4. Should the local repo try to notice if the hooks have been changed\n> > locally before overwriting upon autoupdate? This would be nice to\n> > have, but I don't know how practical it would be. In particular, I\n> > don't know if we can trust that\n> > \"refs/remotes/origin/suggested-hooks\" has not been clobbered.\n> \n> I think it would be nice to have, yeah. It seems important to notify the\n> users that they are executing something different from yesterday, so\n> noticing when we're making changes to the hook sounds useful.\n\nJunio suggested [1] that we store more information about what's going\non, so this might be practical.\n\n[1] https://lore.kernel.org/git/xmqq35thnuqp.fsf@gitster.g/\n\n> > 5. Should we have a command that manually updates the hooks with what's\n> > in \"refs/heads/suggested-hooks\"? This is not in this patch set, but\n> > it sounds like a good idea.\n> \n> Yeah, I think so - that lets a user say \"no, I don't need those hooks\n> (or this update)\" at first, but later change their mind.\n\nOK.\n\n> One thing that sounds useful to me, even at this RFC stage, is\n> documentation showing what this feature looks like to use - for both\n> the administrator setting up the magic hook branch, and the developer\n> cloning and using hooks. I think we want that in some Git manpage\n> eventually anyways, and it would help to see the workflow you're trying\n> to achieve.\n> \n> Thanks for sending - will review the diffs today too.\n> \n> - Emily\n\nAgreed - at the very least, we would need to write this workflow as a\ntest, and we can just reuse the same workflow in documentation. We just\nneed to discuss what the workflow should be :-) (as it is, one thing\nthat is becoming obvious is that currently people prefer to have\nverify-before-update instead of auto-update by default).\n"},{"id":"427922","messageId":"020d01d76491$dcfe7c60$96fb7520$@nexbridge.com","threadId":"55946","inReplyTo":"20210618215848.794617-1-jonathantanmy@google.com","subject":"RE: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-06-18T22:32:41Z","receivedAt":"2021-06-18T22:32:51Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 18, 2021 5:59 PM, Jonathan Tan wrote:\n>> On Wed, Jun 16, 2021 at 04:31:47PM -0700, Jonathan Tan wrote:\n>> >\n>> > I have had to make several design choices (which I will discuss\n>> > later), but now with this implementation, the following workflow is possible:\n>> >\n>> > 1. The remote repo administrator creates a new branch\n>> > \"refs/heads/suggested-hooks\" pointing to a commit that has all the\n>> > hooks that the administrator wants to suggest. The hooks are\n>> > directly referenced by the commit tree (i.e. they are in the \"/\"\n>> > directory).\n>>\n>> I don't really like that this is in the same namespace as branches\n>> users could create themselves. Hm, I think for 'git maintenance'\n>> prefetching we put those refs in some special namespace, right? Can we\n>> do something similar in this case? Would that prevent us from treating\n>> that ref like a normal branch?\n>\n>Do you mean that the server should put it in a different place, the client should put it in a different place, or both?\n\nThis brings up a very awkward question: How are enterprise git servers going to deal with this? I do not see the standard Pull Request mechanism available in GitHub handing placing hooks in different places during a merge operation. Or will this entire concept be omitted from PR?\n\nIt seems like changes to hooks have to be managed in a similar way to standard managed files rather than as exceptions.\n\n-Randall\n\n"},{"id":"427959","messageId":"CAOjrSZtKxEbMEbNZ+KEDfoJSOBSyKPY+PQRaP9sb7xgXiEFJZg@mail.gmail.com","threadId":"55946","inReplyTo":"020d01d76491$dcfe7c60$96fb7520$@nexbridge.com","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Matt Rogers","fromEmail":"mattr94@gmail.com","sentAt":"2021-06-19T07:58:57Z","receivedAt":"2021-06-19T07:59:11Z","isPatch":true,"sender":{"key":"mattr94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5719846?v=4"},"body":"On Sat, Jun 19, 2021 at 3:32 AM Randall S. Becker\n wrote:\n>\n> On June 18, 2021 5:59 PM, Jonathan Tan wrote:\n> This brings up a very awkward question: How are enterprise git servers going to deal with this? I do not see the standard Pull Request mechanism available in GitHub handing placing hooks in different places during a merge operation. Or will this entire concept be omitted from PR?\n>\n\nRelated question, if a project had a collection of suggested hooks,\nand a contributor wanted\nto update them in relation to a new feature or code change, would they\nthen have to create two\nseparate patches/PRs? That feels like it would decentralize discussions/review?\n\nFor example, if I maintained a project on GitHub and a contributor\nwanted to add a clang-tidy\ninvocation as a suggested hook, as well as a config file for it. What\nwould the suggested workflow be?\n\n\"Open 2 Pull Requests one that updates both branches and do reviews\nindependently\"?\n\nIf it was a mailing list would I have to send two separate patches?\n\nI'm not familiar with any workflows or tools that allow you to review\nand accept changes to\ntwo branches as part of the same series of changes.\n\nI also think that the history wouldn't be very clear in this case,\nsince there won't be any obvious\nconnection between updates to the suggested-hooks and updates to the\nrest of the history in\nthis case (other than timestamps I guess, but I think that's a\nrelatively weak form of association)\n\n> It seems like changes to hooks have to be managed in a similar way to standard managed files rather than as exceptions.\n>\n> -Randall\n>\n\n\nThanks,\nMatthew Rogers\n"},{"id":"428003","messageId":"87fsxc47le.fsf@evledraar.gmail.com","threadId":"55946","inReplyTo":"cover.1623881977.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-20T19:51:04Z","receivedAt":"2021-06-20T20:14:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 16 2021, Jonathan Tan wrote:\n\n> This is a continuation of the work from [1]. That work described the\n> reasons, possible features, and possible workflows, but not the\n> implementation in detail. This patch set has an MVP implementation, and\n> my hope is that having a concrete implementation to look at makes it\n> easier to discuss matters of implementation.\n\nMy C on this RFC is:\n\n1) A request that someone reply (there or here would do) to my comments\n on the last iteration of this at:\n https://lore.kernel.org/git/874kghk906.fsf@evledraar.gmail.com/\n\n2) I think you'd get better feedback if you CC'd the people who've been\n actively discussing this in previous rounds.\n\nTo briefly summarize the main gist of that 1):\n\n> This but does not use any features from es/config-based-hooks but is\n> implemented on that branch anyway because firstly, I need an existing\n> command to attach the \"autoupdate\" subcommand (and \"git hook\" works),\n> and secondly, when we test this at $DAYJOB, we will be testing it\n> together with the aforementioned branch.\n>\n> I have had to make several design choices (which I will discuss later),\n> but now with this implementation, the following workflow is possible:\n>\n> 1. The remote repo administrator creates a new branch\n> \"refs/heads/suggested-hooks\" pointing to a commit that has all the\n> hooks that the administrator wants to suggest. The hooks are\n> directly referenced by the commit tree (i.e. they are in the \"/\"\n> directory).\n>\n> 2. When a user clones, Git notices that\n> \"refs/remotes/origin/suggested-hooks\" is present and prints out a\n> message about a command that can be run.\n>\n> 3. If the user runs that command, Git will install the hooks pointed to\n> by that ref, and set hook.autoupdate to true. This config variable\n> is checked whenever \"git fetch\" is run: whenever it notices that\n> \"refs/remotes/origin/suggested-hooks\" changes, it will reinstall the\n> hooks.\n>\n> 4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks\n> will remain.\n>\n> Design choices:\n>\n> 1. Where should the suggested hooks be in the remote repo? A branch,\n> a non-branch ref, a config? I think that a branch is best - it is\n> relatively well-understood and any hooks there can be\n> version-controlled (and its history is independent of the other\n> branches).\n\nFirst, unlike brian I don't (I hope I'm fairly summarizing his view\nhere) disagree mostly or entirely with the existence of such a feature\nat all. I mean, I get the viewpoint that git shouldn't bless what\namounts to an active RCE from the remote.\n\nI just think that we could probably do a better job of it than what\npeople are doing in practice, and I've seen people do stuff like have\nbuild systems setup permanent symlinks to git-hooks/ in the\ntracked dir. We could at least envision a git-native implementation\nasking the user \"do you want this hook update? \".\n\nI just find this design approach completely bizarre as noted (probably\nin less blunt words) in the linked E-Mail.\n\nWe have Emily's series to convert hooks to be config driven that we hope\nto land in some form, at that point they won't be any more of a special\nsnowflake than any other config.\n\nAnd then, instead of doing what I'd think would be the natural result of\nthat: Simply supporting an in-repo top-level \".gitconfig\" file. We're\nstill going to seemingly forever have them be an even more special\nsnowflake with this facility, and the reason seems to be mostly/entirely\nto do with working around some aspect or restriction of Google's\ninternal infrastructure.\n\nI think it's just un-git-y to have a meta-branch that in some way drives\nnot only all other branches, but all other revisions of all branches,\never.\n\nIt breaks expectations around git in lots of different ways, you can't\nfetch a single branch and get its hooks, you can't locally alter, commit\nand update your hooks while e.g. renaming a \"t/\" directory to \"test/\";\nyour hooks and code can't be atomically changed).\n\nI think I get why you want to do it that way, I just don't get why, as\nmostly noted in those earlier rounds why it wouldn't be a better\napproach / more straightforward / more git-y to:\n\n1. Work on getting hooks driven by config \n2. Have a facility to read an in-repo '.gitconfig'; have lots of safety\n valves etc. around this, I suggested starting with a whitelist of the\n N least dangerous config options, e.g. some diff viewing options, or\n a suggested sendemail.to or whatever.\n\n3. Work our way up to trusting that for more dangerous stuff, eventually\n hooks. Most of the legitimate concerns from others with this is\n having some UX where our users won't be trained to just blindly say\n \"yes\" to an alias/hook config that \"rm -rf's /\" or whatever.\n\n If we start experimenting with that with aliases or hooks that can\n run arbitrary code it's like handing a toddlder a shotgun, let's at\n least start with a sharp fork or something (less dangerous config) :)\n\n4. People who want this \"I want my hooks to apply to all revisions ever\"\n could probably get 99% or 100% of what they want if their hook is\n just a stub that does the equivalent of:\n\n sh `curl https://git.google.com/$reponame/hooks/$hookname`\n\n You'd then simply forbid on your servers any changes to a .gitconfig\n that did anything with the hook.* namespace.\n\nWith such an implementation you don't need a magic\n\"refs/remotes/origin/suggested-hooks\" refs, just some state machine (I\nsuggested e.g. GPG signing chains as an eventual end-state, but \"show a\ndiff every time\" would also do) that keeps track of what config (and\nhooks are just one such case) has been OK'd, and which has not.\n\nI'd think it would even work better in the Googleplex, you could clone a\nco-worker's branch and execute their hooks, since they're the same as\nwhat you've pre-approved, you could even clone some random person's fork\nof a \"blessed\" project, because the hooks would be the same `sh $(curl\n)`. That validation could even be a system-level\nin-config hook on your laptop, thus bringing the whole thing full\ncircle...\n"},{"id":"428085","messageId":"20210621183722.1167322-1-jonathantanmy@google.com","threadId":"55946","inReplyTo":"CAOjrSZtKxEbMEbNZ+KEDfoJSOBSyKPY+PQRaP9sb7xgXiEFJZg@mail.gmail.com","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-21T18:37:22Z","receivedAt":"2021-06-21T18:37:28Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Related question, if a project had a collection of suggested hooks,\n> and a contributor wanted\n> to update them in relation to a new feature or code change, would they\n> then have to create two\n> separate patches/PRs? That feels like it would decentralize discussions/review?\n> \n> For example, if I maintained a project on GitHub and a contributor\n> wanted to add a clang-tidy\n> invocation as a suggested hook, as well as a config file for it. What\n> would the suggested workflow be?\n> \n> \"Open 2 Pull Requests one that updates both branches and do reviews\n> independently\"?\n> \n> If it was a mailing list would I have to send two separate patches?\n\nWith the current design, the answer is \"yes\" to both. Having said that,\nfor this particular case, I think that the clang-tidy config file can be\nreviewed and merged first, and then the clang-tidy hook. (The hook needs\nto be written to also work in the absence of the config file, since the\nuser may have checked out an older version, so as far as I can tell,\nthere is no need for precise timing as to when the hook is merged.)\n"},{"id":"428089","messageId":"20210621185858.1169385-1-jonathantanmy@google.com","threadId":"55946","inReplyTo":"87fsxc47le.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-21T18:58:58Z","receivedAt":"2021-06-21T18:59:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Wed, Jun 16 2021, Jonathan Tan wrote:\n> \n> > This is a continuation of the work from [1]. That work described the\n> > reasons, possible features, and possible workflows, but not the\n> > implementation in detail. This patch set has an MVP implementation, and\n> > my hope is that having a concrete implementation to look at makes it\n> > easier to discuss matters of implementation.\n> \n> My C on this RFC is:\n> \n> 1) A request that someone reply (there or here would do) to my comments\n> on the last iteration of this at:\n> https://lore.kernel.org/git/874kghk906.fsf@evledraar.gmail.com/\n\nOK - I'll take a look at that.\n\n> 2) I think you'd get better feedback if you CC'd the people who've been\n> actively discussing this in previous rounds.\n\nGood point.\n\n> > Design choices:\n> >\n> > 1. Where should the suggested hooks be in the remote repo? A branch,\n> > a non-branch ref, a config? I think that a branch is best - it is\n> > relatively well-understood and any hooks there can be\n> > version-controlled (and its history is independent of the other\n> > branches).\n> \n> First, unlike brian I don't (I hope I'm fairly summarizing his view\n> here) disagree mostly or entirely with the existence of such a feature\n> at all. I mean, I get the viewpoint that git shouldn't bless what\n> amounts to an active RCE from the remote.\n> \n> I just think that we could probably do a better job of it than what\n> people are doing in practice, and I've seen people do stuff like have\n> build systems setup permanent symlinks to git-hooks/ in the\n> tracked dir. We could at least envision a git-native implementation\n> asking the user \"do you want this hook update? \".\n> \n> I just find this design approach completely bizarre as noted (probably\n> in less blunt words) in the linked E-Mail.\n\nThat's fair. You suggest an alternative below (and maybe more in the\nlinked e-mail) - let's look at your suggestion...\n\n> We have Emily's series to convert hooks to be config driven that we hope\n> to land in some form, at that point they won't be any more of a special\n> snowflake than any other config.\n> \n> And then, instead of doing what I'd think would be the natural result of\n> that: Simply supporting an in-repo top-level \".gitconfig\" file. We're\n> still going to seemingly forever have them be an even more special\n> snowflake with this facility, and the reason seems to be mostly/entirely\n> to do with working around some aspect or restriction of Google's\n> internal infrastructure.\n\nI don't think that this is \"natural\". In particular, I still don't think\nthat hooks should be tied to code revision. E.g. if we make commits\nbased on an old revision and push them, we still want them to follow the\nlatest requirements.\n\n> I think it's just un-git-y to have a meta-branch that in some way drives\n> not only all other branches, but all other revisions of all branches,\n> ever.\n> \n> It breaks expectations around git in lots of different ways, you can't\n> fetch a single branch and get its hooks,\n\nAre you saying that each branch should have its own hooks? That might be\nreasonable in certain projects, but I don't see how that is a Git\nexpectation.\n\n> you can't locally alter, commit\n> and update your hooks while e.g. renaming a \"t/\" directory to \"test/\";\n> your hooks and code can't be atomically changed).\n\nI still think that hooks should work independent of code versions, so I\nwouldn't think that atomicity here is important.\n\n> I think I get why you want to do it that way, I just don't get why, as\n> mostly noted in those earlier rounds why it wouldn't be a better\n> approach / more straightforward / more git-y to:\n> \n> 1. Work on getting hooks driven by config Emily's series / my split-out \"base\" topic>\n> 2. Have a facility to read an in-repo '.gitconfig'; have lots of safety\n> valves etc. around this, I suggested starting with a whitelist of the\n> N least dangerous config options, e.g. some diff viewing options, or\n> a suggested sendemail.to or whatever.\n\nI've replied to this above.\n\n> 3. Work our way up to trusting that for more dangerous stuff, eventually\n> hooks. Most of the legitimate concerns from others with this is\n> having some UX where our users won't be trained to just blindly say\n> \"yes\" to an alias/hook config that \"rm -rf's /\" or whatever.\n> \n> If we start experimenting with that with aliases or hooks that can\n> run arbitrary code it's like handing a toddlder a shotgun, let's at\n> least start with a sharp fork or something (less dangerous config) :)\n> \n> 4. People who want this \"I want my hooks to apply to all revisions ever\"\n> could probably get 99% or 100% of what they want if their hook is\n> just a stub that does the equivalent of:\n> \n> sh `curl https://git.google.com/$reponame/hooks/$hookname`\n> \n> You'd then simply forbid on your servers any changes to a .gitconfig\n> that did anything with the hook.* namespace.\n\nThis would work if set in .git/config (not version controlled), but not\n.gitconfig (version controlled).\n\n> With such an implementation you don't need a magic\n> \"refs/remotes/origin/suggested-hooks\" refs, just some state machine (I\n> suggested e.g. GPG signing chains as an eventual end-state, but \"show a\n> diff every time\" would also do) that keeps track of what config (and\n> hooks are just one such case) has been OK'd, and which has not.\n\nThis sounds complicated.\n\n> I'd think it would even work better in the Googleplex, you could clone a\n> co-worker's branch and execute their hooks, since they're the same as\n> what you've pre-approved,\n\nIn the presence of .gitconfig, how would you know?\n\n> you could even clone some random person's fork\n> of a \"blessed\" project, because the hooks would be the same `sh $(curl\n> )`. That validation could even be a system-level\n> in-config hook on your laptop, thus bringing the whole thing full\n> circle...\n\nSame here.\n\nIn summary, I think your point of using hook configs + remote-suggested\nconfigs instead of remote-suggested hooks is a reasonable one, but I\ndisagree with your reasons (or, at least, your reasons as I understand\nthem).\n"},{"id":"428098","messageId":"87k0mn2dd3.fsf@evledraar.gmail.com","threadId":"55946","inReplyTo":"20210621185858.1169385-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T19:35:06Z","receivedAt":"2021-06-21T20:04:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 21 2021, Jonathan Tan wrote:\n\n>> On Wed, Jun 16 2021, Jonathan Tan wrote:\n>> \n>> > This is a continuation of the work from [1]. That work described the\n>> > reasons, possible features, and possible workflows, but not the\n>> > implementation in detail. This patch set has an MVP implementation, and\n>> > my hope is that having a concrete implementation to look at makes it\n>> > easier to discuss matters of implementation.\n>> \n>> My C on this RFC is:\n>> \n>> 1) A request that someone reply (there or here would do) to my comments\n>> on the last iteration of this at:\n>> https://lore.kernel.org/git/874kghk906.fsf@evledraar.gmail.com/\n>\n> OK - I'll take a look at that.\n>\n>> 2) I think you'd get better feedback if you CC'd the people who've been\n>> actively discussing this in previous rounds.\n>\n> Good point.\n>\n>> > Design choices:\n>> >\n>> > 1. Where should the suggested hooks be in the remote repo? A branch,\n>> > a non-branch ref, a config? I think that a branch is best - it is\n>> > relatively well-understood and any hooks there can be\n>> > version-controlled (and its history is independent of the other\n>> > branches).\n>> \n>> First, unlike brian I don't (I hope I'm fairly summarizing his view\n>> here) disagree mostly or entirely with the existence of such a feature\n>> at all. I mean, I get the viewpoint that git shouldn't bless what\n>> amounts to an active RCE from the remote.\n>> \n>> I just think that we could probably do a better job of it than what\n>> people are doing in practice, and I've seen people do stuff like have\n>> build systems setup permanent symlinks to git-hooks/ in the\n>> tracked dir. We could at least envision a git-native implementation\n>> asking the user \"do you want this hook update? \".\n>> \n>> I just find this design approach completely bizarre as noted (probably\n>> in less blunt words) in the linked E-Mail.\n>\n> That's fair. You suggest an alternative below (and maybe more in the\n> linked e-mail) - let's look at your suggestion...\n>\n>> We have Emily's series to convert hooks to be config driven that we hope\n>> to land in some form, at that point they won't be any more of a special\n>> snowflake than any other config.\n>> \n>> And then, instead of doing what I'd think would be the natural result of\n>> that: Simply supporting an in-repo top-level \".gitconfig\" file. We're\n>> still going to seemingly forever have them be an even more special\n>> snowflake with this facility, and the reason seems to be mostly/entirely\n>> to do with working around some aspect or restriction of Google's\n>> internal infrastructure.\n>\n> I don't think that this is \"natural\". In particular, I still don't think\n> that hooks should be tied to code revision. E.g. if we make commits\n> based on an old revision and push them, we still want them to follow the\n> latest requirements.\n\nEven for real-world centralized workflow situations where I've seen\npeople think they want that, and the end of the day they almost never\nactually want that.\n\nEven something like code linting is a good example, to make it\nGoogle-specific: say for a Go project: Are you going to pin your linting\ntool/version to whatever understood your YAML format for the linter as\nit was specced 10 years ago when the project started? It's simply a\ngiant hassle to have a piece of code operate on every version of your\nproject ever in a way that doesn't break.\n\nI think in practice the designers of this feature don't actually have\nthat in mind, but a \"close to trunk\" workflow, where you'd expect a hook\nto only need to operate on revisions for the last few weeks or months,\nbecause that'll be the oldest think people create new topics from.\n\nBut I think the burden of proof is really on the other side here,\nsomething that works entirely differently than the rest of git needs to\nhave a good reason. Our in-repo .gitattributes don't work like this, nor\n.gitignore, .mailmap etc.\n\nThere's also real world uses of git where the \"branches\" are wildly\ndivergent, e.g. I've worked on a system automation repo where the\n\"master\" was just a stub template, and every team had their own almost\nentirely different \"repo-like\" branch. Probably a bad idea for various\nreasons, but Git supports it just fine.\n\nFor the centralized use-case what's the problem with just having the\nhook do a 'for-each-ref --format=' invocation or \"cat-file --batch\" on\nthe \"origin\", and eval what it finds there? I'd think that gives you\nwhat you want for the more centarlized workflow, while leaving git's\nimplementation working like the rest of our in-repo files.\n\n>> I think it's just un-git-y to have a meta-branch that in some way drives\n>> not only all other branches, but all other revisions of all branches,\n>> ever.\n>> \n>> It breaks expectations around git in lots of different ways, you can't\n>> fetch a single branch and get its hooks,\n>\n> Are you saying that each branch should have its own hooks? That might be\n> reasonable in certain projects, but I don't see how that is a Git\n> expectation.\n\nIt's a git expectation now that I can add git.git as a remote, also\nchromium.git, and linux.git, fetch them all, and happily switch in the\nsame repo between entirely different codebases that don't share a\nhistory.\n\n>> you can't locally alter, commit\n>> and update your hooks while e.g. renaming a \"t/\" directory to \"test/\";\n>> your hooks and code can't be atomically changed).\n>\n> I still think that hooks should work independent of code versions, so I\n> wouldn't think that atomicity here is important.\n\nCovered above.\n\n>> I think I get why you want to do it that way, I just don't get why, as\n>> mostly noted in those earlier rounds why it wouldn't be a better\n>> approach / more straightforward / more git-y to:\n>> \n>> 1. Work on getting hooks driven by config > Emily's series / my split-out \"base\" topic>\n>> 2. Have a facility to read an in-repo '.gitconfig'; have lots of safety\n>> valves etc. around this, I suggested starting with a whitelist of the\n>> N least dangerous config options, e.g. some diff viewing options, or\n>> a suggested sendemail.to or whatever.\n>\n> I've replied to this above.\n\nNot really, even if we went for this one-HEAD-version-to-rule-them-all\nplan wouldn't it make more sense to generalize it as a\nrefs/remotes/origin/magic-config, and we'd discover a \".gitconfig\" file\nunder that commit/tree.\n\nI.e. whether we generalize this to config in general is orthagonal to\nwhether such config lives in HEAD or in a magic ref.\n\nWith hooks as config I don't see how you'd make any of this\nhook-specific, there's other config where the \"every revision ever\"\napplies much more strongly, e.g. sendemail.to. If that changed for this\nproject tomorrow you wouldn't want a patch based on \"maint\" to send\nthings to a different ML.\n\n>> 3. Work our way up to trusting that for more dangerous stuff, eventually\n>> hooks. Most of the legitimate concerns from others with this is\n>> having some UX where our users won't be trained to just blindly say\n>> \"yes\" to an alias/hook config that \"rm -rf's /\" or whatever.\n>> \n>> If we start experimenting with that with aliases or hooks that can\n>> run arbitrary code it's like handing a toddlder a shotgun, let's at\n>> least start with a sharp fork or something (less dangerous config) :)\n>> \n>> 4. People who want this \"I want my hooks to apply to all revisions ever\"\n>> could probably get 99% or 100% of what they want if their hook is\n>> just a stub that does the equivalent of:\n>> \n>> sh `curl https://git.google.com/$reponame/hooks/$hookname`\n>> \n>> You'd then simply forbid on your servers any changes to a .gitconfig\n>> that did anything with the hook.* namespace.\n>\n> This would work if set in .git/config (not version controlled), but not\n> .gitconfig (version controlled).\n\nSorry, what wouldn't work? I meant you'd forbid pushes to your in-repo\n.gitconfig in your \"master\" branch or whatever, just like you're\npresumably planning some stronger ACLs for this magic hook branch.\n\n>> With such an implementation you don't need a magic\n>> \"refs/remotes/origin/suggested-hooks\" refs, just some state machine (I\n>> suggested e.g. GPG signing chains as an eventual end-state, but \"show a\n>> diff every time\" would also do) that keeps track of what config (and\n>> hooks are just one such case) has been OK'd, and which has not.\n>\n> This sounds complicated.\n\nOn the contrary I think anything that leans into git's\ncontent-addressable security model is way less complicated. You don't\ncare who you fetched Junio's v2.32.0 tag from, what matters is that the\nsigning chain validates.\n\nThe plan of having this magic branch means a whole new trust model for\ngit, you trust magical authorized remotes. If you trust signed content\nchains you can trust hooks if their last modification can be traced to a\nsigning authority you trust.\n\n It's really just:\n\n if (hook_content_changed() && hook_content_same_as_in_ok'd_revision_from_upsteam())\n trust_hooks();\n\nBut while we're on the subject, it seems like a very generous assumption\nto think that just because you trust hooks at a given revision (or\nalways trust the latest), that you implicitly trust them when *combined\nwith* all past and future revisions from the same repository.\n\nEven without a malicious actor that seems like it'll inevitably break in\nall sorts of data-destroying ways. E.g. people commit stuff\naccidentally. A hook run under a \"git bisect\" that naïvely does an \"rm\n*\" will eat your data if you land on a revision that an in-tree \"-rf\"\nfile.\n\nBut once you get to a malicious actor who can say push a topic branch\nbut not hook updates, will your hooks deal with files with whitespace in\nthem, arbitrary crafted content etc?\n\nSo I'd think that's an even better reason to prefer the in-repo\nper-revision atomically committed plan, and only trust hooks for the\nrevision they're shipped with, at least as a default git security model.\n\n>> I'd think it would even work better in the Googleplex, you could clone a\n>> co-worker's branch and execute their hooks, since they're the same as\n>> what you've pre-approved,\n>\n> In the presence of .gitconfig, how would you know?\n\nIf it's the same config, or you can automatically OK it. So \"same\" was\ndiscussed above, or you could trust any hook that's only doing a wget of\nsome trusted domain and piping that to \"sh\".\n\n>> you could even clone some random person's fork\n>> of a \"blessed\" project, because the hooks would be the same `sh $(curl\n>> )`. That validation could even be a system-level\n>> in-config hook on your laptop, thus bringing the whole thing full\n>> circle...\n>\n> Same here.\n>\n> In summary, I think your point of using hook configs + remote-suggested\n> configs instead of remote-suggested hooks is a reasonable one, but I\n> disagree with your reasons (or, at least, your reasons as I understand\n> them).\n\nYou trust e.g. chromium.git's hooks, but I clone it, patch it, and\nre-push it to somegithost.com URL. If you go with trusting content it\nbecomes easy to install those trusted hooks for the common case, but not\nif your entire trust model relies on what URL you git clone'd from.\n"},{"id":"428154","messageId":"YNExhalSLWvmky55@camp.crustytoothpaste.net","threadId":"55946","inReplyTo":"87fsxc47le.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-06-22T00:40:37Z","receivedAt":"2021-06-22T00:40:43Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-06-20 at 19:51:04, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Jun 16 2021, Jonathan Tan wrote:\n> \n> > This but does not use any features from es/config-based-hooks but is\n> > implemented on that branch anyway because firstly, I need an existing\n> > command to attach the \"autoupdate\" subcommand (and \"git hook\" works),\n> > and secondly, when we test this at $DAYJOB, we will be testing it\n> > together with the aforementioned branch.\n> >\n> > I have had to make several design choices (which I will discuss later),\n> > but now with this implementation, the following workflow is possible:\n> >\n> > 1. The remote repo administrator creates a new branch\n> > \"refs/heads/suggested-hooks\" pointing to a commit that has all the\n> > hooks that the administrator wants to suggest. The hooks are\n> > directly referenced by the commit tree (i.e. they are in the \"/\"\n> > directory).\n> >\n> > 2. When a user clones, Git notices that\n> > \"refs/remotes/origin/suggested-hooks\" is present and prints out a\n> > message about a command that can be run.\n> >\n> > 3. If the user runs that command, Git will install the hooks pointed to\n> > by that ref, and set hook.autoupdate to true. This config variable\n> > is checked whenever \"git fetch\" is run: whenever it notices that\n> > \"refs/remotes/origin/suggested-hooks\" changes, it will reinstall the\n> > hooks.\n> >\n> > 4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks\n> > will remain.\n> >\n> > Design choices:\n> >\n> > 1. Where should the suggested hooks be in the remote repo? A branch,\n> > a non-branch ref, a config? I think that a branch is best - it is\n> > relatively well-understood and any hooks there can be\n> > version-controlled (and its history is independent of the other\n> > branches).\n> \n> First, unlike brian I don't (I hope I'm fairly summarizing his view\n> here) disagree mostly or entirely with the existence of such a feature\n> at all. I mean, I get the viewpoint that git shouldn't bless what\n> amounts to an active RCE from the remote.\n\nIt's accurate that I'm generally opposed to such a feature. I feel that\nsuggesting people install hooks is likely to lead to social engineering\nattacks, and it's also likely to lead to bad practices such as the\nexpectation that all developers will install hooks or the use of hooks\ninstead of CI or other effective controls.\n\nIf we do add this feature (which, as I said, I'm opposed to) and we\ndecide to store it in a ref, that ref should not be a normal branch by\ndefault (it should be a special one-level ref, like refs/stash or such),\nand the ref name should be configurable. Not all developers use English\nas their working language and we should respect that.\n\nIn addition, there should be an advice.* option that allows people to\nturn this off once and for all, and it should be clearly documented.\nIdeally it should be off by default.\n\n> I think I get why you want to do it that way, I just don't get why, as\n> mostly noted in those earlier rounds why it wouldn't be a better\n> approach / more straightforward / more git-y to:\n> \n> 1. Work on getting hooks driven by config Emily's series / my split-out \"base\" topic>\n> 2. Have a facility to read an in-repo '.gitconfig'; have lots of safety\n> valves etc. around this, I suggested starting with a whitelist of the\n> N least dangerous config options, e.g. some diff viewing options, or\n> a suggested sendemail.to or whatever.\n\nThis also makes me deeply nervous for much of the same reasons. There\nare situations where e.g. ignoring whitespace can lead to security\nproblems in code review (think Python), and in general it's hard to\nreason about all the ways people can do malicious things. Typically\nadding untrusted config ends poorly (think of all the modeline\nvulnerabilities in Vim).\n\nI'd definitely want support for this to be off with no prompting by\ndefault.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"428156","messageId":"20210622012712.1203208-1-jonathantanmy@google.com","threadId":"55946","inReplyTo":"87k0mn2dd3.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-22T01:27:12Z","receivedAt":"2021-06-22T01:27:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> >> And then, instead of doing what I'd think would be the natural result of\n> >> that: Simply supporting an in-repo top-level \".gitconfig\" file. We're\n> >> still going to seemingly forever have them be an even more special\n> >> snowflake with this facility, and the reason seems to be mostly/entirely\n> >> to do with working around some aspect or restriction of Google's\n> >> internal infrastructure.\n> >\n> > I don't think that this is \"natural\". In particular, I still don't think\n> > that hooks should be tied to code revision. E.g. if we make commits\n> > based on an old revision and push them, we still want them to follow the\n> > latest requirements.\n> \n> Even for real-world centralized workflow situations where I've seen\n> people think they want that, and the end of the day they almost never\n> actually want that.\n> \n> Even something like code linting is a good example, to make it\n> Google-specific: say for a Go project: Are you going to pin your linting\n> tool/version to whatever understood your YAML format for the linter as\n> it was specced 10 years ago when the project started? It's simply a\n> giant hassle to have a piece of code operate on every version of your\n> project ever in a way that doesn't break.\n> \n> I think in practice the designers of this feature don't actually have\n> that in mind, but a \"close to trunk\" workflow, where you'd expect a hook\n> to only need to operate on revisions for the last few weeks or months,\n> because that'll be the oldest think people create new topics from.\n\nThis is probably true, but it still means that people would want any\nhook changes to work retroactively, even if it's just on code for the\nlast few months. So they still need some independence between the hook\nand the main code.\n\n> But I think the burden of proof is really on the other side here,\n> something that works entirely differently than the rest of git needs to\n> have a good reason. Our in-repo .gitattributes don't work like this, nor\n> .gitignore, .mailmap etc.\n\nNot all of Git works this way though - e.g. hooks themselves, and\nconfig.\n\n> There's also real world uses of git where the \"branches\" are wildly\n> divergent, e.g. I've worked on a system automation repo where the\n> \"master\" was just a stub template, and every team had their own almost\n> entirely different \"repo-like\" branch. Probably a bad idea for various\n> reasons, but Git supports it just fine.\n\nI presume this repo either does not use hooks, or its hooks are built\nfor such a branch setup? I envision that the admins of the remote would\nonly suggest hooks that work with their branch setup.\n\n> For the centralized use-case what's the problem with just having the\n> hook do a 'for-each-ref --format=' invocation or \"cat-file --batch\" on\n> the \"origin\", and eval what it finds there? I'd think that gives you\n> what you want for the more centarlized workflow, while leaving git's\n> implementation working like the rest of our in-repo files.\n\nThis would future-proof such a hook, but not make it work retroactively\nin the past. (The repo admin could just include no-op hook for all hooks\nto future-proof, I guess, but this wouldn't work if we ever introduced\nnew hooks.)\n\n> >> I think it's just un-git-y to have a meta-branch that in some way drives\n> >> not only all other branches, but all other revisions of all branches,\n> >> ever.\n> >>\n> >> It breaks expectations around git in lots of different ways, you can't\n> >> fetch a single branch and get its hooks,\n> >\n> > Are you saying that each branch should have its own hooks? That might be\n> > reasonable in certain projects, but I don't see how that is a Git\n> > expectation.\n> \n> It's a git expectation now that I can add git.git as a remote, also\n> chromium.git, and linux.git, fetch them all, and happily switch in the\n> same repo between entirely different codebases that don't share a\n> history.\n\nThis expectation holds only if you know that your hooks can tolerate\nsuch a scenario. If you're using remote-suggested hooks, I don't think\nit's a matter of \"before, I could have many remotes but now I cannot\"\nbut \"before, I had to install these hooks myself and now it's automatic\"\n- whether this workflow is supported or not would be the same before and\nnow.\n\n> >> I think I get why you want to do it that way, I just don't get why, as\n> >> mostly noted in those earlier rounds why it wouldn't be a better\n> >> approach / more straightforward / more git-y to:\n> >>\n> >> 1. Work on getting hooks driven by config >> Emily's series / my split-out \"base\" topic>\n> >> 2. Have a facility to read an in-repo '.gitconfig'; have lots of safety\n> >> valves etc. around this, I suggested starting with a whitelist of the\n> >> N least dangerous config options, e.g. some diff viewing options, or\n> >> a suggested sendemail.to or whatever.\n> >\n> > I've replied to this above.\n> \n> Not really, even if we went for this one-HEAD-version-to-rule-them-all\n> plan wouldn't it make more sense to generalize it as a\n> refs/remotes/origin/magic-config, and we'd discover a \".gitconfig\" file\n> under that commit/tree.\n> \n> I.e. whether we generalize this to config in general is orthagonal to\n> whether such config lives in HEAD or in a magic ref.\n> \n> With hooks as config I don't see how you'd make any of this\n> hook-specific, there's other config where the \"every revision ever\"\n> applies much more strongly, e.g. sendemail.to. If that changed for this\n> project tomorrow you wouldn't want a patch based on \"maint\" to send\n> things to a different ML.\n\nMy opposition to .gitconfig was that it is per-commit, but here it seems\nthat you're saying that there are reasons for it not to be per-commit\n(e.g. the sendemail example).\n\n> >> 4. People who want this \"I want my hooks to apply to all revisions ever\"\n> >> could probably get 99% or 100% of what they want if their hook is\n> >> just a stub that does the equivalent of:\n> >>\n> >> sh `curl https://git.google.com/$reponame/hooks/$hookname`\n> >>\n> >> You'd then simply forbid on your servers any changes to a .gitconfig\n> >> that did anything with the hook.* namespace.\n> >\n> > This would work if set in .git/config (not version controlled), but not\n> > .gitconfig (version controlled).\n> \n> Sorry, what wouldn't work? I meant you'd forbid pushes to your in-repo\n> .gitconfig in your \"master\" branch or whatever, just like you're\n> presumably planning some stronger ACLs for this magic hook branch.\n\nThe \"I want my hooks to apply to all revisions ever\" wouldn't work if it\nwas per-commit .gitconfig. (It would work if it was in the magic\nbranch, but at that time, what I understood by \".gitconfig\" is the\nper-commit version.)\n\n> >> With such an implementation you don't need a magic\n> >> \"refs/remotes/origin/suggested-hooks\" refs, just some state machine (I\n> >> suggested e.g. GPG signing chains as an eventual end-state, but \"show a\n> >> diff every time\" would also do) that keeps track of what config (and\n> >> hooks are just one such case) has been OK'd, and which has not.\n> >\n> > This sounds complicated.\n> \n> On the contrary I think anything that leans into git's\n> content-addressable security model is way less complicated. You don't\n> care who you fetched Junio's v2.32.0 tag from, what matters is that the\n> signing chain validates.\n> \n> The plan of having this magic branch means a whole new trust model for\n> git, you trust magical authorized remotes. If you trust signed content\n> chains you can trust hooks if their last modification can be traced to a\n> signing authority you trust.\n\nThe security model might be preexisting, but the things needing to be\nbuilt around it make it more complicated. Take this case - I presume\nthis means that the client would need to tell Git that a certain public\nkey is considered trusted (perhaps prompted by the server upon clone, so\nthe client would only need to copy and paste \"git trust-key $HTTPS_URL\"\nor something like that). Whenever a user wants to update a hook, they\nwill make a commit, and when the PR is merged, the merge commit (or the\ntip commit if fast-forwarded) must be signed with the same key.\n\nCompare this with trusting that commits coming from a certain HTTPS URL\nare fine. The workflows are the same as for a regular code commit.\n\n> It's really just:\n> \n> if (hook_content_changed() && hook_content_same_as_in_ok'd_revision_from_upsteam())\n> trust_hooks();\n> \n> But while we're on the subject, it seems like a very generous assumption\n> to think that just because you trust hooks at a given revision (or\n> always trust the latest), that you implicitly trust them when *combined\n> with* all past and future revisions from the same repository.\n> \n> Even without a malicious actor that seems like it'll inevitably break in\n> all sorts of data-destroying ways. E.g. people commit stuff\n> accidentally. A hook run under a \"git bisect\" that navely does an \"rm\n> *\" will eat your data if you land on a revision that an in-tree \"-rf\"\n> file.\n\nI think we have the same problem with current hooks and other config,\nand we have been dealing with them relatively well (as far as I know).\n\n> But once you get to a malicious actor who can say push a topic branch\n> but not hook updates, will your hooks deal with files with whitespace in\n> them, arbitrary crafted content etc?\n> \n> So I'd think that's an even better reason to prefer the in-repo\n> per-revision atomically committed plan, and only trust hooks for the\n> revision they're shipped with, at least as a default git security model.\n\nThe malicious actor could include the same hook in their topic branch,\nso I don't see how the per-revision hook is better than the all-revision\nhook.\n\n> >> I'd think it would even work better in the Googleplex, you could clone a\n> >> co-worker's branch and execute their hooks, since they're the same as\n> >> what you've pre-approved,\n> >\n> > In the presence of .gitconfig, how would you know?\n> \n> If it's the same config, or you can automatically OK it. So \"same\" was\n> discussed above, or you could trust any hook that's only doing a wget of\n> some trusted domain and piping that to \"sh\".\n\nExtending trust to hooks that are exactly the same makes sense.\n\n> You trust e.g. chromium.git's hooks, but I clone it, patch it, and\n> re-push it to somegithost.com URL. If you go with trusting content it\n> becomes easy to install those trusted hooks for the common case, but not\n> if your entire trust model relies on what URL you git clone'd from.\n\nIf the repo administrator provides a key and signs their hooks, then the\nrepo itself is indeed more portable and does not need to come from a\nsingle source. This may be useful, but I'm not sure how useful this is,\nsince hooks are typically provided by projects. And as far as I know\nprojects generally have one source of truth, so the portability is not a\nbig plus.\n"},{"id":"428342","messageId":"20210623225809.4023571-1-jonathantanmy@google.com","threadId":"55946","inReplyTo":"YNExhalSLWvmky55@camp.crustytoothpaste.net","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-23T22:58:09Z","receivedAt":"2021-06-23T22:58:23Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > First, unlike brian I don't (I hope I'm fairly summarizing his view\n> > here) disagree mostly or entirely with the existence of such a feature\n> > at all. I mean, I get the viewpoint that git shouldn't bless what\n> > amounts to an active RCE from the remote.\n> \n> It's accurate that I'm generally opposed to such a feature.\n\nFrom emails like [1], I think that you have understood both the pros and\ncons, and decided that the cons outweigh the pros, which is fair. I'll\nreply so that others can know what I think about these points.\n\n[1] https://lore.kernel.org/git/YGzrfaSC4xd75j2U@camp.crustytoothpaste.net/\n\n> I feel that\n> suggesting people install hooks is likely to lead to social engineering\n> attacks,\n\nI think that this vector of social engineering attack already exists, as\nthere are projects that without malice recommend hook installation (so\nan attacker could masquerade as such). It is true that Git itself\nprinting a message recommending hook installation perhaps could lend a\nfalse sense of security, but at least we can control what that message\nsays (as opposed to a project recommending a third-party tool, all with\nmessaging out of our control).\n\n> and it's also likely to lead to bad practices such as the\n> expectation that all developers will install hooks or the use of hooks\n> instead of CI or other effective controls.\n\nI agree that hooks can be overused or misused, but there are still\nlegitimate uses of it that a project might want to use.\n\n> If we do add this feature (which, as I said, I'm opposed to) and we\n> decide to store it in a ref, that ref should not be a normal branch by\n> default (it should be a special one-level ref, like refs/stash or such),\n\nAny particular reason not to expose it as a branch (besides following\nfrom your general idea that a user should seek out such a feature and\nnot have it presented to them up-front)?\n\n> and the ref name should be configurable. Not all developers use English\n> as their working language and we should respect that.\n\nThat makes sense.\n\n> In addition, there should be an advice.* option that allows people to\n> turn this off once and for all, and it should be clearly documented.\n> Ideally it should be off by default.\n\nI don't think this would be considered \"advice\" like the other options,\nbut having an option to turn this off once and for all makes sense.\nMaking it off by default would probably mean that projects that use such\nhooks would recommend cloning with \"git -c my-config=1 clone $URL\", but\nperhaps that's OK.\n\n> > I think I get why you want to do it that way, I just don't get why, as\n> > mostly noted in those earlier rounds why it wouldn't be a better\n> > approach / more straightforward / more git-y to:\n> > \n> > 1. Work on getting hooks driven by config > Emily's series / my split-out \"base\" topic>\n> > 2. Have a facility to read an in-repo '.gitconfig'; have lots of safety\n> > valves etc. around this, I suggested starting with a whitelist of the\n> > N least dangerous config options, e.g. some diff viewing options, or\n> > a suggested sendemail.to or whatever.\n> \n> This also makes me deeply nervous for much of the same reasons. There\n> are situations where e.g. ignoring whitespace can lead to security\n> problems in code review (think Python), and in general it's hard to\n> reason about all the ways people can do malicious things. Typically\n> adding untrusted config ends poorly (think of all the modeline\n> vulnerabilities in Vim).\n> \n> I'd definitely want support for this to be off with no prompting by\n> default.\n\nTo use your example, the model we're proposing is more of only using the\nmodelines from sources we trust - as opposed to ensuring that all\npossible options set by modelines are benign. Admittedly, the\nadministrator of the source may have difficulty ensuring that bad code\ndoesn't slip through code review, for example, but that is a problem\nthey already deal with (at least for projects with any form of\nexecutable code in them, e.g. production code or a build script).\n"},{"id":"428438","messageId":"YNURHyBLaPjlKSSn@camp.crustytoothpaste.net","threadId":"55946","inReplyTo":"20210623225809.4023571-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-06-24T23:11:27Z","receivedAt":"2021-06-24T23:11:39Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-06-23 at 22:58:09, Jonathan Tan wrote:\n> > If we do add this feature (which, as I said, I'm opposed to) and we\n> > decide to store it in a ref, that ref should not be a normal branch by\n> > default (it should be a special one-level ref, like refs/stash or such),\n> \n> Any particular reason not to expose it as a branch (besides following\n> from your general idea that a user should seek out such a feature and\n> not have it presented to them up-front)?\n\nBranches are for the main code of the project. While it's possible to\nhave orphan branches that do other things, I think that's in general an\nanti-pattern, and using a special ref for things which are separate and\nindependent from the main code of the repository would be a more elegant\nsolution.\n\n> > In addition, there should be an advice.* option that allows people to\n> > turn this off once and for all, and it should be clearly documented.\n> > Ideally it should be off by default.\n> \n> I don't think this would be considered \"advice\" like the other options,\n> but having an option to turn this off once and for all makes sense.\n> Making it off by default would probably mean that projects that use such\n> hooks would recommend cloning with \"git -c my-config=1 clone $URL\", but\n> perhaps that's OK.\n\nSure, I'm not picky about what it looks like in \"advice\" vs something\nelse. I think forcing projects to explicitly opt-in to this behavior\nmeans that the social engineering and security problems are much\nreduced, and while I'm still not wild about the idea, I would feel much\nbetter about it.\n\n> > This also makes me deeply nervous for much of the same reasons. There\n> > are situations where e.g. ignoring whitespace can lead to security\n> > problems in code review (think Python), and in general it's hard to\n> > reason about all the ways people can do malicious things. Typically\n> > adding untrusted config ends poorly (think of all the modeline\n> > vulnerabilities in Vim).\n> > \n> > I'd definitely want support for this to be off with no prompting by\n> > default.\n> \n> To use your example, the model we're proposing is more of only using the\n> modelines from sources we trust - as opposed to ensuring that all\n> possible options set by modelines are benign. Admittedly, the\n> administrator of the source may have difficulty ensuring that bad code\n> doesn't slip through code review, for example, but that is a problem\n> they already deal with (at least for projects with any form of\n> executable code in them, e.g. production code or a build script).\n\nAs I think I've previously mentioned, I don't want to receive\nconfiguration of my development environment from sources I trust. Even\nat work, I don't want the repositories I work with to modify my\ndevelopment environment in this way. I tend to have a highly customized\nconfiguration that breaks many people's expectations about tooling, so\nthe cases that this isn't a security problem (in repositories I trust)\ncan still result in a functionality problem.\n\nAlso, since we don't know what repositories the user trusts, the only\nsafe assumption is that the user trusts nothing unless they explicitly\ntell us.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"428649","messageId":"xmqqa6n960t6.fsf@gitster.g","threadId":"55946","inReplyTo":"YNExhalSLWvmky55@camp.crustytoothpaste.net","subject":"Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-28T23:12:37Z","receivedAt":"2021-06-28T23:12:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> If we do add this feature (which, as I said, I'm opposed to) and we\n> decide to store it in a ref, that ref should not be a normal branch by\n> default (it should be a special one-level ref, like refs/stash or such),\n> and the ref name should be configurable. Not all developers use English\n> as their working language and we should respect that.\n\nJust this part.\n\nI am not sure what you are trying to achieve by requiring it to be\nconfigurable. After all, even for names of branches that are used\nto store the code, which is of more significance than this \"unusual\"\nref, we've lived with a hardcoded 'master' and with the server-end\nadvertisability of the configured values (i.e. \"git clone\" was\ndesigned to figure out if the origin used a custom name for the\nprimary line of history and use the same name from the beginning),\nthe end-user sitting on the receiving end did not have to do\nanything special even when the project wanted to use a custom name.\n\nBut this \"unusual\" ref would not have a natural equivalent of \"the\norigin side can point the primary branch with its HEAD\", so by\nallowing it to be configurable, you are pretty much closing the door\nfor those who blindly honor whatever the origin tells them to use\nwhen running \"git clone\" from doing so. I agree that it is a good\nsecurity measure (and I am not sold to the \"remote suggested hooks\"\nidea in the first place), but is that the real reason why you\nsuggested configurability?\n\n"},{"id":"430362","messageId":"cover.1626453569.git.jonathantanmy@google.com","threadId":"55946","inReplyTo":"cover.1623881977.git.jonathantanmy@google.com","subject":"[RFC PATCH v2 0/2] MVP implementation of remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-16T17:57:39Z","receivedAt":"2021-07-16T17:57:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Here's an updated version of the remote-suggested hooks RFC, hopefully\naddressing some of the concerns people have brought up (e.g.\nauto-updating without letting the user verify, or prompting about hooks\nby default). It consists of two main parts:\n\n - Non-interactive prompts during certain Git commands about the\n existence of hooks. These prompts are turned *off* by default.\n\n - New \"git hook\" subcommands that can install these hooks (so that the\n aforementioned Git prompts or out-of-band installation instructions\n can tell users to install these hooks in a platform-independent way).\n These subcommands work whether or not prompts are enabled.\n\nYou can see how they work in patch 2's t1361.\n\nIn doing this, I have tried to scope down the overall effort as much as\npossible (to avoid security risks as much as possible, among other\nthings), concentrating on situations that server-side hooks cannot\nhandle. I think the main thing is that server-side hooks can check and\nprompt upon push, but they cannot catch problems that the client could\nhave noticed upon commit; if the end user pushes a commit chain and only\nnotices problems then, the user would have to redo the commits (possibly\nthrough an interactive rebase on the spot). Catching problems upon\ncommit would prevent this problem, only necessitating amending the\ncommit.\n\nIn the Gerrit use case, Gerrit requires a specific \"Change-Id\" trailer\nin the commit message, but I can foresee the same issue in projects that\nrequire their commit messages to fit a certain template or projects that\nwant lints to be run but their builds, for some reason, don't run\narbitrary code (or they have no builds at all).\n\nTo that end, I have added a prompt for the \"commit-msg\" hook if the\nremote has suggested one, but there are none currently installed.\n(Similar prompts could be added for other commit-related hooks.) The\nother prompts are upon clone and upon fetch (if the hooks have been\nupdated).\n\nJonathan Tan (2):\n hook: move list of hooks\n hook: remote-suggested hooks\n\n builtin/bugreport.c | 38 +-----\n builtin/clone.c | 12 ++\n builtin/fetch.c | 17 +++\n builtin/hook.c | 17 ++-\n hook.c | 185 +++++++++++++++++++++++++++++-\n hook.h | 6 +\n t/t1361-remote-suggested-hooks.sh | 105 +++++++++++++++++\n 7 files changed, 340 insertions(+), 40 deletions(-)\n create mode 100755 t/t1361-remote-suggested-hooks.sh\n\n-- \n2.32.0.402.g57bb445576-goog\n\n"},{"id":"430363","messageId":"74ed668e9bbf56bb7898bcdfaaf77db4f3205fe5.1626453569.git.jonathantanmy@google.com","threadId":"55946","inReplyTo":"cover.1626453569.git.jonathantanmy@google.com","subject":"[RFC PATCH v2 1/2] hook: move list of hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-16T17:57:40Z","receivedAt":"2021-07-16T17:57:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The list of hooks will need to be used outside bugreport, so move it to\na central location.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/bugreport.c | 38 +++-----------------------------------\n hook.c | 34 ++++++++++++++++++++++++++++++++++\n hook.h | 3 +++\n 3 files changed, 40 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 190272ba70..4e0806dff3 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -41,38 +41,6 @@ static void get_system_info(struct strbuf *sys_info)\n \n static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n {\n-\t/*\n-\t * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n-\t * so below is a transcription of `git help hooks`. Later, this should\n-\t * be replaced with some programmatically generated list (generated from\n-\t * doc or else taken from some library which tells us about all the\n-\t * hooks)\n-\t */\n-\tstatic const char *hook[] = {\n-\t\t\"applypatch-msg\",\n-\t\t\"pre-applypatch\",\n-\t\t\"post-applypatch\",\n-\t\t\"pre-commit\",\n-\t\t\"pre-merge-commit\",\n-\t\t\"prepare-commit-msg\",\n-\t\t\"commit-msg\",\n-\t\t\"post-commit\",\n-\t\t\"pre-rebase\",\n-\t\t\"post-checkout\",\n-\t\t\"post-merge\",\n-\t\t\"pre-push\",\n-\t\t\"pre-receive\",\n-\t\t\"update\",\n-\t\t\"post-receive\",\n-\t\t\"post-update\",\n-\t\t\"push-to-checkout\",\n-\t\t\"pre-auto-gc\",\n-\t\t\"post-rewrite\",\n-\t\t\"sendemail-validate\",\n-\t\t\"fsmonitor-watchman\",\n-\t\t\"p4-pre-submit\",\n-\t\t\"post-index-change\",\n-\t};\n \tint i;\n \n \tif (nongit) {\n@@ -81,9 +49,9 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n \t\treturn;\n \t}\n \n-\tfor (i = 0; i < ARRAY_SIZE(hook); i++)\n-\t\tif (hook_exists(hook[i], HOOKDIR_USE_CONFIG))\n-\t\t\tstrbuf_addf(hook_info, \"%s\\n\", hook[i]);\n+\tfor (i = 0; i < hook_name_count; i++)\n+\t\tif (hook_exists(hook_name[i], HOOKDIR_USE_CONFIG))\n+\t\t\tstrbuf_addf(hook_info, \"%s\\n\", hook_name[i]);\n }\n \n static const char * const bugreport_usage[] = {\ndiff --git a/hook.c b/hook.c\nindex ff80e52edd..3ccacb72fa 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -5,6 +5,40 @@\n #include \"run-command.h\"\n #include \"prompt.h\"\n \n+/*\n+ * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n+ * so below is a transcription of `git help hooks`. Later, this should\n+ * be replaced with some programmatically generated list (generated from\n+ * doc or else taken from some library which tells us about all the\n+ * hooks)\n+ */\n+const char *hook_name[] = {\n+\t\"applypatch-msg\",\n+\t\"pre-applypatch\",\n+\t\"post-applypatch\",\n+\t\"pre-commit\",\n+\t\"pre-merge-commit\",\n+\t\"prepare-commit-msg\",\n+\t\"commit-msg\",\n+\t\"post-commit\",\n+\t\"pre-rebase\",\n+\t\"post-checkout\",\n+\t\"post-merge\",\n+\t\"pre-push\",\n+\t\"pre-receive\",\n+\t\"update\",\n+\t\"post-receive\",\n+\t\"post-update\",\n+\t\"push-to-checkout\",\n+\t\"pre-auto-gc\",\n+\t\"post-rewrite\",\n+\t\"sendemail-validate\",\n+\t\"fsmonitor-watchman\",\n+\t\"p4-pre-submit\",\n+\t\"post-index-change\",\n+};\n+int hook_name_count = ARRAY_SIZE(hook_name);\n+\n void free_hook(struct hook *ptr)\n {\n \tif (ptr) {\ndiff --git a/hook.h b/hook.h\nindex f32189380a..d902166408 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -4,6 +4,9 @@\n #include \"strvec.h\"\n #include \"run-command.h\"\n \n+extern const char *hook_name[];\n+extern int hook_name_count;\n+\n struct hook {\n \tstruct list_head list;\n \t/*\n-- \n2.32.0.402.g57bb445576-goog\n\n"},{"id":"430364","messageId":"1ec1c958eb2b8aa2581280d050836dd0e7f6edef.1626453569.git.jonathantanmy@google.com","threadId":"55946","inReplyTo":"cover.1626453569.git.jonathantanmy@google.com","subject":"[RFC PATCH v2 2/2] hook: remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-16T17:57:41Z","receivedAt":"2021-07-16T17:57:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach the \"git hook install all|\" command, that can install\none or all remote-suggested hooks.\n\nIf a configuration option hook.promptRemoteSuggested is set, inform the\nuser of the aforementioned command:\n\n - when cloning, and refs/remotes/origin/suggested-hooks is present in\n the newly cloned repo\n - when fetching, and refs/remotes/origin/suggested-hooks is updated\n - when committing, there is a remote-suggested commit-msg hook, and\n there is currently no commit-msg hook configured\n\nNEEDSWORK: Write a more detailed commit message once the design is\nfinalized.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/clone.c | 12 +++\n builtin/fetch.c | 17 ++++\n builtin/hook.c | 17 +++-\n hook.c | 151 +++++++++++++++++++++++++++++-\n hook.h | 3 +\n t/t1361-remote-suggested-hooks.sh | 105 +++++++++++++++++++++\n 6 files changed, 300 insertions(+), 5 deletions(-)\n create mode 100755 t/t1361-remote-suggested-hooks.sh\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 2a2a03bf76..c2c8596aa9 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1393,6 +1393,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t branch_top.buf, reflog_msg.buf, transport,\n \t\t\t !is_local);\n \n+\tif (hook_should_prompt_suggestions()) {\n+\t\tfor (ref = mapped_refs; ref; ref = ref->next) {\n+\t\t\tif (ref->peer_ref &&\n+\t\t\t !strcmp(ref->peer_ref->name,\n+\t\t\t\t \"refs/remotes/origin/suggested-hooks\")) {\n+\t\t\t\tfprintf(stderr, _(\"The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\\n\"\n+\t\t\t\t\t\t \"Run `git hook install all` to install them.\\n\"));\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n \n \t/*\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 769af53ca4..e86c312473 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -28,6 +28,7 @@\n #include \"promisor-remote.h\"\n #include \"commit-graph.h\"\n #include \"shallow.h\"\n+#include \"hook.h\"\n \n #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n \n@@ -1313,6 +1314,22 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)\n \t\t\t\t ref_map);\n \ttransport_unlock_pack(transport);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n+\n+\tif (hook_should_prompt_suggestions()) {\n+\t\tstruct ref *ref;\n+\n+\t\tfor (ref = ref_map; ref; ref = ref->next) {\n+\t\t\tif (ref->peer_ref &&\n+\t\t\t !strcmp(ref->peer_ref->name,\n+\t\t\t\t \"refs/remotes/origin/suggested-hooks\") &&\n+\t\t\t oidcmp(&ref->old_oid, &ref->peer_ref->old_oid)) {\n+\t\t\t\tfprintf(stderr, _(\"The remote has updated its suggested hooks.\\n\"));\n+\t\t\t\tfprintf(stderr, _(\"Run 'git hook install all' to update.\\n\"));\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \treturn ret;\n }\n \ndiff --git a/builtin/hook.c b/builtin/hook.c\nindex c79a961e80..0334fee967 100644\n--- a/builtin/hook.c\n+++ b/builtin/hook.c\n@@ -139,6 +139,17 @@ static int run(int argc, const char **argv, const char *prefix)\n \treturn rc;\n }\n \n+static int install(int argc, const char **argv, const char *prefix)\n+{\n+\tif (argc < 1)\n+\t\tdie(_(\"You must specify a hook event to install.\"));\n+\tif (!strcmp(argv[1], \"all\"))\n+\t\thook_update_suggested(NULL);\n+\telse\n+\t\thook_update_suggested(argv[1]);\n+\treturn 0;\n+}\n+\n int cmd_hook(int argc, const char **argv, const char *prefix)\n {\n \tconst char *run_hookdir = NULL;\n@@ -152,10 +163,6 @@ int cmd_hook(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_hook_options,\n \t\t\t builtin_hook_usage, PARSE_OPT_KEEP_UNKNOWN);\n \n-\t/* after the parse, we should have \" \" */\n-\tif (argc < 2)\n-\t\tusage_with_options(builtin_hook_usage, builtin_hook_options);\n-\n \tgit_config(git_default_config, NULL);\n \n \n@@ -185,6 +192,8 @@ int cmd_hook(int argc, const char **argv, const char *prefix)\n \t\treturn list(argc, argv, prefix);\n \tif (!strcmp(argv[0], \"run\"))\n \t\treturn run(argc, argv, prefix);\n+\tif (!strcmp(argv[0], \"install\"))\n+\t\treturn install(argc, argv, prefix);\n \n \tusage_with_options(builtin_hook_usage, builtin_hook_options);\n }\ndiff --git a/hook.c b/hook.c\nindex 3ccacb72fa..32eee9abb6 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -4,6 +4,12 @@\n #include \"config.h\"\n #include \"run-command.h\"\n #include \"prompt.h\"\n+#include \"commit.h\"\n+#include \"object.h\"\n+#include \"refs.h\"\n+#include \"tree-walk.h\"\n+#include \"tree.h\"\n+#include \"streaming.h\"\n \n /*\n * NEEDSWORK: Doesn't look like there is a list of all possible hooks;\n@@ -476,6 +482,60 @@ static int notify_hook_finished(int result,\n \treturn 0;\n }\n \n+static struct tree *remote_suggested_hook_tree(int *warning_printed)\n+{\n+\tstruct object_id oid;\n+\tstruct object *obj;\n+\tstruct tree *tree;\n+\n+\tif (read_ref(\"refs/remotes/origin/suggested-hooks\", &oid))\n+\t\treturn NULL;\n+\n+\tobj = parse_object(the_repository, &oid);\n+\tif (obj == NULL) {\n+\t\twarning(_(\"object pointed to by refs/remotes/origin/suggested-hooks '%s' does not exist\"),\n+\t\t\toid_to_hex(&oid));\n+\t\tif (warning_printed)\n+\t\t\t*warning_printed = 1;\n+\t\treturn NULL;\n+\t}\n+\tif (obj->type != OBJ_COMMIT) {\n+\t\twarning(_(\"object pointed to by refs/remotes/origin/suggested-hooks '%s' is not a commit\"),\n+\t\t\toid_to_hex(&oid));\n+\t\tif (warning_printed)\n+\t\t\t*warning_printed = 1;\n+\t\treturn NULL;\n+\t}\n+\n+\ttree = get_commit_tree((struct commit *) obj);\n+\tif (parse_tree(tree)) {\n+\t\twarning(_(\"could not parse tree\"));\n+\t\tif (warning_printed)\n+\t\t\t*warning_printed = 1;\n+\t\treturn NULL;\n+\t}\n+\n+\treturn tree;\n+}\n+\n+static int has_suggested_hook(const char *hookname)\n+{\n+\tstruct tree *tree;\n+\tstruct tree_desc desc;\n+\tstruct name_entry entry;\n+\n+\ttree = remote_suggested_hook_tree(NULL);\n+\tif (!tree)\n+\t\treturn 0;\n+\n+\tinit_tree_desc(&desc, tree->buffer, tree->size);\n+\twhile (tree_entry(&desc, &entry)) {\n+\t\tif (!strcmp(hookname, entry.path))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n int run_hooks(const char *hookname, struct run_hooks_opt *options)\n {\n \tstruct list_head *to_run, *pos = NULL, *tmp = NULL;\n@@ -497,8 +557,16 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options)\n \t\t\t list_del(pos);\n \t}\n \n-\tif (list_empty(to_run))\n+\tif (list_empty(to_run)) {\n+\t\tif (!strcmp(\"commit-msg\", hookname)) {\n+\t\t\tif (has_suggested_hook(hookname) &&\n+\t\t\t !hook_exists(hookname, HOOKDIR_USE_CONFIG)) {\n+\t\t\t\tfprintf(stderr, _(\"No commit-msg hook has been configured, but one is suggested by the remote.\\n\"));\n+\t\t\t\tfprintf(stderr, _(\"Run 'git hook install commit-msg' to install it.\\n\"));\n+\t\t\t}\n+\t\t}\n \t\treturn 0;\n+\t}\n \n \tcb_data.head = to_run;\n \tcb_data.run_me = list_entry(to_run->next, struct hook, list);\n@@ -515,3 +583,84 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options)\n \n \treturn cb_data.rc;\n }\n+\n+static int is_hook(const char *filename)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < hook_name_count; i++) {\n+\t\tif (!strcmp(filename, hook_name[i]))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+void hook_update_suggested(const char *hook_to_update)\n+{\n+\tstruct tree *tree;\n+\tint warning_printed = 0;\n+\tstruct tree_desc desc;\n+\tstruct name_entry entry;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tint hook_found = 0;\n+\n+\ttree = remote_suggested_hook_tree(&warning_printed);\n+\tif (!tree) {\n+\t\tif (!warning_printed)\n+\t\t\twarning(_(\"no such ref refs/remotes/origin/suggested-hooks, not updating hooks\"));\n+\t\treturn;\n+\t}\n+\n+\tinit_tree_desc(&desc, tree->buffer, tree->size);\n+\twhile (tree_entry(&desc, &entry)) {\n+\t\tint fd;\n+\n+\t\tif (hook_to_update && strcmp(hook_to_update, entry.path))\n+\t\t\t/*\n+\t\t\t * We only need to update one hook, and this is not the\n+\t\t\t * hook we're looking for\n+\t\t\t */\n+\t\t\tcontinue;\n+\n+\t\tif (!hook_to_update && !is_hook(entry.path)) {\n+\t\t\twarning(_(\"file '%s' is not a hook; ignoring\"),\n+\t\t\t\tentry.path);\n+\t\t\tcontinue;\n+\t\t}\n+\t\thook_found = 1;\n+\t\tif (S_ISDIR(entry.mode) || S_ISGITLINK(entry.mode)) {\n+\t\t\twarning(_(\"file '%s' is not an ordinary file; ignoring\"),\n+\t\t\t\tentry.path);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_reset(&path);\n+\t\tstrbuf_git_path(&path, \"hooks/%s\", entry.path);\n+\t\tfd = open(path.buf, O_WRONLY | O_CREAT, 0755);\n+\t\tif (fd < 0) {\n+\t\t\twarning_errno(_(\"could not create file '%s'; skipping this hook\"),\n+\t\t\t\t path.buf);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (stream_blob_to_fd(fd, &entry.oid, NULL, 1)) {\n+\t\t\twarning(_(\"could not write to file '%s'; skipping this hook\"),\n+\t\t\t\tpath.buf);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tclose(fd);\n+\t}\n+\tstrbuf_release(&path);\n+\n+\tif (hook_to_update && !hook_found)\n+\t\twarning(_(\"hook '%s' not found\"), hook_to_update);\n+\n+\treturn;\n+}\n+\n+int hook_should_prompt_suggestions(void)\n+{\n+\tint dest = 0;\n+\n+\treturn !git_config_get_bool(\"hook.promptremotesuggested\", &dest) &&\n+\t dest;\n+}\ndiff --git a/hook.h b/hook.h\nindex d902166408..438bf7122e 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -140,3 +140,6 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options);\n void free_hook(struct hook *ptr);\n /* Empties the list at 'head', calling 'free_hook()' on each entry */\n void clear_hook_list(struct list_head *head);\n+\n+void hook_update_suggested(const char *hook_to_update);\n+int hook_should_prompt_suggestions(void);\ndiff --git a/t/t1361-remote-suggested-hooks.sh b/t/t1361-remote-suggested-hooks.sh\nnew file mode 100755\nindex 0000000000..223c65ac99\n--- /dev/null\n+++ b/t/t1361-remote-suggested-hooks.sh\n@@ -0,0 +1,105 @@\n+#!/bin/sh\n+\n+test_description='remote-suggested hooks'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+\n+setup_server () {\n+\tgit init server &&\n+\ttest_when_finished \"rm -rf server\" &&\n+\tcat >server/commit-msg <<-'EOF' &&\n+\techo \"overwrite the commit message\" >$1\n+EOF\n+\tgit -C server add commit-msg &&\n+\ttest_commit -C server commit-with-hook &&\n+\tgit -C server checkout -b suggested-hooks\n+}\n+\n+add_hook_to_server () {\n+\t>server/pre-commit &&\n+\tgit -C server add pre-commit &&\n+\ttest_commit -C server additional-hook\n+}\n+\n+test_expect_success 'suggestion upon clone' '\n+\tsetup_server &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\tgit -c hook.promptRemoteSuggested=1 clone server client 2>err &&\n+\tgrep \"The remote has suggested hooks\" err\n+'\n+\n+test_expect_success 'no suggestion upon clone if not configured' '\n+\tsetup_server &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\tgit clone server client 2>err &&\n+\t! grep \"The remote has suggested hooks\" err\n+'\n+\n+test_expect_success 'suggestion upon fetch if server has updated hooks' '\n+\tsetup_server &&\n+\tgit clone server client &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\tadd_hook_to_server &&\n+\n+\tgit -C client -c hook.promptRemoteSuggested=1 fetch 2>err &&\n+\tgrep \"The remote has updated its suggested hooks\" err\n+'\n+\n+test_expect_success 'no suggestion upon fetch if not configured' '\n+\tsetup_server &&\n+\tgit clone server client &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\tadd_hook_to_server &&\n+\n+\tgit -C client fetch 2>err &&\n+\t! grep \"The remote has updated its suggested hooks\" err\n+'\n+\n+test_expect_success 'no suggestion upon fetch if server has not updated hooks' '\n+\tsetup_server &&\n+\tgit clone server client &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\tgit -C server checkout main &&\n+\ttest_commit -C server not-a-hook-update &&\n+\n+\tgit -C client -c hook.promptRemoteSuggested=1 fetch 2>err &&\n+\t! grep \"The remote has updated its suggested hooks\" err\n+'\n+\n+test_expect_success 'suggest commit-msg hook upon commit' '\n+\tsetup_server &&\n+\tgit clone server client &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\n+\ttest_commit -C client foo 2>err &&\n+\tgrep \"Run .git hook install commit-msg\" err\n+'\n+\n+test_expect_success 'install one suggested hook' '\n+\tsetup_server &&\n+\tgit clone server client &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\n+\tgit -C client hook install commit-msg &&\n+\n+\t# Check that the hook was written by making a commit\n+\ttest_commit -C client bar &&\n+\tgit -C client show >commit &&\n+\tgrep \"overwrite the commit message\" commit\n+'\n+\n+test_expect_success 'install all suggested hooks' '\n+\tsetup_server &&\n+\tadd_hook_to_server &&\n+\tgit clone server client &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\n+\tgit -C client hook install all &&\n+\ttest -f client/.git/hooks/pre-commit &&\n+\ttest -f client/.git/hooks/commit-msg\n+'\n+\n+test_done\n-- \n2.32.0.402.g57bb445576-goog\n\n"},{"id":"430550","messageId":"xmqq8s22ko8k.fsf@gitster.g","threadId":"55946","inReplyTo":"cover.1626453569.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH v2 0/2] MVP implementation of remote-suggested hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-19T21:06:19Z","receivedAt":"2021-07-19T23:11:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Here's an updated version of the remote-suggested hooks RFC, hopefully\n> addressing some of the concerns people have brought up (e.g.\n> auto-updating without letting the user verify, or prompting about hooks\n> by default). It consists of two main parts:\n>\n> - Non-interactive prompts during certain Git commands about the\n> existence of hooks. These prompts are turned *off* by default.\n>\n> - New \"git hook\" subcommands that can install these hooks (so that the\n> aforementioned Git prompts or out-of-band installation instructions\n> can tell users to install these hooks in a platform-independent way).\n> These subcommands work whether or not prompts are enabled.\n>\n> You can see how they work in patch 2's t1361.\n\nI really wanted to try this out, but which commit was this based on?\n\n> In doing this, I have tried to scope down the overall effort as much as\n> possible (to avoid security risks as much as possible, among other\n> things), concentrating on situations that server-side hooks cannot\n> handle. I think the main thing is that server-side hooks can check and\n> prompt upon push, but they cannot catch problems that the client could\n> have noticed upon commit; if the end user pushes a commit chain and only\n> notices problems then, the user would have to redo the commits (possibly\n> through an interactive rebase on the spot). Catching problems upon\n> commit would prevent this problem, only necessitating amending the\n> commit.\n>\n> In the Gerrit use case, Gerrit requires a specific \"Change-Id\" trailer\n> in the commit message, but I can foresee the same issue in projects that\n> require their commit messages to fit a certain template or projects that\n> want lints to be run but their builds, for some reason, don't run\n> arbitrary code (or they have no builds at all).\n>\n> To that end, I have added a prompt for the \"commit-msg\" hook if the\n> remote has suggested one, but there are none currently installed.\n> (Similar prompts could be added for other commit-related hooks.) The\n> other prompts are upon clone and upon fetch (if the hooks have been\n> updated).\n>\n> Jonathan Tan (2):\n> hook: move list of hooks\n> hook: remote-suggested hooks\n>\n> builtin/bugreport.c | 38 +-----\n> builtin/clone.c | 12 ++\n> builtin/fetch.c | 17 +++\n> builtin/hook.c | 17 ++-\n> hook.c | 185 +++++++++++++++++++++++++++++-\n> hook.h | 6 +\n> t/t1361-remote-suggested-hooks.sh | 105 +++++++++++++++++\n> 7 files changed, 340 insertions(+), 40 deletions(-)\n> create mode 100755 t/t1361-remote-suggested-hooks.sh\n"},{"id":"430551","messageId":"xmqqzguij8nq.fsf@gitster.g","threadId":"55946","inReplyTo":"1ec1c958eb2b8aa2581280d050836dd0e7f6edef.1626453569.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-19T21:28:09Z","receivedAt":"2021-07-19T23:11:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Teach the \"git hook install all|\" command, that can install\n> one or all remote-suggested hooks.\n>\n> If a configuration option hook.promptRemoteSuggested is set, inform the\n> user of the aforementioned command:\n>\n> - when cloning, and refs/remotes/origin/suggested-hooks is present in\n> the newly cloned repo\n> - when fetching, and refs/remotes/origin/suggested-hooks is updated\n> - when committing, there is a remote-suggested commit-msg hook, and\n> there is currently no commit-msg hook configured\n>\n> NEEDSWORK: Write a more detailed commit message once the design is\n> finalized.\n\nOK. Let's take this more as WIP.\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 2a2a03bf76..c2c8596aa9 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1393,6 +1393,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \t\t\t branch_top.buf, reflog_msg.buf, transport,\n> \t\t\t !is_local);\n> \n> +\tif (hook_should_prompt_suggestions()) {\n> +\t\tfor (ref = mapped_refs; ref; ref = ref->next) {\n> +\t\t\tif (ref->peer_ref &&\n> +\t\t\t !strcmp(ref->peer_ref->name,\n> +\t\t\t\t \"refs/remotes/origin/suggested-hooks\")) {\n> +\t\t\t\tfprintf(stderr, _(\"The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\\n\"\n> +\t\t\t\t\t\t \"Run `git hook install all` to install them.\\n\"));\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n\nHmph, do we really need to iterate over these mapped refs array? It\nseems to me that it would be vastly simpler to check if that single\nref exists after \"clone\" finishes depositing all the refs we are\nsupposed to create.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 769af53ca4..e86c312473 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -28,6 +28,7 @@\n> #include \"promisor-remote.h\"\n> #include \"commit-graph.h\"\n> #include \"shallow.h\"\n> +#include \"hook.h\"\n> \n> #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n> \n> @@ -1313,6 +1314,22 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)\n> \t\t\t\t ref_map);\n> \ttransport_unlock_pack(transport);\n> \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n> +\n> +\tif (hook_should_prompt_suggestions()) {\n> +\t\tstruct ref *ref;\n> +\n> +\t\tfor (ref = ref_map; ref; ref = ref->next) {\n> +\t\t\tif (ref->peer_ref &&\n> +\t\t\t !strcmp(ref->peer_ref->name,\n> +\t\t\t\t \"refs/remotes/origin/suggested-hooks\") &&\n> +\t\t\t oidcmp(&ref->old_oid, &ref->peer_ref->old_oid)) {\n> +\t\t\t\tfprintf(stderr, _(\"The remote has updated its suggested hooks.\\n\"));\n> +\t\t\t\tfprintf(stderr, _(\"Run 'git hook install all' to update.\\n\"));\n> +\t\t\t\tbreak;\n\nAgain we _could_ do \"remember if we had it and at where, and then\ncompare after we fetch\", but this _might_ be simpler.\n\nI however do not know if it makes sense to have a separate loop just\nfor this. This should be done as a part of store_updated_refs(),\nno?\n\nOn top of this patch, if we wanted to add yet another \"this ref is\nspecial and pay attention to it when it got updated\", it makes a lot\nmore sense not to add yet another loop that is a copy of this one in\nconsume_refs(), but roll the handling of that new ref into a loop\nthat already exists. And for the purpose of such an update, I do\nnot see why that \"loop that already exists\" should not be the one\nthat goes over ref_map in store_updated_refs(). For the same\nreason, \"the remote-tracking ref 'origin/suggested-hooks' is special\"\ncan and should go to an existing loop in store_updated_refs(), no?\n\nHow does this interact with the \"--dry-run\" option, by the way?\nWhat if ref_map proposes to update this suggested-hooks ref, but\n\"--atomic\" fetch feature finds that some other branches do not\nfast-forward and we end up not updating the suggested-hooks ref,\neither?\n\nSo, unlike my earlier guess, it _might_ turn out that \"remember the\nstate before the fetch, see if the fetch actually updated and then\nreport\" could be simpler to design and implement. I dunno.\n"},{"id":"430696","messageId":"20210720204903.3586550-1-jonathantanmy@google.com","threadId":"55946","inReplyTo":"xmqq8s22ko8k.fsf@gitster.g","subject":"Re: [RFC PATCH v2 0/2] MVP implementation of remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-20T20:49:03Z","receivedAt":"2021-07-20T21:02:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > Here's an updated version of the remote-suggested hooks RFC, hopefully\n> > addressing some of the concerns people have brought up (e.g.\n> > auto-updating without letting the user verify, or prompting about hooks\n> > by default). It consists of two main parts:\n> >\n> > - Non-interactive prompts during certain Git commands about the\n> > existence of hooks. These prompts are turned *off* by default.\n> >\n> > - New \"git hook\" subcommands that can install these hooks (so that the\n> > aforementioned Git prompts or out-of-band installation instructions\n> > can tell users to install these hooks in a platform-independent way).\n> > These subcommands work whether or not prompts are enabled.\n> >\n> > You can see how they work in patch 2's t1361.\n> \n> I really wanted to try this out, but which commit was this based on?\n\nAh, good question. Like v1, it's based on the old es/config-based-hooks\n(f2e1003b62 (\"docs: link githooks and git-hook manpages\", 2021-05-27)).\nIn v3, I'll rebase this on ab/config-based-hooks-base instead,\nespecially since that one has a programmatically generated list of hooks\n(instead of a hardcoded one).\n"},{"id":"430701","messageId":"87o8awvglr.fsf@evledraar.gmail.com","threadId":"55946","inReplyTo":"1ec1c958eb2b8aa2581280d050836dd0e7f6edef.1626453569.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T20:55:29Z","receivedAt":"2021-07-20T21:12:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jul 16 2021, Jonathan Tan wrote:\n\n> Teach the \"git hook install all|\" command, that can install\n> one or all remote-suggested hooks.\n>\n> If a configuration option hook.promptRemoteSuggested is set, inform the\n> user of the aforementioned command:\n>\n> - when cloning, and refs/remotes/origin/suggested-hooks is present in\n> the newly cloned repo\n> - when fetching, and refs/remotes/origin/suggested-hooks is updated\n> - when committing, there is a remote-suggested commit-msg hook, and\n> there is currently no commit-msg hook configured\n>\n> NEEDSWORK: Write a more detailed commit message once the design is\n> finalized.\n\nThis is a bit orthagonal to what you're going for I guess, so sorry in\nadvance about the \"but what about\" bikeshedding you must be getting\ntired of by now...\n\n> @@ -1393,6 +1393,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \t\t\t branch_top.buf, reflog_msg.buf, transport,\n> \t\t\t !is_local);\n> \n> +\tif (hook_should_prompt_suggestions()) {\n> +\t\tfor (ref = mapped_refs; ref; ref = ref->next) {\n> +\t\t\tif (ref->peer_ref &&\n> +\t\t\t !strcmp(ref->peer_ref->name,\n> +\t\t\t\t \"refs/remotes/origin/suggested-hooks\")) {\n> +\t\t\t\tfprintf(stderr, _(\"The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\\n\"\n> +\t\t\t\t\t\t \"Run `git hook install all` to install them.\\n\"));\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> \tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n> \n> \t/*\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 769af53ca4..e86c312473 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -28,6 +28,7 @@\n> #include \"promisor-remote.h\"\n> #include \"commit-graph.h\"\n> #include \"shallow.h\"\n> +#include \"hook.h\"\n> \n> #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n> \n> @@ -1313,6 +1314,22 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)\n> \t\t\t\t ref_map);\n> \ttransport_unlock_pack(transport);\n> \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n> +\n> +\tif (hook_should_prompt_suggestions()) {\n> +\t\tstruct ref *ref;\n> +\n> +\t\tfor (ref = ref_map; ref; ref = ref->next) {\n> +\t\t\tif (ref->peer_ref &&\n> +\t\t\t !strcmp(ref->peer_ref->name,\n> +\t\t\t\t \"refs/remotes/origin/suggested-hooks\") &&\n> +\t\t\t oidcmp(&ref->old_oid, &ref->peer_ref->old_oid)) {\n> +\t\t\t\tfprintf(stderr, _(\"The remote has updated its suggested hooks.\\n\"));\n> +\t\t\t\tfprintf(stderr, _(\"Run 'git hook install all' to update.\\n\"));\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> \treturn ret;\n> }\n\n...but this part makes me think that if this is all we're aiming for as\nfar as server-client interaction is concerned we'd be much better off\nwith some general \"server message-of-the-day\" feature. I.e. server says\nwhile advertising:\n\n version 2\n agent=...\n # does protocol v2 have a nicer way to encode this in the capabilities? I think not...\n motd=tellmeaboutref:suggested-hooks;master\n\nClient does, while handshake() etc.:\n\n # other stuff\n command=ls-refs\n ....\n 0000\n # Get motd from server\n command=motd\n 0001\n refat suggested-hooks $SUGGESTED_HOOKS_AT_OID\n refat master $MASTER_AT_OID\n 0000\n\nAnd server says, after just invoking a \"motd\" hook or whatever, which\nwould be passed the git version, the state of any refs we asked politely\nabout and the client was willing to tell us about etc.\n\n Hi, we've got suggested hooks in this repository, it seems:\n if $agent > $min_git_version\n you have a supported git version, great....\n else\n you might want to upgrade your git to....\n fi\n\nWe could even carry this specific message in git.git, but under the hood\nit would be the equivalent of a default 'motd' hook you could enable.\n\nMaybe where you're going with this precludes such a MOTD approach.\n\nFWIW I think there's lots of use-cases for it, and this specific hook\ncase is just one, so if we could make it slightly more general & just\nmake this a special-case of a generally useful facility.\n\nEven for your use-case it would be useful, e.g. the whole discussion\nwe've been having about should the hooks by in a magic ref or your\ncurrent branch or not.\n\nWith a motd hook it doesn't matter, you just make \"git hook install\"\nsupport installing hooks from whatever rev/tree, and a combination of\nthe \"tellmeaboutref\" and that feature means you can pick one or the\nother, or tell users they need to install first\nor whatever.\n\n"},{"id":"430702","messageId":"20210720211139.3592182-1-jonathantanmy@google.com","threadId":"55946","inReplyTo":"xmqqzguij8nq.fsf@gitster.g","subject":"Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-20T21:11:39Z","receivedAt":"2021-07-20T21:16:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> OK. Let's take this more as WIP.\n\nSounds good.\n\n> > diff --git a/builtin/clone.c b/builtin/clone.c\n> > index 2a2a03bf76..c2c8596aa9 100644\n> > --- a/builtin/clone.c\n> > +++ b/builtin/clone.c\n> > @@ -1393,6 +1393,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> > \t\t\t branch_top.buf, reflog_msg.buf, transport,\n> > \t\t\t !is_local);\n> > \n> > +\tif (hook_should_prompt_suggestions()) {\n> > +\t\tfor (ref = mapped_refs; ref; ref = ref->next) {\n> > +\t\t\tif (ref->peer_ref &&\n> > +\t\t\t !strcmp(ref->peer_ref->name,\n> > +\t\t\t\t \"refs/remotes/origin/suggested-hooks\")) {\n> > +\t\t\t\tfprintf(stderr, _(\"The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\\n\"\n> > +\t\t\t\t\t\t \"Run `git hook install all` to install them.\\n\"));\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t}\n> \n> Hmph, do we really need to iterate over these mapped refs array? It\n> seems to me that it would be vastly simpler to check if that single\n> ref exists after \"clone\" finishes depositing all the refs we are\n> supposed to create.\n\nGood point. I'll do that.\n\n> > @@ -1313,6 +1314,22 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)\n> > \t\t\t\t ref_map);\n> > \ttransport_unlock_pack(transport);\n> > \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n> > +\n> > +\tif (hook_should_prompt_suggestions()) {\n> > +\t\tstruct ref *ref;\n> > +\n> > +\t\tfor (ref = ref_map; ref; ref = ref->next) {\n> > +\t\t\tif (ref->peer_ref &&\n> > +\t\t\t !strcmp(ref->peer_ref->name,\n> > +\t\t\t\t \"refs/remotes/origin/suggested-hooks\") &&\n> > +\t\t\t oidcmp(&ref->old_oid, &ref->peer_ref->old_oid)) {\n> > +\t\t\t\tfprintf(stderr, _(\"The remote has updated its suggested hooks.\\n\"));\n> > +\t\t\t\tfprintf(stderr, _(\"Run 'git hook install all' to update.\\n\"));\n> > +\t\t\t\tbreak;\n> \n> Again we _could_ do \"remember if we had it and at where, and then\n> compare after we fetch\", but this _might_ be simpler.\n> \n> I however do not know if it makes sense to have a separate loop just\n> for this. This should be done as a part of store_updated_refs(),\n> no?\n\nI'm OK with it in either place, but it seems to me that this is separate\nfrom storing refs - the suggestion of hooks does not influence how the\nrefs are stored.\n\n> On top of this patch, if we wanted to add yet another \"this ref is\n> special and pay attention to it when it got updated\", it makes a lot\n> more sense not to add yet another loop that is a copy of this one in\n> consume_refs(), but roll the handling of that new ref into a loop\n> that already exists. And for the purpose of such an update, I do\n> not see why that \"loop that already exists\" should not be the one\n> that goes over ref_map in store_updated_refs(). For the same\n> reason, \"the remote-tracking ref 'origin/suggested-hooks' is special\"\n> can and should go to an existing loop in store_updated_refs(), no?\n\nI think 2 loops makes sense - the existing one to store the refs, and\nthe new one I introduced in this patch that handles the special ref.\n(And the handling of \"yet another\" special ref would be rolled into the\nlatter loop.) In this case, I think that these 2 loops should be\nindependent. For example, if the refs in Git are changed to be stored in\nan append-only journal, the former loop would be deleted while the\nlatter loop remains; and if the refs were to be stored as a sorted\narray, the former loop would be retained while the latter loop would be\nchanged to a binary search.\n\nHaving said that, I don't feel strongly about this, so if consensus is\nthat we should move it into stsore_updated_refs(), that's fine too.\n\n> How does this interact with the \"--dry-run\" option, by the way?\n> What if ref_map proposes to update this suggested-hooks ref, but\n> \"--atomic\" fetch feature finds that some other branches do not\n> fast-forward and we end up not updating the suggested-hooks ref,\n> either?\n\nGood point - I'll have to check these.\n\n> So, unlike my earlier guess, it _might_ turn out that \"remember the\n> state before the fetch, see if the fetch actually updated and then\n> report\" could be simpler to design and implement. I dunno.\n\nThat's true. Right now I'm using the refmap data structure, but perhaps\nit's better to consult the ref from the ref backend before and after the\nfetch.\n\nOverall, thanks for taking a look. If you have time, I would also\nappreciate comments on the overall idea of this series (the concept of\nremote-suggested hooks in general, the way that the user finds out about\nthem, and how the user can make use of them). (Same message to other\npotential reviewers - general design comments are potentially more\nuseful, because specific code comments can become moot if there needs to\nbe a change in the overall design.)\n"},{"id":"430704","messageId":"CABURp0q=t+4KM8ET57gZavHCAJSZQoVTmPS=n9rdapDczepdsg@mail.gmail.com","threadId":"55946","inReplyTo":"20210720211139.3592182-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2021-07-20T21:28:58Z","receivedAt":"2021-07-20T21:30:05Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Tue, Jul 20, 2021 at 2:17 PM Jonathan Tan wrote:\n>\n> Overall, thanks for taking a look. If you have time, I would also\n> appreciate comments on the overall idea of this series (the concept of\n> remote-suggested hooks in general, the way that the user finds out about\n> them, and how the user can make use of them).\n\nI'm curious what happens if we have multiple remotes. Do we only take\nsuggestions from `origin`? If not, what happens when there are\nconflicting suggestions from different remotes?\n"},{"id":"430708","messageId":"20210720214809.3596513-1-jonathantanmy@google.com","threadId":"55946","inReplyTo":"87o8awvglr.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-20T21:48:09Z","receivedAt":"2021-07-20T21:48:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> This is a bit orthagonal to what you're going for I guess, so sorry in\n> advance about the \"but what about\" bikeshedding you must be getting\n> tired of by now...\n\nNo - thanks for taking a look. More ideas are always welcome.\n\n> ...but this part makes me think that if this is all we're aiming for as\n> far as server-client interaction is concerned we'd be much better off\n> with some general \"server message-of-the-day\" feature. I.e. server says\n> while advertising:\n> \n> version 2\n> agent=...\n> # does protocol v2 have a nicer way to encode this in the capabilities? I think not...\n> motd=tellmeaboutref:suggested-hooks;master\n\nRight now we don't have a way in capabilities to include arbitrary\nstrings, although we can extend it if needed.\n\n> Client does, while handshake() etc.:\n> \n> # other stuff\n> command=ls-refs\n> ....\n> 0000\n> # Get motd from server\n> command=motd\n> 0001\n> refat suggested-hooks $SUGGESTED_HOOKS_AT_OID\n> refat master $MASTER_AT_OID\n> 0000\n> \n> And server says, after just invoking a \"motd\" hook or whatever, which\n> would be passed the git version, the state of any refs we asked politely\n> about and the client was willing to tell us about etc.\n\nAh...so the main difference is that it is the server that computes\nwhether a message is shown, based on information provided by the client\n(different from my patches wherein the client computes whether a message\nis shown).\n\nI'm not sure how this is better, though. We don't need to build another\nmechanism to print server messages (since it can already do so - the\nsame way it sends progress messages), but then we lose things like\ntranslatability, and we have to build another endpoint for the server\n(\"command=motd\").\n\nAlso, one thing to think about is that we want to be able to prompt\nusers when they run hook-using commands (e.g. \"commit\"). With my\npatches, the necessary information is stored in a ref but with your\nidea, we need to figure out where to store it (and I think that it is\nnot straightforward - I'd rather not use config or extra files in the\n.git directory to store remote state, although if the Git project is OK\nwith doing this, we could do that).\n\n> FWIW I think there's lots of use-cases for it, and this specific hook\n> case is just one, so if we could make it slightly more general & just\n> make this a special-case of a generally useful facility.\n> \n> Even for your use-case it would be useful, e.g. the whole discussion\n> we've been having about should the hooks by in a magic ref or your\n> current branch or not.\n> \n> With a motd hook it doesn't matter, you just make \"git hook install\"\n> support installing hooks from whatever rev/tree, and a combination of\n> the \"tellmeaboutref\" and that feature means you can pick one or the\n> other, or tell users they need to install first\n> or whatever.\n\nTrue - we avoid the discussion by having essentially a new namespace of\nname-to-OID mappings. I still think it's simpler to use refs of some\nsort for this, though. (Also, even if we use a new sort of name-to-OID\nmapping, we need to make a ref for this OID so that it will be fetched,\nso we might as well use a ref for this.)\n"},{"id":"430710","messageId":"20210720215634.3598035-1-jonathantanmy@google.com","threadId":"55946","inReplyTo":"CABURp0q=t+4KM8ET57gZavHCAJSZQoVTmPS=n9rdapDczepdsg@mail.gmail.com","subject":"Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-20T21:56:34Z","receivedAt":"2021-07-20T21:56:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Tue, Jul 20, 2021 at 2:17 PM Jonathan Tan wrote:\n> >\n> > Overall, thanks for taking a look. If you have time, I would also\n> > appreciate comments on the overall idea of this series (the concept of\n> > remote-suggested hooks in general, the way that the user finds out about\n> > them, and how the user can make use of them).\n> \n> I'm curious what happens if we have multiple remotes. Do we only take\n> suggestions from `origin`? If not, what happens when there are\n> conflicting suggestions from different remotes?\n\nIn this patch set, we only take suggestions from \"origin\". Right now my\nidea is to store the name of the ref that we should track (e.g.\n\"refs/remotes/origin/suggested-refs\") and only use that ref for hook\nsuggestions, so we would only take suggestions from one remote. But the\npurpose of this RFC is to discuss questions like this, so feel free to\nlet us know if you have additional use cases.\n"},{"id":"431276","messageId":"YP9Z+pDT6eZtlJhi@google.com","threadId":"55946","inReplyTo":"20210720214809.3596513-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-07-27T00:57:30Z","receivedAt":"2021-07-27T00:57:39Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Jul 20, 2021 at 02:48:09PM -0700, Jonathan Tan wrote:\n> \n> > This is a bit orthagonal to what you're going for I guess, so sorry in\n> > advance about the \"but what about\" bikeshedding you must be getting\n> > tired of by now...\n> \n> No - thanks for taking a look. More ideas are always welcome.\n> \n> > ...but this part makes me think that if this is all we're aiming for as\n> > far as server-client interaction is concerned we'd be much better off\n> > with some general \"server message-of-the-day\" feature. I.e. server says\n> > while advertising:\n> > \n> > version 2\n> > agent=...\n> > # does protocol v2 have a nicer way to encode this in the capabilities? I think not...\n> > motd=tellmeaboutref:suggested-hooks;master\n> \n> Right now we don't have a way in capabilities to include arbitrary\n> strings, although we can extend it if needed.\n> \n> > Client does, while handshake() etc.:\n> > \n> > # other stuff\n> > command=ls-refs\n> > ....\n> > 0000\n> > # Get motd from server\n> > command=motd\n> > 0001\n> > refat suggested-hooks $SUGGESTED_HOOKS_AT_OID\n> > refat master $MASTER_AT_OID\n> > 0000\n> > \n> > And server says, after just invoking a \"motd\" hook or whatever, which\n> > would be passed the git version, the state of any refs we asked politely\n> > about and the client was willing to tell us about etc.\n> \n> Ah...so the main difference is that it is the server that computes\n> whether a message is shown, based on information provided by the client\n> (different from my patches wherein the client computes whether a message\n> is shown).\n> \n> I'm not sure how this is better, though. We don't need to build another\n> mechanism to print server messages (since it can already do so - the\n> same way it sends progress messages), but then we lose things like\n> translatability, and we have to build another endpoint for the server\n> (\"command=motd\").\n> \n> Also, one thing to think about is that we want to be able to prompt\n> users when they run hook-using commands (e.g. \"commit\"). With my\n> patches, the necessary information is stored in a ref but with your\n> idea, we need to figure out where to store it (and I think that it is\n> not straightforward - I'd rather not use config or extra files in the\n> .git directory to store remote state, although if the Git project is OK\n> with doing this, we could do that).\n\nI think this is a pretty important point. To me, the ideal flow looks\nlike this:\n\n - I clone some repo, planning to just use the source code. I ignore the\n hook prompt.\n - I notice some bug which is within my power to fix. I have forgotten\n about the hook prompt, because I was having so much fun using the\n source code in the repo.\n - I 'git commit' - and 'git commit' says, \"Did you know this repo\n suggests installing a commit-msg hook? You can install it by running\n 'git hook install pre-commit' and run it by running 'git commit\n --amend --no-edit'. You can audit the commit-msg hook by running 'git\n hook magic-audit-command-name-tbd'. You can hide this advice .\"\n\nThat way I don't add privilege (tell my computer it's OK to execute code\nI didn't look at) until the very possible moment. This workflow also\ncaptures changing intentions - I did not clone the code intending to\ncontribute back, but at the moment my intentions changed, I was nudged\nto answer differently to a question I was asked with different earlier\nintentions. That use case isn't easy to capture with a MOTD, unless you\nrun one on push, at which point it may be too late (e.g. if while fixing\nI also accidentally uploaded my oauth password, and now it'll live\nforever on GitHub in infamy).\n\nMOTD approach also makes it hard to *update* hooks when the maintainer\nso recommends - would be nice to have something baked in to notice when\nthere are new changes to the hooks, so we hopefully don't have\ndevelopers running hook implementations correlating to the date they\nmost recently cloned the project.\n\n - Emily\n"},{"id":"431278","messageId":"xmqqczr4fsso.fsf@gitster.g","threadId":"55946","inReplyTo":"YP9Z+pDT6eZtlJhi@google.com","subject":"Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-27T01:29:27Z","receivedAt":"2021-07-27T01:29:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> I think this is a pretty important point. To me, the ideal flow looks\n> like this:\n>\n> - I clone some repo, planning to just use the source code. I ignore the\n> hook prompt.\n> - I notice some bug which is within my power to fix. I have forgotten\n> about the hook prompt, because I was having so much fun using the\n> source code in the repo.\n> - I 'git commit' - and 'git commit' says, \"Did you know this repo\n> suggests installing a commit-msg hook? You can install it by running\n> 'git hook install pre-commit' and run it by running 'git commit\n> --amend --no-edit'. You can audit the commit-msg hook by running 'git\n> hook magic-audit-command-name-tbd'. You can hide this advice advice-hiding advice here>.\"\n\nDevil's advocate in me says that delaying this until the last\npossible moment will make people *not* look at the hook's contents\nand just say \"yes\". After all, you have been working on a task and\nreached one milestone (i.e. you are now ready to say \"commit\"), and\nyou want to start recording your thought process before it slips\naway from your mind. To many of us non-multi-tasking types, it is\none of the worst moment to force switching our attention to a\nsecondary task of vetting project supplied hooks---rather, I'd say\n\"Oh, I work for project X and if these hooks are supplied by project\nX, it is good enough for them, and it must be good enough for me\".\n\n> That way I don't add privilege (tell my computer it's OK to execute code\n> I didn't look at) until the very possible moment.\n\nSo this smells a wishful thinking to me.\n\n> MOTD approach also makes it hard to *update* hooks when the maintainer\n> so recommends - would be nice to have something baked in to notice when\n> there are new changes to the hooks, so we hopefully don't have\n> developers running hook implementations correlating to the date they\n> most recently cloned the project.\n\nInteresting. Every time you run \"git commit\", the command will\ncheck the existence of remotes/origin/suggested-hooks, compares the\ninstalled .git/hooks/pre-commit with a blob in the tree stored\nthere, and tells you to update if they are different? Or perhaps\nthe installed hooks record from which blob their contents came from,\nand perform a three-way content level merge to carry local changes\nforward?\n"},{"id":"431367","messageId":"20210727213942.2574308-1-jonathantanmy@google.com","threadId":"55946","inReplyTo":"xmqqczr4fsso.fsf@gitster.g","subject":"Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-27T21:39:42Z","receivedAt":"2021-07-27T21:39:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Emily Shaffer writes:\n> \n> > I think this is a pretty important point. To me, the ideal flow looks\n> > like this:\n> >\n> > - I clone some repo, planning to just use the source code. I ignore the\n> > hook prompt.\n> > - I notice some bug which is within my power to fix. I have forgotten\n> > about the hook prompt, because I was having so much fun using the\n> > source code in the repo.\n> > - I 'git commit' - and 'git commit' says, \"Did you know this repo\n> > suggests installing a commit-msg hook? You can install it by running\n> > 'git hook install pre-commit' and run it by running 'git commit\n> > --amend --no-edit'. You can audit the commit-msg hook by running 'git\n> > hook magic-audit-command-name-tbd'. You can hide this advice > advice-hiding advice here>.\"\n> \n> Devil's advocate in me says that delaying this until the last\n> possible moment will make people *not* look at the hook's contents\n> and just say \"yes\". After all, you have been working on a task and\n> reached one milestone (i.e. you are now ready to say \"commit\"), and\n> you want to start recording your thought process before it slips\n> away from your mind. To many of us non-multi-tasking types, it is\n> one of the worst moment to force switching our attention to a\n> secondary task of vetting project supplied hooks---rather, I'd say\n> \"Oh, I work for project X and if these hooks are supplied by project\n> X, it is good enough for them, and it must be good enough for me\".\n\nI think both \"I want to vet\" and \"good enough for project X is good\nenough for me\" are both reasonable points of view, and this\nremote-suggested hook scheme supports both.\n\n> > MOTD approach also makes it hard to *update* hooks when the maintainer\n> > so recommends - would be nice to have something baked in to notice when\n> > there are new changes to the hooks, so we hopefully don't have\n> > developers running hook implementations correlating to the date they\n> > most recently cloned the project.\n> \n> Interesting. Every time you run \"git commit\", the command will\n> check the existence of remotes/origin/suggested-hooks, compares the\n> installed .git/hooks/pre-commit with a blob in the tree stored\n> there, and tells you to update if they are different? Or perhaps\n> the installed hooks record from which blob their contents came from,\n> and perform a three-way content level merge to carry local changes\n> forward?\n\nWe do notice when there are new changes, but only during fetch. \n\nI don't think we should compare the installed .git/hooks/pre-commit with\nremotes/origin/suggested-hooks (since the user may have locally modified\nthat hook), so a solution involving storing the OID of the installed\nhook somewhere (I haven't figured out where, though) and comparing that\nOID against remotes/origin/suggested-hooks would be reasonable and would\nbe compatible with the current approach (as opposed to the one which\nÆvar describes which, if I understand it correctly, would require\n\"commit\" to access the network to figure out if the hook the client has\nis the latest one).\n"},{"id":"431368","messageId":"xmqqmtq7crdz.fsf@gitster.g","threadId":"55946","inReplyTo":"20210727213942.2574308-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-27T22:40:24Z","receivedAt":"2021-07-27T22:40:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> I think both \"I want to vet\" and \"good enough for project X is good\n> enough for me\" are both reasonable points of view, and this\n> remote-suggested hook scheme supports both.\n\nSure. \n\nI was just pointing out that the design is opinionated and not\ngiving two points of view fair chance to compete. It will strongly\nencourage users to the latter choice by prodding them when they want\nto do a hook-invoking operation (like \"git commit\").\n\nNot that opinionated design is necessarily a bad thing.\n\n> I don't think we should compare the installed .git/hooks/pre-commit with\n> remotes/origin/suggested-hooks (since the user may have locally modified\n> that hook), so a solution involving storing the OID of the installed\n> hook somewhere (I haven't figured out where, though) and comparing that\n> OID against remotes/origin/suggested-hooks would be reasonable and would\n> be compatible with the current approach (as opposed to the one which\n> Ævar describes which, if I understand it correctly, would require\n> \"commit\" to access the network to figure out if the hook the client has\n> is the latest one).\n\nCoping with local modification would not be rocket science.\n\nIf I were to do this, when the end-user approves installation of\nand/or updates from remotes/origin/suggested-hooks/, the following\nwould happen:\n\n (1) If .git/hooks/* does not have the hook installed, copy it from\n the suggested hooks, and append two line trailer:\n\n\t# do not edit below\n\t# hook taken from \n\n (2) If .git/hooks/* does hold the hook, look for the \"hook taken\n from\" trailer\n\n (a) if \"hook taken from\" trailer is missing (i.e. it came from\n somewhere other than \"remote suggested\" workflow) or it does\n not point at a valid blob object, jump to \"conflict exists\"\n below.\n\n (b) using that (old) blob object name, perform (1) to recreate\n the hook the user would have seen when on-disk version of\n hook was created. Difference between that and what is\n on-disk is the end-user customization.\n\n extract the current blob object from the suggested hooks tree\n object, do the same as (1), and then replay the end-user\n customization we figured out above.\n\n If the replaying succeeds cleanly, we are done. Otherwise we\n have conflicts that cannot be resolved automatically.\n\n (c) \"conflict exists\". The usual three-way merge resolution is\n needed. I'd suggest to give users two (or three) files:\n\n - Rename the current version the user has to *.bak;\n - The new version from the project in the final file;\n - The patch obtained in (b) above, if exists in a separate file.\n\n and ask them to carry their customization forward to the\n second one (this is in line with the \"we encourage them to\n adopt the project preferences\" philosophy this proposal is\n taking us, I think).\n\nI think configuration files under /etc/ on Debian-based distros have\nbeen managed in a similar way for at least the past 10 years if not\nlonger, and since we are version control software ourselves, the\nconflict resolution users are asked to perform in (2)-(c) shouldn't\nbe too much of a burden to our users anyway.\n"}]}