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

From: Johannes Sixt <j6t@kdbg.org>
Date: Apr 26, 2019, 21:53 UTC
Message-ID: <cf9818d5-6b58-49bf-8897-28ddb407f557@kdbg.org>
In-Reply-To: <20190426205859.GO6316@genre.crustytoothpaste.net>

Am 26.04.19 um 22:58 schrieb brian m. carlson:

> On Thu, Apr 25, 2019 at 09:40:34PM +0200, Johannes Sixt wrote:
> 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.

That's fine. On Windows, when a hook is present, it is also executable.

Show 8 quoted lines

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

We want to check for two vastly different conditions:

- Do we have to inspect the multi-hook directory? That decision should
be based on existence.

- Do we have to issue a warning? That can be based on the executable
flag. (As I understand, this is just a convenience warning because we do
not want the user to see a cryptic "cannot execute this thing" error or
something.)

I can see that you sense an inconsistency when you treat "not executable" as "does not exist". But that is just too subtle in my book, hard to explain, and not the practice that we are exercising these days.

I'm more concerned about the platform differences that we would have to note in the documentation:

  "To have multple hooks, do X and Y and make sure the standard hook
   file is not executable. Oh, and by the way, if you are on Windows,
   you have to remove the file to make it not executable."

Let's not go there.

-- Hannes