[RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Jonathan Tan]

This is a continuation of the work from [1]. That work described the
reasons, possible features, and possible workflows, but not the
implementation in detail. This patch set has an MVP implementation, and
my hope is that having a concrete implementation to look at makes it
easier to discuss matters of implementation.

This but does not use any features from es/config-based-hooks but is
implemented on that branch anyway because firstly, I need an existing
command to attach the "autoupdate" subcommand (and "git hook" works),
and secondly, when we test this at $DAYJOB, we will be testing it
together with the aforementioned branch.

I have had to make several design choices (which I will discuss later),
but now with this implementation, the following workflow is possible:

 1. The remote repo administrator creates a new branch
    "refs/heads/suggested-hooks" pointing to a commit that has all the
    hooks that the administrator wants to suggest. The hooks are
    directly referenced by the commit tree (i.e. they are in the "/"
    directory).

 2. When a user clones, Git notices that
    "refs/remotes/origin/suggested-hooks" is present and prints out a
    message about a command that can be run.

 3. If the user runs that command, Git will install the hooks pointed to
    by that ref, and set hook.autoupdate to true. This config variable
    is checked whenever "git fetch" is run: whenever it notices that
    "refs/remotes/origin/suggested-hooks" changes, it will reinstall the
    hooks.

 4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks
    will remain.

Design choices:

 1. Where should the suggested hooks be in the remote repo? A branch,
    a non-branch ref, a config? I think that a branch is best - it is
    relatively well-understood and any hooks there can be
    version-controlled (and its history is independent of the other
    branches).

 2. When and how should the local repo update its record of the remote's
    suggested hooks? If we go with storing the hooks in a branch of a
    remote side, this would automatically mean (with the default
    refspec) that it would be in refs/remotes/<remote>/<name>. This
    incurs network and hard disk cost even if the local repo does not
    want to use the suggested hooks, but I think that typically they
    would want to use it if they're going to do any work on the repo
    (they would either have to trust or inspect Makefiles etc. anyway,
    so they can do the same for the hooks), and even if they don't want
    to use the remote's hooks, they probably still want to know what the
    remote suggests.

    So using a branch provides a well-understood way of storing the
    hooks on the remote, transmitting it to the local repo, and storing
    the hooks in the local repo.

    So: what should be the default name of this branch? Presumably, "git
    clone" would need to be able to override this.

 3. How should the local repo detect when the remote has updated its
    suggested hooks? I'm thinking when a certain local ref is updated.
    Right now it's hardcoded, but perhaps "git clone" could detect what
    "refs/heads/suggested-hooks" would correspond to, and then set it in
    a config accordingly. Other options include remembering what the
    remote's "refs/heads/suggested-hooks" used to be and always
    comparing it upon every "ls-refs" call, but I think that the local
    ref method is more straightforward.

 4. Should the local repo try to notice if the hooks have been changed
    locally before overwriting upon autoupdate? This would be nice to
    have, but I don't know how practical it would be. In particular, I
    don't know if we can trust that
    "refs/remotes/origin/suggested-hooks" has not been clobbered.

 5. Should we have a command that manually updates the hooks with what's
    in "refs/heads/suggested-hooks"? This is not in this patch set, but
    it sounds like a good idea.

There are perhaps other points that I haven't thought of, of course.

[1] https://lore.kernel.org/git/pull.908.git.1616105016055.gitgitgadget@gmail.com/

Jonathan Tan (2):
  hook: move list of hooks
  clone,fetch: remote-suggested auto-updating hooks

 builtin/bugreport.c |  38 ++------------
 builtin/clone.c     |  10 ++++
 builtin/fetch.c     |  21 ++++++++
 builtin/hook.c      |  13 +++--
 hook.c              | 118 ++++++++++++++++++++++++++++++++++++++++++++
 hook.h              |   5 ++
 t/t5601-clone.sh    |  36 ++++++++++++++
 7 files changed, 202 insertions(+), 39 deletions(-)

-- 
2.32.0.272.g935e593368-goog

[RFC PATCH 1/2] hook: move list of hooks

[Jonathan Tan]

