Feature: Clone a release of githooks into the install folder

[gabyx]

Ok, this feature simplifies the githooks implementation a lot, by only using git, no curl/wget.

When installing with install.sh [--release-clone-url="..."] we clone a githooks version into $INSTALL_DIR/release which we use for further updates. These changes are backward compatible, (all tests work) and has the following major benefits:

• no more curl/wget, only git
• less tests
• no self-contained install - not neccessary anmore
• less commit fiddling (development hooks)
• download tool removed - not necessary anymore
• all curl/wget tests removed - not necessary anymore
• $INSTALL_DIR/bin/cli.sh dispatches to $INSTALL_DIR/release/cli.sh

Everything works as is.

[coveralls]

Pull Request Test Coverage Report for Build 566

• 129 of 235 (54.89%) changed or added relevant lines in 4 files are covered.
• 65 unchanged lines in 4 files lost coverage.
• Overall coverage decreased (-5.3%) to 81.826%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
uninstall.sh	5	8	62.5%
install.sh	48	74	64.86%
base-template.sh	33	62	53.23%
cli.sh	43	91	47.25%
<!--	Total:	129	235	54.89%	-->

Files with Coverage Reduction	New Missed Lines	%
install.sh	1	83.96%
uninstall.sh	1	84.0%
base-template.sh	2	75.61%
cli.sh	61	82.35%
<!--	Total:	65		-->

Totals
Change from base Build 562:	-5.3%
Covered Lines:	1963
Relevant Lines:	2399

---

💛 - Coveralls

[gabyx]

@rycus86 Could you please give this (IMO beneficial improvements (its now like a homebrew update install :-P) a review. Dont know why coverage decrease, as it shouldn't as we took out lots of functionality which makes it simpler == better. I am currently still fixing some travis stuff. Implementation is finished.

[matthijskooijman]

Thanks for picking this up! I am not familiar enough with the code (and don't have enough time now) to do a thorough review, but I do have some comments.

First, responding to https://github.com/rycus86/githooks/issues/83#issuecomment-615943130, which I think applies to this implementation:

so install.sh would only basically clone this repo from a branch releases or s)

Oh, that's also an interesting thought, might even simplify things even more (but might require a working git client, or at least a working unzip if you let install.sh download the zip from github).

base_template.sh gets copied for each hook into the git repo or it does not if we have set a core.hooksPath and we installed to use the core.hooksPath mode. Here might be room for improvement, by not copying the whole file, but this is another story...

I think using symlinks, or a one-liner script that execs the original base-template.sh would be a good improvement here. That would also mean that updates are automatic, solving:

We bookkeep a "cat ~/.githooks/autoupdate/registered" list with all repos (which are not single installs) which will be updated.

(except when a new hook is added, or the clone is moved, then you would need to update all repos)

Then, responding to this PR:

no more curl/wget, only git

If you really intend to not use curl/wget but only git, that means the user must start with a git clone. So then why would the install script then do another clone (AFAIU it does)? I can imagine it does not and you always install using git, or you (also) support a curl/wget install where install.sh does do a git clone?

Looking at the changes, I see no addition of symlinks/single-line shims for each available hook, so I assume this means install.sh creates them? I would suggest adding them to the repo directly, so you can do a manual install by pointing the template or global hooks dir directly into this repo, without having to call install.sh?

[rycus86]

To add to this, the whole project started as a script to set up your git template dir, so when creating new clones, they would automatically have the smarts to run hooks that you check into your repo. Plus it had to find existing repos on installing githooks, so if you already checked out a repo, hooks inside could start working. This is in line with how Git template directories work, and it's the reason why not everything is symlinked to a single set of files.

Another option that was added later is to support git core.hooksPath which does keep one set of files, so that's an option.

Getting the initial template and install script files from this repo via git clone is pretty much the same as getting it with curl/wget, so I'm not sure why this would need to change now. Having said that, I kind of like the updates via git pull, so perhaps there's that bit to improve.

[gabyx]

@matthijskooijman Your comments are for sure valuable, but as rycus said, the project has much more subtleties than from the first sight. As fas as "to be in controll" concerns: I think its not that opaque. The whole stuff is managed inside the install folder e.g. ~/.githooks. The install.sh script also servers another purpose of only just cloneing a release, it manages other repos and also other already in place hooks as @rycus86 already mentioned and in the future (hopefully not) its possible to have backward compatibility stuff there too... so if we have an uninstall script which cleans git config variablesgithooks.xxx we definitely have a install script... and also this install script is the ground truth for updates too (and a handy way for inexperienced users just to set up everything under the install folder), in this PR we pull the release and execute it. @matthijskooijman: The procedure is more that you get the install.sh script (zip file, git clone if you like..., curl/wget) and then everything gets setup (git clone again, which might be counter intuitive...). However you have git installed and otherwise there is no reason to install this project at all! ;)

