From: Jeff King Date: Fri, 29 May 2026 19:23:50 GMT Subject: Re: git hook question Message-ID: <20260529192350.GB1711766@coredump.intra.peff.net> In-Reply-To: On Fri, May 29, 2026 at 12:11:59PM -0400, Wesley Schwengle wrote: > > git config hook.npm-test.command 'npm run test #' > > > > Git will paste together the shell command: > > > > npm run test # "$@" > > That doesn't work on my side: > > $ cat ~/.config/git/js.config && git config --get hook.npm-test.command && > GIT_TRACE=1 git poh > [hook "npm-test"] > event = pre-push > command = npm run test # > enabled = true The "#" is being eaten by the config parser as a comment, so the value is effectively the same as what you originally had. As you noticed, putting it in double-quotes fixes that, though I'd probably do the whole thing for readability like: command = "npm run test #" Which is also what "git config" would write with the command I showed above. > Also seems to fail: > > [hook "npm-test"] > event = pre-push > command = git npm-test > enabled = true > > [alias] > npm-test = !f() { npm run test; }; f This is also a config quoting problem. Both "#" and semicolon begin comments. Putting the whole thing in double-quotes works. > The following circles back a little to the first response. > > Tt kind of diverges from `git hook run pre-push' and how additional > arguments are given on the command line with that invocation. Wrappers need > to become aware on way it is called, either via hook or via a manual way, > because of the `remote url' that gets added. I don't think the hooks themselves should need to be aware. If somebody is calling "git hook run pre-push" without providing arguments, they are breaking the contract to the hooks. You can get away with it if you know your particular hooks do not care about those arguments, but in the general case, what should a pre-push hook that _does_ care about the remote name do when it doesn't get any arguments? It's an error. I guess there's a more fundamental question: why are you running "git hook" in the first place? If it is just to test out your hooks, that's fine. But to make the test more realistic, you may want to give it arguments (and stdin input) to match the specific hook you're testing. > Normal hooks get that info via their STDIN, wouldn't this also make sense > for these type of hooks? It makes differentiation much easier. Usually we pass fixed-size information via arguments, and arbitrary-sized information over stdin. In pre-push you have the joy of dealing with both. So your "npm run test" hook is also going to have its stdin hooked up to a pipe with the ref updates sent over it. That might be OK if it never reads from stdin, but it may also cause some surprises depending on what the "test" target runs under the hood. So whether you are getting input as arguments or over stdin, it's probably something the hook needs to deal with (or at least think about). -Peff