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

Commit Message

Jonathan Tan June 16, 2021, 11:31 p.m. UTC

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 <[e-mail]>
---
 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(-)

Comments

Emily Shaffer June 18, 2021, 9:32 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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
>

Patch

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