diff mbox series

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

Message ID 1ec1c958eb2b8aa2581280d050836dd0e7f6edef.1626453569.git.jonathantanmy@google.com (mailing list archive)
State New, archived
Headers show
Series MVP implementation of remote-suggested hooks | expand

Commit Message

Jonathan Tan July 16, 2021, 5:57 p.m. UTC
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.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/clone.c                   |  12 +++
 builtin/fetch.c                   |  17 ++++
 builtin/hook.c                    |  17 +++-
 hook.c                            | 151 +++++++++++++++++++++++++++++-
 hook.h                            |   3 +
 t/t1361-remote-suggested-hooks.sh | 105 +++++++++++++++++++++
 6 files changed, 300 insertions(+), 5 deletions(-)
 create mode 100755 t/t1361-remote-suggested-hooks.sh

Comments

Junio C Hamano July 19, 2021, 9:28 p.m. UTC | #1
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.
Ævar Arnfjörð Bjarmason July 20, 2021, 8:55 p.m. UTC | #2
On Fri, Jul 16 2021, Jonathan Tan wrote:

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

> @@ -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.
Jonathan Tan July 20, 2021, 9:11 p.m. UTC | #3
> OK.  Let's take this more as WIP.

Sounds good.

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

Good point. I'll do that.

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

I'm OK with it in either place, but it seems to me that this is separate
from storing refs - the suggestion of hooks does not influence how the
refs are stored.

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

I think 2 loops makes sense - the existing one to store the refs, and
the new one I introduced in this patch that handles the special ref.
(And the handling of "yet another" special ref would be rolled into the
latter loop.) In this case, I think that these 2 loops should be
independent. For example, if the refs in Git are changed to be stored in
an append-only journal, the former loop would be deleted while the
latter loop remains; and if the refs were to be stored as a sorted
array, the former loop would be retained while the latter loop would be
changed to a binary search.

Having said that, I don't feel strongly about this, so if consensus is
that we should move it into stsore_updated_refs(), that's fine too.

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

Good point - I'll have to check these.

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

That's true. Right now I'm using the refmap data structure, but perhaps
it's better to consult the ref from the ref backend before and after the
fetch.

Overall, thanks for taking a look. If you have time, I would also
appreciate comments on the overall idea of this series (the concept of
remote-suggested hooks in general, the way that the user finds out about
them, and how the user can make use of them). (Same message to other
potential reviewers - general design comments are potentially more
useful, because specific code comments can become moot if there needs to
be a change in the overall design.)
Phil Hord July 20, 2021, 9:28 p.m. UTC | #4
On Tue, Jul 20, 2021 at 2:17 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> Overall, thanks for taking a look. If you have time, I would also
> appreciate comments on the overall idea of this series (the concept of
> remote-suggested hooks in general, the way that the user finds out about
> them, and how the user can make use of them).

I'm curious what happens if we have multiple remotes.  Do we only take
suggestions from `origin`?  If not, what happens when there are
conflicting suggestions from different remotes?
Jonathan Tan July 20, 2021, 9:48 p.m. UTC | #5
> 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...

No - thanks for taking a look. More ideas are always welcome.

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

Right now we don't have a way in capabilities to include arbitrary
strings, although we can extend it if needed.

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

Ah...so the main difference is that it is the server that computes
whether a message is shown, based on information provided by the client
(different from my patches wherein the client computes whether a message
is shown).

I'm not sure how this is better, though. We don't need to build another
mechanism to print server messages (since it can already do so - the
same way it sends progress messages), but then we lose things like
translatability, and we have to build another endpoint for the server
("command=motd").

Also, one thing to think about is that we want to be able to prompt
users when they run hook-using commands (e.g. "commit"). With my
patches, the necessary information is stored in a ref but with your
idea, we need to figure out where to store it (and I think that it is
not straightforward - I'd rather not use config or extra files in the
.git directory to store remote state, although if the Git project is OK
with doing this, we could do that).

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

