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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 20, 2021, 20:55 UTC
Message-ID: <87o8awvglr.fsf@evledraar.gmail.com>
In-Reply-To: <1ec1c958eb2b8aa2581280d050836dd0e7f6edef.1626453569.git.jonathantanmy@google.com>

On Fri, Jul 16 2021, Jonathan Tan wrote:

Show 14 quoted lines

> 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...

Show 53 quoted lines

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