The list of hooks will need to be used outside bugreport, so move it to
a central location.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/bugreport.c | 38 +++-----------------------------------
 hook.c              | 34 ++++++++++++++++++++++++++++++++++
 hook.h              |  3 +++
 3 files changed, 40 insertions(+), 35 deletions(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 190272ba70..4e0806dff3 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -41,38 +41,6 @@ static void get_system_info(struct strbuf *sys_info)
 
 static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 {
-	/*
-	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
-	 * so below is a transcription of `git help hooks`. Later, this should
-	 * be replaced with some programmatically generated list (generated from
-	 * doc or else taken from some library which tells us about all the
-	 * hooks)
-	 */
-	static const char *hook[] = {
-		"applypatch-msg",
-		"pre-applypatch",
-		"post-applypatch",
-		"pre-commit",
-		"pre-merge-commit",
-		"prepare-commit-msg",
-		"commit-msg",
-		"post-commit",
-		"pre-rebase",
-		"post-checkout",
-		"post-merge",
-		"pre-push",
-		"pre-receive",
-		"update",
-		"post-receive",
-		"post-update",
-		"push-to-checkout",
-		"pre-auto-gc",
-		"post-rewrite",
-		"sendemail-validate",
-		"fsmonitor-watchman",
-		"p4-pre-submit",
-		"post-index-change",
-	};
 	int i;
 
 	if (nongit) {
@@ -81,9 +49,9 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 		return;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(hook); i++)
-		if (hook_exists(hook[i], HOOKDIR_USE_CONFIG))
-			strbuf_addf(hook_info, "%s\n", hook[i]);
+	for (i = 0; i < hook_name_count; i++)
+		if (hook_exists(hook_name[i], HOOKDIR_USE_CONFIG))
+			strbuf_addf(hook_info, "%s\n", hook_name[i]);
 }
 
 static const char * const bugreport_usage[] = {
diff --git a/hook.c b/hook.c
index ff80e52edd..3ccacb72fa 100644
--- a/hook.c
+++ b/hook.c
@@ -5,6 +5,40 @@
 #include "run-command.h"
 #include "prompt.h"
 
+/*
+ * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
+ * so below is a transcription of `git help hooks`. Later, this should
+ * be replaced with some programmatically generated list (generated from
+ * doc or else taken from some library which tells us about all the
+ * hooks)
+ */
+const char *hook_name[] = {
+	"applypatch-msg",
+	"pre-applypatch",
+	"post-applypatch",
+	"pre-commit",
+	"pre-merge-commit",
+	"prepare-commit-msg",
+	"commit-msg",
+	"post-commit",
+	"pre-rebase",
+	"post-checkout",
+	"post-merge",
+	"pre-push",
+	"pre-receive",
+	"update",
+	"post-receive",
+	"post-update",
+	"push-to-checkout",
+	"pre-auto-gc",
+	"post-rewrite",
+	"sendemail-validate",
+	"fsmonitor-watchman",
+	"p4-pre-submit",
+	"post-index-change",
+};
+int hook_name_count = ARRAY_SIZE(hook_name);
+
 void free_hook(struct hook *ptr)
 {
 	if (ptr) {
diff --git a/hook.h b/hook.h
index f32189380a..d902166408 100644
--- a/hook.h
+++ b/hook.h
@@ -4,6 +4,9 @@
 #include "strvec.h"
 #include "run-command.h"
 
+extern const char *hook_name[];
+extern int hook_name_count;
+
 struct hook {
 	struct list_head list;
 	/*
-- 
2.32.0.272.g935e593368-goog

Re: [RFC PATCH 1/2] hook: move list of hooks

[Emily Shaffer]

On Wed, Jun 16, 2021 at 04:31:48PM -0700, Jonathan Tan wrote:
> 
> The list of hooks will need to be used outside bugreport, so move it to
> a central location.

Hm, does this fight with
https://lore.kernel.org/git/cover-0.3-0000000000-20210617T100239Z-avarab@gmail.com
?

This patch itself looks pretty straightforward, but I think it is a good
idea to take a look at avarab's change first.

 - Emily

> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  builtin/bugreport.c | 38 +++-----------------------------------
>  hook.c              | 34 ++++++++++++++++++++++++++++++++++
>  hook.h              |  3 +++
>  3 files changed, 40 insertions(+), 35 deletions(-)
> 
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 190272ba70..4e0806dff3 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -41,38 +41,6 @@ static void get_system_info(struct strbuf *sys_info)
>  
>  static void get_populated_hooks(struct strbuf *hook_info, int nongit)
>  {
> -	/*
> -	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
> -	 * so below is a transcription of `git help hooks`. Later, this should
> -	 * be replaced with some programmatically generated list (generated from
> -	 * doc or else taken from some library which tells us about all the
> -	 * hooks)
> -	 */
> -	static const char *hook[] = {
> -		"applypatch-msg",
> -		"pre-applypatch",
> -		"post-applypatch",
> -		"pre-commit",
> -		"pre-merge-commit",
> -		"prepare-commit-msg",
> -		"commit-msg",
> -		"post-commit",
> -		"pre-rebase",
> -		"post-checkout",
> -		"post-merge",
> -		"pre-push",
> -		"pre-receive",
> -		"update",
> -		"post-receive",
> -		"post-update",
> -		"push-to-checkout",
> -		"pre-auto-gc",
> -		"post-rewrite",
> -		"sendemail-validate",
> -		"fsmonitor-watchman",
> -		"p4-pre-submit",
> -		"post-index-change",
> -	};
>  	int i;
>  
>  	if (nongit) {
> @@ -81,9 +49,9 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
>  		return;
>  	}
>  
> -	for (i = 0; i < ARRAY_SIZE(hook); i++)
> -		if (hook_exists(hook[i], HOOKDIR_USE_CONFIG))
> -			strbuf_addf(hook_info, "%s\n", hook[i]);
> +	for (i = 0; i < hook_name_count; i++)
> +		if (hook_exists(hook_name[i], HOOKDIR_USE_CONFIG))
> +			strbuf_addf(hook_info, "%s\n", hook_name[i]);
>  }
>  
>  static const char * const bugreport_usage[] = {
> diff --git a/hook.c b/hook.c
> index ff80e52edd..3ccacb72fa 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -5,6 +5,40 @@
>  #include "run-command.h"
>  #include "prompt.h"
>  
> +/*
> + * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
> + * so below is a transcription of `git help hooks`. Later, this should
> + * be replaced with some programmatically generated list (generated from
> + * doc or else taken from some library which tells us about all the
> + * hooks)
> + */
> +const char *hook_name[] = {
> +	"applypatch-msg",
> +	"pre-applypatch",
> +	"post-applypatch",
> +	"pre-commit",
> +	"pre-merge-commit",
> +	"prepare-commit-msg",
> +	"commit-msg",
> +	"post-commit",
> +	"pre-rebase",
> +	"post-checkout",
> +	"post-merge",
> +	"pre-push",
> +	"pre-receive",
> +	"update",
> +	"post-receive",
> +	"post-update",
> +	"push-to-checkout",
> +	"pre-auto-gc",
> +	"post-rewrite",
> +	"sendemail-validate",
> +	"fsmonitor-watchman",
> +	"p4-pre-submit",
> +	"post-index-change",
> +};
> +int hook_name_count = ARRAY_SIZE(hook_name);
> +
>  void free_hook(struct hook *ptr)
>  {
>  	if (ptr) {
> diff --git a/hook.h b/hook.h
> index f32189380a..d902166408 100644
> --- a/hook.h
> +++ b/hook.h
> @@ -4,6 +4,9 @@
>  #include "strvec.h"
>  #include "run-command.h"
>  
> +extern const char *hook_name[];
> +extern int hook_name_count;
> +
>  struct hook {
>  	struct list_head list;
>  	/*
> -- 
> 2.32.0.272.g935e593368-goog
> 

Re: [RFC PATCH 1/2] hook: move list of hooks

[Jonathan Tan]

> On Wed, Jun 16, 2021 at 04:31:48PM -0700, Jonathan Tan wrote:
> > 
> > The list of hooks will need to be used outside bugreport, so move it to
> > a central location.
> 
> Hm, does this fight with
> https://lore.kernel.org/git/cover-0.3-0000000000-20210617T100239Z-avarab@gmail.com
> ?
> 
> This patch itself looks pretty straightforward, but I think it is a good
> idea to take a look at avarab's change first.
> 
>  - Emily

Hmm...you might be right. In any case, this is just an RFC patch set so
I presume avarab's will be merged first (and then I can use his hook
list in a subsequent version of my patch set).

[RFC PATCH 2/2] clone,fetch: remote-suggested auto-updating hooks

[Jonathan Tan]

Upon cloning, if a ref refs/remotes/origin/suggested-hooks is present,
Git will inform the user that they can run a command to install those
hooks and keep them auto-updated upon eacn fetch. If that command is
run, hooks will be installed (overriding existing ones) and every fetch
will check if that ref would be updated, and if yes, reinstall the hooks
from the new commit that the ref points to.

NEEDSWORK: Write a more detailed commit message once the design is
finalized.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/clone.c  | 10 ++++++
 builtin/fetch.c  | 21 ++++++++++++
 builtin/hook.c   | 13 +++++---
 hook.c           | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
 hook.h           |  2 ++
 t/t5601-clone.sh | 36 +++++++++++++++++++++
 6 files changed, 162 insertions(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 2a2a03bf76..0ab4494bcd 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1393,6 +1393,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			   branch_top.buf, reflog_msg.buf, transport,
 			   !is_local);
 
+	for (ref = mapped_refs; ref; ref = ref->next) {
+		if (ref->peer_ref &&
+		    !strcmp(ref->peer_ref->name,
+			    "refs/remotes/origin/suggested-hooks")) {
+			fprintf(stderr, _("The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\n"
+					  "Run `git hook autoupdate` to download them, install them, and turn on automatic update of hooks.\n"));
+			break;
+		}
+	}
+
 	update_head(our_head_points_at, remote_head, reflog_msg.buf);
 
 	/*
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 769af53ca4..0618d1f091 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -28,6 +28,7 @@
 #include "promisor-remote.h"
 #include "commit-graph.h"
 #include "shallow.h"
+#include "hook.h"
 
 #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
 
@@ -84,6 +85,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
 static int fetch_write_commit_graph = -1;
 static int stdin_refspecs = 0;
 static int negotiate_only;
+static int hook_autoupdate;
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
 {
@@ -123,6 +125,11 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(k, "hook.autoupdate")) {
+		hook_autoupdate = git_config_bool(k, v);
+		return 0;
+	}
+
 	return git_default_config(k, v, cb);
 }
 
@@ -1313,6 +1320,20 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)
 				 ref_map);
 	transport_unlock_pack(transport);
 	trace2_region_leave("fetch", "consume_refs", the_repository);
+
+	if (hook_autoupdate) {
+		struct ref *ref;
+
+		for (ref = ref_map; ref; ref = ref->next) {
+			if (ref->peer_ref &&
+			    !strcmp(ref->peer_ref->name,
+				    "refs/remotes/origin/suggested-hooks")) {
+				hook_update_suggested();
+				break;
+			}
+		}
+	}
+
 	return ret;
 }
 
diff --git a/builtin/hook.c b/builtin/hook.c
index c79a961e80..26d657a48a 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -139,6 +139,13 @@ static int run(int argc, const char **argv, const char *prefix)
 	return rc;
 }
 
+static int autoupdate(int argc, const char **argv, const char *prefix)
+{
+	git_config_set("hook.autoupdate", "true");
+	hook_update_suggested();
+	return 0;
+}
+
 int cmd_hook(int argc, const char **argv, const char *prefix)
 {
 	const char *run_hookdir = NULL;
@@ -152,10 +159,6 @@ int cmd_hook(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, builtin_hook_options,
 			     builtin_hook_usage, PARSE_OPT_KEEP_UNKNOWN);
 
-	/* after the parse, we should have "<command> <hookname> <args...>" */
-	if (argc < 2)
-		usage_with_options(builtin_hook_usage, builtin_hook_options);
-
 	git_config(git_default_config, NULL);
 
 
@@ -185,6 +188,8 @@ int cmd_hook(int argc, const char **argv, const char *prefix)
 		return list(argc, argv, prefix);
 	if (!strcmp(argv[0], "run"))
 		return run(argc, argv, prefix);
+	if (!strcmp(argv[0], "autoupdate"))
+		return autoupdate(argc, argv, prefix);
 
 	usage_with_options(builtin_hook_usage, builtin_hook_options);
 }
diff --git a/hook.c b/hook.c
index 3ccacb72fa..5730e6e501 100644
--- a/hook.c
+++ b/hook.c
@@ -4,6 +4,12 @@
 #include "config.h"
 #include "run-command.h"
 #include "prompt.h"
+#include "commit.h"
+#include "object.h"
+#include "refs.h"
+#include "tree-walk.h"
+#include "tree.h"
+#include "streaming.h"
 
 /*
  * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
@@ -515,3 +521,81 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options)
 
 	return cb_data.rc;
 }
+
+static int is_hook(const char *filename)
+{
+	int i;
+
+	for (i = 0; i < hook_name_count; i++) {
+		if (!strcmp(filename, hook_name[i]))
+			return 1;
+	}
+	return 0;
+}
+
+void hook_update_suggested(void)
+{
+	struct object_id oid;
+	struct object *obj;
+	struct tree *tree;
+	struct tree_desc desc;
+	struct name_entry entry;
+	struct strbuf path = STRBUF_INIT;
+
+	if (read_ref("refs/remotes/origin/suggested-hooks", &oid)) {
+		warning(_("no such ref refs/remotes/origin/suggested-hooks, not updating hooks"));
+		return;
+	}
+
+	obj = parse_object(the_repository, &oid);
+	if (obj == NULL) {
+		warning(_("object pointed to by refs/remotes/origin/suggested-hooks '%s' does not exist"),
+			oid_to_hex(&oid));
+		return;
+	}
+	if (obj->type != OBJ_COMMIT) {
+		warning(_("object pointed to by refs/remotes/origin/suggested-hooks '%s' is not a commit"),
+			oid_to_hex(&oid));
+		return;
+	}
+
+	tree = get_commit_tree((struct commit *) obj);
+	if (parse_tree(tree)) {
+		warning(_("could not parse tree"));
+		return;
+	}
+
+	init_tree_desc(&desc, tree->buffer, tree->size);
+	while (tree_entry(&desc, &entry)) {
+		int fd;
+
+		if (!is_hook(entry.path)) {
+			warning(_("file '%s' is not a hook; ignoring"),
+				entry.path);
+			continue;
+		}
+		if (S_ISDIR(entry.mode) || S_ISGITLINK(entry.mode)) {
+			warning(_("file '%s' is not an ordinary file; ignoring"),
+				entry.path);
+			continue;
+		}
+
+		strbuf_reset(&path);
+		strbuf_git_path(&path, "hooks/%s", entry.path);
+		fd = open(path.buf, O_WRONLY | O_CREAT, 0755);
+		if (fd < 0) {
+			warning_errno(_("could not create file '%s'; skipping this hook"),
+				      path.buf);
+			continue;
+		}
+		if (stream_blob_to_fd(fd, &entry.oid, NULL, 1)) {
+			warning(_("could not write to file '%s'; skipping this hook"),
+				path.buf);
+			continue;
+		}
+		close(fd);
+	}
+	strbuf_release(&path);
+
+	return;
+}
diff --git a/hook.h b/hook.h
index d902166408..b0bfd3719b 100644
--- a/hook.h
+++ b/hook.h
@@ -140,3 +140,5 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options);
 void free_hook(struct hook *ptr);
 /* Empties the list at 'head', calling 'free_hook()' on each entry */
 void clear_hook_list(struct list_head *head);
+
+void hook_update_suggested(void);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index c0688467e7..21b96a4999 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -752,6 +752,42 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe
 	git clone --filter=blob:limit=0 "file://$(pwd)/server" client
 '
 
+test_expect_success 'detect suggested hook branch during clone' '
+	rm -rf server client &&
+
+	test_create_repo server &&
+	echo "echo foo" >server/pre-commit &&
+	git -C server add pre-commit &&
+	test_commit -C server hook-and-not-hook &&
+	git -C server checkout -b suggested-hooks &&
+	git clone server client 2>err &&
+	test_i18ngrep "The remote has suggested hooks" err
+'
+
+test_expect_success 'autoupdate' '
+	git -C client hook autoupdate &&
+
+	# Check correct config written
+	git -C client config --local hook.autoupdate >actual &&
+	echo "true" >expect &&
+	test_cmp expect actual &&
+
+	# Check hook written
+	cat client/.git/hooks/pre-commit >actual &&
+	echo "echo foo" >expect &&
+	test_cmp expect actual &&
+
+	# Install new hook and fetch it
+	echo "echo bar" >server/pre-commit &&
+	git -C server commit -am "new hook" &&
+	git -C client fetch &&
+
+	# Check new hook written
+	cat client/.git/hooks/pre-commit >actual &&
+	echo "echo bar" >expect &&
+	test_cmp expect actual
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
2.32.0.272.g935e593368-goog

Re: [RFC PATCH 2/2] clone,fetch: remote-suggested auto-updating hooks

[Emily Shaffer]

On Wed, Jun 16, 2021 at 04:31:49PM -0700, Jonathan Tan wrote:
> 
> Upon cloning, if a ref refs/remotes/origin/suggested-hooks is present,
> Git will inform the user that they can run a command to install those
> hooks and keep them auto-updated upon eacn fetch. If that command is
> run, hooks will be installed (overriding existing ones) and every fetch
> will check if that ref would be updated, and if yes, reinstall the hooks
> from the new commit that the ref points to.
> 
> NEEDSWORK: Write a more detailed commit message once the design is
> finalized.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  builtin/clone.c  | 10 ++++++
>  builtin/fetch.c  | 21 ++++++++++++
>  builtin/hook.c   | 13 +++++---
>  hook.c           | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
>  hook.h           |  2 ++
>  t/t5601-clone.sh | 36 +++++++++++++++++++++
>  6 files changed, 162 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 2a2a03bf76..0ab4494bcd 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1393,6 +1393,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  			   branch_top.buf, reflog_msg.buf, transport,
>  			   !is_local);
>  
> +	for (ref = mapped_refs; ref; ref = ref->next) {
> +		if (ref->peer_ref &&
> +		    !strcmp(ref->peer_ref->name,
> +			    "refs/remotes/origin/suggested-hooks")) {
> +			fprintf(stderr, _("The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\n"
> +					  "Run `git hook autoupdate` to download them, install them, and turn on automatic update of hooks.\n"));

It is possible to clone but to name your origin something besides
'origin', isn't it? *looks..* I guess 'origin' is added by default, but
couldn't a user rename their origin remote later on, or want hooks from
somewhere else? For example:

 - User creates a fork with GitHub web UI
 - User clones their own fork (origin = nasamuffin/someproject)
 - User decides they should probably get latest and greatest code from
   upstream instead, so they add new remote (upstream =
   someproject/someproject)
 - The hooks are in someproject/someproject, and maybe only showed up
   after user forked from web UI. Oops.

Anyway, it might be nice to have this generalized to any remote, not
just 'origin' - maybe I even want to pull in hooks from upstream as well
as hooks from downstream fork with confidential patches added. (I had a
remote setup like this when working on OpenBMC, so it is not beyond
realm of possibility.)

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 769af53ca4..0618d1f091 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -28,6 +28,7 @@
>  #include "promisor-remote.h"
>  #include "commit-graph.h"
>  #include "shallow.h"
> +#include "hook.h"
>  
>  #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
>  
> @@ -84,6 +85,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
>  static int fetch_write_commit_graph = -1;
>  static int stdin_refspecs = 0;
>  static int negotiate_only;
> +static int hook_autoupdate;
>  
>  static int git_fetch_config(const char *k, const char *v, void *cb)
>  {
> @@ -123,6 +125,11 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>  		return 0;
>  	}
>  
> +	if (!strcmp(k, "hook.autoupdate")) {
> +		hook_autoupdate = git_config_bool(k, v);
> +		return 0;
> +	}
> +

Checking if we should look for hook changes every time we fetch. Ok.

> @@ -1313,6 +1320,20 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)
>  				 ref_map);
>  	transport_unlock_pack(transport);
>  	trace2_region_leave("fetch", "consume_refs", the_repository);
> +
> +	if (hook_autoupdate) {
> +		struct ref *ref;
> +
> +		for (ref = ref_map; ref; ref = ref->next) {
> +			if (ref->peer_ref &&
> +			    !strcmp(ref->peer_ref->name,
> +				    "refs/remotes/origin/suggested-hooks")) {
> +				hook_update_suggested();
> +				break;
> +			}
> +		}
> +	}
> +

And if fetch gave us a ref like suggested-hooks, then we should go do
hook update operation. Ok.

> +static int autoupdate(int argc, const char **argv, const char *prefix)
> +{
> +	git_config_set("hook.autoupdate", "true");
> +	hook_update_suggested();
> +	return 0;
> +}

The name is a little bit vague to me - this function turns on autoupdate
and performs one hook update, so I think the name at least should cover
that it's related to hooks, and that it's doing setup.

If we wanted to gate "automagically turn on autoupdate" behind a
system-level config like I mentioned in my review of the cover letter,
we could do it here, for example by setting "hook.autoupdate" to the
value of "experimental.autoupdatehooks" (or whatever name is least
painful).

> +
>  int cmd_hook(int argc, const char **argv, const char *prefix)
>  {
>  	const char *run_hookdir = NULL;
> @@ -152,10 +159,6 @@ int cmd_hook(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix, builtin_hook_options,
>  			     builtin_hook_usage, PARSE_OPT_KEEP_UNKNOWN);
>  
> -	/* after the parse, we should have "<command> <hookname> <args...>" */
> -	if (argc < 2)
> -		usage_with_options(builtin_hook_usage, builtin_hook_options);
> -
>  	git_config(git_default_config, NULL);
>  
>  
> @@ -185,6 +188,8 @@ int cmd_hook(int argc, const char **argv, const char *prefix)
>  		return list(argc, argv, prefix);
>  	if (!strcmp(argv[0], "run"))
>  		return run(argc, argv, prefix);
> +	if (!strcmp(argv[0], "autoupdate"))
> +		return autoupdate(argc, argv, prefix);
>  
>  	usage_with_options(builtin_hook_usage, builtin_hook_options);
>  }
> diff --git a/hook.c b/hook.c
> index 3ccacb72fa..5730e6e501 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -4,6 +4,12 @@
>  #include "config.h"
>  #include "run-command.h"
>  #include "prompt.h"
> +#include "commit.h"
> +#include "object.h"
> +#include "refs.h"
> +#include "tree-walk.h"
> +#include "tree.h"
> +#include "streaming.h"
>  
>  /*
>   * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
> @@ -515,3 +521,81 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options)
>  
>  	return cb_data.rc;
>  }
> +
> +static int is_hook(const char *filename)
> +{
> +	int i;
> +
> +	for (i = 0; i < hook_name_count; i++) {
> +		if (!strcmp(filename, hook_name[i]))
> +			return 1;
> +	}
> +	return 0;
> +}
> +
> +void hook_update_suggested(void)
> +{
> +	struct object_id oid;
> +	struct object *obj;
> +	struct tree *tree;
> +	struct tree_desc desc;
> +	struct name_entry entry;
> +	struct strbuf path = STRBUF_INIT;
> +
> +	if (read_ref("refs/remotes/origin/suggested-hooks", &oid)) {
> +		warning(_("no such ref refs/remotes/origin/suggested-hooks, not updating hooks"));
> +		return;
> +	}
> +
> +	obj = parse_object(the_repository, &oid);
> +	if (obj == NULL) {
> +		warning(_("object pointed to by refs/remotes/origin/suggested-hooks '%s' does not exist"),
> +			oid_to_hex(&oid));
> +		return;
> +	}
> +	if (obj->type != OBJ_COMMIT) {
> +		warning(_("object pointed to by refs/remotes/origin/suggested-hooks '%s' is not a commit"),
> +			oid_to_hex(&oid));
> +		return;
> +	}
> +
> +	tree = get_commit_tree((struct commit *) obj);
> +	if (parse_tree(tree)) {
> +		warning(_("could not parse tree"));
> +		return;
> +	}
> +
> +	init_tree_desc(&desc, tree->buffer, tree->size);
> +	while (tree_entry(&desc, &entry)) {
> +		int fd;
> +
> +		if (!is_hook(entry.path)) {
> +			warning(_("file '%s' is not a hook; ignoring"),
> +				entry.path);
> +			continue;
> +		}
> +		if (S_ISDIR(entry.mode) || S_ISGITLINK(entry.mode)) {
> +			warning(_("file '%s' is not an ordinary file; ignoring"),
> +				entry.path);
> +			continue;
> +		}
> +
> +		strbuf_reset(&path);
> +		strbuf_git_path(&path, "hooks/%s", entry.path);
> +		fd = open(path.buf, O_WRONLY | O_CREAT, 0755);
> +		if (fd < 0) {
> +			warning_errno(_("could not create file '%s'; skipping this hook"),
> +				      path.buf);
> +			continue;
> +		}
> +		if (stream_blob_to_fd(fd, &entry.oid, NULL, 1)) {
> +			warning(_("could not write to file '%s'; skipping this hook"),
> +				path.buf);
> +			continue;
> +		}
> +		close(fd);
> +	}
> +	strbuf_release(&path);
> +
> +	return;
> +}

I saw that you wanted to reduce reliance on es/config-based-hooks, which
makes sense; but if you wanted to do it the configgy way, I'd instead
add a local config-based hook with a command something like `exec -
<<<$(git show refs/suggested-hooks:presubmit)` (that's not quite
perfect, but I hope gets the point across - materialize the file from
the local ref and execute it, rather than copying it into .git/hooks).

One bonus of doing it that way is that user can commit to their local
suggested-hooks branch and try out their new hook (and even send a
review from there with the change they liked). Hrm.

> diff --git a/hook.h b/hook.h
> index d902166408..b0bfd3719b 100644
> --- a/hook.h
> +++ b/hook.h
> @@ -140,3 +140,5 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options);
>  void free_hook(struct hook *ptr);
>  /* Empties the list at 'head', calling 'free_hook()' on each entry */
>  void clear_hook_list(struct list_head *head);
> +
> +void hook_update_suggested(void);
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index c0688467e7..21b96a4999 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -752,6 +752,42 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe
>  	git clone --filter=blob:limit=0 "file://$(pwd)/server" client
>  '
>  
> +test_expect_success 'detect suggested hook branch during clone' '
> +	rm -rf server client &&
> +
> +	test_create_repo server &&
> +	echo "echo foo" >server/pre-commit &&
> +	git -C server add pre-commit &&
> +	test_commit -C server hook-and-not-hook &&
> +	git -C server checkout -b suggested-hooks &&
> +	git clone server client 2>err &&
> +	test_i18ngrep "The remote has suggested hooks" err
> +'
> +
> +test_expect_success 'autoupdate' '
> +	git -C client hook autoupdate &&
> +
> +	# Check correct config written
> +	git -C client config --local hook.autoupdate >actual &&
> +	echo "true" >expect &&
> +	test_cmp expect actual &&
> +
> +	# Check hook written
> +	cat client/.git/hooks/pre-commit >actual &&
> +	echo "echo foo" >expect &&
> +	test_cmp expect actual &&
> +
> +	# Install new hook and fetch it
> +	echo "echo bar" >server/pre-commit &&
> +	git -C server commit -am "new hook" &&
> +	git -C client fetch &&
> +
> +	# Check new hook written
> +	cat client/.git/hooks/pre-commit >actual &&
> +	echo "echo bar" >expect &&
> +	test_cmp expect actual
> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd
>  
> -- 
> 2.32.0.272.g935e593368-goog
> 

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>  1. The remote repo administrator creates a new branch
>     "refs/heads/suggested-hooks" pointing to a commit that has all the
>     hooks that the administrator wants to suggest. The hooks are
>     directly referenced by the commit tree (i.e. they are in the "/"
>     directory).

wants to suggest?  They simply suggest ;-)

>  2. When a user clones, Git notices that
>     "refs/remotes/origin/suggested-hooks" is present and prints out a
>     message about a command that can be run.

Can be run to install?  Or can be run to first inspect?  Or both?

>  3. If the user runs that command, Git will install the hooks pointed to
>     by that ref, and set hook.autoupdate to true. This config variable
>     is checked whenever "git fetch" is run: whenever it notices that
>     "refs/remotes/origin/suggested-hooks" changes, it will reinstall the
>     hooks.
>
>  4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks
>     will remain.

OK, so "verify even if you implicitly trust" is actively discouraged.

> Design choices:
>
>  1. Where should the suggested hooks be in the remote repo? A branch,
>     a non-branch ref, a config? I think that a branch is best - it is
>     relatively well-understood and any hooks there can be
>     version-controlled (and its history is independent of the other
>     branches).

As people mentioned in the previous discussions, "independent of the
other branches" has advantages and disadvantages.  The most recent
set of hooks may have some that would not work well with older
codebase, so care must be taken to ensure any hook works on across
versions of the main codebase.  Which may not be a huge downside,
but something users must be aware of.

>  2. When and how should the local repo update its record of the remote's
>     suggested hooks? If we go with storing the hooks in a branch of a
>     remote side, this would automatically mean (with the default
>     refspec) that it would be in refs/remotes/<remote>/<name>. This
>     incurs network and hard disk cost even if the local repo does not
>     want to use the suggested hooks, but I think that typically they
>     would want to use it if they're going to do any work on the repo
>     (they would either have to trust or inspect Makefiles etc. anyway,
>     so they can do the same for the hooks), and even if they don't want
>     to use the remote's hooks, they probably still want to know what the
>     remote suggests.

A way to see what changes are made to recommendation would be
useful, and a branch that mostly linearly progresses is a good way
to give it to the users.

Of course, that can be done with suggested hooks inline with the
rest of the main codebase, too.

>  4. Should the local repo try to notice if the hooks have been changed
>     locally before overwriting upon autoupdate? This would be nice to
>     have, but I don't know how practical it would be. In particular, I
>     don't know if we can trust that
>     "refs/remotes/origin/suggested-hooks" has not been clobbered.

Meaning clobbered by malicious parties?  Then the whole thing is a
no-go from security point of view.  Presumably you trust the main
content enough to work on top of it, so as long as you can trust
that refs/remotes/origin/hooks to the same degree that you would
trust refs/remotes/origin/master, I do not think it is a problem.

Whatever mechanism you use to materialize an updated version of the
hooks can and should record which commit on the suggested-hooks
branch the .git/hooks/* file is based on.  Then when the user wants
to update to a different version of suggested-hooks (either because
you auto-detected, or the user issued a command to update), you have

 - The current version in .git/hooks/*

 - The old pristine version (you recorded the commit when you
   updated the .git/hooks/* copy the last time)

 - The new pristine version (you have a remote-tracking branch).

and it is not a brain surgery to perform three-way merge to update
the first using the difference between the second and the third.

>  5. Should we have a command that manually updates the hooks with what's
>     in "refs/heads/suggested-hooks"? This is not in this patch set, but
>     it sounds like a good idea.

I wonder if having it bound as a submodule to a known location in
the main contents tree makes it easier to manage and more flexible.
Just like you can update the working tree of a submodule to a
version that is bound to the superproject tree, or to a more recent
version of the "branch" in the submodule, you can support workflows
that allow suggested hooks to advance independent of the main
contents version and that uses a specific version of suggested hooks
tied to the main contents.



> There are perhaps other points that I haven't thought of, of course.
>
> [1] https://lore.kernel.org/git/pull.908.git.1616105016055.gitgitgadget@gmail.com/
>
> Jonathan Tan (2):
>   hook: move list of hooks
>   clone,fetch: remote-suggested auto-updating hooks
>
>  builtin/bugreport.c |  38 ++------------
>  builtin/clone.c     |  10 ++++
>  builtin/fetch.c     |  21 ++++++++
>  builtin/hook.c      |  13 +++--
>  hook.c              | 118 ++++++++++++++++++++++++++++++++++++++++++++
>  hook.h              |   5 ++
>  t/t5601-clone.sh    |  36 ++++++++++++++
>  7 files changed, 202 insertions(+), 39 deletions(-)

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Jonathan Tan]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> >  1. The remote repo administrator creates a new branch
> >     "refs/heads/suggested-hooks" pointing to a commit that has all the
> >     hooks that the administrator wants to suggest. The hooks are
> >     directly referenced by the commit tree (i.e. they are in the "/"
> >     directory).
> 
> wants to suggest?  They simply suggest ;-)

Ah yes :-)

> 
> >  2. When a user clones, Git notices that
> >     "refs/remotes/origin/suggested-hooks" is present and prints out a
> >     message about a command that can be run.
> 
> Can be run to install?  Or can be run to first inspect?  Or both?

Right now I only have a command that installs, but I can provide the
appropriate "cat-file" invocations to inspect them as well.

> >  3. If the user runs that command, Git will install the hooks pointed to
> >     by that ref, and set hook.autoupdate to true. This config variable
> >     is checked whenever "git fetch" is run: whenever it notices that
> >     "refs/remotes/origin/suggested-hooks" changes, it will reinstall the
> >     hooks.
> >
> >  4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks
> >     will remain.
> 
> OK, so "verify even if you implicitly trust" is actively discouraged.

Yes I was thinking of the model in which we already trust upstream, but
I agree that verification can be useful. I think we can print the
"cat-file" commands needed to verify before installing, and add a mode
in which we tell the user that the hooks have been updated (but not
automatically install them).

> > Design choices:
> >
> >  1. Where should the suggested hooks be in the remote repo? A branch,
> >     a non-branch ref, a config? I think that a branch is best - it is
> >     relatively well-understood and any hooks there can be
> >     version-controlled (and its history is independent of the other
> >     branches).
> 
> As people mentioned in the previous discussions, "independent of the
> other branches" has advantages and disadvantages.  The most recent
> set of hooks may have some that would not work well with older
> codebase, so care must be taken to ensure any hook works on across
> versions of the main codebase.  Which may not be a huge downside,
> but something users must be aware of.

That's true - and on the flip side, I would presume that the
hook-introducing admin would usually want those hooks to apply
retroactively too (say, to someone updating a "maint" branch). I think
it's more flexible if hooks are in an independent branch, though - (if
independent branch) a hook can be written to tolerate an old codebase,
but if embedded in the code branch, such a hook cannot apply
retroactively to old code.

> >  2. When and how should the local repo update its record of the remote's
> >     suggested hooks? If we go with storing the hooks in a branch of a
> >     remote side, this would automatically mean (with the default
> >     refspec) that it would be in refs/remotes/<remote>/<name>. This
> >     incurs network and hard disk cost even if the local repo does not
> >     want to use the suggested hooks, but I think that typically they
> >     would want to use it if they're going to do any work on the repo
> >     (they would either have to trust or inspect Makefiles etc. anyway,
> >     so they can do the same for the hooks), and even if they don't want
> >     to use the remote's hooks, they probably still want to know what the
> >     remote suggests.
> 
> A way to see what changes are made to recommendation would be
> useful, and a branch that mostly linearly progresses is a good way
> to give it to the users.
> 
> Of course, that can be done with suggested hooks inline with the
> rest of the main codebase, too.

That's true.

> >  4. Should the local repo try to notice if the hooks have been changed
> >     locally before overwriting upon autoupdate? This would be nice to
> >     have, but I don't know how practical it would be. In particular, I
> >     don't know if we can trust that
> >     "refs/remotes/origin/suggested-hooks" has not been clobbered.
> 
> Meaning clobbered by malicious parties?  Then the whole thing is a
> no-go from security point of view.  Presumably you trust the main
> content enough to work on top of it, so as long as you can trust
> that refs/remotes/origin/hooks to the same degree that you would
> trust refs/remotes/origin/master, I do not think it is a problem.

I meant clobbered by the user - should have made that more clear. To
elaborate...

> Whatever mechanism you use to materialize an updated version of the
> hooks can and should record which commit on the suggested-hooks
> branch the .git/hooks/* file is based on.  Then when the user wants
> to update to a different version of suggested-hooks (either because
> you auto-detected, or the user issued a command to update), you have
> 
>  - The current version in .git/hooks/*
> 
>  - The old pristine version (you recorded the commit when you
>    updated the .git/hooks/* copy the last time)
> 
>  - The new pristine version (you have a remote-tracking branch).
> 
> and it is not a brain surgery to perform three-way merge to update
> the first using the difference between the second and the third.

...I was having a difficult time figuring out where to store such
information (ref? config? I wouldn't be surprised if a user saw a value
there and thought that they could change it). Perhaps it could be a
separate file like .git/shallow.

I'm not fully convinced that maintaining the ability to retain local
hook modifications by supporting three-way merges is important, but if
it is, it might be brain surgery to figure out the UX when merge
conflicts occur. Normally these conflicts can be written to the worktree
and index, but we don't have those in the case of hooks.

> >  5. Should we have a command that manually updates the hooks with what's
> >     in "refs/heads/suggested-hooks"? This is not in this patch set, but
> >     it sounds like a good idea.
> 
> I wonder if having it bound as a submodule to a known location in
> the main contents tree makes it easier to manage and more flexible.
> Just like you can update the working tree of a submodule to a
> version that is bound to the superproject tree, or to a more recent
> version of the "branch" in the submodule, you can support workflows
> that allow suggested hooks to advance independent of the main
> contents version and that uses a specific version of suggested hooks
> tied to the main contents.

This would make the hooks dependent on the commit checked out (with
perhaps a bit more leeway in that our implementation could be flexible
and use a commit later than what's in the gitlink), with its own pros
and cons (as you said earlier).

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Emily Shaffer]

On Wed, Jun 16, 2021 at 04:31:47PM -0700, Jonathan Tan wrote:
> 
> I have had to make several design choices (which I will discuss later),
> but now with this implementation, the following workflow is possible:
> 
>  1. The remote repo administrator creates a new branch
>     "refs/heads/suggested-hooks" pointing to a commit that has all the
>     hooks that the administrator wants to suggest. The hooks are
>     directly referenced by the commit tree (i.e. they are in the "/"
>     directory).

I don't really like that this is in the same namespace as branches users
could create themselves. Hm, I think for 'git maintenance' prefetching
we put those refs in some special namespace, right? Can we do something
similar in this case? Would that prevent us from treating that ref like
a normal branch?

> 
>  2. When a user clones, Git notices that
>     "refs/remotes/origin/suggested-hooks" is present and prints out a
>     message about a command that can be run.
> 
>  3. If the user runs that command, Git will install the hooks pointed to
>     by that ref, and set hook.autoupdate to true. This config variable
>     is checked whenever "git fetch" is run: whenever it notices that
>     "refs/remotes/origin/suggested-hooks" changes, it will reinstall the
>     hooks.

I think this is where most people will have a lot to say from security
perspective. It will be important to make sure that:
 - "git fetch" only automatically updates these scripts when fetching
   from the same remote they were initially fetched from, over some
   secure protocol
 - hook.autoupdate setting maybe should not be default, or should at
   least be able to override during the install command. (One suggestion
   was to also set some experimental.suggestedHooks setting, or
   something, to turn on "automatically set up autoupdate" behavior?)
 - we should notify the user that their hooks were updated/reinstalled
   when that happens later on. (Maybe you did this, I didn't look at the
   diff quite yet)

>  4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks
>     will remain.
> 
> Design choices:
> 
>  1. Where should the suggested hooks be in the remote repo? A branch,
>     a non-branch ref, a config? I think that a branch is best - it is
>     relatively well-understood and any hooks there can be
>     version-controlled (and its history is independent of the other
>     branches).

I agree - a branch without any prior parent sounds ideal to me, that is,
the repo owner setting up the hooks can say 'git checkout --orphan
suggested-hooks'. I guess if they want the branch to have history
shared with the rest of the project there's no reason not to do that,
either.

Do we want to provide helpful tooling for the administrator to create
this magic branch? At the very least I guess we want some documentation,
but I wonder if it's possible to make a helper (maybe a 'git hook'
subcommand) without being more prescriptive than Git project usually is.

>  3. How should the local repo detect when the remote has updated its
>     suggested hooks? I'm thinking when a certain local ref is updated.
>     Right now it's hardcoded, but perhaps "git clone" could detect what
>     "refs/heads/suggested-hooks" would correspond to, and then set it in
>     a config accordingly. Other options include remembering what the
>     remote's "refs/heads/suggested-hooks" used to be and always
>     comparing it upon every "ls-refs" call, but I think that the local
>     ref method is more straightforward.

Hm, I like that idea as it's analogous to remote tracking branch + local
tracking branch (which is a pretty common pattern for people to use).

> 
>  4. Should the local repo try to notice if the hooks have been changed
>     locally before overwriting upon autoupdate? This would be nice to
>     have, but I don't know how practical it would be. In particular, I
>     don't know if we can trust that
>     "refs/remotes/origin/suggested-hooks" has not been clobbered.

I think it would be nice to have, yeah. It seems important to notify the
users that they are executing something different from yesterday, so
noticing when we're making changes to the hook sounds useful.

> 
>  5. Should we have a command that manually updates the hooks with what's
>     in "refs/heads/suggested-hooks"? This is not in this patch set, but
>     it sounds like a good idea.

Yeah, I think so - that lets a user say "no, I don't need those hooks
(or this update)" at first, but later change their mind.

One thing that sounds useful to me, even at this RFC stage, is
documentation showing what this feature looks like to use - for both
the administrator setting up the magic hook branch, and the developer
cloning and using hooks. I think we want that in some Git manpage
eventually anyways, and it would help to see the workflow you're trying
to achieve.

Thanks for sending - will review the diffs today too.

 - Emily

> 
> There are perhaps other points that I haven't thought of, of course.
> 
> [1] https://lore.kernel.org/git/pull.908.git.1616105016055.gitgitgadget@gmail.com/
> 
> Jonathan Tan (2):
>   hook: move list of hooks
>   clone,fetch: remote-suggested auto-updating hooks
> 
>  builtin/bugreport.c |  38 ++------------
>  builtin/clone.c     |  10 ++++
>  builtin/fetch.c     |  21 ++++++++
>  builtin/hook.c      |  13 +++--
>  hook.c              | 118 ++++++++++++++++++++++++++++++++++++++++++++
>  hook.h              |   5 ++
>  t/t5601-clone.sh    |  36 ++++++++++++++
>  7 files changed, 202 insertions(+), 39 deletions(-)
> 
> -- 
> 2.32.0.272.g935e593368-goog
> 

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Jonathan Tan]

> On Wed, Jun 16, 2021 at 04:31:47PM -0700, Jonathan Tan wrote:
> > 
> > I have had to make several design choices (which I will discuss later),
> > but now with this implementation, the following workflow is possible:
> > 
> >  1. The remote repo administrator creates a new branch
> >     "refs/heads/suggested-hooks" pointing to a commit that has all the
> >     hooks that the administrator wants to suggest. The hooks are
> >     directly referenced by the commit tree (i.e. they are in the "/"
> >     directory).
> 
> I don't really like that this is in the same namespace as branches users
> could create themselves. Hm, I think for 'git maintenance' prefetching
> we put those refs in some special namespace, right? Can we do something
> similar in this case? Would that prevent us from treating that ref like
> a normal branch?

Do you mean that the server should put it in a different place, the
client should put it in a different place, or both?

> >  2. When a user clones, Git notices that
> >     "refs/remotes/origin/suggested-hooks" is present and prints out a
> >     message about a command that can be run.
> > 
> >  3. If the user runs that command, Git will install the hooks pointed to
> >     by that ref, and set hook.autoupdate to true. This config variable
> >     is checked whenever "git fetch" is run: whenever it notices that
> >     "refs/remotes/origin/suggested-hooks" changes, it will reinstall the
> >     hooks.
> 
> I think this is where most people will have a lot to say from security
> perspective. It will be important to make sure that:
>  - "git fetch" only automatically updates these scripts when fetching
>    from the same remote they were initially fetched from, over some
>    secure protocol

Putting the hooks in a branch does mean that "git fetch" gets these
scripts from the same remote (as opposed to, say, putting them in a
submodule or referring to them by URL), and I think it's easier to
understand (from the user point of view) that branch equals same remote
(rather than say that a submodule or URL is considered to be from the
same remote if $CRITERIA).

>  - hook.autoupdate setting maybe should not be default, or should at
>    least be able to override during the install command. (One suggestion
>    was to also set some experimental.suggestedHooks setting, or
>    something, to turn on "automatically set up autoupdate" behavior?)

OK, this makes sense.

>  - we should notify the user that their hooks were updated/reinstalled
>    when that happens later on. (Maybe you did this, I didn't look at the
>    diff quite yet)

I didn't do this, but this is a good idea.

> > Design choices:
> > 
> >  1. Where should the suggested hooks be in the remote repo? A branch,
> >     a non-branch ref, a config? I think that a branch is best - it is
> >     relatively well-understood and any hooks there can be
> >     version-controlled (and its history is independent of the other
> >     branches).
> 
> I agree - a branch without any prior parent sounds ideal to me, that is,
> the repo owner setting up the hooks can say 'git checkout --orphan
> suggested-hooks'. I guess if they want the branch to have history
> shared with the rest of the project there's no reason not to do that,
> either.

How would the branch share history with the rest of the project? If you
mean that we should allow the users to store hooks as part of the main
codebase (within a configurable path, say, /suggested_hooks) and then
point both "main" (or whatever the default branch is) and
"refs/remotes/origin/suggested-hooks" to the same place, then that
makes sense (although I would say that it still sounds better to have
separate histories).

> Do we want to provide helpful tooling for the administrator to create
> this magic branch? At the very least I guess we want some documentation,
> but I wonder if it's possible to make a helper (maybe a 'git hook'
> subcommand) without being more prescriptive than Git project usually is.

This sounds like the equivalent of "git checkout --orphan", as you
suggested above. We could just write it out in the documentation. I'm
not completely opposed to a special command, though.

> >  3. How should the local repo detect when the remote has updated its
> >     suggested hooks? I'm thinking when a certain local ref is updated.
> >     Right now it's hardcoded, but perhaps "git clone" could detect what
> >     "refs/heads/suggested-hooks" would correspond to, and then set it in
> >     a config accordingly. Other options include remembering what the
> >     remote's "refs/heads/suggested-hooks" used to be and always
> >     comparing it upon every "ls-refs" call, but I think that the local
> >     ref method is more straightforward.
> 
> Hm, I like that idea as it's analogous to remote tracking branch + local
> tracking branch (which is a pretty common pattern for people to use).

Yeah - that is partly why I like the branch idea (something already
generally understood by people).

> >  4. Should the local repo try to notice if the hooks have been changed
> >     locally before overwriting upon autoupdate? This would be nice to
> >     have, but I don't know how practical it would be. In particular, I
> >     don't know if we can trust that
> >     "refs/remotes/origin/suggested-hooks" has not been clobbered.
> 
> I think it would be nice to have, yeah. It seems important to notify the
> users that they are executing something different from yesterday, so
> noticing when we're making changes to the hook sounds useful.

Junio suggested [1] that we store more information about what's going
on, so this might be practical.

[1] https://lore.kernel.org/git/xmqq35thnuqp.fsf@gitster.g/

> >  5. Should we have a command that manually updates the hooks with what's
> >     in "refs/heads/suggested-hooks"? This is not in this patch set, but
> >     it sounds like a good idea.
> 
> Yeah, I think so - that lets a user say "no, I don't need those hooks
> (or this update)" at first, but later change their mind.

OK.

> One thing that sounds useful to me, even at this RFC stage, is
> documentation showing what this feature looks like to use - for both
> the administrator setting up the magic hook branch, and the developer
> cloning and using hooks. I think we want that in some Git manpage
> eventually anyways, and it would help to see the workflow you're trying
> to achieve.
> 
> Thanks for sending - will review the diffs today too.
> 
>  - Emily

Agreed - at the very least, we would need to write this workflow as a
test, and we can just reuse the same workflow in documentation. We just
need to discuss what the workflow should be :-) (as it is, one thing
that is becoming obvious is that currently people prefer to have
verify-before-update instead of auto-update by default).

RE: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Randall S. Becker]

On June 18, 2021 5:59 PM, Jonathan Tan wrote:
>> On Wed, Jun 16, 2021 at 04:31:47PM -0700, Jonathan Tan wrote:
>> >
>> > I have had to make several design choices (which I will discuss
>> > later), but now with this implementation, the following workflow is possible:
>> >
>> >  1. The remote repo administrator creates a new branch
>> >     "refs/heads/suggested-hooks" pointing to a commit that has all the
>> >     hooks that the administrator wants to suggest. The hooks are
>> >     directly referenced by the commit tree (i.e. they are in the "/"
>> >     directory).
>>
>> I don't really like that this is in the same namespace as branches
>> users could create themselves. Hm, I think for 'git maintenance'
>> prefetching we put those refs in some special namespace, right? Can we
>> do something similar in this case? Would that prevent us from treating
>> that ref like a normal branch?
>
>Do you mean that the server should put it in a different place, the client should put it in a different place, or both?

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?

It seems like changes to hooks have to be managed in a similar way to standard managed files rather than as exceptions.

-Randall

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Matt Rogers]

On Sat, Jun 19, 2021 at 3:32 AM Randall S. Becker
<rsbecker@nexbridge.com> wrote:
>
> On June 18, 2021 5:59 PM, Jonathan Tan wrote:
> 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?
>

Related question, if a project had a collection of suggested hooks,
and a contributor wanted
to update them in relation to a new feature or code change, would they
then have to create two
separate patches/PRs?  That feels like it would decentralize discussions/review?

For example, if I maintained a project on GitHub and a contributor
wanted to add a clang-tidy
invocation as a suggested hook, as well as a config file for it.  What
would the suggested workflow be?

"Open 2 Pull Requests one that updates both branches and do reviews
independently"?

If it was a mailing list would I have to send two separate patches?

I'm not familiar with any workflows or tools that allow you to review
and accept changes to
two branches as part of the same series of changes.

I also think that the history wouldn't be very clear in this case,
since there won't be any obvious
connection between updates to the suggested-hooks and updates to the
rest of the history in
this case (other than timestamps I guess, but I think that's a
relatively weak form of association)

> It seems like changes to hooks have to be managed in a similar way to standard managed files rather than as exceptions.
>
> -Randall
>


Thanks,
Matthew Rogers

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Jonathan Tan]

> Related question, if a project had a collection of suggested hooks,
> and a contributor wanted
> to update them in relation to a new feature or code change, would they
> then have to create two
> separate patches/PRs?  That feels like it would decentralize discussions/review?
> 
> For example, if I maintained a project on GitHub and a contributor
> wanted to add a clang-tidy
> invocation as a suggested hook, as well as a config file for it.  What
> would the suggested workflow be?
> 
> "Open 2 Pull Requests one that updates both branches and do reviews
> independently"?
> 
> If it was a mailing list would I have to send two separate patches?

With the current design, the answer is "yes" to both. Having said that,
for this particular case, I think that the clang-tidy config file can be
reviewed and merged first, and then the clang-tidy hook. (The hook needs
to be written to also work in the absence of the config file, since the
user may have checked out an older version, so as far as I can tell,
there is no need for precise timing as to when the hook is merged.)

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 16 2021, Jonathan Tan wrote:

> This is a continuation of the work from [1]. That work described the
> reasons, possible features, and possible workflows, but not the
> implementation in detail. This patch set has an MVP implementation, and
> my hope is that having a concrete implementation to look at makes it
> easier to discuss matters of implementation.

My C on this RFC is:

1) A request that someone reply (there or here would do) to my comments
   on the last iteration of this at:
   https://lore.kernel.org/git/874kghk906.fsf@evledraar.gmail.com/

2) I think you'd get better feedback if you CC'd the people who've been
  actively discussing this in previous rounds.

To briefly summarize the main gist of that 1):

> This but does not use any features from es/config-based-hooks but is
> implemented on that branch anyway because firstly, I need an existing
> command to attach the "autoupdate" subcommand (and "git hook" works),
> and secondly, when we test this at $DAYJOB, we will be testing it
> together with the aforementioned branch.
>
> I have had to make several design choices (which I will discuss later),
> but now with this implementation, the following workflow is possible:
>
>  1. The remote repo administrator creates a new branch
>     "refs/heads/suggested-hooks" pointing to a commit that has all the
>     hooks that the administrator wants to suggest. The hooks are
>     directly referenced by the commit tree (i.e. they are in the "/"
>     directory).
>
>  2. When a user clones, Git notices that
>     "refs/remotes/origin/suggested-hooks" is present and prints out a
>     message about a command that can be run.
>
>  3. If the user runs that command, Git will install the hooks pointed to
>     by that ref, and set hook.autoupdate to true. This config variable
>     is checked whenever "git fetch" is run: whenever it notices that
>     "refs/remotes/origin/suggested-hooks" changes, it will reinstall the
>     hooks.
>
>  4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks
>     will remain.
>
> Design choices:
>
>  1. Where should the suggested hooks be in the remote repo? A branch,
>     a non-branch ref, a config? I think that a branch is best - it is
>     relatively well-understood and any hooks there can be
>     version-controlled (and its history is independent of the other
>     branches).

First, unlike brian I don't (I hope I'm fairly summarizing his view
here) disagree mostly or entirely with the existence of such a feature
at all. I mean, I get the viewpoint that git shouldn't bless what
amounts to an active RCE from the remote.

I just think that we could probably do a better job of it than what
people are doing in practice, and I've seen people do stuff like have
build systems setup permanent symlinks to git-hooks/<some-name> in the
tracked dir. We could at least envision a git-native implementation
asking the user "do you want this hook update? <shows diff>".

I just find this design approach completely bizarre as noted (probably
in less blunt words) in the linked E-Mail.

We have Emily's series to convert hooks to be config driven that we hope
to land in some form, at that point they won't be any more of a special
snowflake than any other config.

And then, instead of doing what I'd think would be the natural result of
that: Simply supporting an in-repo top-level ".gitconfig" file. We're
still going to seemingly forever have them be an even more special
snowflake with this facility, and the reason seems to be mostly/entirely
to do with working around some aspect or restriction of Google's
internal infrastructure.

I think it's just un-git-y to have a meta-branch that in some way drives
not only all other branches, but all other revisions of all branches,
ever.

It breaks expectations around git in lots of different ways, you can't
fetch a single branch and get its hooks, you can't locally alter, commit
and update your hooks while e.g. renaming a "t/" directory to "test/";
your hooks and code can't be atomically changed).

I think I get why you want to do it that way, I just don't get why, as
mostly noted in those earlier rounds why it wouldn't be a better
approach / more straightforward / more git-y to:

1. Work on getting hooks driven by config <this is happening with
   Emily's series / my split-out "base" topic>
2. Have a facility to read an in-repo '.gitconfig'; have lots of safety
   valves etc. around this, I suggested starting with a whitelist of the
   N least dangerous config options, e.g. some diff viewing options, or
   a suggested sendemail.to or whatever.

3. Work our way up to trusting that for more dangerous stuff, eventually
   hooks. Most of the legitimate concerns from others with this is
   having some UX where our users won't be trained to just blindly say
   "yes" to an alias/hook config that "rm -rf's /" or whatever.

   If we start experimenting with that with aliases or hooks that can
   run arbitrary code it's like handing a toddlder a shotgun, let's at
   least start with a sharp fork or something (less dangerous config) :)

4. People who want this "I want my hooks to apply to all revisions ever"
   could probably get 99% or 100% of what they want if their hook is
   just a stub that does the equivalent of:

       sh `curl https://git.google.com/$reponame/hooks/$hookname`

   You'd then simply forbid on your servers any changes to a .gitconfig
   that did anything with the hook.* namespace.

With such an implementation you don't need a magic
"refs/remotes/origin/suggested-hooks" refs, just some state machine (I
suggested e.g. GPG signing chains as an eventual end-state, but "show a
diff every time" would also do) that keeps track of what config (and
hooks are just one such case) has been OK'd, and which has not.

I'd think it would even work better in the Googleplex, you could clone a
co-worker's branch and execute their hooks, since they're the same as
what you've pre-approved, you could even clone some random person's fork
of a "blessed" project, because the hooks would be the same `sh $(curl
<url I already trust>)`. That validation could even be a system-level
in-config hook on your laptop, thus bringing the whole thing full
circle...

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Jonathan Tan]

> On Wed, Jun 16 2021, Jonathan Tan wrote:
> 
> > This is a continuation of the work from [1]. That work described the
> > reasons, possible features, and possible workflows, but not the
> > implementation in detail. This patch set has an MVP implementation, and
> > my hope is that having a concrete implementation to look at makes it
> > easier to discuss matters of implementation.
> 
> My C on this RFC is:
> 
> 1) A request that someone reply (there or here would do) to my comments
>    on the last iteration of this at:
>    https://lore.kernel.org/git/874kghk906.fsf@evledraar.gmail.com/

OK - I'll take a look at that.

> 2) I think you'd get better feedback if you CC'd the people who've been
>   actively discussing this in previous rounds.

Good point.

> > Design choices:
> >
> >  1. Where should the suggested hooks be in the remote repo? A branch,
> >     a non-branch ref, a config? I think that a branch is best - it is
> >     relatively well-understood and any hooks there can be
> >     version-controlled (and its history is independent of the other
> >     branches).
> 
> First, unlike brian I don't (I hope I'm fairly summarizing his view
> here) disagree mostly or entirely with the existence of such a feature
> at all. I mean, I get the viewpoint that git shouldn't bless what
> amounts to an active RCE from the remote.
> 
> I just think that we could probably do a better job of it than what
> people are doing in practice, and I've seen people do stuff like have
> build systems setup permanent symlinks to git-hooks/<some-name> in the
> tracked dir. We could at least envision a git-native implementation
> asking the user "do you want this hook update? <shows diff>".
> 
> I just find this design approach completely bizarre as noted (probably
> in less blunt words) in the linked E-Mail.

That's fair. You suggest an alternative below (and maybe more in the
linked e-mail) - let's look at your suggestion...

> We have Emily's series to convert hooks to be config driven that we hope
> to land in some form, at that point they won't be any more of a special
> snowflake than any other config.
> 
> And then, instead of doing what I'd think would be the natural result of
> that: Simply supporting an in-repo top-level ".gitconfig" file. We're
> still going to seemingly forever have them be an even more special
> snowflake with this facility, and the reason seems to be mostly/entirely
> to do with working around some aspect or restriction of Google's
> internal infrastructure.

I don't think that this is "natural". In particular, I still don't think
that hooks should be tied to code revision. E.g. if we make commits
based on an old revision and push them, we still want them to follow the
latest requirements.

> I think it's just un-git-y to have a meta-branch that in some way drives
> not only all other branches, but all other revisions of all branches,
> ever.
> 
> It breaks expectations around git in lots of different ways, you can't
> fetch a single branch and get its hooks,

Are you saying that each branch should have its own hooks? That might be
reasonable in certain projects, but I don't see how that is a Git
expectation.

> you can't locally alter, commit
> and update your hooks while e.g. renaming a "t/" directory to "test/";
> your hooks and code can't be atomically changed).

I still think that hooks should work independent of code versions, so I
wouldn't think that atomicity here is important.

> I think I get why you want to do it that way, I just don't get why, as
> mostly noted in those earlier rounds why it wouldn't be a better
> approach / more straightforward / more git-y to:
> 
> 1. Work on getting hooks driven by config <this is happening with
>    Emily's series / my split-out "base" topic>
> 2. Have a facility to read an in-repo '.gitconfig'; have lots of safety
>    valves etc. around this, I suggested starting with a whitelist of the
>    N least dangerous config options, e.g. some diff viewing options, or
>    a suggested sendemail.to or whatever.

I've replied to this above.

> 3. Work our way up to trusting that for more dangerous stuff, eventually
>    hooks. Most of the legitimate concerns from others with this is
>    having some UX where our users won't be trained to just blindly say
>    "yes" to an alias/hook config that "rm -rf's /" or whatever.
> 
>    If we start experimenting with that with aliases or hooks that can
>    run arbitrary code it's like handing a toddlder a shotgun, let's at
>    least start with a sharp fork or something (less dangerous config) :)
> 
> 4. People who want this "I want my hooks to apply to all revisions ever"
>    could probably get 99% or 100% of what they want if their hook is
>    just a stub that does the equivalent of:
> 
>        sh `curl https://git.google.com/$reponame/hooks/$hookname`
> 
>    You'd then simply forbid on your servers any changes to a .gitconfig
>    that did anything with the hook.* namespace.

This would work if set in .git/config (not version controlled), but not
.gitconfig (version controlled).

> With such an implementation you don't need a magic
> "refs/remotes/origin/suggested-hooks" refs, just some state machine (I
> suggested e.g. GPG signing chains as an eventual end-state, but "show a
> diff every time" would also do) that keeps track of what config (and
> hooks are just one such case) has been OK'd, and which has not.

This sounds complicated.

> I'd think it would even work better in the Googleplex, you could clone a
> co-worker's branch and execute their hooks, since they're the same as
> what you've pre-approved,

In the presence of .gitconfig, how would you know?

> you could even clone some random person's fork
> of a "blessed" project, because the hooks would be the same `sh $(curl
> <url I already trust>)`. That validation could even be a system-level
> in-config hook on your laptop, thus bringing the whole thing full
> circle...

Same here.

In summary, I think your point of using hook configs + remote-suggested
configs instead of remote-suggested hooks is a reasonable one, but I
disagree with your reasons (or, at least, your reasons as I understand
them).

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 21 2021, Jonathan Tan wrote:

>> On Wed, Jun 16 2021, Jonathan Tan wrote:
>> 
>> > This is a continuation of the work from [1]. That work described the
>> > reasons, possible features, and possible workflows, but not the
>> > implementation in detail. This patch set has an MVP implementation, and
>> > my hope is that having a concrete implementation to look at makes it
>> > easier to discuss matters of implementation.
>> 
>> My C on this RFC is:
>> 
>> 1) A request that someone reply (there or here would do) to my comments
>>    on the last iteration of this at:
>>    https://lore.kernel.org/git/874kghk906.fsf@evledraar.gmail.com/
>
> OK - I'll take a look at that.
>
>> 2) I think you'd get better feedback if you CC'd the people who've been
>>   actively discussing this in previous rounds.
>
> Good point.
>
>> > Design choices:
>> >
>> >  1. Where should the suggested hooks be in the remote repo? A branch,
>> >     a non-branch ref, a config? I think that a branch is best - it is
>> >     relatively well-understood and any hooks there can be
>> >     version-controlled (and its history is independent of the other
>> >     branches).
>> 
>> First, unlike brian I don't (I hope I'm fairly summarizing his view
>> here) disagree mostly or entirely with the existence of such a feature
>> at all. I mean, I get the viewpoint that git shouldn't bless what
>> amounts to an active RCE from the remote.
>> 
>> I just think that we could probably do a better job of it than what
>> people are doing in practice, and I've seen people do stuff like have
>> build systems setup permanent symlinks to git-hooks/<some-name> in the
>> tracked dir. We could at least envision a git-native implementation
>> asking the user "do you want this hook update? <shows diff>".
>> 
>> I just find this design approach completely bizarre as noted (probably
>> in less blunt words) in the linked E-Mail.
>
> That's fair. You suggest an alternative below (and maybe more in the
> linked e-mail) - let's look at your suggestion...
>
>> We have Emily's series to convert hooks to be config driven that we hope
>> to land in some form, at that point they won't be any more of a special
>> snowflake than any other config.
>> 
>> And then, instead of doing what I'd think would be the natural result of
>> that: Simply supporting an in-repo top-level ".gitconfig" file. We're
>> still going to seemingly forever have them be an even more special
>> snowflake with this facility, and the reason seems to be mostly/entirely
>> to do with working around some aspect or restriction of Google's
>> internal infrastructure.
>
> I don't think that this is "natural". In particular, I still don't think
> that hooks should be tied to code revision. E.g. if we make commits
> based on an old revision and push them, we still want them to follow the
> latest requirements.

Even for real-world centralized workflow situations where I've seen
people think they want that, and the end of the day they almost never
actually want that.

Even something like code linting is a good example, to make it
Google-specific: say for a Go project: Are you going to pin your linting
tool/version to whatever understood your YAML format for the linter as
it was specced 10 years ago when the project started?  It's simply a
giant hassle to have a piece of code operate on every version of your
project ever in a way that doesn't break.

I think in practice the designers of this feature don't actually have
that in mind, but a "close to trunk" workflow, where you'd expect a hook
to only need to operate on revisions for the last few weeks or months,
because that'll be the oldest think people create new topics from.

But I think the burden of proof is really on the other side here,
something that works entirely differently than the rest of git needs to
have a good reason. Our in-repo .gitattributes don't work like this, nor
.gitignore, .mailmap etc.

There's also real world uses of git where the "branches" are wildly
divergent, e.g. I've worked on a system automation repo where the
"master" was just a stub template, and every team had their own almost
entirely different "repo-like" branch. Probably a bad idea for various
reasons, but Git supports it just fine.

For the centralized use-case what's the problem with just having the
hook do a 'for-each-ref --format=' invocation or "cat-file --batch" on
the "origin", and eval what it finds there? I'd think that gives you
what you want for the more centarlized workflow, while leaving git's
implementation working like the rest of our in-repo files.

>> I think it's just un-git-y to have a meta-branch that in some way drives
>> not only all other branches, but all other revisions of all branches,
>> ever.
>> 
>> It breaks expectations around git in lots of different ways, you can't
>> fetch a single branch and get its hooks,
>
> Are you saying that each branch should have its own hooks? That might be
> reasonable in certain projects, but I don't see how that is a Git
> expectation.

It's a git expectation now that I can add git.git as a remote, also
chromium.git, and linux.git, fetch them all, and happily switch in the
same repo between entirely different codebases that don't share a
history.

>> you can't locally alter, commit
>> and update your hooks while e.g. renaming a "t/" directory to "test/";
>> your hooks and code can't be atomically changed).
>
> I still think that hooks should work independent of code versions, so I
> wouldn't think that atomicity here is important.

Covered above.

>> I think I get why you want to do it that way, I just don't get why, as
>> mostly noted in those earlier rounds why it wouldn't be a better
>> approach / more straightforward / more git-y to:
>> 
>> 1. Work on getting hooks driven by config <this is happening with
>>    Emily's series / my split-out "base" topic>
>> 2. Have a facility to read an in-repo '.gitconfig'; have lots of safety
>>    valves etc. around this, I suggested starting with a whitelist of the
>>    N least dangerous config options, e.g. some diff viewing options, or
>>    a suggested sendemail.to or whatever.
>
> I've replied to this above.

Not really, even if we went for this one-HEAD-version-to-rule-them-all
plan wouldn't it make more sense to generalize it as a
refs/remotes/origin/magic-config, and we'd discover a ".gitconfig" file
under that commit/tree.

I.e. whether we generalize this to config in general is orthagonal to
whether such config lives in HEAD or in a magic ref.

With hooks as config I don't see how you'd make any of this
hook-specific, there's other config where the "every revision ever"
applies much more strongly, e.g. sendemail.to. If that changed for this
project tomorrow you wouldn't want a patch based on "maint" to send
things to a different ML.

>> 3. Work our way up to trusting that for more dangerous stuff, eventually
>>    hooks. Most of the legitimate concerns from others with this is
>>    having some UX where our users won't be trained to just blindly say
>>    "yes" to an alias/hook config that "rm -rf's /" or whatever.
>> 
>>    If we start experimenting with that with aliases or hooks that can
>>    run arbitrary code it's like handing a toddlder a shotgun, let's at
>>    least start with a sharp fork or something (less dangerous config) :)
>> 
>> 4. People who want this "I want my hooks to apply to all revisions ever"
>>    could probably get 99% or 100% of what they want if their hook is
>>    just a stub that does the equivalent of:
>> 
>>        sh `curl https://git.google.com/$reponame/hooks/$hookname`
>> 
>>    You'd then simply forbid on your servers any changes to a .gitconfig
>>    that did anything with the hook.* namespace.
>
> This would work if set in .git/config (not version controlled), but not
> .gitconfig (version controlled).

Sorry, what wouldn't work? I meant you'd forbid pushes to your in-repo
.gitconfig in your "master" branch or whatever, just like you're
presumably planning some stronger ACLs for this magic hook branch.

>> With such an implementation you don't need a magic
>> "refs/remotes/origin/suggested-hooks" refs, just some state machine (I
>> suggested e.g. GPG signing chains as an eventual end-state, but "show a
>> diff every time" would also do) that keeps track of what config (and
>> hooks are just one such case) has been OK'd, and which has not.
>
> This sounds complicated.

On the contrary I think anything that leans into git's
content-addressable security model is way less complicated. You don't
care who you fetched Junio's v2.32.0 tag from, what matters is that the
signing chain validates.

The plan of having this magic branch means a whole new trust model for
git, you trust magical authorized remotes. If you trust signed content
chains you can trust hooks if their last modification can be traced to a
signing authority you trust.

    It's really just:

        if (hook_content_changed() && hook_content_same_as_in_ok'd_revision_from_upsteam())
            trust_hooks();

But while we're on the subject, it seems like a very generous assumption
to think that just because you trust hooks at a given revision (or
always trust the latest), that you implicitly trust them when *combined
with* all past and future revisions from the same repository.

Even without a malicious actor that seems like it'll inevitably break in
all sorts of data-destroying ways. E.g. people commit stuff
accidentally. A hook run under a "git bisect" that naïvely does an "rm
*" will eat your data if you land on a revision that an in-tree "-rf"
file.

But once you get to a malicious actor who can say push a topic branch
but not hook updates, will your hooks deal with files with whitespace in
them, arbitrary crafted content etc?

So I'd think that's an even better reason to prefer the in-repo
per-revision atomically committed plan, and only trust hooks for the
revision they're shipped with, at least as a default git security model.

>> I'd think it would even work better in the Googleplex, you could clone a
>> co-worker's branch and execute their hooks, since they're the same as
>> what you've pre-approved,
>
> In the presence of .gitconfig, how would you know?

If it's the same config, or you can automatically OK it. So "same" was
discussed above, or you could trust any hook that's only doing a wget of
some trusted domain and piping that to "sh".

>> you could even clone some random person's fork
>> of a "blessed" project, because the hooks would be the same `sh $(curl
>> <url I already trust>)`. That validation could even be a system-level
>> in-config hook on your laptop, thus bringing the whole thing full
>> circle...
>
> Same here.
>
> In summary, I think your point of using hook configs + remote-suggested
> configs instead of remote-suggested hooks is a reasonable one, but I
> disagree with your reasons (or, at least, your reasons as I understand
> them).

You trust e.g. chromium.git's hooks, but I clone it, patch it, and
re-push it to somegithost.com URL. If you go with trusting content it
becomes easy to install those trusted hooks for the common case, but not
if your entire trust model relies on what URL you git clone'd from.

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Jonathan Tan]

> >> And then, instead of doing what I'd think would be the natural result of
> >> that: Simply supporting an in-repo top-level ".gitconfig" file. We're
> >> still going to seemingly forever have them be an even more special
> >> snowflake with this facility, and the reason seems to be mostly/entirely
> >> to do with working around some aspect or restriction of Google's
> >> internal infrastructure.
> >
> > I don't think that this is "natural". In particular, I still don't think
> > that hooks should be tied to code revision. E.g. if we make commits
> > based on an old revision and push them, we still want them to follow the
> > latest requirements.
> 
> Even for real-world centralized workflow situations where I've seen
> people think they want that, and the end of the day they almost never
> actually want that.
> 
> Even something like code linting is a good example, to make it
> Google-specific: say for a Go project: Are you going to pin your linting
> tool/version to whatever understood your YAML format for the linter as
> it was specced 10 years ago when the project started?  It's simply a
> giant hassle to have a piece of code operate on every version of your
> project ever in a way that doesn't break.
> 
> I think in practice the designers of this feature don't actually have
> that in mind, but a "close to trunk" workflow, where you'd expect a hook
> to only need to operate on revisions for the last few weeks or months,
> because that'll be the oldest think people create new topics from.

This is probably true, but it still means that people would want any
hook changes to work retroactively, even if it's just on code for the
last few months. So they still need some independence between the hook
and the main code.

> But I think the burden of proof is really on the other side here,
> something that works entirely differently than the rest of git needs to
> have a good reason. Our in-repo .gitattributes don't work like this, nor
> .gitignore, .mailmap etc.

Not all of Git works this way though - e.g. hooks themselves, and
config.

> There's also real world uses of git where the "branches" are wildly
> divergent, e.g. I've worked on a system automation repo where the
> "master" was just a stub template, and every team had their own almost
> entirely different "repo-like" branch. Probably a bad idea for various
> reasons, but Git supports it just fine.

I presume this repo either does not use hooks, or its hooks are built
for such a branch setup? I envision that the admins of the remote would
only suggest hooks that work with their branch setup.

> For the centralized use-case what's the problem with just having the
> hook do a 'for-each-ref --format=' invocation or "cat-file --batch" on
> the "origin", and eval what it finds there? I'd think that gives you
> what you want for the more centarlized workflow, while leaving git's
> implementation working like the rest of our in-repo files.

This would future-proof such a hook, but not make it work retroactively
in the past. (The repo admin could just include no-op hook for all hooks
to future-proof, I guess, but this wouldn't work if we ever introduced
new hooks.)

> >> I think it's just un-git-y to have a meta-branch that in some way drives
> >> not only all other branches, but all other revisions of all branches,
> >> ever.
> >>
> >> It breaks expectations around git in lots of different ways, you can't
> >> fetch a single branch and get its hooks,
> >
> > Are you saying that each branch should have its own hooks? That might be
> > reasonable in certain projects, but I don't see how that is a Git
> > expectation.
> 
> It's a git expectation now that I can add git.git as a remote, also
> chromium.git, and linux.git, fetch them all, and happily switch in the
> same repo between entirely different codebases that don't share a
> history.

This expectation holds only if you know that your hooks can tolerate
such a scenario. If you're using remote-suggested hooks, I don't think
it's a matter of "before, I could have many remotes but now I cannot"
but "before, I had to install these hooks myself and now it's automatic"
- whether this workflow is supported or not would be the same before and
now.

> >> I think I get why you want to do it that way, I just don't get why, as
> >> mostly noted in those earlier rounds why it wouldn't be a better
> >> approach / more straightforward / more git-y to:
> >>
> >> 1. Work on getting hooks driven by config <this is happening with
> >>    Emily's series / my split-out "base" topic>
> >> 2. Have a facility to read an in-repo '.gitconfig'; have lots of safety
> >>    valves etc. around this, I suggested starting with a whitelist of the
> >>    N least dangerous config options, e.g. some diff viewing options, or
> >>    a suggested sendemail.to or whatever.
> >
> > I've replied to this above.
> 
> Not really, even if we went for this one-HEAD-version-to-rule-them-all
> plan wouldn't it make more sense to generalize it as a
> refs/remotes/origin/magic-config, and we'd discover a ".gitconfig" file
> under that commit/tree.
> 
> I.e. whether we generalize this to config in general is orthagonal to
> whether such config lives in HEAD or in a magic ref.
> 
> With hooks as config I don't see how you'd make any of this
> hook-specific, there's other config where the "every revision ever"
> applies much more strongly, e.g. sendemail.to. If that changed for this
> project tomorrow you wouldn't want a patch based on "maint" to send
> things to a different ML.

My opposition to .gitconfig was that it is per-commit, but here it seems
that you're saying that there are reasons for it not to be per-commit
(e.g. the sendemail example).

> >> 4. People who want this "I want my hooks to apply to all revisions ever"
> >>    could probably get 99% or 100% of what they want if their hook is
> >>    just a stub that does the equivalent of:
> >>
> >>        sh `curl https://git.google.com/$reponame/hooks/$hookname`
> >>
> >>    You'd then simply forbid on your servers any changes to a .gitconfig
> >>    that did anything with the hook.* namespace.
> >
> > This would work if set in .git/config (not version controlled), but not
> > .gitconfig (version controlled).
> 
> Sorry, what wouldn't work? I meant you'd forbid pushes to your in-repo
> .gitconfig in your "master" branch or whatever, just like you're
> presumably planning some stronger ACLs for this magic hook branch.

The "I want my hooks to apply to all revisions ever" wouldn't work if it
was per-commit .gitconfig. (It would work if it was in the magic
branch, but at that time, what I understood by ".gitconfig" is the
per-commit version.)

> >> With such an implementation you don't need a magic
> >> "refs/remotes/origin/suggested-hooks" refs, just some state machine (I
> >> suggested e.g. GPG signing chains as an eventual end-state, but "show a
> >> diff every time" would also do) that keeps track of what config (and
> >> hooks are just one such case) has been OK'd, and which has not.
> >
> > This sounds complicated.
> 
> On the contrary I think anything that leans into git's
> content-addressable security model is way less complicated. You don't
> care who you fetched Junio's v2.32.0 tag from, what matters is that the
> signing chain validates.
> 
> The plan of having this magic branch means a whole new trust model for
> git, you trust magical authorized remotes. If you trust signed content
> chains you can trust hooks if their last modification can be traced to a
> signing authority you trust.

The security model might be preexisting, but the things needing to be
built around it make it more complicated. Take this case - I presume
this means that the client would need to tell Git that a certain public
key is considered trusted (perhaps prompted by the server upon clone, so
the client would only need to copy and paste "git trust-key $HTTPS_URL"
or something like that). Whenever a user wants to update a hook, they
will make a commit, and when the PR is merged, the merge commit (or the
tip commit if fast-forwarded) must be signed with the same key.

Compare this with trusting that commits coming from a certain HTTPS URL
are fine. The workflows are the same as for a regular code commit.

>     It's really just:
> 
>         if (hook_content_changed() && hook_content_same_as_in_ok'd_revision_from_upsteam())
>             trust_hooks();
> 
> But while we're on the subject, it seems like a very generous assumption
> to think that just because you trust hooks at a given revision (or
> always trust the latest), that you implicitly trust them when *combined
> with* all past and future revisions from the same repository.
> 
> Even without a malicious actor that seems like it'll inevitably break in
> all sorts of data-destroying ways. E.g. people commit stuff
> accidentally. A hook run under a "git bisect" that navely does an "rm
> *" will eat your data if you land on a revision that an in-tree "-rf"
> file.

I think we have the same problem with current hooks and other config,
and we have been dealing with them relatively well (as far as I know).

> But once you get to a malicious actor who can say push a topic branch
> but not hook updates, will your hooks deal with files with whitespace in
> them, arbitrary crafted content etc?
> 
> So I'd think that's an even better reason to prefer the in-repo
> per-revision atomically committed plan, and only trust hooks for the
> revision they're shipped with, at least as a default git security model.

The malicious actor could include the same hook in their topic branch,
so I don't see how the per-revision hook is better than the all-revision
hook.

> >> I'd think it would even work better in the Googleplex, you could clone a
> >> co-worker's branch and execute their hooks, since they're the same as
> >> what you've pre-approved,
> >
> > In the presence of .gitconfig, how would you know?
> 
> If it's the same config, or you can automatically OK it. So "same" was
> discussed above, or you could trust any hook that's only doing a wget of
> some trusted domain and piping that to "sh".

Extending trust to hooks that are exactly the same makes sense.

> You trust e.g. chromium.git's hooks, but I clone it, patch it, and
> re-push it to somegithost.com URL. If you go with trusting content it
> becomes easy to install those trusted hooks for the common case, but not
> if your entire trust model relies on what URL you git clone'd from.

If the repo administrator provides a key and signs their hooks, then the
repo itself is indeed more portable and does not need to come from a
single source. This may be useful, but I'm not sure how useful this is,
since hooks are typically provided by projects. And as far as I know
projects generally have one source of truth, so the portability is not a
big plus.

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 4094 bytes --]

On 2021-06-20 at 19:51:04, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jun 16 2021, Jonathan Tan wrote:
> 
> > This but does not use any features from es/config-based-hooks but is
> > implemented on that branch anyway because firstly, I need an existing
> > command to attach the "autoupdate" subcommand (and "git hook" works),
> > and secondly, when we test this at $DAYJOB, we will be testing it
> > together with the aforementioned branch.
> >
> > I have had to make several design choices (which I will discuss later),
> > but now with this implementation, the following workflow is possible:
> >
> >  1. The remote repo administrator creates a new branch
> >     "refs/heads/suggested-hooks" pointing to a commit that has all the
> >     hooks that the administrator wants to suggest. The hooks are
> >     directly referenced by the commit tree (i.e. they are in the "/"
> >     directory).
> >
> >  2. When a user clones, Git notices that
> >     "refs/remotes/origin/suggested-hooks" is present and prints out a
> >     message about a command that can be run.
> >
> >  3. If the user runs that command, Git will install the hooks pointed to
> >     by that ref, and set hook.autoupdate to true. This config variable
> >     is checked whenever "git fetch" is run: whenever it notices that
> >     "refs/remotes/origin/suggested-hooks" changes, it will reinstall the
> >     hooks.
> >
> >  4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks
> >     will remain.
> >
> > Design choices:
> >
> >  1. Where should the suggested hooks be in the remote repo? A branch,
> >     a non-branch ref, a config? I think that a branch is best - it is
> >     relatively well-understood and any hooks there can be
> >     version-controlled (and its history is independent of the other
> >     branches).
> 
> First, unlike brian I don't (I hope I'm fairly summarizing his view
> here) disagree mostly or entirely with the existence of such a feature
> at all. I mean, I get the viewpoint that git shouldn't bless what
> amounts to an active RCE from the remote.

It's accurate that I'm generally opposed to such a feature.  I feel that
suggesting people install hooks is likely to lead to social engineering
attacks, and it's also likely to lead to bad practices such as the
expectation that all developers will install hooks or the use of hooks
instead of CI or other effective controls.

If we do add this feature (which, as I said, I'm opposed to) and we
decide to store it in a ref, that ref should not be a normal branch by
default (it should be a special one-level ref, like refs/stash or such),
and the ref name should be configurable.  Not all developers use English
as their working language and we should respect that.

In addition, there should be an advice.* option that allows people to
turn this off once and for all, and it should be clearly documented.
Ideally it should be off by default.

> I think I get why you want to do it that way, I just don't get why, as
> mostly noted in those earlier rounds why it wouldn't be a better
> approach / more straightforward / more git-y to:
> 
> 1. Work on getting hooks driven by config <this is happening with
>    Emily's series / my split-out "base" topic>
> 2. Have a facility to read an in-repo '.gitconfig'; have lots of safety
>    valves etc. around this, I suggested starting with a whitelist of the
>    N least dangerous config options, e.g. some diff viewing options, or
>    a suggested sendemail.to or whatever.

This also makes me deeply nervous for much of the same reasons.  There
are situations where e.g. ignoring whitespace can lead to security
problems in code review (think Python), and in general it's hard to
reason about all the ways people can do malicious things.  Typically
adding untrusted config ends poorly (think of all the modeline
vulnerabilities in Vim).

I'd definitely want support for this to be off with no prompting by
default.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Jonathan Tan]

> > First, unlike brian I don't (I hope I'm fairly summarizing his view
> > here) disagree mostly or entirely with the existence of such a feature
> > at all. I mean, I get the viewpoint that git shouldn't bless what
> > amounts to an active RCE from the remote.
> 
> It's accurate that I'm generally opposed to such a feature.

From emails like [1], I think that you have understood both the pros and
cons, and decided that the cons outweigh the pros, which is fair. I'll
reply so that others can know what I think about these points.

[1] https://lore.kernel.org/git/YGzrfaSC4xd75j2U@camp.crustytoothpaste.net/

> I feel that
> suggesting people install hooks is likely to lead to social engineering
> attacks,

I think that this vector of social engineering attack already exists, as
there are projects that without malice recommend hook installation (so
an attacker could masquerade as such). It is true that Git itself
printing a message recommending hook installation perhaps could lend a
false sense of security, but at least we can control what that message
says (as opposed to a project recommending a third-party tool, all with
messaging out of our control).

> and it's also likely to lead to bad practices such as the
> expectation that all developers will install hooks or the use of hooks
> instead of CI or other effective controls.

I agree that hooks can be overused or misused, but there are still
legitimate uses of it that a project might want to use.

> If we do add this feature (which, as I said, I'm opposed to) and we
> decide to store it in a ref, that ref should not be a normal branch by
> default (it should be a special one-level ref, like refs/stash or such),

Any particular reason not to expose it as a branch (besides following
from your general idea that a user should seek out such a feature and
not have it presented to them up-front)?

> and the ref name should be configurable.  Not all developers use English
> as their working language and we should respect that.

That makes sense.

> In addition, there should be an advice.* option that allows people to
> turn this off once and for all, and it should be clearly documented.
> Ideally it should be off by default.

I don't think this would be considered "advice" like the other options,
but having an option to turn this off once and for all makes sense.
Making it off by default would probably mean that projects that use such
hooks would recommend cloning with "git -c my-config=1 clone $URL", but
perhaps that's OK.

> > I think I get why you want to do it that way, I just don't get why, as
> > mostly noted in those earlier rounds why it wouldn't be a better
> > approach / more straightforward / more git-y to:
> > 
> > 1. Work on getting hooks driven by config <this is happening with
> >    Emily's series / my split-out "base" topic>
> > 2. Have a facility to read an in-repo '.gitconfig'; have lots of safety
> >    valves etc. around this, I suggested starting with a whitelist of the
> >    N least dangerous config options, e.g. some diff viewing options, or
> >    a suggested sendemail.to or whatever.
> 
> This also makes me deeply nervous for much of the same reasons.  There
> are situations where e.g. ignoring whitespace can lead to security
> problems in code review (think Python), and in general it's hard to
> reason about all the ways people can do malicious things.  Typically
> adding untrusted config ends poorly (think of all the modeline
> vulnerabilities in Vim).
> 
> I'd definitely want support for this to be off with no prompting by
> default.

To use your example, the model we're proposing is more of only using the
modelines from sources we trust - as opposed to ensuring that all
possible options set by modelines are benign. Admittedly, the
administrator of the source may have difficulty ensuring that bad code
doesn't slip through code review, for example, but that is a problem
they already deal with (at least for projects with any form of
executable code in them, e.g. production code or a build script).

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3263 bytes --]

On 2021-06-23 at 22:58:09, Jonathan Tan wrote:
> > If we do add this feature (which, as I said, I'm opposed to) and we
> > decide to store it in a ref, that ref should not be a normal branch by
> > default (it should be a special one-level ref, like refs/stash or such),
> 
> Any particular reason not to expose it as a branch (besides following
> from your general idea that a user should seek out such a feature and
> not have it presented to them up-front)?

Branches are for the main code of the project.  While it's possible to
have orphan branches that do other things, I think that's in general an
anti-pattern, and using a special ref for things which are separate and
independent from the main code of the repository would be a more elegant
solution.

> > In addition, there should be an advice.* option that allows people to
> > turn this off once and for all, and it should be clearly documented.
> > Ideally it should be off by default.
> 
> I don't think this would be considered "advice" like the other options,
> but having an option to turn this off once and for all makes sense.
> Making it off by default would probably mean that projects that use such
> hooks would recommend cloning with "git -c my-config=1 clone $URL", but
> perhaps that's OK.

Sure, I'm not picky about what it looks like in "advice" vs something
else.  I think forcing projects to explicitly opt-in to this behavior
means that the social engineering and security problems are much
reduced, and while I'm still not wild about the idea, I would feel much
better about it.

> > This also makes me deeply nervous for much of the same reasons.  There
> > are situations where e.g. ignoring whitespace can lead to security
> > problems in code review (think Python), and in general it's hard to
> > reason about all the ways people can do malicious things.  Typically
> > adding untrusted config ends poorly (think of all the modeline
> > vulnerabilities in Vim).
> > 
> > I'd definitely want support for this to be off with no prompting by
> > default.
> 
> To use your example, the model we're proposing is more of only using the
> modelines from sources we trust - as opposed to ensuring that all
> possible options set by modelines are benign. Admittedly, the
> administrator of the source may have difficulty ensuring that bad code
> doesn't slip through code review, for example, but that is a problem
> they already deal with (at least for projects with any form of
> executable code in them, e.g. production code or a build script).

As I think I've previously mentioned, I don't want to receive
configuration of my development environment from sources I trust.  Even
at work, I don't want the repositories I work with to modify my
development environment in this way.  I tend to have a highly customized
configuration that breaks many people's expectations about tooling, so
the cases that this isn't a security problem (in repositories I trust)
can still result in a functionality problem.

Also, since we don't know what repositories the user trusts, the only
safe assumption is that the user trusts nothing unless they explicitly
tell us.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> If we do add this feature (which, as I said, I'm opposed to) and we
> decide to store it in a ref, that ref should not be a normal branch by
> default (it should be a special one-level ref, like refs/stash or such),
> and the ref name should be configurable.  Not all developers use English
> as their working language and we should respect that.

Just this part.

I am not sure what you are trying to achieve by requiring it to be
configurable.  After all, even for names of branches that are used
to store the code, which is of more significance than this "unusual"
ref, we've lived with a hardcoded 'master' and with the server-end
advertisability of the configured values (i.e. "git clone" was
designed to figure out if the origin used a custom name for the
primary line of history and use the same name from the beginning),
the end-user sitting on the receiving end did not have to do
anything special even when the project wanted to use a custom name.

But this "unusual" ref would not have a natural equivalent of "the
origin side can point the primary branch with its HEAD", so by
allowing it to be configurable, you are pretty much closing the door
for those who blindly honor whatever the origin tells them to use
when running "git clone" from doing so.  I agree that it is a good
security measure (and I am not sold to the "remote suggested hooks"
idea in the first place), but is that the real reason why you
suggested configurability?

[RFC PATCH v2 0/2] MVP implementation of remote-suggested hooks

[Jonathan Tan]

Here's an updated version of the remote-suggested hooks RFC, hopefully
addressing some of the concerns people have brought up (e.g.
auto-updating without letting the user verify, or prompting about hooks
by default). It consists of two main parts:

 - Non-interactive prompts during certain Git commands about the
   existence of hooks. These prompts are turned *off* by default.

 - New "git hook" subcommands that can install these hooks (so that the
   aforementioned Git prompts or out-of-band installation instructions
   can tell users to install these hooks in a platform-independent way).
   These subcommands work whether or not prompts are enabled.

You can see how they work in patch 2's t1361.

In doing this, I have tried to scope down the overall effort as much as
possible (to avoid security risks as much as possible, among other
things), concentrating on situations that server-side hooks cannot
handle. I think the main thing is that server-side hooks can check and
prompt upon push, but they cannot catch problems that the client could
have noticed upon commit; if the end user pushes a commit chain and only
notices problems then, the user would have to redo the commits (possibly
through an interactive rebase on the spot). Catching problems upon
commit would prevent this problem, only necessitating amending the
commit.

In the Gerrit use case, Gerrit requires a specific "Change-Id" trailer
in the commit message, but I can foresee the same issue in projects that
require their commit messages to fit a certain template or projects that
want lints to be run but their builds, for some reason, don't run
arbitrary code (or they have no builds at all).

To that end, I have added a prompt for the "commit-msg" hook if the
remote has suggested one, but there are none currently installed.
(Similar prompts could be added for other commit-related hooks.) The
other prompts are upon clone and upon fetch (if the hooks have been
updated).

Jonathan Tan (2):
  hook: move list of hooks
  hook: remote-suggested hooks

 builtin/bugreport.c               |  38 +-----
 builtin/clone.c                   |  12 ++
 builtin/fetch.c                   |  17 +++
 builtin/hook.c                    |  17 ++-
 hook.c                            | 185 +++++++++++++++++++++++++++++-
 hook.h                            |   6 +
 t/t1361-remote-suggested-hooks.sh | 105 +++++++++++++++++
 7 files changed, 340 insertions(+), 40 deletions(-)
 create mode 100755 t/t1361-remote-suggested-hooks.sh

-- 
2.32.0.402.g57bb445576-goog

[RFC PATCH v2 1/2] hook: move list of hooks

[Jonathan Tan]

The list of hooks will need to be used outside bugreport, so move it to
a central location.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/bugreport.c | 38 +++-----------------------------------
 hook.c              | 34 ++++++++++++++++++++++++++++++++++
 hook.h              |  3 +++
 3 files changed, 40 insertions(+), 35 deletions(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 190272ba70..4e0806dff3 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -41,38 +41,6 @@ static void get_system_info(struct strbuf *sys_info)
 
 static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 {
-	/*
-	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
-	 * so below is a transcription of `git help hooks`. Later, this should
-	 * be replaced with some programmatically generated list (generated from
-	 * doc or else taken from some library which tells us about all the
-	 * hooks)
-	 */
-	static const char *hook[] = {
-		"applypatch-msg",
-		"pre-applypatch",
-		"post-applypatch",
-		"pre-commit",
-		"pre-merge-commit",
-		"prepare-commit-msg",
-		"commit-msg",
-		"post-commit",
-		"pre-rebase",
-		"post-checkout",
-		"post-merge",
-		"pre-push",
-		"pre-receive",
-		"update",
-		"post-receive",
-		"post-update",
-		"push-to-checkout",
-		"pre-auto-gc",
-		"post-rewrite",
-		"sendemail-validate",
-		"fsmonitor-watchman",
-		"p4-pre-submit",
-		"post-index-change",
-	};
 	int i;
 
 	if (nongit) {
@@ -81,9 +49,9 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 		return;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(hook); i++)
-		if (hook_exists(hook[i], HOOKDIR_USE_CONFIG))
-			strbuf_addf(hook_info, "%s\n", hook[i]);
+	for (i = 0; i < hook_name_count; i++)
+		if (hook_exists(hook_name[i], HOOKDIR_USE_CONFIG))
+			strbuf_addf(hook_info, "%s\n", hook_name[i]);
 }
 
 static const char * const bugreport_usage[] = {
diff --git a/hook.c b/hook.c
index ff80e52edd..3ccacb72fa 100644
--- a/hook.c
+++ b/hook.c
@@ -5,6 +5,40 @@
 #include "run-command.h"
 #include "prompt.h"
 
+/*
+ * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
+ * so below is a transcription of `git help hooks`. Later, this should
+ * be replaced with some programmatically generated list (generated from
+ * doc or else taken from some library which tells us about all the
+ * hooks)
+ */
+const char *hook_name[] = {
+	"applypatch-msg",
+	"pre-applypatch",
+	"post-applypatch",
+	"pre-commit",
+	"pre-merge-commit",
+	"prepare-commit-msg",
+	"commit-msg",
+	"post-commit",
+	"pre-rebase",
+	"post-checkout",
+	"post-merge",
+	"pre-push",
+	"pre-receive",
+	"update",
+	"post-receive",
+	"post-update",
+	"push-to-checkout",
+	"pre-auto-gc",
+	"post-rewrite",
+	"sendemail-validate",
+	"fsmonitor-watchman",
+	"p4-pre-submit",
+	"post-index-change",
+};
+int hook_name_count = ARRAY_SIZE(hook_name);
+
 void free_hook(struct hook *ptr)
 {
 	if (ptr) {
diff --git a/hook.h b/hook.h
index f32189380a..d902166408 100644
--- a/hook.h
+++ b/hook.h
@@ -4,6 +4,9 @@
 #include "strvec.h"
 #include "run-command.h"
 
+extern const char *hook_name[];
+extern int hook_name_count;
+
 struct hook {
 	struct list_head list;
 	/*
-- 
2.32.0.402.g57bb445576-goog

[RFC PATCH v2 2/2] hook: remote-suggested hooks

[Jonathan Tan]

Teach the "git hook install all|<hook-name>" command, that can install
one or all remote-suggested hooks.

If a configuration option hook.promptRemoteSuggested is set, inform the
user of the aforementioned command:

 - when cloning, and refs/remotes/origin/suggested-hooks is present in
   the newly cloned repo
 - when fetching, and refs/remotes/origin/suggested-hooks is updated
 - when committing, there is a remote-suggested commit-msg hook, and
   there is currently no commit-msg hook configured

NEEDSWORK: Write a more detailed commit message once the design is
finalized.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/clone.c                   |  12 +++
 builtin/fetch.c                   |  17 ++++
 builtin/hook.c                    |  17 +++-
 hook.c                            | 151 +++++++++++++++++++++++++++++-
 hook.h                            |   3 +
 t/t1361-remote-suggested-hooks.sh | 105 +++++++++++++++++++++
 6 files changed, 300 insertions(+), 5 deletions(-)
 create mode 100755 t/t1361-remote-suggested-hooks.sh

diff --git a/builtin/clone.c b/builtin/clone.c
index 2a2a03bf76..c2c8596aa9 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1393,6 +1393,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			   branch_top.buf, reflog_msg.buf, transport,
 			   !is_local);
 
+	if (hook_should_prompt_suggestions()) {
+		for (ref = mapped_refs; ref; ref = ref->next) {
+			if (ref->peer_ref &&
+			    !strcmp(ref->peer_ref->name,
+				    "refs/remotes/origin/suggested-hooks")) {
+				fprintf(stderr, _("The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\n"
+						  "Run `git hook install all` to install them.\n"));
+				break;
+			}
+		}
+	}
+
 	update_head(our_head_points_at, remote_head, reflog_msg.buf);
 
 	/*
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 769af53ca4..e86c312473 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -28,6 +28,7 @@
 #include "promisor-remote.h"
 #include "commit-graph.h"
 #include "shallow.h"
+#include "hook.h"
 
 #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
 
@@ -1313,6 +1314,22 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)
 				 ref_map);
 	transport_unlock_pack(transport);
 	trace2_region_leave("fetch", "consume_refs", the_repository);
+
+	if (hook_should_prompt_suggestions()) {
+		struct ref *ref;
+
+		for (ref = ref_map; ref; ref = ref->next) {
+			if (ref->peer_ref &&
+			    !strcmp(ref->peer_ref->name,
+				    "refs/remotes/origin/suggested-hooks") &&
+			    oidcmp(&ref->old_oid, &ref->peer_ref->old_oid)) {
+				fprintf(stderr, _("The remote has updated its suggested hooks.\n"));
+				fprintf(stderr, _("Run 'git hook install all' to update.\n"));
+				break;
+			}
+		}
+	}
+
 	return ret;
 }
 
diff --git a/builtin/hook.c b/builtin/hook.c
index c79a961e80..0334fee967 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -139,6 +139,17 @@ static int run(int argc, const char **argv, const char *prefix)
 	return rc;
 }
 
+static int install(int argc, const char **argv, const char *prefix)
+{
+	if (argc < 1)
+		die(_("You must specify a hook event to install."));
+	if (!strcmp(argv[1], "all"))
+		hook_update_suggested(NULL);
+	else
+		hook_update_suggested(argv[1]);
+	return 0;
+}
+
 int cmd_hook(int argc, const char **argv, const char *prefix)
 {
 	const char *run_hookdir = NULL;
@@ -152,10 +163,6 @@ int cmd_hook(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, builtin_hook_options,
 			     builtin_hook_usage, PARSE_OPT_KEEP_UNKNOWN);
 
-	/* after the parse, we should have "<command> <hookname> <args...>" */
-	if (argc < 2)
-		usage_with_options(builtin_hook_usage, builtin_hook_options);
-
 	git_config(git_default_config, NULL);
 
 
@@ -185,6 +192,8 @@ int cmd_hook(int argc, const char **argv, const char *prefix)
 		return list(argc, argv, prefix);
 	if (!strcmp(argv[0], "run"))
 		return run(argc, argv, prefix);
+	if (!strcmp(argv[0], "install"))
+		return install(argc, argv, prefix);
 
 	usage_with_options(builtin_hook_usage, builtin_hook_options);
 }
diff --git a/hook.c b/hook.c
index 3ccacb72fa..32eee9abb6 100644
--- a/hook.c
+++ b/hook.c
@@ -4,6 +4,12 @@
 #include "config.h"
 #include "run-command.h"
 #include "prompt.h"
+#include "commit.h"
+#include "object.h"
+#include "refs.h"
+#include "tree-walk.h"
+#include "tree.h"
+#include "streaming.h"
 
 /*
  * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
@@ -476,6 +482,60 @@ static int notify_hook_finished(int result,
 	return 0;
 }
 
+static struct tree *remote_suggested_hook_tree(int *warning_printed)
+{
+	struct object_id oid;
+	struct object *obj;
+	struct tree *tree;
+
+	if (read_ref("refs/remotes/origin/suggested-hooks", &oid))
+		return NULL;
+
+	obj = parse_object(the_repository, &oid);
+	if (obj == NULL) {
+		warning(_("object pointed to by refs/remotes/origin/suggested-hooks '%s' does not exist"),
+			oid_to_hex(&oid));
+		if (warning_printed)
+			*warning_printed = 1;
+		return NULL;
+	}
+	if (obj->type != OBJ_COMMIT) {
+		warning(_("object pointed to by refs/remotes/origin/suggested-hooks '%s' is not a commit"),
+			oid_to_hex(&oid));
+		if (warning_printed)
+			*warning_printed = 1;
+		return NULL;
+	}
+
+	tree = get_commit_tree((struct commit *) obj);
+	if (parse_tree(tree)) {
+		warning(_("could not parse tree"));
+		if (warning_printed)
+			*warning_printed = 1;
+		return NULL;
+	}
+
+	return tree;
+}
+
+static int has_suggested_hook(const char *hookname)
+{
+	struct tree *tree;
+	struct tree_desc desc;
+	struct name_entry entry;
+
+	tree = remote_suggested_hook_tree(NULL);
+	if (!tree)
+		return 0;
+
+	init_tree_desc(&desc, tree->buffer, tree->size);
+	while (tree_entry(&desc, &entry)) {
+		if (!strcmp(hookname, entry.path))
+			return 1;
+	}
+	return 0;
+}
+
 int run_hooks(const char *hookname, struct run_hooks_opt *options)
 {
 	struct list_head *to_run, *pos = NULL, *tmp = NULL;
@@ -497,8 +557,16 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options)
 			    list_del(pos);
 	}
 
-	if (list_empty(to_run))
+	if (list_empty(to_run)) {
+		if (!strcmp("commit-msg", hookname)) {
+			if (has_suggested_hook(hookname) &&
+			    !hook_exists(hookname, HOOKDIR_USE_CONFIG)) {
+				fprintf(stderr, _("No commit-msg hook has been configured, but one is suggested by the remote.\n"));
+				fprintf(stderr, _("Run 'git hook install commit-msg' to install it.\n"));
+			}
+		}
 		return 0;
+	}
 
 	cb_data.head = to_run;
 	cb_data.run_me = list_entry(to_run->next, struct hook, list);
@@ -515,3 +583,84 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options)
 
 	return cb_data.rc;
 }
+
+static int is_hook(const char *filename)
+{
+	int i;
+
+	for (i = 0; i < hook_name_count; i++) {
+		if (!strcmp(filename, hook_name[i]))
+			return 1;
+	}
+	return 0;
+}
+
+void hook_update_suggested(const char *hook_to_update)
+{
+	struct tree *tree;
+	int warning_printed = 0;
+	struct tree_desc desc;
+	struct name_entry entry;
+	struct strbuf path = STRBUF_INIT;
+	int hook_found = 0;
+
+	tree = remote_suggested_hook_tree(&warning_printed);
+	if (!tree) {
+		if (!warning_printed)
+			warning(_("no such ref refs/remotes/origin/suggested-hooks, not updating hooks"));
+		return;
+	}
+
+	init_tree_desc(&desc, tree->buffer, tree->size);
+	while (tree_entry(&desc, &entry)) {
+		int fd;
+
+		if (hook_to_update && strcmp(hook_to_update, entry.path))
+			/*
+			 * We only need to update one hook, and this is not the
+			 * hook we're looking for
+			 */
+			continue;
+
+		if (!hook_to_update && !is_hook(entry.path)) {
+			warning(_("file '%s' is not a hook; ignoring"),
+				entry.path);
+			continue;
+		}
+		hook_found = 1;
+		if (S_ISDIR(entry.mode) || S_ISGITLINK(entry.mode)) {
+			warning(_("file '%s' is not an ordinary file; ignoring"),
+				entry.path);
+			continue;
+		}
+
+		strbuf_reset(&path);
+		strbuf_git_path(&path, "hooks/%s", entry.path);
+		fd = open(path.buf, O_WRONLY | O_CREAT, 0755);
+		if (fd < 0) {
+			warning_errno(_("could not create file '%s'; skipping this hook"),
+				      path.buf);
+			continue;
+		}
+		if (stream_blob_to_fd(fd, &entry.oid, NULL, 1)) {
+			warning(_("could not write to file '%s'; skipping this hook"),
+				path.buf);
+			continue;
+		}
+		close(fd);
+	}
+	strbuf_release(&path);
+
+	if (hook_to_update && !hook_found)
+		warning(_("hook '%s' not found"), hook_to_update);
+
+	return;
+}
+
+int hook_should_prompt_suggestions(void)
+{
+	int dest = 0;
+
+	return !git_config_get_bool("hook.promptremotesuggested", &dest) &&
+	       dest;
+}
diff --git a/hook.h b/hook.h
index d902166408..438bf7122e 100644
--- a/hook.h
+++ b/hook.h
@@ -140,3 +140,6 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options);
 void free_hook(struct hook *ptr);
 /* Empties the list at 'head', calling 'free_hook()' on each entry */
 void clear_hook_list(struct list_head *head);
+
+void hook_update_suggested(const char *hook_to_update);
+int hook_should_prompt_suggestions(void);
diff --git a/t/t1361-remote-suggested-hooks.sh b/t/t1361-remote-suggested-hooks.sh
new file mode 100755
index 0000000000..223c65ac99
--- /dev/null
+++ b/t/t1361-remote-suggested-hooks.sh
@@ -0,0 +1,105 @@
+#!/bin/sh
+
+test_description='remote-suggested hooks'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+setup_server () {
+	git init server &&
+	test_when_finished "rm -rf server" &&
+	cat >server/commit-msg <<-'EOF' &&
+	echo "overwrite the commit message" >$1
+EOF
+	git -C server add commit-msg &&
+	test_commit -C server commit-with-hook &&
+	git -C server checkout -b suggested-hooks
+}
+
+add_hook_to_server () {
+	>server/pre-commit &&
+	git -C server add pre-commit &&
+	test_commit -C server additional-hook
+}
+
+test_expect_success 'suggestion upon clone' '
+	setup_server &&
+	test_when_finished "rm -rf client" &&
+	git -c hook.promptRemoteSuggested=1 clone server client 2>err &&
+	grep "The remote has suggested hooks" err
+'
+
+test_expect_success 'no suggestion upon clone if not configured' '
+	setup_server &&
+	test_when_finished "rm -rf client" &&
+	git clone server client 2>err &&
+	! grep "The remote has suggested hooks" err
+'
+
+test_expect_success 'suggestion upon fetch if server has updated hooks' '
+	setup_server &&
+	git clone server client &&
+	test_when_finished "rm -rf client" &&
+	add_hook_to_server &&
+
+	git -C client -c hook.promptRemoteSuggested=1 fetch 2>err &&
+	grep "The remote has updated its suggested hooks" err
+'
+
+test_expect_success 'no suggestion upon fetch if not configured' '
+	setup_server &&
+	git clone server client &&
+	test_when_finished "rm -rf client" &&
+	add_hook_to_server &&
+
+	git -C client fetch 2>err &&
+	! grep "The remote has updated its suggested hooks" err
+'
+
+test_expect_success 'no suggestion upon fetch if server has not updated hooks' '
+	setup_server &&
+	git clone server client &&
+	test_when_finished "rm -rf client" &&
+	git -C server checkout main &&
+	test_commit -C server not-a-hook-update &&
+
+	git -C client -c hook.promptRemoteSuggested=1 fetch 2>err &&
+	! grep "The remote has updated its suggested hooks" err
+'
+
+test_expect_success 'suggest commit-msg hook upon commit' '
+	setup_server &&
+	git clone server client &&
+	test_when_finished "rm -rf client" &&
+
+	test_commit -C client foo 2>err &&
+	grep "Run .git hook install commit-msg" err
+'
+
+test_expect_success 'install one suggested hook' '
+	setup_server &&
+	git clone server client &&
+	test_when_finished "rm -rf client" &&
+
+	git -C client hook install commit-msg &&
+
+	# Check that the hook was written by making a commit
+	test_commit -C client bar &&
+	git -C client show >commit &&
+	grep "overwrite the commit message" commit
+'
+
+test_expect_success 'install all suggested hooks' '
+	setup_server &&
+	add_hook_to_server &&
+	git clone server client &&
+	test_when_finished "rm -rf client" &&
+
+	git -C client hook install all &&
+	test -f client/.git/hooks/pre-commit &&
+	test -f client/.git/hooks/commit-msg
+'
+
+test_done
-- 
2.32.0.402.g57bb445576-goog

Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Teach the "git hook install all|<hook-name>" command, that can install
> one or all remote-suggested hooks.
>
> If a configuration option hook.promptRemoteSuggested is set, inform the
> user of the aforementioned command:
>
>  - when cloning, and refs/remotes/origin/suggested-hooks is present in
>    the newly cloned repo
>  - when fetching, and refs/remotes/origin/suggested-hooks is updated
>  - when committing, there is a remote-suggested commit-msg hook, and
>    there is currently no commit-msg hook configured
>
> NEEDSWORK: Write a more detailed commit message once the design is
> finalized.

OK.  Let's take this more as WIP.

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 2a2a03bf76..c2c8596aa9 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1393,6 +1393,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  			   branch_top.buf, reflog_msg.buf, transport,
>  			   !is_local);
>  
> +	if (hook_should_prompt_suggestions()) {
> +		for (ref = mapped_refs; ref; ref = ref->next) {
> +			if (ref->peer_ref &&
> +			    !strcmp(ref->peer_ref->name,
> +				    "refs/remotes/origin/suggested-hooks")) {
> +				fprintf(stderr, _("The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\n"
> +						  "Run `git hook install all` to install them.\n"));
> +				break;
> +			}
> +		}

Hmph, do we really need to iterate over these mapped refs array?  It
seems to me that it would be vastly simpler to check if that single
ref exists after "clone" finishes depositing all the refs we are
supposed to create.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 769af53ca4..e86c312473 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -28,6 +28,7 @@
>  #include "promisor-remote.h"
>  #include "commit-graph.h"
>  #include "shallow.h"
> +#include "hook.h"
>  
>  #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
>  
> @@ -1313,6 +1314,22 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)
>  				 ref_map);
>  	transport_unlock_pack(transport);
>  	trace2_region_leave("fetch", "consume_refs", the_repository);
> +
> +	if (hook_should_prompt_suggestions()) {
> +		struct ref *ref;
> +
> +		for (ref = ref_map; ref; ref = ref->next) {
> +			if (ref->peer_ref &&
> +			    !strcmp(ref->peer_ref->name,
> +				    "refs/remotes/origin/suggested-hooks") &&
> +			    oidcmp(&ref->old_oid, &ref->peer_ref->old_oid)) {
> +				fprintf(stderr, _("The remote has updated its suggested hooks.\n"));
> +				fprintf(stderr, _("Run 'git hook install all' to update.\n"));
> +				break;

Again we _could_ do "remember if we had it and at where, and then
compare after we fetch", but this _might_ be simpler.

I however do not know if it makes sense to have a separate loop just
for this.  This should be done as a part of store_updated_refs(),
no?

On top of this patch, if we wanted to add yet another "this ref is
special and pay attention to it when it got updated", it makes a lot
more sense not to add yet another loop that is a copy of this one in
consume_refs(), but roll the handling of that new ref into a loop
that already exists.  And for the purpose of such an update, I do
not see why that "loop that already exists" should not be the one
that goes over ref_map in store_updated_refs().  For the same
reason, "the remote-tracking ref 'origin/suggested-hooks' is special"
can and should go to an existing loop in store_updated_refs(), no?

How does this interact with the "--dry-run" option, by the way?
What if ref_map proposes to update this suggested-hooks ref, but
"--atomic" fetch feature finds that some other branches do not
fast-forward and we end up not updating the suggested-hooks ref,
either?

So, unlike my earlier guess, it _might_ turn out that "remember the
state before the fetch, see if the fetch actually updated and then
report" could be simpler to design and implement.  I dunno.

Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks

[Jonathan Tan]

> OK.  Let's take this more as WIP.

Sounds good.

> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index 2a2a03bf76..c2c8596aa9 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -1393,6 +1393,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >  			   branch_top.buf, reflog_msg.buf, transport,
> >  			   !is_local);
> >  
> > +	if (hook_should_prompt_suggestions()) {
> > +		for (ref = mapped_refs; ref; ref = ref->next) {
> > +			if (ref->peer_ref &&
> > +			    !strcmp(ref->peer_ref->name,
> > +				    "refs/remotes/origin/suggested-hooks")) {
> > +				fprintf(stderr, _("The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\n"
> > +						  "Run `git hook install all` to install them.\n"));
> > +				break;
> > +			}
> > +		}
> 
> Hmph, do we really need to iterate over these mapped refs array?  It
> seems to me that it would be vastly simpler to check if that single
> ref exists after "clone" finishes depositing all the refs we are
> supposed to create.

Good point. I'll do that.

> > @@ -1313,6 +1314,22 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)
> >  				 ref_map);
> >  	transport_unlock_pack(transport);
> >  	trace2_region_leave("fetch", "consume_refs", the_repository);
> > +
> > +	if (hook_should_prompt_suggestions()) {
> > +		struct ref *ref;
> > +
> > +		for (ref = ref_map; ref; ref = ref->next) {
> > +			if (ref->peer_ref &&
> > +			    !strcmp(ref->peer_ref->name,
> > +				    "refs/remotes/origin/suggested-hooks") &&
> > +			    oidcmp(&ref->old_oid, &ref->peer_ref->old_oid)) {
> > +				fprintf(stderr, _("The remote has updated its suggested hooks.\n"));
> > +				fprintf(stderr, _("Run 'git hook install all' to update.\n"));
> > +				break;
> 
> Again we _could_ do "remember if we had it and at where, and then
> compare after we fetch", but this _might_ be simpler.
> 
> I however do not know if it makes sense to have a separate loop just
> for this.  This should be done as a part of store_updated_refs(),
> no?

I'm OK with it in either place, but it seems to me that this is separate
from storing refs - the suggestion of hooks does not influence how the
refs are stored.

> On top of this patch, if we wanted to add yet another "this ref is
> special and pay attention to it when it got updated", it makes a lot
> more sense not to add yet another loop that is a copy of this one in
> consume_refs(), but roll the handling of that new ref into a loop
> that already exists.  And for the purpose of such an update, I do
> not see why that "loop that already exists" should not be the one
> that goes over ref_map in store_updated_refs().  For the same
> reason, "the remote-tracking ref 'origin/suggested-hooks' is special"
> can and should go to an existing loop in store_updated_refs(), no?

I think 2 loops makes sense - the existing one to store the refs, and
the new one I introduced in this patch that handles the special ref.
(And the handling of "yet another" special ref would be rolled into the
latter loop.) In this case, I think that these 2 loops should be
independent. For example, if the refs in Git are changed to be stored in
an append-only journal, the former loop would be deleted while the
latter loop remains; and if the refs were to be stored as a sorted
array, the former loop would be retained while the latter loop would be
changed to a binary search.

Having said that, I don't feel strongly about this, so if consensus is
that we should move it into stsore_updated_refs(), that's fine too.

> How does this interact with the "--dry-run" option, by the way?
> What if ref_map proposes to update this suggested-hooks ref, but
> "--atomic" fetch feature finds that some other branches do not
> fast-forward and we end up not updating the suggested-hooks ref,
> either?

Good point - I'll have to check these.

> So, unlike my earlier guess, it _might_ turn out that "remember the
> state before the fetch, see if the fetch actually updated and then
> report" could be simpler to design and implement.  I dunno.

That's true. Right now I'm using the refmap data structure, but perhaps
it's better to consult the ref from the ref backend before and after the
fetch.

Overall, thanks for taking a look. If you have time, I would also
appreciate comments on the overall idea of this series (the concept of
remote-suggested hooks in general, the way that the user finds out about
them, and how the user can make use of them). (Same message to other
potential reviewers - general design comments are potentially more
useful, because specific code comments can become moot if there needs to
be a change in the overall design.)

Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks

[Phil Hord]

On Tue, Jul 20, 2021 at 2:17 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> Overall, thanks for taking a look. If you have time, I would also
> appreciate comments on the overall idea of this series (the concept of
> remote-suggested hooks in general, the way that the user finds out about
> them, and how the user can make use of them).

I'm curious what happens if we have multiple remotes.  Do we only take
suggestions from `origin`?  If not, what happens when there are
conflicting suggestions from different remotes?

Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks

[Jonathan Tan]

> On Tue, Jul 20, 2021 at 2:17 PM Jonathan Tan <jonathantanmy@google.com> wrote:
> >
> > Overall, thanks for taking a look. If you have time, I would also
> > appreciate comments on the overall idea of this series (the concept of
> > remote-suggested hooks in general, the way that the user finds out about
> > them, and how the user can make use of them).
> 
> I'm curious what happens if we have multiple remotes.  Do we only take
> suggestions from `origin`?  If not, what happens when there are
> conflicting suggestions from different remotes?

In this patch set, we only take suggestions from "origin". Right now my
idea is to store the name of the ref that we should track (e.g.
"refs/remotes/origin/suggested-refs") and only use that ref for hook
suggestions, so we would only take suggestions from one remote. But the
purpose of this RFC is to discuss questions like this, so feel free to
let us know if you have additional use cases.

Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 16 2021, Jonathan Tan wrote:

> Teach the "git hook install all|<hook-name>" command, that can install
> one or all remote-suggested hooks.
>
> If a configuration option hook.promptRemoteSuggested is set, inform the
> user of the aforementioned command:
>
>  - when cloning, and refs/remotes/origin/suggested-hooks is present in
>    the newly cloned repo
>  - when fetching, and refs/remotes/origin/suggested-hooks is updated
>  - when committing, there is a remote-suggested commit-msg hook, and
>    there is currently no commit-msg hook configured
>
> NEEDSWORK: Write a more detailed commit message once the design is
> finalized.

This is a bit orthagonal to what you're going for I guess, so sorry in
advance about the "but what about" bikeshedding you must be getting
tired of by now...

> @@ -1393,6 +1393,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  			   branch_top.buf, reflog_msg.buf, transport,
>  			   !is_local);
>  
> +	if (hook_should_prompt_suggestions()) {
> +		for (ref = mapped_refs; ref; ref = ref->next) {
> +			if (ref->peer_ref &&
> +			    !strcmp(ref->peer_ref->name,
> +				    "refs/remotes/origin/suggested-hooks")) {
> +				fprintf(stderr, _("The remote has suggested hooks in refs/remotes/origin/suggested-hooks.\n"
> +						  "Run `git hook install all` to install them.\n"));
> +				break;
> +			}
> +		}
> +	}
> +
>  	update_head(our_head_points_at, remote_head, reflog_msg.buf);
>  
>  	/*
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 769af53ca4..e86c312473 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -28,6 +28,7 @@
>  #include "promisor-remote.h"
>  #include "commit-graph.h"
>  #include "shallow.h"
> +#include "hook.h"
>  
>  #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
>  
> @@ -1313,6 +1314,22 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)
>  				 ref_map);
>  	transport_unlock_pack(transport);
>  	trace2_region_leave("fetch", "consume_refs", the_repository);
> +
> +	if (hook_should_prompt_suggestions()) {
> +		struct ref *ref;
> +
> +		for (ref = ref_map; ref; ref = ref->next) {
> +			if (ref->peer_ref &&
> +			    !strcmp(ref->peer_ref->name,
> +				    "refs/remotes/origin/suggested-hooks") &&
> +			    oidcmp(&ref->old_oid, &ref->peer_ref->old_oid)) {
> +				fprintf(stderr, _("The remote has updated its suggested hooks.\n"));
> +				fprintf(stderr, _("Run 'git hook install all' to update.\n"));
> +				break;
> +			}
> +		}
> +	}
> +
>  	return ret;
>  }

...but this part makes me think that if this is all we're aiming for as
far as server-client interaction is concerned we'd be much better off
with some general "server message-of-the-day" feature. I.e. server says
while advertising:

    version 2
    agent=...
    # does protocol v2 have a nicer way to encode this in the capabilities? I think not...
    motd=tellmeaboutref:suggested-hooks;master

Client does, while handshake() etc.:

    # other stuff
    command=ls-refs
    ....
    0000
    # Get motd from server
    command=motd
    0001
    refat suggested-hooks $SUGGESTED_HOOKS_AT_OID
    refat master $MASTER_AT_OID
    0000

And server says, after just invoking a "motd" hook or whatever, which
would be passed the git version, the state of any refs we asked politely
about and the client was willing to tell us about etc.

    Hi, we've got suggested hooks in this repository, it seems:
    if $agent > $min_git_version
        you have a supported git version, great....
    else
        <sadtrombone> you might want to upgrade your git to....
    fi

We could even carry this specific message in git.git, but under the hood
it would be the equivalent of a default 'motd' hook you could enable.

Maybe where you're going with this precludes such a MOTD approach.

FWIW I think there's lots of use-cases for it, and this specific hook
case is just one, so if we could make it slightly more general & just
make this a special-case of a generally useful facility.

Even for your use-case it would be useful, e.g. the whole discussion
we've been having about should the hooks by in a magic ref or your
current branch or not.

With a motd hook it doesn't matter, you just make "git hook install"
support installing hooks from whatever rev/tree, and a combination of
the "tellmeaboutref" and that feature means you can pick one or the
other, or tell users they need to install <some custom dependency> first
or whatever.

Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks

[Jonathan Tan]

> This is a bit orthagonal to what you're going for I guess, so sorry in
> advance about the "but what about" bikeshedding you must be getting
> tired of by now...

No - thanks for taking a look. More ideas are always welcome.

> ...but this part makes me think that if this is all we're aiming for as
> far as server-client interaction is concerned we'd be much better off
> with some general "server message-of-the-day" feature. I.e. server says
> while advertising:
> 
>     version 2
>     agent=...
>     # does protocol v2 have a nicer way to encode this in the capabilities? I think not...
>     motd=tellmeaboutref:suggested-hooks;master

Right now we don't have a way in capabilities to include arbitrary
strings, although we can extend it if needed.

> Client does, while handshake() etc.:
> 
>     # other stuff
>     command=ls-refs
>     ....
>     0000
>     # Get motd from server
>     command=motd
>     0001
>     refat suggested-hooks $SUGGESTED_HOOKS_AT_OID
>     refat master $MASTER_AT_OID
>     0000
> 
> And server says, after just invoking a "motd" hook or whatever, which
> would be passed the git version, the state of any refs we asked politely
> about and the client was willing to tell us about etc.

Ah...so the main difference is that it is the server that computes
whether a message is shown, based on information provided by the client
(different from my patches wherein the client computes whether a message
is shown).

I'm not sure how this is better, though. We don't need to build another
mechanism to print server messages (since it can already do so - the
same way it sends progress messages), but then we lose things like
translatability, and we have to build another endpoint for the server
("command=motd").

Also, one thing to think about is that we want to be able to prompt
users when they run hook-using commands (e.g. "commit"). With my
patches, the necessary information is stored in a ref but with your
idea, we need to figure out where to store it (and I think that it is
not straightforward - I'd rather not use config or extra files in the
.git directory to store remote state, although if the Git project is OK
with doing this, we could do that).

> FWIW I think there's lots of use-cases for it, and this specific hook
> case is just one, so if we could make it slightly more general & just
> make this a special-case of a generally useful facility.
> 
> Even for your use-case it would be useful, e.g. the whole discussion
> we've been having about should the hooks by in a magic ref or your
> current branch or not.
> 
> With a motd hook it doesn't matter, you just make "git hook install"
> support installing hooks from whatever rev/tree, and a combination of
> the "tellmeaboutref" and that feature means you can pick one or the
> other, or tell users they need to install <some custom dependency> first
> or whatever.

True - we avoid the discussion by having essentially a new namespace of
name-to-OID mappings. I still think it's simpler to use refs of some
sort for this, though. (Also, even if we use a new sort of name-to-OID
mapping, we need to make a ref for this OID so that it will be fetched,
so we might as well use a ref for this.)

Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks

[Emily Shaffer]

On Tue, Jul 20, 2021 at 02:48:09PM -0700, Jonathan Tan wrote:
> 
> > This is a bit orthagonal to what you're going for I guess, so sorry in
> > advance about the "but what about" bikeshedding you must be getting
> > tired of by now...
> 
> No - thanks for taking a look. More ideas are always welcome.
> 
> > ...but this part makes me think that if this is all we're aiming for as
> > far as server-client interaction is concerned we'd be much better off
> > with some general "server message-of-the-day" feature. I.e. server says
> > while advertising:
> > 
> >     version 2
> >     agent=...
> >     # does protocol v2 have a nicer way to encode this in the capabilities? I think not...
> >     motd=tellmeaboutref:suggested-hooks;master
> 
> Right now we don't have a way in capabilities to include arbitrary
> strings, although we can extend it if needed.
> 
> > Client does, while handshake() etc.:
> > 
> >     # other stuff
> >     command=ls-refs
> >     ....
> >     0000
> >     # Get motd from server
> >     command=motd
> >     0001
> >     refat suggested-hooks $SUGGESTED_HOOKS_AT_OID
> >     refat master $MASTER_AT_OID
> >     0000
> > 
> > And server says, after just invoking a "motd" hook or whatever, which
> > would be passed the git version, the state of any refs we asked politely
> > about and the client was willing to tell us about etc.
> 
> Ah...so the main difference is that it is the server that computes
> whether a message is shown, based on information provided by the client
> (different from my patches wherein the client computes whether a message
> is shown).
> 
> I'm not sure how this is better, though. We don't need to build another
> mechanism to print server messages (since it can already do so - the
> same way it sends progress messages), but then we lose things like
> translatability, and we have to build another endpoint for the server
> ("command=motd").
> 
> Also, one thing to think about is that we want to be able to prompt
> users when they run hook-using commands (e.g. "commit"). With my
> patches, the necessary information is stored in a ref but with your
> idea, we need to figure out where to store it (and I think that it is
> not straightforward - I'd rather not use config or extra files in the
> .git directory to store remote state, although if the Git project is OK
> with doing this, we could do that).

I think this is a pretty important point. To me, the ideal flow looks
like this:

 - I clone some repo, planning to just use the source code. I ignore the
   hook prompt.
 - I notice some bug which is within my power to fix. I have forgotten
   about the hook prompt, because I was having so much fun using the
   source code in the repo.
 - I 'git commit' - and 'git commit' says, "Did you know this repo
   suggests installing a commit-msg hook? You can install it by running
   'git hook install pre-commit' and run it by running 'git commit
   --amend --no-edit'. You can audit the commit-msg hook by running 'git
   hook magic-audit-command-name-tbd'. You can hide this advice <typical
   advice-hiding advice here>."

That way I don't add privilege (tell my computer it's OK to execute code
I didn't look at) until the very possible moment. This workflow also
captures changing intentions - I did not clone the code intending to
contribute back, but at the moment my intentions changed, I was nudged
to answer differently to a question I was asked with different earlier
intentions. That use case isn't easy to capture with a MOTD, unless you
run one on push, at which point it may be too late (e.g. if while fixing
I also accidentally uploaded my oauth password, and now it'll live
forever on GitHub in infamy).

MOTD approach also makes it hard to *update* hooks when the maintainer
so recommends - would be nice to have something baked in to notice when
there are new changes to the hooks, so we hopefully don't have
developers running hook implementations correlating to the date they
most recently cloned the project.

 - Emily

Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> I think this is a pretty important point. To me, the ideal flow looks
> like this:
>
>  - I clone some repo, planning to just use the source code. I ignore the
>    hook prompt.
>  - I notice some bug which is within my power to fix. I have forgotten
>    about the hook prompt, because I was having so much fun using the
>    source code in the repo.
>  - I 'git commit' - and 'git commit' says, "Did you know this repo
>    suggests installing a commit-msg hook? You can install it by running
>    'git hook install pre-commit' and run it by running 'git commit
>    --amend --no-edit'. You can audit the commit-msg hook by running 'git
>    hook magic-audit-command-name-tbd'. You can hide this advice <typical
>    advice-hiding advice here>."

Devil's advocate in me says that delaying this until the last
possible moment will make people *not* look at the hook's contents
and just say "yes".  After all, you have been working on a task and
reached one milestone (i.e. you are now ready to say "commit"), and
you want to start recording your thought process before it slips
away from your mind.  To many of us non-multi-tasking types, it is
one of the worst moment to force switching our attention to a
secondary task of vetting project supplied hooks---rather, I'd say
"Oh, I work for project X and if these hooks are supplied by project
X, it is good enough for them, and it must be good enough for me".

> That way I don't add privilege (tell my computer it's OK to execute code
> I didn't look at) until the very possible moment.

So this smells a wishful thinking to me.

> MOTD approach also makes it hard to *update* hooks when the maintainer
> so recommends - would be nice to have something baked in to notice when
> there are new changes to the hooks, so we hopefully don't have
> developers running hook implementations correlating to the date they
> most recently cloned the project.

Interesting.  Every time you run "git commit", the command will
check the existence of remotes/origin/suggested-hooks, compares the
installed .git/hooks/pre-commit with a blob in the tree stored
there, and tells you to update if they are different?  Or perhaps
the installed hooks record from which blob their contents came from,
and perform a three-way content level merge to carry local changes
forward?

Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks

[Jonathan Tan]

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > I think this is a pretty important point. To me, the ideal flow looks
> > like this:
> >
> >  - I clone some repo, planning to just use the source code. I ignore the
> >    hook prompt.
> >  - I notice some bug which is within my power to fix. I have forgotten
> >    about the hook prompt, because I was having so much fun using the
> >    source code in the repo.
> >  - I 'git commit' - and 'git commit' says, "Did you know this repo
> >    suggests installing a commit-msg hook? You can install it by running
> >    'git hook install pre-commit' and run it by running 'git commit
> >    --amend --no-edit'. You can audit the commit-msg hook by running 'git
> >    hook magic-audit-command-name-tbd'. You can hide this advice <typical
> >    advice-hiding advice here>."
> 
> Devil's advocate in me says that delaying this until the last
> possible moment will make people *not* look at the hook's contents
> and just say "yes".  After all, you have been working on a task and
> reached one milestone (i.e. you are now ready to say "commit"), and
> you want to start recording your thought process before it slips
> away from your mind.  To many of us non-multi-tasking types, it is
> one of the worst moment to force switching our attention to a
> secondary task of vetting project supplied hooks---rather, I'd say
> "Oh, I work for project X and if these hooks are supplied by project
> X, it is good enough for them, and it must be good enough for me".

I think both "I want to vet" and "good enough for project X is good
enough for me" are both reasonable points of view, and this
remote-suggested hook scheme supports both.

> > MOTD approach also makes it hard to *update* hooks when the maintainer
> > so recommends - would be nice to have something baked in to notice when
> > there are new changes to the hooks, so we hopefully don't have
> > developers running hook implementations correlating to the date they
> > most recently cloned the project.
> 
> Interesting.  Every time you run "git commit", the command will
> check the existence of remotes/origin/suggested-hooks, compares the
> installed .git/hooks/pre-commit with a blob in the tree stored
> there, and tells you to update if they are different?  Or perhaps
> the installed hooks record from which blob their contents came from,
> and perform a three-way content level merge to carry local changes
> forward?

We do notice when there are new changes, but only during fetch. 

I don't think we should compare the installed .git/hooks/pre-commit with
remotes/origin/suggested-hooks (since the user may have locally modified
that hook), so a solution involving storing the OID of the installed
hook somewhere (I haven't figured out where, though) and comparing that
OID against remotes/origin/suggested-hooks would be reasonable and would
be compatible with the current approach (as opposed to the one which
Ævar describes which, if I understand it correctly, would require
"commit" to access the network to figure out if the hook the client has
is the latest one).

Re: [RFC PATCH v2 2/2] hook: remote-suggested hooks

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> I think both "I want to vet" and "good enough for project X is good
> enough for me" are both reasonable points of view, and this
> remote-suggested hook scheme supports both.

Sure.  

I was just pointing out that the design is opinionated and not
giving two points of view fair chance to compete.  It will strongly
encourage users to the latter choice by prodding them when they want
to do a hook-invoking operation (like "git commit").

Not that opinionated design is necessarily a bad thing.

> I don't think we should compare the installed .git/hooks/pre-commit with
> remotes/origin/suggested-hooks (since the user may have locally modified
> that hook), so a solution involving storing the OID of the installed
> hook somewhere (I haven't figured out where, though) and comparing that
> OID against remotes/origin/suggested-hooks would be reasonable and would
> be compatible with the current approach (as opposed to the one which
> Ævar describes which, if I understand it correctly, would require
> "commit" to access the network to figure out if the hook the client has
> is the latest one).

Coping with local modification would not be rocket science.

If I were to do this, when the end-user approves installation of
and/or updates from remotes/origin/suggested-hooks/, the following
would happen:

 (1) If .git/hooks/* does not have the hook installed, copy it from
     the suggested hooks, and append two line trailer:

	# do not edit below
	# hook taken from <suggested hooks blob object name>

 (2) If .git/hooks/* does hold the hook, look for the "hook taken
     from" trailer

   (a) if "hook taken from" trailer is missing (i.e. it came from
       somewhere other than "remote suggested" workflow) or it does
       not point at a valid blob object, jump to "conflict exists"
       below.

   (b) using that (old) blob object name, perform (1) to recreate
       the hook the user would have seen when on-disk version of
       hook was created.  Difference between that and what is
       on-disk is the end-user customization.

       extract the current blob object from the suggested hooks tree
       object, do the same as (1), and then replay the end-user
       customization we figured out above.

       If the replaying succeeds cleanly, we are done.  Otherwise we
       have conflicts that cannot be resolved automatically.

   (c) "conflict exists".  The usual three-way merge resolution is
       needed.  I'd suggest to give users two (or three) files:

       - Rename the current version the user has to *.bak;
       - The new version from the project in the final file;
       - The patch obtained in (b) above, if exists in a separate file.

       and ask them to carry their customization forward to the
       second one (this is in line with the "we encourage them to
       adopt the project preferences" philosophy this proposal is
       taking us, I think).

I think configuration files under /etc/ on Debian-based distros have
been managed in a similar way for at least the past 10 years if not
longer, and since we are version control software ourselves, the
conflict resolution users are asked to perform in (2)-(c) shouldn't
be too much of a burden to our users anyway.

Re: [RFC PATCH v2 0/2] MVP implementation of remote-suggested hooks

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Here's an updated version of the remote-suggested hooks RFC, hopefully
> addressing some of the concerns people have brought up (e.g.
> auto-updating without letting the user verify, or prompting about hooks
> by default). It consists of two main parts:
>
>  - Non-interactive prompts during certain Git commands about the
>    existence of hooks. These prompts are turned *off* by default.
>
>  - New "git hook" subcommands that can install these hooks (so that the
>    aforementioned Git prompts or out-of-band installation instructions
>    can tell users to install these hooks in a platform-independent way).
>    These subcommands work whether or not prompts are enabled.
>
> You can see how they work in patch 2's t1361.

I really wanted to try this out, but which commit was this based on?

> In doing this, I have tried to scope down the overall effort as much as
> possible (to avoid security risks as much as possible, among other
> things), concentrating on situations that server-side hooks cannot
> handle. I think the main thing is that server-side hooks can check and
> prompt upon push, but they cannot catch problems that the client could
> have noticed upon commit; if the end user pushes a commit chain and only
> notices problems then, the user would have to redo the commits (possibly
> through an interactive rebase on the spot). Catching problems upon
> commit would prevent this problem, only necessitating amending the
> commit.
>
> In the Gerrit use case, Gerrit requires a specific "Change-Id" trailer
> in the commit message, but I can foresee the same issue in projects that
> require their commit messages to fit a certain template or projects that
> want lints to be run but their builds, for some reason, don't run
> arbitrary code (or they have no builds at all).
>
> To that end, I have added a prompt for the "commit-msg" hook if the
> remote has suggested one, but there are none currently installed.
> (Similar prompts could be added for other commit-related hooks.) The
> other prompts are upon clone and upon fetch (if the hooks have been
> updated).
>
> Jonathan Tan (2):
>   hook: move list of hooks
>   hook: remote-suggested hooks
>
>  builtin/bugreport.c               |  38 +-----
>  builtin/clone.c                   |  12 ++
>  builtin/fetch.c                   |  17 +++
>  builtin/hook.c                    |  17 ++-
>  hook.c                            | 185 +++++++++++++++++++++++++++++-
>  hook.h                            |   6 +
>  t/t1361-remote-suggested-hooks.sh | 105 +++++++++++++++++
>  7 files changed, 340 insertions(+), 40 deletions(-)
>  create mode 100755 t/t1361-remote-suggested-hooks.sh

Re: [RFC PATCH v2 0/2] MVP implementation of remote-suggested hooks

[Jonathan Tan]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > Here's an updated version of the remote-suggested hooks RFC, hopefully
> > addressing some of the concerns people have brought up (e.g.
> > auto-updating without letting the user verify, or prompting about hooks
> > by default). It consists of two main parts:
> >
> >  - Non-interactive prompts during certain Git commands about the
> >    existence of hooks. These prompts are turned *off* by default.
> >
> >  - New "git hook" subcommands that can install these hooks (so that the
> >    aforementioned Git prompts or out-of-band installation instructions
> >    can tell users to install these hooks in a platform-independent way).
> >    These subcommands work whether or not prompts are enabled.
> >
> > You can see how they work in patch 2's t1361.
> 
> I really wanted to try this out, but which commit was this based on?

Ah, good question. Like v1, it's based on the old es/config-based-hooks
(f2e1003b62 ("docs: link githooks and git-hook manpages", 2021-05-27)).
In v3, I'll rebase this on ab/config-based-hooks-base instead,
especially since that one has a programmatically generated list of hooks
(instead of a hardcoded one).