rycus86 / githooks

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

Install option: "Purge all existing hooks" added #53

Closed gabyx closed 4 years ago

gabyx commented 4 years ago

According to #51. Dont run lfs hooks twice, installing into a repo where lfs hooks are already places is not soo good, since we are handling lfs internally. Either

The workaround is implemented.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 440


Changes Missing Coverage Covered Lines Changed/Added Lines %
cli.sh 15 18 83.33%
install.sh 2 5 40.0%
<!-- Total: 17 23 73.91% -->
Totals Coverage Status
Change from base Build 429: -0.2%
Covered Lines: 1726
Relevant Lines: 1987

💛 - Coveralls
rycus86 commented 4 years ago

How about if we try to peek into the front of the hook file and find out if it's an LFS hook, in which case we just delete it rather than move it to replaced? That would be less of an intrusive operation than just purging all existing hooks.

I believe the LFS go code does the same, reads the first kilobyte or so and checks if it looks like an LFS hook.

gabyx commented 4 years ago

Ah, really, ok, might be a solution yes. But in that way we have more deps to LFS which is a bit sad. The thing is people can modify the hooks which LFS placed, and add stuff to it at the end. So you would accidentaly delete a hook the user really needed... Either sed/replace all git lfs invocations or leave the decision to the user, by the rahter ugly install flag I introduced.

rycus86 commented 4 years ago

Yeah, LFS wants to take over hooks, but we do too, so 🤷‍♂️🙂

rycus86 commented 4 years ago

Should we park this for now, and just add (another) workaround for LFS? For reference, this is how LFS itself checks if it has its hooks already: https://github.com/git-lfs/git-lfs/blob/a0030968cef7b7c3c36d10f6137da98107db34c1/lfs/hook.go#L137

So, it compares the first 1024 bytes of the content only, users are free to add to the end.

gabyx commented 4 years ago

Close this as #60 . Implements the bahavior, it works pretty nicely,