Refactor shared hooks settings

[gabyx]

Based on discussion in #122. Based on #119.

[coveralls]

Pull Request Test Coverage Report for Build 806

• 388 of 537 (72.25%) changed or added relevant lines in 4 files are covered.
• 6 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-2.08%) to 80.671%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
base-template.sh	113	120	94.17%
cli.sh	205	243	84.36%
install.sh	66	170	38.82%
<!--	Total:	388	537	72.25%	-->

Files with Coverage Reduction	New Missed Lines	%
uninstall.sh	1	86.75%
install.sh	2	68.32%
cli.sh	3	84.6%
<!--	Total:	6		-->

Totals
Change from base Build 751:	-2.08%
Covered Lines:	2358
Relevant Lines:	2923

---

💛 - Coveralls

[gabyx]

@rycus86 Its kind of difficult and to much work I have time for right now to rebase this changes onto master, and then again resolve all merge conflicts again... Can we make a dev branch. and merge the revised #119 (improve legacy transform as discussed, also its not yet tested) into the dev first. And then we merge, #125 . And afterwards I will test and improve again. on the dev branch. Until we merge both changes back. Is that a plan?

[rycus86]

@rycus86 Its kind of difficult and to much work I have time for right now to rebase this changes onto master, and then again resolve all merge conflicts again... Can we make a dev branch. and merge the revised #119 (improve legacy transform as discussed, also its not yet tested) into the dev first. And then we merge, #125 . And afterwards I will test and improve again. on the dev branch. Until we merge both changes back. Is that a plan?

Yeah, if it's too hard to pull out those changes, let's merge them, it's just going to be a bit harder to review. Can you merge all you changes into #125 (or #119) instead of creating a new branch on this side?

[gabyx]

Jeah I can do that! thanks! I merge into #125

[gabyx]

Lets first dicuss all these changes, then we can discuss the legacy transform, it maybe needs a test (more work ;-))

[gabyx]

@rycus86 : All tests pass. Thanks for the review.

[gabyx]

@rycus86 If you find some time reviewing this on the weekend would be great, so I can improve it next week. I wont have time on the weekend, :-)

[rycus86]

https://github.com/rycus86/githooks/pull/125#issuecomment-694373103

Sure, let me try to make sure it's reviewed for you by the end of this week. Thanks a lot!

[rycus86]

Added a bunch of comments. The main change looks OK to me, just please address the comments. Thanks a lot!

[gabyx]

Added everthing of your input to the last commit. Thx :-) Should be mergable if tests succeed. I test first the update legacy transform again. and give you an OK.

[gabyx]

@matthijskooijman: Thanks for the review! I was thinking that as well -> git config supports multiple entries You are right, it would be nice to have .shared as a list of git config values. I try to append a change to this review. As we already do soem migration (which I tested) we can actually pretty simply add this feature I think.

[gabyx]

@matthijskooijman, @rycus86 : The slight problem we have with legacy_transformations_split_global_shared_entries is that we run this always. Because up to this PR (unfortunately) we dont have the last commit SHA which we would need to check if the update should trigger this transform (meaning we update from a too old githooks)...

However, 2 things:

• we might decide e.g. in a year or so, we anyway drop all these legacy_transforms by making a v2 version by deprecating master and making a new master-v2 or somthing like this...
• Bevor this PR: commas in URLS are really rare, and we anyway based our code on URLs not having commas therefore, it should be fine to leave this legacy transform in for a while...

[matthijskooijman]

Because up to this PR (unfortunately) we dont have the last commit SHA which we would need to check if the update should trigger this transform (meaning we update from a too old githooks)...

But you could still run this migration when no --upgrade-from-commit SHA (or whatever the option is called exactly) option is given, right? IOW, if the option is not given, just set the "from commit" variable to the SHA of the current master (before this PR).

[gabyx]

Ah jeah, good catch

[gabyx]

@matthijskooijman, @rycus86 : Can you quickly look at legacy_transformations_after_update in the last commit. This should set flags to trigger legacy transforms when ever the last commit < feature commit (this PR) and feature commit <= current commit. Does it look right, IMO it does :see_no_evil:

[matthijskooijman]

One more thought: There is a transformation that runs on a repo now (transform local .githooks/.shared into config), but when is that ran exactly? It seems only when installing into repo, or is that also ran periodically or when updating on all registered repos? I guess maybe this particular transformation only needs to run when there are local urls in .githooks/.shared, which triggers an error message which I suspect prompts the user to reinstall into that repo to transform?

