Automatic apply can be set up at installation to prevent manual action to active shared hook

ocebr commented 2 months ago

Hello,

I suggest this small changes to installation process. Indeed, we faced some issue when running git commands with no interactive shell (for example Vscode git plugin failed).

With this changes, users are asked (only if they choose to install/modify shared hook) whether they want to enable automatic apply of shared hook in git repository.

Automatic apply of shared hook was previously disabled.
Would you like to enable automatic apply of shared hook in git repository? [Y/n] Y

Automatic update checks are now enabled

For this purpose a variable is added into gitconfig global file : githooks.sharedautoapply. Base-template.sh evaluates the value when executing a new shared hook.

For example :


$ git hooks list
> pre-commit
  - toto.sh (pending / new / shared:global)
$ git add .

$ git commit -m test
? New hook file found: /home/oc/.githooks/shared/br-githooks-shared/pre-commit/toto.sh
 Already accepted
Everything's clear, let's push those changes
[main e97zffa] test
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 file.txt

$ git hooks list
> pre-commit
  - toto.sh (active / shared:global)

rycus86 commented 1 month ago

Thanks for this suggestion! We already have trusted repo support, though it might not apply for shared hooks right now.

If the repository contains a .githooks/trust-all file, it is marked as a trusted repository. On the first interaction with hooks, Githooks will ask for confirmation that the user trusts all existing and future hooks in the repository, and if she does, no more confirmation prompts will be shown. This can be reverted by running either the git config --unset githooks.trust.all, or the git hooks config reset trusted command. This is a per-repository setting. These can be set up and changed with the command line helper tool as well, run git hooks trust help and git hooks config help for more information.

I would suggest we make git hooks trust deal with this new setting rather than adding a global gitconfig manually - what do you think?

ocebr commented 1 month ago

Thanks, I'll check git hook trust. Nevertheless, I see "On the first interaction with hooks, Githooks will ask for confirmation that the user trusts all existing and future hooks in the repository," The point of this feature was to never ask for user confirmation. Do you think we could change the behavior of git hook trust to not ask for it if the repo is from a shared ?

rycus86 commented 1 month ago

I think once the user should consent to these things, I'm open to adjusting how that happens, but I think it should still happen. For example a runbook could explain to your users that they need to install Githooks, then run a git hooks trust command of some sorts, then it would auto-accept everything.

ocebr commented 1 month ago

I understand. I could adjust my changes to ask instead if the new submitted shared hook should be trusted or not. If yes, execute the git hook trusted command. What do you think ?

rycus86 commented 1 month ago

Sorry, I didn't get how the submission happens? Can you please explain the steps a user would go through and where do they accept it? 🙇

ocebr commented 1 month ago

In install script setup_shared_hook_repositories lets user configure shared hook. I added my changes here to automatically apply the hook. I imagined theses steps:

User runs install script
User submits a shared hook
User accepts to trust this new shared hook
Installation completes
Shared hook are automatically applied

For that, I might need to change is_trusted_repo as you suggest to be applicable for shared hook

rycus86 commented 1 month ago

Ah yes, sorry, I didn't remember there's a step in the install process where you can set up shared hooks during installation. Yeah, it would make sense to allow trusting them there, either asking one by one after each of them, or maybe based on having the trust marker in the shared hook repo, not sure which one works out best UX wise.

I'm open to this change, it would be also good if git hooks trust could manage it post installation. Let me know when the code is in reviewable state and I can enable the GitHub actions workflow on this PR. Also let me know if you need any help or have questions, though I might not be able to get to them during the weekend, I should be able to continue on Monday.

ocebr commented 1 month ago

No problem ahah ! I was wondering too about post installation.

I'll get back to work and let you know when I'm good. Thank you for your help ! 😄

ocebr commented 1 month ago

Hello, I ended up choosing to trust shared repo when they have the trust marker (.githooks/trust-all file in the repo).

I tried to cover as many use cases as I could. Let me know if something has been left out !

I made changes to :

During installation if user set shared hook repo, it will be asked to turn on/off in the global setting the trust of repo containing a .githook/trust-all file
In CLI, git hooks trust function can now deal with global hook, by adding a global trust setting or removing it.
In CLI, git hooks shared ask user when adding a global shared hook if if want to trust globally hook if they contain a .githooks/trust-all file.
in CLI, git hook list also show a hook as active if the file trust-all if found in the repository

The code is ready for review. Let me know if something's going wrong 😊

rycus86 commented 1 month ago

Thanks! I've enabled the GitHub actions workflow on this PR now. I'll try having a proper review in a couple of days, just off sick at the moment. In the meantime, please have a look at any failures the tests might flag up, and it would be great if the new functionality could be covered by a test as well - I believe there are some existing test cases exercising shared hook repos.

ocebr commented 1 month ago

Hello, I have fixed and add some tests. Could you approve for the test to be run again please ?

ocebr commented 1 month ago

Hello, it should be ok now. 😊

ocebr commented 1 month ago

Could you relaunch please ?

ocebr commented 1 month ago

Hello, I fixed the tests, they passed locally.

rycus86 commented 4 weeks ago

No worries, thanks for pushing this forward! I'll try to have a look this week, sorry it's been a bit busy and haven't had a chance to look in depth yet.

coveralls commented 4 weeks ago

Pull Request Test Coverage Report for Build 10349651669

Details

72 of 84 (85.71%) changed or added relevant lines in 3 files are covered.
7 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.1%) to 79.969%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
install.sh	6	8	75.0%
cli.sh	49	59	83.05%
<!--	Total:	72	84	85.71%	-->

Files with Coverage Reduction	New Missed Lines	%
base-template.sh	1	87.39%
cli.sh	6	83.22%
<!--	Total:	7	-->

Totals
Change from base Build 4543718405:	-0.1%
Covered Lines:	2539
Relevant Lines:	3175

💛 - Coveralls

rycus86 commented 4 weeks ago

Hm, seeing some fatal: ref updates aborted by hook on CI, but weirdly not on all builds. 🤔

ocebr commented 4 weeks ago

Yes they appeared on these tests, opensuse, fedore, arch, I don't really know why, I tested on master and same errors showed up

ocebr commented 4 weeks ago

About the white spaces test, test 110 and 119 are failing, do you have any idea how to fix them ?

rycus86 commented 4 weeks ago

If I remember correctly, they test that things work ok when repos are inside a folder where the full path contains a space. If some quoting has changed around variables, that could potentially break it?

ocebr commented 2 weeks ago

Hello, @rycus86 did you have the time to get a look on the latest changes ? And the test failing even on master branch ?

rycus86 commented 2 weeks ago

Sorry, I didn't manage to get around to this yet. :( I'll try having a look when I can.

ocebr commented 2 weeks ago

Sorry, I didn't manage to get around to this yet. :( I'll try having a look when I can.

No problem ! :) I was just checking you did not forget about this ahah. I'll use my branch for the moment

rycus86 / githooks