rycus86 / githooks

Githooks: per-repo and global Git hooks with version control
MIT License
381 stars 20 forks source link

Bugfix: Fail on not available shared hook. #22

Closed gabyx closed 5 years ago

gabyx commented 5 years ago

The step-103.sh should be failing. I wrote it because it does not in my setup.

-> Installing shared hooks and then accidentially removing them and making a commit should fail because the hooks are not there.

Solution: Do an update if the hooks are not there, or fail with an error "run git hooks shared update"

gabyx commented 5 years ago

The thing is, a clone of a repo with a .shared file results in

git clone https://git.cyfex.com/cyfex/GitUmstellung.git Test
Cloning into 'Test'...
remote: Enumerating objects: 550, done.
remote: Counting objects: 100% (550/550), done.
remote: Compressing objects: 100% (414/414), done.
remote: Total 550 (delta 306), reused 184 (delta 116) eceiving objects:  67% (369/550), 7.89 MiB
Receiving objects: 100% (550/550), 15.50 MiB | 4.71 MiB/s, done.
Resolving deltas: 100% (306/306), done.
F:/Repositories/Test/.git/hooks/post-checkout: line 425: cd: /c/Users/gabriel.nuetzi/.githooks/shared/*: No such file or directory

Which should not happen, its the same error. The hooks are not there yet and should probably be installed, right?

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 204


Changes Missing Coverage Covered Lines Changed/Added Lines %
cli.sh 12 14 85.71%
uninstall.sh 8 12 66.67%
base-template.sh 20 37 54.05%
<!-- Total: 40 63 63.49% -->
Totals Coverage Status
Change from base Build 200: -0.6%
Covered Lines: 1538
Relevant Lines: 1690

💛 - Coveralls
gabyx commented 5 years ago

There are two things:

I think its really good to have a hard fail, if shared hooks are not there, because shit can happen :-) for users which just clone and then commit, or accidentially messed up something...

What do you think?

gabyx commented 5 years ago

I fixed the following things: execute_shared_hooks:

gabyx commented 5 years ago

Thanks for reviewing this.

gabyx commented 5 years ago

Thanks for merging this. I think its important as it improves usage and safety.

rycus86 commented 5 years ago

Thanks for opening this PR! I'll try to review it as soon as I can this coming week!

rycus86 commented 5 years ago

OK, so I fixed up the tests (thanks again for pointing out the bugs) and it's still passing. Can you please try to tell me again what exactly is the problem? Let's start with a failing test case, then work from there, because this PR made some big changes already, and I'm not clear on why it was needed. Thanks!

rycus86 commented 5 years ago

I've tried your new test step-103 on latest master and it's working fine if you don't expect it to fail where I don't expect it to fail. :) There's some spam on the output about not finding shared hooks, but otherwise everything works as expected, commits are successful and non-existing hooks are skipped. What did you expect?

gabyx commented 5 years ago

See the changes https://github.com/rycus86/githooks/pull/22/commits/e020127fc0c3e8d10782b34b3520e4d6859e0313

gabyx commented 5 years ago

Thx will look at it this eveninh maybe

Von meinem iPhone gesendet

Am 21.07.2019 um 13:01 schrieb Viktor Adam notifications@github.com:

@rycus86 commented on this pull request.

In tests/step-103.sh:

+if [ "$RESULT" = "" ]; then

  • echo "! Expected shared hooks to be installed."
  • exit 1 +fi +git commit -m "Test" || exit 1
  • +# Remove all shared hooks +git hooks shared purge || exit 1

  • +# Clone a new one +echo "Cloning" +cd /tmp || exit 1 +git clone /tmp/test103 test103-clone && cd test103-clone || exit 1

  • +# Remove all shared hooks +git hooks shared purge || exit 1 Can you please also add a test that fails on the double purge with the new config option set to true?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

gabyx commented 5 years ago

Finally all tests good :-)

rycus86 commented 5 years ago

Thanks for this again @gabyx !