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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 25, 2019, 09:39 UTC
Message-ID: <87lfzys9t1.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqo94uzyxa.fsf@gitster-ct.c.googlers.com>

On Thu, Apr 25 2019, Junio C Hamano wrote:

Show 20 quoted lines

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