An alternative could be to run such repo-specific transformations whenever you work with a repo (maybe even from within a git hook)? To facilitate that, I think that it might be useful to store the "last transformed" hash in each repo as a config directive (e.g. the githooks version that was current the last time transformations were ran (or considered to be ran)), which could serve the same function as the GITHOOKS_CLONE_UPDATED_FROM_COMMIT variable for global transformations?

[gabyx]

An alternative could be to run such repo-specific transformations whenever you work with a repo

Hm, I rather have not anything in base-template.sh or the cli.sh regarding legacy transformation. If we get around that otherwise, I like to keep it that way. cli.sh and base-template.sh should in all circumenstance kept clean from any strange transformation checks, legacy warnings and stuff if possible. Legacy transformation and migration should normally anyway be kept close together as possible in install.sh, to keep it clean...

You are correct: The autoupdate tirggered by base-template will install into registered repos and the legacy transform for .githooks/.shared kicks in (if the update includes that feature) and we only move local urls (if any detected) to --local githooks.shared. I think thats reasonable.

[gabyx]

• Shouldn't all transformations be made conditional on the commit upgraded from? Now there are a bunch of transformations which re-ran on every upgrade (not a problem per se, since they mostly just unset old and unused values, but now that there is a good mechanism to run them only once, might as well use it?)

You are correct! -> I left them unconditional because it wont really hurt and to much work for the small benefit to use the conditional approach as well.... But for new migrations we should now use the conditional approach, for sure.

• Why does legacy_transformations_start exist? Why not run all transformations after update? In practice, the "start" transformations that are new, aren't run until after the update anyway (since before they are not present), so having these "start" transformations only means that these transformations are ran twice on every future update too (once before the update, once after)?

Your concerns are duly noted: I looked at this again, and wanted to move all in this function into transformations _after_ update. But actually I think it had a prupose: We needed these transformations exactly there, because the later code (update_release_clone) will use new values directly (githooks.cloneDir) etc... These transformations do not hurt but you are right, its a bit ugly... Maybe there is a way to move it. But I think it needs to stay there...

[matthijskooijman]

Hm, I rather have not anything in base-template.sh or the cli.sh regarding legacy transformation.

Ah, good point. Fine like this, then.

We needed these transformations exactly there, because the later code (update_release_clone) will use new values directly (githooks.cloneDir) etc...

But if these transformations run before the update, they will already have ran after the previous update, so the new values are already available? Maybe there was some corner case here when this new post-install stuff was introduced, but I can't quite think of any.

I left them unconditional because it wont really hurt and to much work for the small benefit to use the conditional approach as well....

We can always do that later still. It would clean up things and make it more clear how transformations are supposed to work, though.

One more thing I forgot to mention before: Now you check the commit updated from and set e.g. LEGACY_TRANSFORM_SPLIT_GLOBAL_SHARED_ENTRIES based on that. Then, inside e.g. legacy_transformations_split_global_shared_entries you check for that variable and do nothing if it is not set.

I would suggest:

• Moving the check whether to run to outside of the function, so the function always does as advertised, and the calling place makes it more clear which transformations are run under which conditions.
• Maybe even remove the variable completely and (combined with the legacy_transformation_update_includes function you already added) just check the hash before calling the function.

This would result in e.g.:

if legacy_transformation_update_includes ab86d2a529f58744a71e79227e434f19b84589e6; then
    legacy_transformations_split_global_shared_entries
fi

IMHO this is a lot more easy to read. Note that I left out the $INTERNAL_UPDATED_FROM_COMMIT argument, since that can just be used from global scope I believe.

This change does mean that legacy_transformations_after_update does not have much to do anymore, just setting a default for INTERNAL_UPDATED_FROM_COMMIT. However, I would suggest that that default could be better set at the bottom of parse_command_line_arguments (if --internal-postupdate was passed but not --internal-updated-from).

[gabyx]

This change does mean that legacy_transformations_after_update does not have much to do anymore, just setting a default for INTERNAL_UPDATED_FROM_COMMIT. However, I would suggest that that default could be better set at the bottom of parse_command_line_arguments (if --internal-postupdate was passed but not --internal-updated-from).

As said, I try to gather stuff concerning legacy 💩 in denoted functions, just to make sure it does not spoil other more proper code... I need to set LEGACY_TRANSFORM_MOVE_LOCAL_PATHS which gets used later during install_into_repo...

