rycus86 / githooks

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

Remove single install feature #92

Closed gabyx closed 4 years ago

gabyx commented 4 years ago

This removes the single repo install stuff.

Legacy Fixing

It seems to work fine. Tests should work. Still need to check...

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 592


Changes Missing Coverage Covered Lines Changed/Added Lines %
cli.sh 21 39 53.85%
base-template.sh 36 57 63.16%
install.sh 89 181 49.17%
<!-- Total: 158 289 54.67% -->
Files with Coverage Reduction New Missed Lines %
uninstall.sh 2 84.51%
base-template.sh 6 76.97%
install.sh 8 73.68%
cli.sh 61 82.42%
<!-- Total: 77 -->
Totals Coverage Status
Change from base Build 578: -4.5%
Covered Lines: 1902
Relevant Lines: 2388

💛 - Coveralls
gabyx commented 4 years ago

I dont understand why test-arch.sh is failing on travis. Locally it works. Some permissions strangeness?? @rycus86 Do you have a clue??

rycus86 commented 4 years ago

Hm, weird. Do you run the whole Arch test locally? Seems to be these two are failing:

Failed tests: 
- /var/lib/tests/step-103.sh (1 -- ! Expected shared hooks to be installed. 'ls: cannot access '/root/.githooks/shared': Operation not permitted')
- /var/lib/tests/step-109.sh (1 -- ! Expected only server hooks to be installed (0))

What are those two tests doing? Anything in there you could think of going wrong? Btw, coverage also has two failing tests but different ones. :(

step-042.sh (1 -- ! Could not reset origin/master to trigger update.)

Not sure which one is the other failure unfortunately, kcov eats some of the output I suspect.

gabyx commented 4 years ago

Coverage is a pain in the ass: kcov has some issues (using ubuntu 20.04 with docker) since in later versions >v34. coverage output is always 0, I tried to fix the coverage docker image. so for without any luck. Not even locally the test run with >v33. v33 has some old git I usuppose. But git reset --hard HEAD^ should work (that the test which is failing in coverage). Strange. I try to address arch tests first. I have no idea... :-) Lets see..

rycus86 commented 4 years ago

I tried to update kcov once but gave up, too much has changed between 33 and 34. :(

rycus86 commented 4 years ago

Thanks for the PR @gabyx and for the review @matthijskooijman !

This is pretty massive, so I'll probably only be able to look at it properly over the weekend. I wanted to ask though, can we not break this up into multiple changes so there isn't so much changing at each step, and we could actually properly reason about and review each of those changes? Something like:

  1. Show deprecation warning when running hooks in existing single install repos, also make new single installs fail with a message that it's unsupported
  2. Get the cli/aliased script run from the release checkout
  3. Generate a new template folder to use for init.templateDir and core.hooksPath target somewhere around (maybe inside) the release checkout and rip out the logic to try and find this place during install -- maybe at this point we actually have to have the generated wrappers already so we don't get a dirty checkout, I'm not sure
  4. Use wrappers instead of copies of the base template
  5. All the other things I forgot...
rycus86 commented 4 years ago

Forgot to add: with those steps being one PR each.

gabyx commented 4 years ago

I tried to update kcov once but gave up, too much has changed between 33 and 34. :(

The update is not the problem, the bash execution is :-) I can send the converage.sh docker changes in a gist for you to test...

gabyx commented 4 years ago

Steps 1 is good! but single installs are just repos which have hooks which are potentially (if not searched during update/install) not updated so a deprecation warning inside githooks.sh is for no use. If they get updated, the are now symlinks wrappers.

Steps 2-4 are I think already done. I dont know exactly what you mean with 3 exactly @rycus86 : Be awarem the change look big, but are not. There is not much logic change, its only removing single install repos. We still have:

   .githooks/
      - release/ --> same
      - bin/  --> should be removed, not needed anymore (need to check that)
      - shared/ --> same
     -  templates/ --> all files are the githooks-symlink.sh wrapper here
      - registerd -> registration mechanism (all repos using githooks and not using core.hooksPath are listed here, might be beneficial later on!, its not used anymore yet...)

Ist mostly renaming. (I could have done this maybe, in a seperate PR, but ok here it is)

I know that splitting things up would be nice, but once you looked through the changes you will see that its not really much of a nuisance. :-)

