Install into bare repos.

[gabyx]

Some initial fixes to allow bare repo installs. Tests should be added... @rycus86 Could you do this?

[coveralls]

Pull Request Test Coverage Report for Build 457

• 60 of 66 (90.91%) changed or added relevant lines in 4 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-0.03%) to 87.061%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
base-template.sh	1	2	50.0%
uninstall.sh	8	10	80.0%
install.sh	38	41	92.68%
<!--	Total:	60	66	90.91%	-->

Files with Coverage Reduction	New Missed Lines	%
cli.sh	1	90.22%
<!--	Total:	1		-->

Totals
Change from base Build 455:	-0.03%
Covered Lines:	1736
Relevant Lines:	1994

---

💛 - Coveralls

[gabyx]

Does travis tests not run anymore?

[rycus86]

There's been some issues with Travis yesterday too, it might be a bit borked.

[rycus86]

Oh, here it is: https://travis-ci.org/rycus86/githooks/builds/607703891

[gabyx]

I added two tests. GIT_DIR is again leaking (git version 2.22), I am curious if we should safe guard this. Thats why I needed to do PULL_OUTPUT=$(git --work-tree="$SHARED_ROOT" --git-dir="$SHARED_ROOT/.git" pull 2>&1) This sucks... I did not find which command seems to leak, sometime its arbitrarily...

[gabyx]

Tests almost pass. However the whitespace test fails with the test 110. Somthing with whitespaces in shared repo url (its a local one)... but set_shared_root looks good... and IFS also...?

[rycus86]

Tests almost pass. However the whitespace test fails with the test 110. Somthing with whitespaces in shared repo url (its a local one)... but set_shared_root looks good... and IFS also...?

It's odd. Try quoting the $(..) expression in execute_local_shared_hooks and execute_global_shared_hooks, see if that helps? I'd expect other tests to break too though if that would be the case.

[gabyx]

The following: git hooks shared add "/tmp/test 110/hooks" || exit 1 results in a splitted args $2/$3...? Strange everything is quoted through the chain??

[gabyx]

Ahh fixed, damm -> The sed replacement is the issue. git hooks shared add /tmp/test110/hooks || exit 1 works

[gabyx]

Added a bunch of comments, but the main gist is that I don't understand why the new Git root detection works (if I run the command on my machine, it returns false as expected - but then how would Githooks know it's a Git repo directory?)

See the comments above in the review.

[rycus86]

Thanks! I'll try to have a proper look this weekend.

[gabyx]

OK this push should work now :-)

[gabyx]

Ok, now almost all tests worked, but only the LFS test hanged -> Wiht some investigation I found, its really bad to call git pull and git clone (git hooks update shared) without really resetting the environment -> so the only good and robust solution is env -i git clone . Since a git clone inside cli.sh would run hooks base-template.sh-post-checkout eventually calling git lfs and leaking GIT_DIR variables and probably other variables really affect these commands. So I guarded now these with env -i where happily all test work now. Strange thing...

[gabyx]

The original is_running_in_git_repo_root() already worked everywhere inside a git repo... we can also rename it to something else: Suggestion?

[rycus86]

Thanks for the changes so far! I left some more comments.

I still have concerns about this PR in general: it feels like this might be a pretty niche use-case that most people won't need, or you could only apply it to a very small number of repos where it might make sense - would you agree?

I'm thinking that maybe you'd be better off with a simple hook on bare repos without Githooks in the mix - should we explore how that might look like?

I'm trying to establish 1) what value this PR and Githooks would give you on bare repos, 2) how commonly do you need this on your bare repos, 3) is it worth the complexity? Thanks!

[gabyx]

With my changes now, which are not really much after your review I think the value is the following: Since we would like to use Githooks company-wide, where all its update behavior come in handy, since we can manage hooks at a central place. It would be also very handy to use the exact same behavior on our repositories on the server, of course the update mechanism might be disabled and update is needed to be done manually by git hooks updateor git hooks shared update since people are working in parallel (git push) and I dont know how that works out... But the thing is, git has bare and non-bare repositories, and why should Githooks not support this. Every hook manager really should, most don't, and thats cumbersome, because there is not really much of a difference considering hooks in a bare and non-bare repo. I wouldn't think there is now much more complexity for Githooks. The changes with env -i and stuff are actually strange things which really can happen also without the support for bare repos, they just came up in the test procedure, and made me puzzle and worrying about why the hack -> thinking more deeply about it goes into the direction that "are most git invocations really safe here, or might it do stupid stuff". However, we can always of course copy our own hooks to the repository of course without having githooks installed on the server ;-). The good thing is. When you make new repos on lets say Gitea, in the background the githook manager gets installed directyl from the templates and everything is setup, meaning global shared hooks are run for example and so forth :-). Its really handy I think :-). Thats why I'd think it would be valuable to have this. otherwise, we are back to managing global shared hooks manually on gitea when adding a new repo, Meaning -> People need to contact the admin -> the admin logs into the container, makes the adjustments, with githooks this is not needed anymore :-)

[gabyx]

Or thats basically my motivation behind it :-). If it works out, I will see, so far, we are testing the hooks manager with a few people before we distribute a global company wide install script, which sets up everything nicely for everybody including githooks :-)

[rycus86]

OK, thanks for the explanation! You're right, the latest changes don't seem much and should improve things anyway, so let's do it!

I have a few minor things left plus some unresolved comments, I'll mark those up for you to fix then it should be good to go. 🎉

Please also make sure your final commit message reflects this PR rather than "Fixes". :)

Thanks a lot for your work on this!

[rycus86]

Added a /final suffix to my remaining comments to help you search for them here.

[gabyx]

Hi; =)

• Changed all with 👍
• EXISTING_REPO_ROOT=$(dirname "$EXISTING") # EXISTING = "/root/bla/repo/.git" dirname returns the directory path (parent path)... so its correct. = /root/bla/repo. Thats what the commands should receive...
• if [ -f /dev/tty ]; then is still needed to suppress error output : "file descriptor not found bla bla". Of course it works, but is just strange output when no terminal/console could be allocated...

[gabyx]

Possible to merge? Thx :-)

[rycus86]

Sorry, haven't had a chance to look at this yet, I'll try this week. Thanks for your patience and all the changes! :)

[rycus86]

Sorry again for the delay and thanks for your patience! One of these days I will manage to get to my laptop and review these PRs, not only on my phone...

[gabyx]

Rebased onto master.

[rycus86]

Thanks! I was just looking at this, I think it's good to go. There are a few /final comments you haven't replied yet, can you just double-check them amd reply before I merge? Ignore the one about dirname, you explained and I realized I thought wrong what that does. :)

[rycus86]

Let me know when you finished with this, then I'll get it in master. :tada: On second thought, I don't want to rush you, I'll check tomorrow or whenever you're ready. Thanks!

[gabyx]

Thx. I replied. Maybe check the install_ and uninstall_ function again. Maybe you see some errors I made...

[gabyx]

FYI: This is ready.

[rycus86]

Thanks a lot! I'll try to review it in an hour or so.

[gabyx]

Made the changes, reverted the /dev/tty.

[rycus86]

Thanks a lot for your hard work on this!