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

[Emily Shaffer <emilyshaffer@google.com>]

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