matthijskooijman commented 4 years ago

I think that putting the s/yes/true/ and s/base-template.sh/githooks.sh/ into their own commits would already significantly clean up things. If you did those with a big search/replace, those should be easy: Just go back to master, do each of the search/replaces and commit them separately, then do git reset original_sha . to reset the index to the state the PR is in now, then commit that as a third commit (and run git checkout . to match the working copy with the index again). Then you'll have two commits with the two replaces, leaving one commit with the actual changes.

It might be good to pick out more changes from that last commit, but that is then already easier.

gabyx commented 4 years ago

This is true. I made some greate cleanup right now. I probably need to make stage all regex replaces to a seperate commit, but dont have time right now. Maybe you can go look over the latest changes. It condensed again. Tests alpine, alpine-user work so far.

The legacy fixes still need some work I think.

gabyx commented 4 years ago

Implemented some git logic to detect if we need to apply a legacy fix. This is done rigth after we updated the release clone. This needs a proper review, and we need to test this legacy patch stuff.

Note: ONLY install.sh applies updates (merging of the fetched branch) of the release clone anymore. cli.sh and githooks.sh are only allowed to just fetch changes or newly-clone (in case a release clone is missing -> mostly for tests...)

Note: Warning the user that single installs are not supported anymore if we detect somehow a repo haveing this config set: I dont know what we should do there -> Single installs have their own copied old hooks.

This took me now a lot of time. I am off climbing in the weekend. @rycus86 @matthijskooijman If you could look over it again in the meanteim, would be nice. I try to refactor the regex replaces when I have time.

rycus86 commented 4 years ago

Implemented some git logic to detect if we need to apply a legacy fix. This is done rigth after we updated the release clone. This needs a proper review, and we need to test this legacy patch stuff.

Not really sure how this relates to deprecating/removing single installs. Again, I'd recommend we don't sink any more time into this big PR that combines a few changes together, and instead break it up into smaller pieces so it's easier to reason about code and test changes. Thanks!

gabyx commented 4 years ago

I removed the renaming from the PR. This can be done at last. Once again: removing single installs (including --single option in install.sh) means we need a fixup because:

gabyx commented 4 years ago

@rycus86 : I would love to have simple steps here. But I could not come up with different PR because of the following:

Show deprecation warning when running hooks in existing single install repos, also make new single installs fail with a message that it's unsupported

First: As soon as such a PRs Install script gets fetched (global install), we should make sure that there are no single install repos anymore. We have no way of detecting single installs, they have their own script and will continue to run (with our now updated install dir, which is kind of ugly, so they continue to run based on a newer version of the install folder, of course this will work as we did not change lots of stuff inside, the structure is the same). Since we cannot update these hooks, we can also not show a deprecation warning when running such a hook. Second: We can however show a deprecation warning when ever this PRs install script is fetched an we are inside an old single install repo and do a single install --single. and we would use any command from the cli.h. Any inputs on how to proceed here are welcome I try to make such a PR, from this one first if its feasible, and then add another PR for the stuff of the other points below:

Get the cli/aliased script run from the release checkout

That we could move out in a seperate PR I think, but its a small change and already included in this PR...

Generate a new template folder to use for init.templateDir and core.hooksPath target somewhere around (maybe inside) the release checkout and rip out the logic to try and find this place during install -- maybe at this point we actually have to have the generated wrappers already so we don't get a dirty checkout, I'm not sure Use wrappers instead of copies of the base template

This is already done, in this PR we use the same .githooks/templates folder and just place checked in symlink script githooks-symlink.sh for each hook.

rycus86 commented 4 years ago

Ah yes, good point on the already existing single install repos, we can't change the hooks that are already in there easily. We should fail new installs though with a clear deprecation message, and updates in a single install repo should tell users how to fix it, for example use git hooks config something to mark it as non-single repo (I think we have this already), then retry the update.

If the other changes are small anyway, that would be all the more reason to have them in separate PRs, please. I'm particularly interested in the change to symlinks as a separate PR, and thought a smaller PR for the cli change could also be beneficial to keep it focused and easy to review.