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@xxxxxxxxxx>
> ---
> 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
>
