Re: [PATCH 1/5] run-command: add preliminary support for multiple hooks

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Apr 24, 2019, 22:32 UTC
Message-ID: <20190424223240.GI6316@genre.crustytoothpaste.net>
In-Reply-To: <xmqqo94w2l3k.fsf@gitster-ct.c.googlers.com>

On Wed, Apr 24, 2019 at 11:27:59AM +0900, Junio C Hamano wrote:

Show 25 quoted lines

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> > diff --git a/run-command.c b/run-command.c
> > index 3449db319b..669af5ebc7 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -1308,58 +1308,137 @@ int async_with_fork(void)
> >  #endif
> >  }
> >  
> > +static int has_hook(struct strbuf *path, int strip)
> > +{
> > +	if (access(path->buf, X_OK) < 0) {
> 
> Does ".git/post-commit" that is not an executable exist?
> 
> It was perfectly fine for find_hook() to say "there is no hook for
> post-commit" in the old world in such a case, because the
> unexecutable file it found is not going to be run anyway.
> 
> But it is not clear if has_hook(), that affects "there is no single
> hook file for post-commit, so let's look at post-commit.d" decision
> made by find_hooks(), should behave that way.  It somehow feels more
> intuitive if a post-commit file that is not executable, by merely
> existing, stops post-commit.d directory from being scanned, at least
> to me.

Unfortunately, we used to have Git versions that wrote an non-executable file in place instead of a ".sample" file when initializing the repository. We have a warning for that, but I have many repositories that have lived long enough that they're still affected (and I've turned off the warning for that reason). I feel like we'll be creating surprising behavior for long-term users.

Also, if someone is using an old manager script that uses the ".d" directory or some other workaround, it's easy enough for them to simply clear the executable bit; they need not delete it, and can restore it at any time simply by toggling the bit.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204