True - we avoid the discussion by having essentially a new namespace of
name-to-OID mappings. I still think it's simpler to use refs of some
sort for this, though. (Also, even if we use a new sort of name-to-OID
mapping, we need to make a ref for this OID so that it will be fetched,
so we might as well use a ref for this.)
Jonathan Tan July 20, 2021, 9:56 p.m. UTC | #6
> On Tue, Jul 20, 2021 at 2:17 PM Jonathan Tan <jonathantanmy@google.com> wrote:
> >
> > Overall, thanks for taking a look. If you have time, I would also
> > appreciate comments on the overall idea of this series (the concept of
> > remote-suggested hooks in general, the way that the user finds out about
> > them, and how the user can make use of them).
> 
> I'm curious what happens if we have multiple remotes.  Do we only take
> suggestions from `origin`?  If not, what happens when there are
> conflicting suggestions from different remotes?

In this patch set, we only take suggestions from "origin". Right now my
idea is to store the name of the ref that we should track (e.g.
"refs/remotes/origin/suggested-refs") and only use that ref for hook
suggestions, so we would only take suggestions from one remote. But the
purpose of this RFC is to discuss questions like this, so feel free to
let us know if you have additional use cases.
Emily Shaffer July 27, 2021, 12:57 a.m. UTC | #7
On Tue, Jul 20, 2021 at 02:48:09PM -0700, Jonathan Tan wrote:
> 
> > 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...
> 
> No - thanks for taking a look. More ideas are always welcome.
> 
> > ...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
> 
> Right now we don't have a way in capabilities to include arbitrary
> strings, although we can extend it if needed.
> 
> > 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.
> 
> Ah...so the main difference is that it is the server that computes
> whether a message is shown, based on information provided by the client
> (different from my patches wherein the client computes whether a message
> is shown).
> 
> I'm not sure how this is better, though. We don't need to build another
> mechanism to print server messages (since it can already do so - the
> same way it sends progress messages), but then we lose things like
> translatability, and we have to build another endpoint for the server
> ("command=motd").
> 
> Also, one thing to think about is that we want to be able to prompt
> users when they run hook-using commands (e.g. "commit"). With my
> patches, the necessary information is stored in a ref but with your
> idea, we need to figure out where to store it (and I think that it is
> not straightforward - I'd rather not use config or extra files in the
> .git directory to store remote state, although if the Git project is OK
> with doing this, we could do that).

I think this is a pretty important point. To me, the ideal flow looks
like this:

 - I clone some repo, planning to just use the source code. I ignore the
   hook prompt.
 - I notice some bug which is within my power to fix. I have forgotten
   about the hook prompt, because I was having so much fun using the
   source code in the repo.
 - I 'git commit' - and 'git commit' says, "Did you know this repo
   suggests installing a commit-msg hook? You can install it by running
   'git hook install pre-commit' and run it by running 'git commit
   --amend --no-edit'. You can audit the commit-msg hook by running 'git
   hook magic-audit-command-name-tbd'. You can hide this advice <typical
   advice-hiding advice here>."

That way I don't add privilege (tell my computer it's OK to execute code
I didn't look at) until the very possible moment. This workflow also
captures changing intentions - I did not clone the code intending to
contribute back, but at the moment my intentions changed, I was nudged
to answer differently to a question I was asked with different earlier
intentions. That use case isn't easy to capture with a MOTD, unless you
run one on push, at which point it may be too late (e.g. if while fixing
I also accidentally uploaded my oauth password, and now it'll live
forever on GitHub in infamy).

MOTD approach also makes it hard to *update* hooks when the maintainer
so recommends - would be nice to have something baked in to notice when
there are new changes to the hooks, so we hopefully don't have
developers running hook implementations correlating to the date they
most recently cloned the project.

 - Emily
Junio C Hamano July 27, 2021, 1:29 a.m. UTC | #8
Emily Shaffer <emilyshaffer@google.com> writes:

