rycus86 / githooks

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

Remove single install #112

Closed gabyx closed 4 years ago

gabyx commented 4 years ago

Thats the changeset without any symlinks. Tests seem to work so far.

We need to check if we made all path from older update clear to fail. I hope we did. Here nothing really much changed.

I refactored also in this PR

Note: its good to do this right here, because the registered list only contained non-single installs. And I think the feature to have now all repos registered isnt that bad since, we could potentially act on this list in future updates. I would this feature still in. might be handy ...

gabyx commented 4 years ago

Shall I also move the autoupdate changes out of this PR. Its a bit more, work, hm... Are you able to look over all this renaming, would be greate, otherwise I try to remove it and add it back in in another PR which succeeds this one...? let me know

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 704


Changes Missing Coverage Covered Lines Changed/Added Lines %
install.sh 44 46 95.65%
uninstall.sh 4 6 66.67%
<!-- Total: 53 57 92.98% -->
Files with Coverage Reduction New Missed Lines %
install.sh 1 79.45%
uninstall.sh 1 84.05%
<!-- Total: 2 -->
Totals Coverage Status
Change from base Build 699: -0.4%
Covered Lines: 2071
Relevant Lines: 2559

💛 - Coveralls
rycus86 commented 4 years ago

There are some test failures here, but otherwise the change looks OK, you can leave the config move in this PR if you want.

One question I have is whether we need the legacy transformations in the uninstall script as well? Don't we fetch that with curl? If yes, we'd potentially get a newer version than what you have locally, right?

gabyx commented 4 years ago

nono :-) we never use curl anymore, everything goes over the release clone :-) the legacy stuff should be in the uninstall script as well to prevent dangling old stuff...

gabyx commented 4 years ago

I look at the tests later thanks!

rycus86 commented 4 years ago

nono :-) we never use curl anymore, everything goes over the release clone :-)

Hm, https://github.com/rycus86/githooks/blob/master/README.md#uninstalling uses curl to fetch the uninstall script, does that internally update the release clone now to just delete it?

gabyx commented 4 years ago

nono :-) we never use curl anymore, everything goes over the release clone :-)

Hm, https://github.com/rycus86/githooks/blob/master/README.md#uninstalling uses curl to fetch the uninstall script, does that internally update the release clone now to just delete it?

Ah hm.. we might have some inconsistency here, you are right. One should really call git hooks uninstall and not use the curl method preferably? But we can also check in uninstall.sh if there is a release clone, if true we dispatch to the install script there... other wise we just continue with the script...?

rycus86 commented 4 years ago

nono :-) we never use curl anymore, everything goes over the release clone :-)

Hm, https://github.com/rycus86/githooks/blob/master/README.md#uninstalling uses curl to fetch the uninstall script, does that internally update the release clone now to just delete it?

Ah hm.. we might have some inconsistency here, you are right. One should really call git hooks uninstall and not use the curl method preferably? But we can also check in uninstall.sh if there is a release clone, if true we dispatch to the install script there... other wise we just continue with the script...?

Yeah, I'm not too bothered how it happens tbh as long as it works for old and new installations I guess on a best effort.

gabyx commented 4 years ago

I will take a look at this a new PR. Pushed again, hopefully the tests now work. Forgot to call prepare_target_template_dir always... Good we have tests :-)

rycus86 commented 4 years ago

Looks better, still some core.hooksPath issues it seems?

gabyx commented 4 years ago

Jeah test-067.sh was the problem. I thought I can remove

# Using core.hooksPath implies it applies to all repo's
    if is_single_repo_install && use_core_hookspath; then
        echo "! Cannot use --single and --use-core-hookspath together" >&2
        return 1
    fi

which was wrong...

gabyx commented 4 years ago

Question: Shouldn't all .sh scripts inside tests be executable. Why is no .sh script executable in the repo?

rycus86 commented 4 years ago

Question: Shouldn't all .sh scripts inside tests be executable. Why is no .sh script executable in the repo?

I always call them with sh <something>.sh so I never bothered to set them I think. The actual steps should also not run by themselves, only through the test runner I guess.

Is it causing any problems for you or just curious?

gabyx commented 4 years ago

Nono, I was just thinking, ah ok, tough the reasoning is all scripts which should not be executable by them self shouldn't be executable.

Maybe make all tests-*.sh scripts executable. though I dont have to write all ways sh ... :)?

rycus86 commented 4 years ago

Yeah, put up a PR if you have strong feelings about those test runners. :)

rycus86 commented 4 years ago

Looks like Travis is happy, let me try to review this then I'll merge it. 🎉

gabyx commented 4 years ago

Thx!

rycus86 commented 4 years ago

OK, nothing serious, added a few comments for you to look at, please just open another PR if you think there's something to do with them. Merging this one now.