Also I am in agreement, that one should not really change anything which githooks does, if it does something wrong -> Bugfix -> add a test -> PR. Glad we have good test coverage from @rycus86

For this PR, I keep everthing which is not really important right now out of the discussion, which we rather leave for further improvement (which might really benefit, I mean @matthijskooijman tought about only having only a git repo (no install.sh) would be nice, but I am not sure if that gonna work out....

[gabyx]

@rycus86 : I will try to fix the travis stuff tonight. More or less the PR should work =) P.S Hehe: We can also clone differently I ve just noticed : --single --branch "master" will incorporate that...

[gabyx]

All tests pass now. Review should be good to go :-) Thx @rycus86 .

[rycus86]

All tests pass now. Review should be good to go :-) Thx @rycus86 .

Thanks @gabyx ! I'll need to check out the code to properly review this change, but from a glance on my mobile, it looks all right. 🎉 I'll try to get it reviewed this weekend.

A question on updating to this from the "old" way, how's that work for people? Will it just switch over seamlessly?

[gabyx]

I think, it should switch seamlessly: The scripts (cli.sh, hooks) get the install.sh from the github, then it will create the clone if its not there, I think in any cirumstance. I will check this path...

[gabyx]

I think, it should switch seamlessly: The scripts (cli.sh, hooks) get the install.sh from the github, then it will create the clone if its not there, I think in any cirumstance. I will check this path...

Checked: Should work :crossed_fingers:

[rycus86]

Thanks a lot for your work on this @gabyx ! I've added a bunch of comments, let me know what do you think! Thanks!

[gabyx]

Please check the latest commit after which I can squash everything together, some rename and inclusion of some cli.sh funtionality.

[gabyx]

One unresovled issue left, then we would be able to merge this :-) which I am looking forward too =). Note: I would need to change the cli.sh dispathc in /bin to a copy still, this is better behavior for now...

@rycus86 : A suggestion: Would you welcome a PR (after this one) which basically gathers general functionality into general.sh which we can either source in cli.sh and also base-template.sh or directly include into (as before). In that way we would greatly improve the code basis since a lot of functionality is quadruplet :-) (install, base-template, uninstall. cli) etc... What do you think? With this PR things will get easier for such an approach

[rycus86]

OK, thanks for the ping! I'll have a final pass over this in a bit, then it should be good to go.

Would you welcome a PR (after this one) which basically gathers general functionality into general.sh which we can either source in cli.sh and also base-template.sh or directly include into (as before). In that way we would greatly improve the code basis since a lot of functionality is quadruplet :-) (install, base-template, uninstall. cli) etc...

Yep, agree. Now that we know that we have all the files from the repo locally, we should be able to include functions from those known script files, so yeah, totally open to it.

[matthijskooijman]

I haven't reviewed this PR in detail, but there are two things I originally suggested in #83 that are not part of this PR yet. For ease of review, they are probably best done in a separate PR after this one, though.