> I think this is a pretty important point. To me, the ideal flow looks
> like this:
>
>  - I clone some repo, planning to just use the source code. I ignore the
>    hook prompt.
>  - I notice some bug which is within my power to fix. I have forgotten
>    about the hook prompt, because I was having so much fun using the
>    source code in the repo.
>  - I 'git commit' - and 'git commit' says, "Did you know this repo
>    suggests installing a commit-msg hook? You can install it by running
>    'git hook install pre-commit' and run it by running 'git commit
>    --amend --no-edit'. You can audit the commit-msg hook by running 'git
>    hook magic-audit-command-name-tbd'. You can hide this advice <typical
>    advice-hiding advice here>."

Devil's advocate in me says that delaying this until the last
possible moment will make people *not* look at the hook's contents
and just say "yes".  After all, you have been working on a task and
reached one milestone (i.e. you are now ready to say "commit"), and
you want to start recording your thought process before it slips
away from your mind.  To many of us non-multi-tasking types, it is
one of the worst moment to force switching our attention to a
secondary task of vetting project supplied hooks---rather, I'd say
"Oh, I work for project X and if these hooks are supplied by project
X, it is good enough for them, and it must be good enough for me".

> That way I don't add privilege (tell my computer it's OK to execute code
> I didn't look at) until the very possible moment.

So this smells a wishful thinking to me.

> MOTD approach also makes it hard to *update* hooks when the maintainer
> so recommends - would be nice to have something baked in to notice when
> there are new changes to the hooks, so we hopefully don't have
> developers running hook implementations correlating to the date they
> most recently cloned the project.

Interesting.  Every time you run "git commit", the command will
check the existence of remotes/origin/suggested-hooks, compares the
installed .git/hooks/pre-commit with a blob in the tree stored
there, and tells you to update if they are different?  Or perhaps
the installed hooks record from which blob their contents came from,
and perform a three-way content level merge to carry local changes
forward?
Jonathan Tan July 27, 2021, 9:39 p.m. UTC | #9
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > I think this is a pretty important point. To me, the ideal flow looks
> > like this:
> >
> >  - I clone some repo, planning to just use the source code. I ignore the
> >    hook prompt.
> >  - I notice some bug which is within my power to fix. I have forgotten
> >    about the hook prompt, because I was having so much fun using the
> >    source code in the repo.
> >  - I 'git commit' - and 'git commit' says, "Did you know this repo
> >    suggests installing a commit-msg hook? You can install it by running
> >    'git hook install pre-commit' and run it by running 'git commit
> >    --amend --no-edit'. You can audit the commit-msg hook by running 'git
> >    hook magic-audit-command-name-tbd'. You can hide this advice <typical
> >    advice-hiding advice here>."
> 
> Devil's advocate in me says that delaying this until the last
> possible moment will make people *not* look at the hook's contents
> and just say "yes".  After all, you have been working on a task and
> reached one milestone (i.e. you are now ready to say "commit"), and
> you want to start recording your thought process before it slips
> away from your mind.  To many of us non-multi-tasking types, it is
> one of the worst moment to force switching our attention to a
> secondary task of vetting project supplied hooks---rather, I'd say
> "Oh, I work for project X and if these hooks are supplied by project
> X, it is good enough for them, and it must be good enough for me".

I think both "I want to vet" and "good enough for project X is good
enough for me" are both reasonable points of view, and this
remote-suggested hook scheme supports both.

> > MOTD approach also makes it hard to *update* hooks when the maintainer
> > so recommends - would be nice to have something baked in to notice when
> > there are new changes to the hooks, so we hopefully don't have
> > developers running hook implementations correlating to the date they
> > most recently cloned the project.
> 
> Interesting.  Every time you run "git commit", the command will
> check the existence of remotes/origin/suggested-hooks, compares the
> installed .git/hooks/pre-commit with a blob in the tree stored
> there, and tells you to update if they are different?  Or perhaps
> the installed hooks record from which blob their contents came from,
> and perform a three-way content level merge to carry local changes
> forward?

