search

gabyx / Githooks

🦎 Githooks: per-repo and shared Git hooks with version control and auto update. [✩Star] if you're using it!
MIT License
102 stars 4 forks source link

Allow `git hooks install` to use templates #140

Closed giggio closed 3 months ago

giggio commented 6 months ago

Right now git hooks install will take from a static asset. If you have custom templates they are not used.

So we have 2 behaviors:

  1. for new repos (clone, init) the templates are used
  2. for existing repos, running git hooks install the templates are not used.

I'd like to be able to always use the templates, an option so that git hooks install uses the templates too.

Reference: https://github.com/gabyx/Githooks/blob/b67a88924dcb0d4b784fa5c49f8ac6a86f8ab919/githooks/hooks/wrapper.go#L29-L31

gabyx commented 6 months ago

Hi @giggio : I think you pointed out some diescrepancy thats right:

I think Githooks did never actually support to adjust the run-wrapper.sh which are the same (generated) for all files in hooks/*. This folder was mainly used for a complete manual install.

You are right: When you use --use-core-hookspath or --use-manual or nothing specified (so in all cases) we write the --template-dir hooks from the static assets. On git clone etc when nothing specified is used, the template dir will just get used and writes the folder .git/hooks, so thats right (the only way for us to deploy the run-wrappers automatic on git clone).

But yes, when you use git hooks install we use again the static embedded files and we dont look at the template folder. That for sure results in a discrepancy when you want to adjust the --template-dir and do some modifications. I assume you wanted to change the run-wrappers.sh in the ${--template-dir}/hooks, right?

I am just wondering if its a good idea to change this behavior, to find the relevant hook in the --template-dir instead of static files for git hooks install. The thing is: the --template-dir will anyway get overwritten again by any update Githooks does. So its a bit a foot gun if you change them. Maybe you can elaborate more on your use case so I can think some more.

gabyx commented 6 months ago

You could use GITHOOKS_RUNNER and set it in you rshell and we change 

# Read the runner script from the local/global or system config
if [ -z "$GITHOOKS_RUNNER" ]; then
GITHOOKS_RUNNER=$(git config githooks.runner)
fi
giggio commented 6 months ago

To me the greatest problem is consistency. The user will see one behavior if they clone or init, and another when they git hooks install. These should not be different.

On the other side, I understand that the user probably wants the default hooks.

Maybe you could add a flag to the config, something like githooks.useDefaultWrappers, which defaults to true, and the user could change it if they want (an advanced scenario).

gabyx commented 6 months ago

No there is no discrepancy: git hooks install will install the exact same run-wrapper as were installed in the template dir chosen as install time (default: <prefix>/.githooks/template/hooks). Of course when you want to change the run-wrappers (which was never a scenerio so far). If you give me a reason why you want the run-wrapper changed (I understood that you came from the $HOME env. case etc... down to this issue)

giggio commented 6 months ago

My only use case (so far) was fixing the environment variables.

BTW, I commited my files, you can see what I'm doing here:

https://github.com/giggio/dotfiles/blob/main/home/.gittemplate/hooks/pre-commit

gabyx commented 5 months ago

@giggio: I thougth of this again: I guess one should install the runwrappers and then use the installed ones for git hooks install or with a flag git hooks install --from-template-dir:

  1. In manual mode copy from githooks.manualTemplateDir/hooks
  2. In normal mode copy from init.templateDir/hooks
  3. In core.hooksPath panic, does not make sense.

This beahvior gets complicated to implement and handle correctly for git hooks uninstall because I must detect the runwrapper by the embedded url...: So any user changes to the runwrapper (if you make changes to the directories in 1 or 2.) -> might damage the behavior of Githooks and things get more arkane... So far git hooks install just writes the maintained run-wrappers (hooks) and removes unmaintained once, taking special care of Git LFS hooks, when uninstalling (reinstating them etc...)

I am under the impression one should not change the run-wrapper.sh in anyway, and that justifies to have it embedded statically and only use this one (sidenote: repos using Githooks are registered, during update Githooks will reinstall itself to those and write run-wrappers, potentially new ones, but this seldom happens...).

I agree the behavior is a bit strange especially in manual mode, because for what use is the githooks.manualTemplateDir then anyway, maybe only for manual use of

git config --local core.hooksPath $(git config githooks.manualTemplateDir)/hooks

which you can do always, and if Githooks run-wrappers are still inside githooks.manualTemplateDir (I hope...) it will also take care of GIT LFS (if not ->your problem, because someone might have thougth to remove the run-wrapper post-commit who knows why...)

Maybe it needs more clarification for what templateDir and manualTemplateDir is. The first is for git clone eventually and managed with a normal install, the latter is here for no good reason, maybe just remove it...??

Its just quite complicated with the current future set to maintain everything. I am questioning, if really the normal mode with init.templateDir is really usefull..., maybe for version 3 or so one could reduce the future set and make it dead simple, Git has just to many options to configure stuff... everybody wants something else.

What do you think with all this information?

giggio commented 5 months ago

Its just quite complicated with the current future set to maintain everything. I am questioning, if really the normal mode with init.templateDir is really usefull..., maybe for version 3 or so one could reduce the future set and make it dead simple, Git has just to many options to configure stuff... everybody wants something else.

I believe it is a bit confusing, now. I didn't know much about git hooks when searching for all of it, I knew what they could do, but not the details. I evaluated some tools and came to this one, which seemed mature enough and with a good level of maintenance and current investment. But I was very confused about how everything worked, there are too many options. Installing was confusing too, I didn't know what I was doing, and the installer asked me questions that I didn't know how to answer. Now it is clear to me, but it took some time.

I believe that having the normal mode is useful, especially in environments where you want things standardized, like corporate environments. This is not my case at the moment, I'm working on my personal machine, and I'm not going to use hooks for every project, so I went for the manual mode. But I have worked on these standardization efforts, and I can assure you, this would be useful. But... it could be replaced with a global hooksPath, so...

I guess one should install the runwrappers and then use the installed ones for git hooks install or with a flag git hooks install --from-template-dir:

  1. In manual mode copy from githooks.manualTemplateDir/hooks
  2. In normal mode copy from init.templateDir/hooks
  3. In core.hooksPath panic, does not make sense.

This makes sense to me.

I agree the behavior is a bit strange especially in manual mode, because for what use is the githooks.manualTemplateDir then anyway, maybe only for manual use of

git config --local core.hooksPath $(git config githooks.manualTemplateDir)/hooks

I agree with this too, you have overlapping features. Manual mode and the hooksPath mode do the same thing if you don't customize the run wrappers. Assuming that this is the case, I'd throw one of them away, probably manual mode, and keep only hooksPath, with a managed directory that you update when you update the tool, something like the current githooks.manualTemplateDir. And you don't need to register those repos anymore.

Thinking about it, maybe you could manage everything with hooksPath. What today is normal mode could be handled with a global hooksPath, and manual mode could simply set hooksPath on that one repo. And you keep the run wrappers at the Githooks install dir, so you don't need to register and later update the repo files. What do you think?

giggio commented 5 months ago

FYI: I have changed my approach after reflecting about it, I was using my manual mode by installing the run wrappers to the local repo directory. It is much better to use it centralized with hooksPath. Check out my alias to do that:

https://github.com/giggio/dotfiles/blob/9464648c39654ebbb221ab09c95a29753283ed49/home/.gitconfig#L70-L72

gabyx commented 5 months ago

Your input is certainly appreciated. A global usage comes with pain as soon as you use homebrew and other tooling which uses Git heavily. So there Githooks will always run etc etc... Not so desirable. Thats why I think. Manual approach is always better.

I think about the following for v3:

So the install mode: init.templateDir is not needed anymore. The only caveat is that core.hooksPath is not that safe, meaning, that if the path points to some non-existing directory, you want notice. Thats kind of shitty. That does not happen with hook run-wrappers directly inside the .git/hooks folder. Also the registered mechanism falls away, since we only update the --template-dir.

or maybe the follwing:

giggio commented 5 months ago

I agree setup should be simplified. Two modes is enough. And I'd go with the first option, set the repo's core.hooksPath, and that is it. There is no need to use repo's .git/hooks, and the tool doesn't need to manage it anymore. This is what I'm doing right now in my environment, that is what my git alias does. :)

Not needing init.templateDir is good, as the global core.hooksPath already solves it.

Regarding the directory not existing, this would only happen if the user manually deletes it. They broke it, they fix it. What you could do is provide guidance in case this happens, in the docs. And even warn the user to not touch that directory, let them know that it is managed by the tool.

gabyx commented 3 months ago

I close this issue. I refered to this in #152. I will use the discussion in the future for the implementation etc. So the discussion can also continue here.

gabyx commented 3 months ago

@giggio : Version 3.0.1 has been released, Install modes are now easier and in the sense we discussed. Check the release notes for further information.

giggio commented 2 months ago

@gabyx I'm trying them now, and it seems much better.