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

[Junio C Hamano <gitster@pobox.com>]

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.