1. install.sh still copies the full base-template.sh content into the individual hooks. This leads to a lot of duplication of code (once per hook for a global template or global hook dir, once per hook per repo when installing into repositories) and makes it more work to update hooks. I would suggest installing not a copy of base-template.sh, but a symlink or tiny wrapper, so that there is always only one actual copy of base-template.sh present (which should maybe then also be renamed, since it will not longer be a template).
   2. Individual hooks are still created by install.sh, rather than being pre-created in the repository. If they would be pre-created, installation could (also) be done without install.sh by simply setting the git global hooks directory to the directory inside the repo with the hooks (and combined with 1, this needs no real code duplication other than a single-line wrapper maybe).

Since symlinks are probably not portable to Windows, a one-line wrapper is probably better. For implementing that, there's two challenges:

• Finding out where base-template.sh lives. For pre-created hook wrappers, this could probably do something like DIR=$(cd $(basename $0) && pwd) which is a common trick to get the absolute path to the current script even when symlinks or relatived paths are involved. The path to base-template.sh can then be derived from the hook dir. When installing the wrappers into the actual repository hook dirs, this won't work because base-template.sh lives somewhere else completely. In that case, you could hardcode the path in the wrapper script when copying it in install.sh, or maybe the wrapper could just read the install path from a git config variable (I suspect this is already set by install.sh?). The latter has the extra advantage that moving the git checkout is possibly without having to reprocess all git repos.
• Executing base-template.sh without changing $0 (so base-template still knows which hook was called). I think this can be done with plain sh using the -c option. E.g. exec sh -c /path/to/base-template.sh "$0" "$@". Here, -c specifies the command to execute, while the arguments after that specify $0, $1, etc. Haven't tested this yet, though.

[rycus86]

Thanks @matthijskooijman ! I agree these should be looked at in separate PR(s).

1. This is consistent with Git template dirs, which is the original way of Githooks was. The easier updates is an interesting aspect, I'd consider this for that only really, I'm not really convinced the duplication is necessarily bad here.

2. Isn't this essentially the same as the core.hooksPath installation method we already have?

[matthijskooijman]

2. Isn't this essentially the same as the core.hooksPath installation method we already have?

No, since for that installation method, install.sh still has to generate a directory with all the hooks (with copies or wrappers of base-template.sh). What I'm suggesting is to pre-generate this directory and commit it (if they're thing wrappers, you'll have some code duplication but only a single-line script that should pretty much never need to be changed). With such a pregenerated, committed directory, you can just set core.hooksPath manually, without ever needing install.sh.

[rycus86]

What's the benefit of that? I'm not sure I see the problem you're trying to solve?

[matthijskooijman]

The "problem" of having to run an install script. If the hooks would be pregenerated, you could (optionally) install githooks by just cloning and setting a git config hook, without needing the install script at all.

Also, it would mean that there is no (generated) code anywhere which is not tracked by git. It seems unlikely that these tiny wrappers would need to be modified, but when they are git tracked, you can at least be sure (and if they do need a fix, updating them is just a matter of a git pull).

See also https://github.com/rycus86/githooks/issues/83#issuecomment-616623562

[rycus86]

Yep, as mentioned there, for your use-case, the best solution would probably be what you described in https://github.com/rycus86/githooks/issues/83#issuecomment-614800948 - I don't really think though that your use-case is so common that it warrants a new installation method.

The whole promise of this project is that you run a single-liner install script, and you're ready to go. If that is not what you desire, it can be worked around it as you mentioned. I'm concerned about trying to support all the installation methods we can think of as some of them would work for some, and some for some others, but it could get confusing. If all you need to get where you want to be is to have those hook wrappers in a single directory wrapping a call to base-template.sh, we can probably get that added with a little blurb in the README. If you open a PR, we can perhaps discuss it more effectively?

[matthijskooijman]

I don't really think though that your use-case is so common that it warrants a new installation method.

Yeah, I can see that. Though I guess this same pre-generated directory could also be used by the install script in the "global template dir" or "global hooks dir" scenarios (just not in the "install into repo hooks dir" scenario, because then the path to base-template.sh would be different).

