git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Emily Shaffer <emilyshaffer@google.com>
Date
Jun 18, 2021, 21:32 UTC
Message-ID
<YM0Q6AynHC675ndd@google.com>
In-Reply-To
<0f03e956c183d827c186b6fb85b92bfd31d2dc47.1623881977.git.jonathantanmy@google.com>
On Wed, Jun 16, 2021 at 04:31:49PM -0700, Jonathan Tan wrote:
Show 35 quoted lines
> 
> 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.)

Show 29 quoted lines
> 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.
Show 18 quoted lines
> @@ -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.

Show 6 quoted lines
> +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).

Show 123 quoted lines
> +
>  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.

Show 60 quoted lines
> 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
> 
Previous: Jonathan TanNext: Junio C Hamano
Message 6 of 36 in “MVP implementation of remote-suggested hooks”
  1. 0/2 MVP implementation of remote-suggested hooksJonathan Tan, Jun 16, 2021
  2. 1/2 hook: move list of hooksJonathan Tan, Jun 16, 2021
  3. Emily ShafferJun 18, 2021
  4. Jonathan TanJun 18, 2021
  5. 2/2 clone,fetch: remote-suggested auto-updating hooksJonathan Tan, Jun 16, 2021
  6. Emily ShafferJun 18, 2021
  7. Junio C HamanoJun 17, 2021
  8. Jonathan TanJun 18, 2021
  9. Emily ShafferJun 18, 2021
  10. Jonathan TanJun 18, 2021
  11. Randall S. BeckerJun 18, 2021
  12. Matt RogersJun 19, 2021
  13. Jonathan TanJun 21, 2021
  14. Ævar Arnfjörð BjarmasonJun 20, 2021
  15. Jonathan TanJun 21, 2021
  16. Ævar Arnfjörð BjarmasonJun 21, 2021
  17. Jonathan TanJun 22, 2021
  18. brian m. carlsonJun 22, 2021
  19. Jonathan TanJun 23, 2021
  20. brian m. carlsonJun 24, 2021
  21. Junio C HamanoJun 28, 2021
  22. 0/2 MVP implementation of remote-suggested hooksJonathan Tan, Jul 16, 2021
  23. 1/2 hook: move list of hooksJonathan Tan, Jul 16, 2021
  24. 2/2 hook: remote-suggested hooksJonathan Tan, Jul 16, 2021
  25. Junio C HamanoJul 19, 2021
  26. Jonathan TanJul 20, 2021
  27. Phil HordJul 20, 2021
  28. Jonathan TanJul 20, 2021
  29. Ævar Arnfjörð BjarmasonJul 20, 2021
  30. Jonathan TanJul 20, 2021
  31. Emily ShafferJul 27, 2021
  32. Junio C HamanoJul 27, 2021
  33. Jonathan TanJul 27, 2021
  34. Junio C HamanoJul 27, 2021
  35. Junio C HamanoJul 19, 2021
  36. Jonathan TanJul 20, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.