[matthijskooijman]

I need to set LEGACY_TRANSFORM_MOVE_LOCAL_PATHS which gets used later during install_into_repo...

My suggestion was to also remove that variable, and before calling legacy_transformation_adjust_local_paths (or inside it if your prefer) just call legacy_transformation_update_includes directly, just like I suggested for legacy_transformations_split_global_shared_entries. If you do that, then the only thing that's left is setting the default for INTERNAL_UPDATED_FROM_COMMIT, which could then be moved into parse_command_line_arguments where I think it makes more sense.

[rycus86]

Thanks for all the discussion happening here! I haven't had a chance to review the latest changes yet, but wanted to jump in with my 2c here on legacy transformations. Instead of trying to be clever and work out what commit we've come from, can we write the transformations in a way that they check first whether there's anything to do (regardless of version/commit) and bail out if everything is already fine? For example the local path transformation could do a simple grep on that .shared file, and only do the update logic if it contains local paths. This way it doesn't matter what commit you update from, and can run this transformation as many times as you want as it will really only do meaningful changes once.

What do you both think?

[gabyx]

@rycus86: Ok, but I'd rather have the following logic:

1. haveing a check for the version first if we really classify for strange legacy transforms and stuff and then¨
2. check if anything needs to be done

You are right for the local paths transforms we can always do this, but the , split in githooks.shared you really want to do only once (because url can contain commas 🙈 ) I decided to put also the local path transforms in the if guard 1.

[gabyx]

last commit 656eae6 includes the changes with review for the legacy transform checks and stuff...

[gabyx]

I try (maybe to much work instead of a manual test) to think about a test which tests update procedure... To trigger exactly the legacy transforms...

[gabyx]

@rycus86, @matthijskooijman : Guys, our first update test has arrived: see 119. It works so far on alpine, hope the other systems passes as well. Is it clean enough? And coverage also increased ;-)

P.S: And of course there where 1-2 Bugs, (forgot to set IFS_COMMA_NEWLINE, plus wrong variable name...)

[gabyx]

Commit: 54e77bc is a Bugfix which was not detected up to now...

[gabyx]

I think, since we tested the legacy transform. This PR should be good to go now.

Some summary for the review mess above. Thanks @matthijskooijman and @rycus86.

• implemented url and local paths, local paths not allowed in .shared file
• implemented --local githooks.shared, kept doc consistent, we have --shared, --local and --global configurable settings for shared hooks.
• split all githooks.shared into multiple githooks.shared settings with git config --get-all
• legacy transform with test199.sh for migrating the above three points.
  • for normal installs we migrate, for core.hooksPath installs we output a warning telling the user he needs to fix it
• new pre-commit hook which tests for set -x because I had checked in already such a thing... :see_no_evil:
• bugfix: c369be7, wrong clone statements also during testing and looking at the output I realized that so far wrappers get installed into release clone and internal shared hooks accidentially, I disabled this by --template-dir=/dev/null since we really dont want them to be there.
• bugfix: 54e77bc, no install into existing repos when using core hooks path install, this was unseen and did not break test fortunately, but better dont do it...

@rycus86 : What needs to be done here, I think its good to be merged. or did I miss anything. @rycus86 : Ok now everything should be passing with a4e8512

[gabyx]

@rycus86 Please see my latest commits. Also for useCoreHooksPath the variable failOnNonExistingHooks comes here very handy :-)

[gabyx]

Coverage went down significantly due to test119, which I dont know how to cover yet. @rycus86 : Would that PR be ready to merge, if no major resistance?

[rycus86]

Coverage went down significantly due to test119, which I dont know how to cover yet. @rycus86 : Would that PR be ready to merge, if no major resistance?

I'm still not convinced about the whole commit tracking thing for upgrades, but it doesn't sound like I can talk you out of it. I'll try to have a look later and if there's nothing else standing out, I'll merge this in.

[gabyx]

We could keep the local paths transform out from local commit tracking, and just warn and let it fail after the users try to use them. But as I just found out: I also split everything into seperate lines with this legacy transform, because we only allow seperate lines in .shared files from this PR on. Urls/Paths can have commas. So that makes the "local path transform" also dependend on the fact "you only want to do that once" -> meaning we need this tracking as in the case of the "global config splitting".

[gabyx]

Thanks for looking it through again! Maybe you spot some more errors, I hope now all bad ones should be gone. Thanks @matthijskooijman for the reviews.