Feature: Symlink base-template.sh

[gabyx]

Symlink base-template.sh

as a Bonus:

• hooks folder with all generated hooks for simple installing (cloning and setting core.hooksPath)

[coveralls]

Pull Request Test Coverage Report for Build 722

• 14 of 23 (60.87%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+1.2%) to 81.257%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
base-template.sh	7	9	77.78%
install.sh	4	6	66.67%
base-template-wrapper.sh	3	8	37.5%
<!--	Total:	14	23	60.87%	-->

Totals
Change from base Build 710:	1.2%
Covered Lines:	2133
Relevant Lines:	2625

---

💛 - Coveralls

[rycus86]

Looking forward to this! Can we please make sure we don't also put symlinks and other changes in this PR?

[gabyx]

Jeah, good point, I try to remove the symlinks first. Will make things easier also for me

[gabyx]

Lets see what we can do now with the symlinks, that PR we have to discuss thoroughly. Lets see if it works...

[gabyx]

So far we get some troubles with the symlinks (?)

trace: exec: git-lfs post-checkout /root/.githooks /tmp/test103-clone/.git/hooks/post-checkout 0000000000000000000000000000000000000000 674229b3a048ab86f8486367a7f7a5942974aa5b 1
14:02:57.051695 run-command.c:663       trace: run_command: git-lfs post-checkout /root/.githooks /tmp/test103-clone/.git/hooks/post-checkout 0000000000000000000000000000000000000000 674229b3a048ab86f8486367a7f7a5942974aa5b 1
This should be run through Git's post-checkout hook.  Run `git lfs update` to install it.

[gabyx]

Ah found it. Forgot to shift arguments

[gabyx]

Now tests should all pass.

[gabyx]

@rycus86 : The problem with the tests are the following (cant shift to many): If we want to go down the road of having wrappers placed, we have the adjusted the arguments to base-template.sh (rather naming this at the end githooks.sh) We should rewrite all tests (might get the PR to overboard, but its the only solution) from

HOOK_NAME=post-merge HOOK_FOLDER=$(pwd)/.git/hooks \
        sh /var/lib/githooks/base-template.sh "" "" unused

to

sh /var/lib/githooks/base-template.sh $(pwd)/.git/hooks/post-merge "" unused 

or directly calling the

INSTALL_DIR="..."  HOOK_NAME=post-merge HOOK_FOLDER=$(pwd)/.git/hooks
    sh base-template-symlink.sh unused

It gets nasty, but doable...

@rycus86 What do you think?

[rycus86]

I'll have a look later today, can you please hold off until then? I need to understand the problem better first. Thanks!

[gabyx]

Jeah for sure!, Just look at the tests, alpine works, debain doesnt... etc

[rycus86]

A quick Googling says we could just fix the shift rather than rewriting all the tests: https://superuser.com/a/897149

Haven't looked properly yet though, but could be something to look into in the meantime.

[rycus86]

Also, I suggest not using the name "symlink" for this new file. An actual "symlink" is a different thing on Unix, which could have been used instead of the wrapper/stub that you're using now, but for portability reasons (i.e. Windows has no perfect equivalent of symlinks) we're not doing that. Calling this file base-template-symlink.sh would likely confuse people into thinking symlinks were somehow used.

I agree, symlink might be a bit misleading.

Also, I would suggest renaming base-template.sh, since now it stays in the git repository and is not copied anywhere, it's not really a template any more.

I'm not too fussed about this, it is still a thing other things refer to. If you find a name that describes it really well, let's change it, otherwise I'd leave it. 🤷‍♂️

How about hook-runner.sh instead of base-template.sh (since that's what it does, it runs hooks when called)?

It does run hooks, besides other things. 🙂 Not really happy with that name, but I'm also not sure I have anything better at the moment. Keep the ideas coming! 👍

Then for the "symlink", I was thinking of something like hook-stub.sh, hook-shim.sh or hook-wrapper.sh, but looking at the proper meaning of stub, shim and wrapper, none of them seem a perfect fit. Maybe the symlink could be hook-template.sh, since this is now the script that's copied into the different places (even though it's copied verbatim, not really changing anything like you would expect with a template, other than the name of the file maybe).

Also consider dispatch, though I think wrapper could also work as @matthijskooijman suggested.

[gabyx]

All good comment, just let you know, I want have lots of time in the next 4 Weeks, unfortunately. If anybody wants to make further progress, please go on!. I had some troubles with the tests which made me stumble upon other stuff. I will read your comments later! Thanks for the review.

[rycus86]

All good comment, just let you know, I want have lots of time in the next 4 Weeks, unfortunately. If anybody wants to make further progress, please go on!. I had some troubles with the tests which made me stumble upon other stuff. I will read your comments later!

No worries @gabyx , thanks a lot for your hard work on these PRs! I'll see of I can have a look at them while you're away. 👍

[gabyx]

Some update: I finally continued a bit. Had some changes left in the worktree, Figured out how to do adjust the tests now. All tests pass now. Lets see what travis says

See last commit, for review changes.

• I added githooks.runner which is a good point. Makes things easier.
• The manual execution now all execute through the base-template-wrapper.sh (later to be renamed!) to test the whole chain.
• Though, I dont like the direct execution tests, since this makes tests much harder to correct/predict since we use sed replacements and change the code. So in the future it would be somehow better to rewrite them to just use a proper install like the other tests (would there be a problem with that)

@rycus86 Could you have look at this. This needs some serious review I think. I will also test whats gonna happen (:bomb: ) when we update to this code and such...

[rycus86]

Thanks a lot for picking this up again @gabyx ! I'll try to find some time later to review your latest changes.

[gabyx]

Tests now all pass, had again to adjust the docker copy command

[gabyx]

Check the new pregenerated hooks, since we now rely only on GITHOOKS_RUNNER, they seem ok.

• I am only a bit worried about $0 is this alawys an absolut path or also relative? We had a discussion on this I think @matthijskooijman ?

[matthijskooijman]

I am only a bit worried about $0 is this alawys an absolut path or also relative? We had a discussion on this I think @matthijskooijman ?

It depends on how the script is executed. In general, it should either be absolute, or relative to the current directory. I wouldn't be suprised if git makes sure it is absolute always, but if you want to ensure it is absolute, a common approach is something like:

HOOK_FOLDER="$(cd "$(dirname "$1")" && pwd)"

So change to the folder you're interested in and use pwd to print the full path, all in a subshell so the current shell's working dir does not change.

[gabyx]

Incoporated input from @matthijskooijman.

[matthijskooijman]

Looks good, thanks!

[gabyx]

@rycus86 Any progress ;-)?

[rycus86]

Sorry, I'm super behind on this. 😔 Apologies for the delay, I'll try to get around to reviewing and merging this PR this week, I see that it already has green builds and had a few reviews from @matthijskooijman Thanks both!

[rycus86]

Overall LGTM. :+1: Just left a couple of comments if you could look at @gabyx

[rycus86]

Awesome stuff, thanks a lot @gabyx ! :tada: