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

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Apr 26, 2019, 20:58 UTC
Message-ID: <20190426205859.GO6316@genre.crustytoothpaste.net>
In-Reply-To: <2a511c3c-ef8d-07c5-80e2-398780ee6ed4@kdbg.org>

On Thu, Apr 25, 2019 at 09:40:34PM +0200, Johannes Sixt wrote:

Show 15 quoted lines

> Am 25.04.19 um 02:55 schrieb Junio C Hamano:
> > Johannes Sixt <j6t@kdbg.org> writes:
> > 
> >> Furthermore, basing a decision on whether a file is executable won't
> >> work on Windows as intended. So, it is better to aim for an existence check.
> > 
> > That is a good point.
> > 
> > So it may be OK for "do we have a single hook script for this hook
> > name?" to say "no" when the path exists but not executable on
> > POSIXPERM systems, but it is better to say "yes" for consistency
> > across platforms (I think that is one of the reasons why we use
> > .sample suffix these days).
> 
> All correct.

I would like to point out that we still have to perform an executability check before we run the hook or we'll get errors printed to the user. As I mentioned, there are many people with repositories that have the non-.sample files. For me, any repository older than about five years will likely have those files.

Show 9 quoted lines

> > And for the same reason, for the purpose of deciding "because we do
> > not have a single hook script, let's peek into .d directory
> > ourselves", mere presence of the file with that name, regardless of
> > the executable bit, should signal that we should not handle the .d
> > directory.
> > 
> > IOW, you think access(X_OK) should be more like access(F_OK)?
> 
> Yes, that's my conclusion.

Right now, we have a standard way to handle the way we handle hooks: if they are not executable, we warn and pretend there's no hook. With this new paradigm, we have to check whether the main hook is executable, and if it's not, we then have to check whether it's present, and if so, we skip the multiple hooks.

I understand the executable bit is not useful on Windows, but on Unix, we should be consistent with how we treat the hooks.

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