we can probably get that added with a little blurb in the README. If you open a PR, we can perhaps discuss it more effectively?

That's fair, I'll see if I can find some time for that later. Regardless, I think that fixing 1. of https://github.com/rycus86/githooks/pull/86#issuecomment-619825822 would be separate from this (and probably best done before trying to fix or document 2.)?

[rycus86]

I don't really think though that your use-case is so common that it warrants a new installation method.

Yeah, I can see that. Though I guess this same pre-generated directory could also be used by the install script in the "global template dir" or "global hooks dir" scenarios (just not in the "install into repo hooks dir" scenario, because then the path to base-template.sh would be different).

For the existing installation methods, I'm happy with the copying those files around, pointing them back to the base-template.sh with an absolute path potentially.

we can probably get that added with a little blurb in the README. If you open a PR, we can perhaps discuss it more effectively?

That's fair, I'll see if I can find some time for that later. Regardless, I think that fixing 1. of #86 (comment) would be separate from this (and probably best done before trying to fix or document 2.)?

Yep, I agree.

[rycus86]

Thanks a lot again @gabyx for your work on this! :tada:

[gabyx]

Ok, I try to wrap up things here as it got a bit complicated :-):

@matthijskooijman Would like to essentially place wrappers (simulating symlinks) (of course by using git config githooks.installDir) into the hooks forlder. That would be nice, but has at the moment some problems with

• --single installs. These installs need a full-blown hooks script inside the hooks folder and not a wrapper (because they never get updated or only manually, --single is like "detached" from updates)
• using these wrappers gets a little bit messy, as we really need to distinguish between wrappers or full-blown scripts... (note: cli.sh and other stuff), I think its possible to handle both cases without any hassle, but thats only my guess...
• forgetting --single installs that would make things easier, but there is still the update disable feature, such that you do not update the hooks inside a repo, so wrappers are implicity updated which we dont want...

So with the current features (leaving out the discussion about their usefulness) its kind of troublesome. However I agree that having pregenerated wrapper hooks checked in would be lovely... but I think copying stuff isn't that of a problem (ok, we have duplicates for each hook script, which is a bit stupid....): But we could maybe do this:

Method 1

pregenerated hook wrapper (e.g pre-commit)

DIR=$(cd $(basename $0) && pwd)
if is_not_single_install && is_update_disabled; then
# dispatch to
  $INSTALL_DIR=$(git config githooks.installDir)
  $INSTALL_DIR/release/base-template.sh ...
else
  # use template next to us...
  $DIR/githooks/base-template.sh  # LINE A (might source other files inside $DIR/githooks/)
fi

So we have a githooks.sh (base-template.sh) in the hooks folder by default for any install (no matter what) this is the only script which gets updated during update. Any non-single-install repo and non-update-disabled repo will just use the release folder which will exist.

