[rush] Rush doesn't acknowledge git config core.hooksPath #3004

Activity

1. Daniel (D4N14L)

   added

   help wantedIf you're looking to contribute, this issue is a good place to start!If you're looking to contribute, this issue is a good place to start!

   on Nov 4, 2021
2. D4N14L commented on Nov 4, 2021

   

   Daniel (D4N14L)

   on Nov 4, 2021

   Contributor

   More actions

   This looks to be the case. Thanks Dmitrii Kartashev (@Faithfinder) for reporting this! I don't really have the bandwidth to take a look at this myself, but if you wanted to take a look/make a fix, here's a couple places to start:

   In the below file, we find the target directory for the .git folder and install our hooks into there:
   

   rushstack/apps/rush-lib/src/logic/base/BaseInstallManager.ts

   Line 317 in 15222e4

   const hookDestination: string | undefined = git.getHooksFolder();

   There's really a couple approaches, either querying for the core.hooksPath value and using that as our destination to install the hooks to, or setting the git hooks folder to the one where we currently keep our git hooks (common/git-hooks) and avoiding an install process of any kind (though, this may be an issue with husky if it's already tinkering with the core.hooksPath). When this functionality was implemented in Rush, it's likely this property in Git wasn't available.
3. octogonz commented on Nov 9, 2021

   

   Pete Gonzalez (octogonz)

   on Nov 9, 2021

   Collaborator

   More actions

   I've been migrating a monorepo from lerna + yarn workspaces, and it had git hooks before, except they were implemented by husky. And husky change the hooks installation path to it's own.

   I see. In the past, Husky would use a postinstall script to wire up a bunch of Node.js script stubs under .git/hooks/*. As of Husky 7, it appears that they changed their strategy to leverage the new core.hooksPath setting to relocate the hooks path:

   • The setting is written into <repo>/.git/config on the developer's local machine
   • As Dmitrii Kartashev (@Faithfinder) pointed out, this setting persists between branches, even after Husky has been removed from your repo
   • The setting is ignored unless you have Git version 2.9.0 or later

   My first thought was that Rush should simply query core.hooksPath and honor it. However... that seems problematic:

   • What if the hook scripts use relative paths that assume that they are installed under .git/hooks/pre-commit instead of .husky/pre-commit?
   • Or what if core.hooksPath is pointing to some unusual path outside the repo? (Git's docs suggest a use case where you’d like to centrally configure your Git hooks instead of configuring them on a per-repository basis.)

   I wonder if it would be better for Rush to warn the user to do git config --unset core.hooksPath. To ensure deterministic behavior, that's probably what we'd prefer for a CI machine to do if there was a concern that Husky might have been invoked by someone's branch on that machine.
4. Faithfinder commented on Nov 9, 2021

   

   Dmitrii Kartashev (Faithfinder)

   on Nov 9, 2021

   Last edited by Faithfinder

   ContributorAuthor

   More actions

   I'll defend the "honor the setting" approach, since that's what my PR does.
   Both question are, basically, about relative paths.

   I'm not sure it's reasonable for rush to concern itself with how people write their hooks. If the specific monorepo wants the hooks to be in a specific location, it can write an "Am I in the right place?" check at the beginning of the script.
   And if it's a migration from another tool, like my case, then I can guarantee that whoever does the migration has to, anyway:

   • provide instructions for developers on how to work in the new world.
   • Rewrite the hooks from husky, since it's syntax is different anyway

   The person doing the migration should check that their hooks are working. Since my PR now shows where they are installed, they have information they need to debug. They can either give an instruction to developers to unset the variable, or update the hook scripts to warn about non-standard location.
   

   The PR discussion is more relevant
5. Pete Gonzalez (octogonz)

   closed this as completedin #3013on Dec 14, 2021
6. Ian Clanton-Thuon (iclanton)

   added this to Bug Triageon Aug 15, 2023
7. Ian Clanton-Thuon (iclanton)

   moved this to Closed in Bug Triageon Aug 15, 2023