Node: discuss: we need to take better care when landing commits

[evanlucas]

4 out of the last 10 commits do not adhere to our commit guidelines. We need to take more care when landing these commits:

$ git rev-list 29ca969..HEAD | xargs core-validate-commit 
Commit e7c077c610afa371430180fbd447bfef60ebc5ea
   ETITLETOOLONG stream: make null an invalid chunk to write in object mode
Commit ec2822adaad76b126b5cccdeaa1addf2376c9aa6
   OK
Commit 75487f0db80e70a3e27fabfe323a33258dfbbea8
   ETITLETOOLONG module: fix resolution of filename with trailing slash
   ELINETOOLONG A recent optimization of module loading performance [1] forgot to check that
   ELINETOOLONG [1] https://github.com/nodejs/node/pull/5172/commits/ae18bbef48d87d9c641df85369f62cfd5ed8c250
Commit 8636af10129e822d8ae928ef3470a4dff9c4192a
   OK
Commit 6198472d8390d9476f555c634b7aa66ce6c6d0fe
   EINVALIDSUBSYSTEM stream_base
Commit e1cf634a0bd0cae2b54c60c8f19fc29079bdc309
   OK
Commit 5f0fcd6245a28761b0e8a565e99bca5ae2b5432e
   OK
Commit 9c2b8ecc54e327498944b45881a168c5fded96cb
   ETITLETOOLONG doc: note that zlib.flush acts after pending writes
Commit 2dc5ad460a8bc014a47ca25c73a8f343ef296d27
   OK
Commit 54dd7c38e507b35ee0ffadc41a716f1782b0d32f
   OK

I've been working on a tool to help validate the build system(s), line length, and format in general. Maybe we could add a jenkins job that validates the commit message or something?

[bnoordhuis]

Sounds like a plan. To avoid the chicken/egg problem, the tool should perhaps be written in python? I think it could be a script in <10 lines in tools/.

[jasnell]

I've had "implement a commit linter" on my todo list for quite some time but haven't been able to get around to it. Having something that can run as a pre-push git hook that can bail if things aren't right would be excellent.

[cjihrig]

@bnoordhuis do you really think it would be a problem? Our linter is already written in JavaScript. Seems like Node would have to be extremely broken for it to be an issue.

[bnoordhuis]

I was thinking more of people that want to do small documentation updates and don't want to wait for a full build.

[mscdex]

Can something like this be added to the github bot perhaps? That way you wouldn't have to rely on locally-installed git hooks.

[jasnell]

It possibly could, but it'd be helpful to have a final check before the
actual push to land to PR.
On Apr 21, 2016 9:04 AM, "Brian White" [email protected] wrote:

Can something like this be added to the github bot perhaps? That way you
wouldn't have to rely on locally-installed git hooks.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/issues/6331#issuecomment-212987650

[silverwind]

I've been thinking about a commit landing CLI tool that could handle Reviewed-By, PR-URL, formatting and more (some details at https://github.com/nodejs/build/issues/325), but I've been putting it off because it's not trivial to interface with git rebase -i (https://github.com/nodejs/build/issues/324#issuecomment-182129573).

[MylesBorins]

So we will likely have a chicken / egg problem with validating the commit in CI as well. I think it makes sense to do a validation on title / content... but meta data won't be added until the commit is landed.

Incremental steps ftw though... I'd love to see something in CI that can warn us about non meta data related problems

[mhdawson]

I'd favor something like another make target along the lines of make checkcommit. That would be easy for collaborators to run after they update the top commit but before they push. I might need to take an argument with the number of commits to validate in order to handle cases there the PR consists of more than one. This probably fits with Ben's suggestion that it be written in python and live in tools

[Fishrock123]

👍 I've long stated that I thought a CLI tool was appropriate for these problems. As a note, @rvagg has a tool for part of the commit metadata: https://github.com/rvagg/iojs-tools/tree/master/pr-metadata

[Fishrock123]

Also, I think we should avoid more python since this isn't for bootstrapping. We'll probably get more contributions if it is in JavaScript.

[jbergstroem]

I'd like to move this logic to the github bot and create two jobs we can run on all pr's (all commits), namely:

1. commit message validation
2. linting

These are both cheap enough to run a lot and I'd like to introduce a rule that each branch needs a force push (if you're able) with the commit that intends to land in master so that this bot can deem it ok. It's not perfect and doesn't solve the "someone else merges" problem (pushing to a pr/12345 doesn't work), but its an improvement for the standard workflow.

[Fishrock123]

closing in favor of https://github.com/nodejs/node/issues/6543