search

tarmolov / git-hooks-js

A tool to manage and run project git hooks
167 stars 30 forks source link

1.1.1 is broken #22

Closed TitanNano closed 7 years ago

TitanNano commented 8 years ago

release 1.1.1 broke our git hooks. We have a pre-commit and a commit-msg hook.

The pre-commit runs, but doesn't create any terminal output anymore. The commit-msg should validate the commit message, I'm not sure if that works, but after everything is done, the commit wasn't created.

There are no errors, so I don't know why this is happening. I reverted back to 1.1.0 and everything is working again.

tarmolov commented 8 years ago

I couldn't reproduce your issue :(

Here is my test project:

$ git clone https://github.com/tarmolov/git-hooks-tests.git
$ cd git-hooks-tests
$ touch test
$ git add test
$  git commit -m "Test commit"
pre-commit
commit-msg
[master 41b91d7] Test commit
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 test

pre-commit and commit-msg were run as expected.

Could you create a simple example which demonstrates the issue? Or test which fails but shouldn't?

tarmolov commented 8 years ago

@TitanNano I really want to help you and fix the bug if it exists. Please provide an extra information.

TitanNano commented 8 years ago

@tarmolov sorry for the late reply. I try to find an example for the issue tomorrow.

TitanNano commented 8 years ago

@tarmolov I have good news. I did future tests and I could resolve the issue by deleting .git/hooks and moving .git/hooks.old to .git/hooks. Then re-run npm install git-hooks to reinstall v1.1.1 and everything works as expected.

So I guess you need to reinstall the hooks after an update. Right now I can see that it says "hooks already installed".

tarmolov commented 8 years ago

It must work during execution of npm install

When you install a new version of git-hooks:

  1. The previous version uninstalls hooks (mv .git/hooks.old .git/hooks)
  2. The new version reinstalls them.

I'll take a closer look at this part of the tool. Thx

TitanNano commented 8 years ago

When I install an update the output is the following:

Jovans-MacBook-Air-3:zmbCORE Jovan$ npm install

> git-hooks@1.1.0 postinstall /Users/Jovan/zmb/zmbCORE/Code/zmbCORE/node_modules/git-hooks
> git-hooks --install

git-hooks already installed
zmbcore@1.0.0 /Users/Jovan/zmb/zmbCORE/Code/zmbCORE
└── git-hooks@1.1.0

and no action is performed. .git/hooks.old only contains the sample hooks from git.

tarmolov commented 8 years ago

It's ok to see such message when you executer npm install in second time. It's not ok to see it during update of git-hooks.

TitanNano commented 8 years ago

but npm install and npm update both trigger the same postinstall script. It doesn't difference between an update and a re-install.

tarmolov commented 8 years ago 
$ npm install git-hooks@1.0.0

> git-hooks@1.0.0 postinstall
> git-hooks --install

git-hooks@1.0.0 node_modules/git-hooks
$ npm install git-hooks@1.1.0

> git-hooks@1.0.0 preuninstall
> git-hooks --uninstall

> git-hooks@1.1.0 postinstall 
> git-hooks --install

git-hooks@1.1.0 node_modules/git-hooks
$ npm install

> git-hooks@1.1.0 postinstal
> git-hooks --install

git-hooks already installed

You can notice that git-hooks removes the previous version before install a new one. But when you execute npm install without parameters, it won't execute uninstall. That's how npm works. I'm afraid I can do nothing there :(

TitanNano commented 8 years ago

hmm the uninstall steps are never performed.

$ npm install git-hooks@1.1.0

> git-hooks@1.1.0 postinstall /Users/Jovan/zmb/zmbCORE/Code/zmbCORE/node_modules/git-hooks
> git-hooks --install

git-hooks already installed
zmbcore@1.0.0 /Users/Jovan/zmb/zmbCORE/Code/zmbCORE
└── git-hooks@1.1.0 

$ npm install git-hooks@1.1.1

> git-hooks@1.1.1 postinstall /Users/Jovan/zmb/zmbCORE/Code/zmbCORE/node_modules/git-hooks
> git-hooks --install

git-hooks already installed
zmbcore@1.0.0 /Users/Jovan/zmb/zmbCORE/Code/zmbCORE
└── git-hooks@1.1.1

You detect if the hooks are already installed and interrupt if that is the case. Could you simply run the uninstall script when you detect an existing installation instead of quitting and then carry on with the installation?

tarmolov commented 8 years ago

What version of nodejs and npm do you use?

TitanNano commented 8 years ago
tarmolov commented 8 years ago

Looks like a npm bug. https://github.com/npm/npm/issues/13963

getsnoopy commented 7 years ago

Yeah, I agree with @TitanNano. What are the issues with just uninstalling and reinstalling the hooks every time in, say, the postinstall script? This would handle the cases of specifying the package name & version to npm install or npm update, but also when you do a plain npm install / npm update in the directory.

tarmolov commented 7 years ago

All breaking changes were reverted.