Re: [PATCH] Give support for hooks based on platform

From: Jeff King <peff@peff.net>
Date: Aug 23, 2021, 18:32 UTC
Message-ID: <YSPpySdd5qqDeNbm@coredump.intra.peff.net>
In-Reply-To: <xmqqfsv0qby7.fsf@gitster.g>

On Mon, Aug 23, 2021 at 10:59:28AM -0700, Junio C Hamano wrote:

Show 32 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > On Sun, Aug 22, 2021 at 10:07:41PM +0000, brian m. carlson wrote:
> >
> >> > The point is that in many cases a dependency with a script language is
> >> > created only to make the hook actions portable from a platform to
> >> > other, but what this script in its essence does is a thing that could
> >> > be done with basic tools delivered with the current operating system.
> >> 
> >> Then, in general, it can be done in a shell script containing an if-then
> >> statement per platform using the native tools, so I'm not seeing the
> >> particular reason that this series is necessary if the hooks being
> >> executed aren't binaries.  All systems on which Git runs must contain a
> >> POSIX-compatible shell.
> >
> > This is my gut feeling, too (whether users know it or not, even on
> > Windows most programs specified by config are being run by the shell).
> >
> > However, I do think there is room for Git to make this case a bit
> > easier: conditional config includes. Once we are able to specify hooks
> > via config (which is being worked on elsewhere), then we ought to be
> > able to implement an includeIf like:
> >
> >   [includeIf "uname_s:linux"]
> >   path = linux-hooks.config
> >   [includeIf "uname_s:windows"]
> >   path = windows-hooks.config
> >
> > The advantage being that this could apply to _all_ config, and not just
> > hooks.
> 
> Heh, it seems great minds think alike.

One important distinction in what you wrote is that you're expecting the user to set dev.host once. That nicely sidesteps any question of "how does Git label each platform?", but it does mean the user has to do that setup manually (which maybe is amortized across many repos, but in practice for many people I suspect is no better than them setting up the correct "include" in the first place).

I hoped that by calling it "uname_s", it would be clear it was the same as "uname -s", and then we could blame any naming confusion on the OS. :)

But even if it is not used for this particular application, I think the [includeIf "var:..."] you proposed might be a reasonable thing to support.

-Peff