[WIP] git-hooks: add ncu-git-hooks to install pre-commit hook

[priyank-p]

This still needs some work on some parts of code but mostly is ready. Also needs docs and perhaps tests.

Basic Way to use:

ncu-git-hooks install <hook>

And after that, it should run ./configure && make -j4 test on Unix while vcbuild test on windows when you make a commit. It should also work on windows, if not we can use PowerShell later.

[codecov[bot]]

Codecov Report

Merging #224 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #224   +/-   ##
=======================================
  Coverage   88.01%   88.01%           
=======================================
  Files          19       19           
  Lines         701      701           
=======================================
  Hits          617      617           
  Misses         84       84

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8c6f7cc...58aa26b. Read the comment docs.

[joyeecheung]

Thanks for picking this up!

And after that, it should run ./configure && make -j4 test on Unix while vcbuild test on windows when you make a commit.

I don't think that's what most people do though? Also I've seen many people developing on a local branch or in their own fork before submitting a PR and the code there is usually not ready for a full test before it gets out of WIP (especially when it comes to passing linters). This seems more suitable for a pre-push hook at the very least (ideally, a push to a branch without a PR opened).

[priyank-p]

That makes pre-commit easier then, so it would be just make lint or vcbuild lint.

[priyank-p]

Currently works like this:

[joyeecheung]

@cPhost From what I can tell most people would not want running linters on commit in their workflow, which is why ‘make test-only’ exists. Mind that commit is not reserved for changes that will be summitted right away, a lot of times it’s just about checking WIP code into the source control but at that point you don’t really care about linters or tests since it is just a WIP.

[priyank-p]

So we want to run make test-only for each commit instead of linter or test?

It looks like the workflow of node is quite not the same as other projects, which is because in the PR mostly some commit are not standalone sometimes and so they are squashed into one while landing.

Is it that we don't need pre-commit? I used it because was pretty easy to do than pre-push where we would need to get commit hash of each commit made and send it through core-validate-commit. (+ some more things to consider too. so didn't quite want to take it just yet).

[joyeecheung]

The primary reason I would say is that make test or make test-only takes a lot time to run, especially when you are switching between branches and need to recompile. As far as I know there are quite a few collaborators who have their own shell commands that only run the tests relevant to recent changes instead of running the whole test suite, and sometimes I do want to use that when I am developing a PR and don't really care if they pass the whole test suite yet or think it's trivial enough that running the whole test suite is an overkill.

Also yes because node takes the git commit history seriously and most of the times the commits should be able to pass standalone when they land (but not necessarily when they are being developed).

[joyeecheung]

So we want to run make test-only for each commit instead of linter or test?

IMO nothing should be done in the pre-commit hooks. The ideal would be to only run those things when commits are being pushed to a branch with PR opened or the master/staging branch.

[priyank-p]

OK, makes sense, I will start implementing pre-push hook then.

[codebytere]

Can this be closed?

[priyank-p]

I will close this out!