On Thu, Apr 25 2019, Junio C Hamano wrote:
> Johannes Sixt <j6t@xxxxxxxx> 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).
>
> 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)?
To me this is another point in favor of bypassing this problem entirely
and adopting the semantics GitLab (and it seems others) use. I.e. in
order execute:
.git/hooks/pre-receive .git/hooks/pre-receive.d/*
Instead of going further down this avenue of:
if exists_or_executable_or_whatever .git/hooks/pre-receive
then
.git/hooks/pre-receive
else
for hook in .git/hooks/pre-receive.d/*
do
$hook
done
fi
It also:
1) Makes it easier for users to experiment with more granular hooks if
they have one big pre-receive hook by adding pre-receive.d/* hooks
without having to move their existing pre-receive to
pre-receive.d/000-existing hook (which will be incompatible across
git versions!).
2) Is compatible with any existing trampoline scripts you might want to
migrate from that *don't* use pre-receive.d/*, e.g. one script in
our infrastructure (that I didn't write) does:
my ($hook_phase, $dir) = fileparse($0);
my $exit = 0;
my @hooks = glob("${dir}${hook_phase}-*");
for my $hook (@hooks) {
next unless -x $hook;
$exit |= system $hook;
}
exit ($exit >> 8);
I.e. you have a ".git/hooks/pre-receive" trampoline and it runs
".git/hooks/pre-receive-*" scripts.
It occurs to me that we might want to make things configurable for the
#2 case. I.e. have a core.hooksDSuffix=".d/" by default, but you could
also set it to "-". So we'd then construct a glob of either
"pre-receive.d/*" or "pre-receive-*" from that.