A future PR, I mentioned, about sourcing general functionality is also not so easy because of LINE A above. So we would need to place all needed scripts also into the hooks folder. In that case this means we have a set of files base-template.sh, etc... which we could deploy to .git/hooks/githooks folder. If we would do that for every install into a repo, no matter what even if we dont use them and use the $INSTALL_DIR, we do not really gain anything (IMO) since these files .git/hooks/githooks/* need to be updated anyhow. So copying the base-template.sh for every hooks is not so far of the galaxy.

I think a valable solution would be to have the wrappers only supporting this:

Method 2

pregenerated hook wrapper (e.g pre-commit)

  DIR=$(cd $(basename $0) && pwd)
  $DIR/githooks/base-template.sh

Then I would replace the name base-template.sh with just githooks.sh which is the main entry script for the manager... I only think of such a PR for gathering shared functionality into scripts... @rycus86 What do you think?

Annotation 1: I just tried to gather functionality in general.sh and it filled 157 lines only and the changes with sourcing are a pain in the ass :-):

• when we dont have the release folder we are not allowed to use not yet sourced functions (sad... ) install.sh as a "clone-before-running-install.sh` feature up to L43 ...

I think "sourcing" it is not worth the effort. Injecting directly on commit might be worth it, but that is now postponed on my list...

[matthijskooijman]

Single / disabled auto-update

--single installs. These installs need a full-blown hooks script inside the hooks folder and not a wrapper (because they never get updated or only manually, --single is like "detached" from updates)

AFAIU, single is essentially the same as auto-update disabled, but additionally prevents the templates from being installed for future repos, right? For the purpose of this discussion, I think single and auto-update are essentially the same thing.

For new repos, disabled updates would be a matter of just copying base-template.sh into the git repo instead and using that local copy from the wrappers rather than the globally installed base-template.sh. In the future, this might also include other files that base-template.sh depends on.

For existing repos, things get a bit more complicated. Disabling automatic updates would mean retro-actively copying in base-template.sh (et al) into the git repo and updating the wrapper scripts to use it (which can happen through a git config variable).

Needed at all?

I do wonder: Do we really need disabled auto-update at all? I can't quickly come up with any specific usescases for this. I can imagine that if updating is an explicit action, tracking repos and copying files around to them, you might want to not do that automatically (to stay in control). Also, because disabling auto-update is fairly easy to implement without adding complexity, that would make sense. But is it really needed?

Note that a single install, e.g. installing just some scripts into a single repo without making any git clone of this githooks repository elsewhere, I can imagine that that would still be supported, but that can just copy the needed files (either through a temporary clone, or maybe directly from git).

Method 1

If we would do that for every install into a repo, no matter what even if we dont use them and use the $INSTALL_DIR, we do not really gain anything (IMO) since these files .git/hooks/githooks/* need to be updated anyhow.

Yeah, if you want to keep supporting update-disabled exactly as now, by flipping a git config var, then you would indeed need to update these files even when unused, which is pointless and probably even more confusing for users.

Method 2

So let me check if I got the idea right:

• You have pre-generated wrapers that live in the githooks repo
• For core.hooksPath installs, git would just be pointed at that directory, so updating is just a matter of git pull.
• For in-repository installs, the entire hooks directory (wrappers and base-template.sh) can just be copied into the target git repository.
• How would templating work? I originally thought that the template dir (init.templateDir) could simply be pointed at the pre-generated hooks directory, but looking more closely I see that this is a template for the entire .git directory, not just the hooks. So a user might already have a template directory and you would really need to install into (e.g. copy the pregenerated hooks directory into) that existing directory rather than just point git at the githooks repo.

Method 3 (new)

Method 2 still needs the (I suspect somewhat complicated) auto-update logic (which tracks all git repositories and copies new files into them). I guess my ideal approach would simply not need this anymore at all, where you would either:

• Copy actual code (not just wrappers) into the git repo and not get any automatic updates.
• Copy wrappers into the repo (possibly via the template dir) that reference to githooks repo (possibly via a githooks.installDir config) so always get auto-updated.
• Use core.Hookspath to point at (pre)generated hooks in the githooks repo, so always get auto-updated.

Switching between auto-updates and no auto-updates would be more tricky (essentially need a re-install into the repo, I guess).

Doing this would probably allow greatly simplifying updates (just do a git pull). OTOH, some of the auto-update copying code would still need to be kept around for backward compatibility, you'd still have to update existing repos to the new scheme, so maybe that voids these advantages...

Wrapper script

Regardless of the method chose, I think the wrapper script should use the . operator to start the actual script:

  DIR=$(cd $(basename $0) && pwd)
  . $DIR/githooks/base-template.sh

Without the ., $0 will be set to base-template.sh, losing info about what hook is called. I previously suggested using sh -c for that, but just using . seems easier.

[rycus86]

Yeah, I was thinking of something like Method 3 above. I'd like to see how this works on a PR, as it could get complicated a little bit, with trying to point both the Git template dir and the core.hooksPath to a set of pre-generated files, or pehaps we'd need multiple sets of pre-generated files.

Single install is a bit of a wildcard, I wouldn't worry too much about it. I'd copy the hooks into the single install repo as necessary, not pointing outside of it at all. Then if we want to make it not-single (if that's an option today), then we can re-run the installation as it was suggested above.

Auto-updates are a bit interesting, and I guess I could have thought about this before merging this PR. Previously, it would have checked if we have a new update and ask the user if they want to install it right now. With this PR, we'll always update I guess - do we even have the prompt still @gabyx ? I guess we could always store the active commit hash and hard reset there if the user doesn't want to update just yet. For disabling the auto-update, we can perhaps make that a global only option, then you won't be able to change this per repo, but I think that's probably fine.

For the wrapper, I don't really mind how it works as long as it works on POSIX and Git Bash on Windows.

[rycus86]

Also, should we move this discussion back to #83 instead of this closed PR? 🙂

[rycus86]

With this PR, we'll always update I guess - do we even have the prompt still @gabyx ?

OK, looks like the prompt is still there but we will have updated the local clone already at that point. I guess it's not the end of the world. In the seamless-update mode we're planning, I guess users won't be given a prompt, we just display that we have updated if auto updates are enabled.

[gabyx]

Thx: @matthijskooijman For the summary. I think all your findings are correct, and your feeling about single repo install is the same I have. Its basically the same as disabling the auto update.

Meaning copy everything to the repo which is needed.

So can we agree on something like:

• Make the single install equal to disable update
• Keep the registration mechanism for update-disabled repos (as it would be nice, since it helps the user to see all repos where updates are disabled, and there is a prompt asking if he would like to update also these update-disabled repos (maybe we could) just print these registered repos and -> let the user know "To update them please run git hooks update --only-disabled" which runs through all registered repos and copies the new hooks into them.

So the only change we have is that we basically place wrapper scripts, and when updates are disabled and enabled we do copying or place wrappers,, thats about all I think ... I might have time to make a PR and see how it goes.

[rycus86]

From the README:

There is also an option to run the install script for the repository in the current directory only, without setting up the Git templates for any future repositories. For this, run the command below. ... You can change this setting later with the command line helper tool, running the git hooks config [set|reset] single command, which affects how future updates are run, when started from the local repository.

So yeah, it's mostly about disabling update, but also not installing anything outside of the current repo you're installing into.

To be honest, if we're going to do wrappers pointing back to a central checkout of the base template, I'd say let's just deprecate single install very quickly, then let's break it when the wrappers are ready to go. It will be a nightmare having to maintain it and switch back and forth between single and regular installs too. If you can think of a way to keep it working and not be a burden, then we can talk about it though.

For the same reason, I would remove the repo tracking as well, just have a global switch for updates or no updates, then at an actual update we just do a git pull of the target branch and that's it (possibly some post-install script in case we do need to set something up).

How does this sound to you both?

[gabyx]

Oh, jeah, I tried to keep it working in my test changes and its really a nightmare (also to maintain and understand the logic. I think deprecating this feature is the best option.

Thanks for your suggestion, it is very much in line with my thoughts. Its probably the best, and things get easier! I will try this tonight. Will simplyfy a lot I think. Only having a global update is better.

[rycus86]

All right, looking forward to the PR!

We should also get the deprecation started, would be good to have some warning showing up for single install repos for at least a week before it disappears, though not super certain that's really that much time... At the same time, no idea if people do actually use single install or not, having no analytics on things. :(

I'll see if I can get a change in this week to add a warning message.

[matthijskooijman]

Yeah, simple sounds good :-D

I guess you would still have three flavours:

1. Install wrappers into separate repositories
2. Install wrappers into a template directory
3. Install wrappers into a global hooks directory (or point the global hooks directory at a pregenerated global hooks directory in the repo, as discussed before).

All of these would point to the same base script that lives in a git checkout, so all of these update in the same way. Right?

As for migration - I guess you could maybe do a one-off conversion of all repos to the new wrapper-based install based on the existing repo tracking (maybe ask for confirmation first and cancel the update otherwise)?

[gabyx]

See #92