Correctly resolve git hooks directory using git rev-parse

[VanTanev]

Fixes #990

There is no reasonable way to add tests for this, as far as I can figure.

[VanTanev]

Tangential, but this doesn't work with knip --directory ../some-folder-one-level-up - This is because the gitHooksPath() function is called in a context, where the --directory parameter doesn't exist, and we can't pass cwd option to child_process.execSync()

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/knip@991

commit: 6aec576

[webpro]

Fixes #990

Thanks for the PR!

There is no reasonable way to add tests for this, as far as I can figure.

We do have https://github.com/webpro-nl/knip/blob/main/packages/knip/test/plugins/lefthook-v1.test.ts + https://github.com/webpro-nl/knip/tree/main/packages/knip/fixtures/plugins/lefthook-v1 to prevent regression for regular .git/hooks lookups.

[VanTanev]

Which now fails because it depended on the old static behavior... OK, let me look at it and see if I can come up with a test

[VanTanev]

@webpro The tests are fairly ugly, but that is because git hook paths are resolved at knip import time, and do not resolve relatively to the cwd argument. This means the only way to set process.cwd() is to do process.chdir() before the import statement :-/

[VanTanev]

Leaving aside the biome formatting issue, this probably fails because the tests do not have execution isolation.

Running only the lefthook test on its own works fine.

So there is a module import that happens earlier, and the incorrect paths get cached here:

knip/packages/knip/src/plugins/lefthook/index.ts

Lines 16 to 18 in 529a734

	const gitHookPaths = getGitHookPaths();
	const config = ['lefthook.yml', ...gitHookPaths];

This is not easy to fix, unless plugin config variable option turns into a function that gets knip options and returns an array.

[VanTanev]

This is a workaround for configs getting calculated way too early with a static process.cwd()

[webpro]

How about adding a separate Actions workflow to assert git worktrees work properly? Could that simplify things, or maybe you have other ideas?

[VanTanev]

You mean, go down to a terminal to execute the worktree tests instead of trying to run them within the test runner?

I think that would work well. There must be some best practices around doing that, I'll look around at some tools that need to manipulate git repositories at how they do their tests and come back to this.

[webpro]

There don't even necessarily need to be a test runner per se, only shell commands to set up the situation, add some files perhaps (maybe prepare a fixtures folder for this, but without a feature.test.ts file), and run knip. If all commands (including knip) exit with code zero then the workflow (which itself is the test!) succeeds.

[VanTanev]

Yep! Will look into doing that if not tomorrow by the end of the week. Thanks for helping me get this to a good state!

[webpro]

Is there still interest in finishing this PR? No worries if not, but I'm going to clean things up a bit around here.

[VanTanev]

I definitely want to push this through, as it's still a problem in my day-to-day work. It just seemed daunting every time I thought to return to it, as writing the correct tests seems quite complicated, even though the underlying fix is so straightforward: 2fe0a85

But please, keep it open for a while longer, and I'll make a best effort to get this fixed properly.

[webpro]

Sure, no hurries, just wanted to check if it's still interesting/relevant.

[VanTanev]

Hey @webpro, I added a workflow that verifies worktree behavor and can correctly fail:
https://github.com/VanTanev/knip/actions/runs/16552992919/job/46810512675

This PR is good for re-review.

Let me know if you'd like me to improve this PR in any way before it's good for merge, and thank you for the patience!

[VanTanev]

Huh, wonder what causes these failures, tests were running fine locally. Will investigate.