nlf/precommit-hook — pull request #39 by Trott (2014-08-03 18:43:47+00:00)
Title: Do not clobber pre-existing hook

**Do not merge.** This isn't quite there, yet.

This code change will print a warning rather than clobbering a pre-existing hook. If this is not actually a desirable change, then just close this PR and I'll stop thinking about this altogether.

As it stands, this code change will:

* install fine if there is no existing hook
* install fine if there is a preexisting hook and this package has been installed already (because it runs the uninstall script first in that case, which removes the hook)
* update fine (because it checks to make sure it is an install and not a mere update, which come to think of it might not be necessary because I'm not sure that `npm update` runs the install script...)
* print a warning if this is a new install and there is a pre-existing hook; the hook is not clobbered but the package still gets installed (which is what happens if you run it from something that isn't a .git project too; that code was already there)

Aside from the relatively minor issues of making the warning harder to miss and further updating the README to reflect this behavior, the big issue I've run into is this:

* If you do a new install of the package when there is a pre-existing hook, as mentioned above, it will print a warning and not clobber the hook but still install the package. Fine so far. But if you then uninstall the package, it will remove the pre-existing hook. Yikes. Not what we want.

I can think of several solutions to this issue, all with different trade-offs. The one I like the most is to just not have the uninstall script remove the hook. Just get rid of the uninstall script altogether and have documentation indicating that uninstalling the package will do that (uninstall the package) but that if you want to uninstall the hook, you need to manually remove it. That, however, seems like a big user experience change (as does the do-not-clobber-existing-hook change) and I thought maybe I'd throw that around a bit before actually, you know, implementing it.

Thoughts? Is all of this work completely wrong-headed to begin with?