search

rycus86 / githooks

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

Disabling/enabling doesn work if repo is trusted #137

Closed gabyx closed 4 years ago

gabyx commented 4 years ago

From the readme:

When they have, it will prompt for confirmation whether you accept those changes or not, and you can also disable specific hooks to skip running them until you decide otherwise.

As the "disable>" tag is inside the checksum file, this enabling/disabling mechanism only works if the repo is not trusted. I think enabling/disabling is orthogonal to checking if the hooks are trusted or not... and is probably in the current implementation a design oversight?

rycus86 commented 4 years ago

Yeah, that does seem odd. Not sure what's the correct behaviour, because of I trust a repo I want to run all hooks that it contains, whereas I want selectively enable/disable hooks in regular (non-trusted) repos.

I agree that this edge case should be clarified in the README though at least.

What is your opinion? Do you think we should be able to disable hooks even in trusted repos?

gabyx commented 4 years ago

I think the feature should be orthogonal to the trust checksum. I will addressed that already in the Go rewrite. Its a to big hassle I think to support that in this bash impl. again. Its not soo important. =) was just noticing this when implementing the stuff...

rycus86 commented 4 years ago

It's also orthogonal from the point that you don't have to accept trusting all hooks in a repo I think, so even though the repo wants to be trusted, you could ignore that and selectively enable/disable the hooks you want -- if I remember correctly, I may be wrong here.

Again, I'd advise to NOT change this as part of the Go port, so that we can test the rewrite easily without having to also check 7 different, unrelated changes. That gets hard when things don't work, and you can't tell if it doesn't work because of the rewrite, or because of a bug in this other change.

gabyx commented 4 years ago

if I remember correctly, I may be wrong here. You are correct.