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

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jul 19, 2021, 21:28 UTC
Message-ID
<xmqqzguij8nq.fsf@gitster.g>
In-Reply-To
<1ec1c958eb2b8aa2581280d050836dd0e7f6edef.1626453569.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
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.
OK.  Let's take this more as WIP.
Show 18 quoted lines
> 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.

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

Previous: Jonathan TanNext: Jonathan Tan
Message 25 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.