We do notice when there are new changes, but only during fetch. 

I don't think we should compare the installed .git/hooks/pre-commit with
remotes/origin/suggested-hooks (since the user may have locally modified
that hook), so a solution involving storing the OID of the installed
hook somewhere (I haven't figured out where, though) and comparing that
OID against remotes/origin/suggested-hooks would be reasonable and would
be compatible with the current approach (as opposed to the one which
Ævar describes which, if I understand it correctly, would require
"commit" to access the network to figure out if the hook the client has
is the latest one).
Junio C Hamano July 27, 2021, 10:40 p.m. UTC | #10
Jonathan Tan <jonathantanmy@google.com> writes:

> I think both "I want to vet" and "good enough for project X is good
> enough for me" are both reasonable points of view, and this
> remote-suggested hook scheme supports both.

Sure.  

I was just pointing out that the design is opinionated and not
giving two points of view fair chance to compete.  It will strongly
encourage users to the latter choice by prodding them when they want
to do a hook-invoking operation (like "git commit").

Not that opinionated design is necessarily a bad thing.

> I don't think we should compare the installed .git/hooks/pre-commit with
> remotes/origin/suggested-hooks (since the user may have locally modified
> that hook), so a solution involving storing the OID of the installed
> hook somewhere (I haven't figured out where, though) and comparing that
> OID against remotes/origin/suggested-hooks would be reasonable and would
> be compatible with the current approach (as opposed to the one which
> Ævar describes which, if I understand it correctly, would require
> "commit" to access the network to figure out if the hook the client has
> is the latest one).

Coping with local modification would not be rocket science.

If I were to do this, when the end-user approves installation of
and/or updates from remotes/origin/suggested-hooks/, the following
would happen:

 (1) If .git/hooks/* does not have the hook installed, copy it from
     the suggested hooks, and append two line trailer:

	# do not edit below
	# hook taken from <suggested hooks blob object name>

 (2) If .git/hooks/* does hold the hook, look for the "hook taken
     from" trailer

   (a) if "hook taken from" trailer is missing (i.e. it came from
       somewhere other than "remote suggested" workflow) or it does
       not point at a valid blob object, jump to "conflict exists"
       below.

   (b) using that (old) blob object name, perform (1) to recreate
       the hook the user would have seen when on-disk version of
       hook was created.  Difference between that and what is
       on-disk is the end-user customization.

       extract the current blob object from the suggested hooks tree
       object, do the same as (1), and then replay the end-user
       customization we figured out above.

       If the replaying succeeds cleanly, we are done.  Otherwise we
       have conflicts that cannot be resolved automatically.

   (c) "conflict exists".  The usual three-way merge resolution is
       needed.  I'd suggest to give users two (or three) files:

       - Rename the current version the user has to *.bak;
       - The new version from the project in the final file;
       - The patch obtained in (b) above, if exists in a separate file.

       and ask them to carry their customization forward to the
       second one (this is in line with the "we encourage them to
       adopt the project preferences" philosophy this proposal is
       taking us, I think).

I think configuration files under /etc/ on Debian-based distros have
been managed in a similar way for at least the past 10 years if not
longer, and since we are version control software ourselves, the
conflict resolution users are asked to perform in (2)-(c) shouldn't
be too much of a burden to our users anyway.
diff mbox series

Patch

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;
+			}
+		}
+	}
+
 	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;
 }
 
diff --git a/builtin/hook.c b/builtin/hook.c
index c79a961e80..0334fee967 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -139,6 +139,17 @@  static int run(int argc, const char **argv, const char *prefix)
 	return rc;
 }
 
+static int install(int argc, const char **argv, const char *prefix)
+{
+	if (argc < 1)
+		die(_("You must specify a hook event to install."));
+	if (!strcmp(argv[1], "all"))
+		hook_update_suggested(NULL);
+	else
+		hook_update_suggested(argv[1]);
+	return 0;
+}
+
 int cmd_hook(int argc, const char **argv, const char *prefix)
 {
 	const char *run_hookdir = NULL;
@@ -152,10 +163,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 +192,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], "install"))
+		return install(argc, argv, prefix);
 
 	usage_with_options(builtin_hook_usage, builtin_hook_options);
 }
diff --git a/hook.c b/hook.c
index 3ccacb72fa..32eee9abb6 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;
@@ -476,6 +482,60 @@  static int notify_hook_finished(int result,
 	return 0;
 }
 
+static struct tree *remote_suggested_hook_tree(int *warning_printed)
+{
+	struct object_id oid;
+	struct object *obj;
+	struct tree *tree;
+
+	if (read_ref("refs/remotes/origin/suggested-hooks", &oid))
+		return NULL;
+
+	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));
+		if (warning_printed)
+			*warning_printed = 1;
+		return NULL;
+	}
+	if (obj->type != OBJ_COMMIT) {
+		warning(_("object pointed to by refs/remotes/origin/suggested-hooks '%s' is not a commit"),
+			oid_to_hex(&oid));
+		if (warning_printed)
+			*warning_printed = 1;
+		return NULL;
+	}
+
+	tree = get_commit_tree((struct commit *) obj);
+	if (parse_tree(tree)) {
+		warning(_("could not parse tree"));
+		if (warning_printed)
+			*warning_printed = 1;
+		return NULL;
+	}
+
+	return tree;
+}
+
+static int has_suggested_hook(const char *hookname)
+{
+	struct tree *tree;
+	struct tree_desc desc;
+	struct name_entry entry;
+
+	tree = remote_suggested_hook_tree(NULL);
+	if (!tree)
+		return 0;
+
+	init_tree_desc(&desc, tree->buffer, tree->size);
+	while (tree_entry(&desc, &entry)) {
+		if (!strcmp(hookname, entry.path))
+			return 1;
+	}
+	return 0;
+}
+
 int run_hooks(const char *hookname, struct run_hooks_opt *options)
 {
 	struct list_head *to_run, *pos = NULL, *tmp = NULL;
@@ -497,8 +557,16 @@  int run_hooks(const char *hookname, struct run_hooks_opt *options)
 			    list_del(pos);
 	}
 
-	if (list_empty(to_run))
+	if (list_empty(to_run)) {
+		if (!strcmp("commit-msg", hookname)) {
+			if (has_suggested_hook(hookname) &&
+			    !hook_exists(hookname, HOOKDIR_USE_CONFIG)) {
+				fprintf(stderr, _("No commit-msg hook has been configured, but one is suggested by the remote.\n"));
+				fprintf(stderr, _("Run 'git hook install commit-msg' to install it.\n"));
+			}
+		}
 		return 0;
+	}
 
 	cb_data.head = to_run;
 	cb_data.run_me = list_entry(to_run->next, struct hook, list);
@@ -515,3 +583,84 @@  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(const char *hook_to_update)
+{
+	struct tree *tree;
+	int warning_printed = 0;
+	struct tree_desc desc;
+	struct name_entry entry;
+	struct strbuf path = STRBUF_INIT;
+	int hook_found = 0;
+
+	tree = remote_suggested_hook_tree(&warning_printed);
+	if (!tree) {
+		if (!warning_printed)
+			warning(_("no such ref refs/remotes/origin/suggested-hooks, not updating hooks"));
+		return;
+	}
+
+	init_tree_desc(&desc, tree->buffer, tree->size);
+	while (tree_entry(&desc, &entry)) {
+		int fd;
+
+		if (hook_to_update && strcmp(hook_to_update, entry.path))
+			/*
+			 * We only need to update one hook, and this is not the
+			 * hook we're looking for
+			 */
+			continue;
+
+		if (!hook_to_update && !is_hook(entry.path)) {
+			warning(_("file '%s' is not a hook; ignoring"),
+				entry.path);
+			continue;
+		}
+		hook_found = 1;
+		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);
+
+	if (hook_to_update && !hook_found)
+		warning(_("hook '%s' not found"), hook_to_update);
+
+	return;
+}
+
+int hook_should_prompt_suggestions(void)
+{
+	int dest = 0;
+
+	return !git_config_get_bool("hook.promptremotesuggested", &dest) &&
+	       dest;
+}
diff --git a/hook.h b/hook.h
index d902166408..438bf7122e 100644
--- a/hook.h
+++ b/hook.h
@@ -140,3 +140,6 @@  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(const char *hook_to_update);
+int hook_should_prompt_suggestions(void);
diff --git a/t/t1361-remote-suggested-hooks.sh b/t/t1361-remote-suggested-hooks.sh
new file mode 100755
index 0000000000..223c65ac99
--- /dev/null
+++ b/t/t1361-remote-suggested-hooks.sh
@@ -0,0 +1,105 @@ 
+#!/bin/sh
+
+test_description='remote-suggested hooks'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+setup_server () {
+	git init server &&
+	test_when_finished "rm -rf server" &&
+	cat >server/commit-msg <<-'EOF' &&
+	echo "overwrite the commit message" >$1
+EOF
+	git -C server add commit-msg &&
+	test_commit -C server commit-with-hook &&
+	git -C server checkout -b suggested-hooks
+}
+
+add_hook_to_server () {
+	>server/pre-commit &&
+	git -C server add pre-commit &&
+	test_commit -C server additional-hook
+}
+
+test_expect_success 'suggestion upon clone' '
+	setup_server &&
+	test_when_finished "rm -rf client" &&
+	git -c hook.promptRemoteSuggested=1 clone server client 2>err &&
+	grep "The remote has suggested hooks" err
+'
+
+test_expect_success 'no suggestion upon clone if not configured' '
+	setup_server &&
+	test_when_finished "rm -rf client" &&
+	git clone server client 2>err &&
+	! grep "The remote has suggested hooks" err
+'
+
+test_expect_success 'suggestion upon fetch if server has updated hooks' '
+	setup_server &&
+	git clone server client &&
+	test_when_finished "rm -rf client" &&
+	add_hook_to_server &&
+
+	git -C client -c hook.promptRemoteSuggested=1 fetch 2>err &&
+	grep "The remote has updated its suggested hooks" err
+'
+
+test_expect_success 'no suggestion upon fetch if not configured' '
+	setup_server &&
+	git clone server client &&
+	test_when_finished "rm -rf client" &&
+	add_hook_to_server &&
+
+	git -C client fetch 2>err &&
+	! grep "The remote has updated its suggested hooks" err
+'
+
+test_expect_success 'no suggestion upon fetch if server has not updated hooks' '
+	setup_server &&
+	git clone server client &&
+	test_when_finished "rm -rf client" &&
+	git -C server checkout main &&
+	test_commit -C server not-a-hook-update &&
+
+	git -C client -c hook.promptRemoteSuggested=1 fetch 2>err &&
+	! grep "The remote has updated its suggested hooks" err
+'
+
+test_expect_success 'suggest commit-msg hook upon commit' '
+	setup_server &&
+	git clone server client &&
+	test_when_finished "rm -rf client" &&
+
+	test_commit -C client foo 2>err &&
+	grep "Run .git hook install commit-msg" err
+'
+
+test_expect_success 'install one suggested hook' '
+	setup_server &&
+	git clone server client &&
+	test_when_finished "rm -rf client" &&
+
+	git -C client hook install commit-msg &&
+
+	# Check that the hook was written by making a commit
+	test_commit -C client bar &&
+	git -C client show >commit &&
+	grep "overwrite the commit message" commit
+'
+
+test_expect_success 'install all suggested hooks' '
+	setup_server &&
+	add_hook_to_server &&
+	git clone server client &&
+	test_when_finished "rm -rf client" &&
+
+	git -C client hook install all &&
+	test -f client/.git/hooks/pre-commit &&
+	test -f client/.git/hooks/commit-msg
+'
+
+test_done