Skip to content

dist: add commit message length checker#8202

Merged
aabadie merged 1 commit intoRIOT-OS:masterfrom
miri64:dist/tools/commit-msg-checker
Dec 6, 2017
Merged

dist: add commit message length checker#8202
aabadie merged 1 commit intoRIOT-OS:masterfrom
miri64:dist/tools/commit-msg-checker

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Dec 4, 2017

This checks if the commit messages of a PR follow the 50/72 rule. This way we don't merge these kind of commit messages accidentally anymore ;-).

@miri64 miri64 added Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools labels Dec 4, 2017
@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 4, 2017
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Dec 4, 2017

Why is Travis not executing that script?

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Dec 4, 2017

Mh... if I run make static-tests locally, it works...

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Dec 4, 2017

Why is Travis not executing that script?

(I was mistaken with that assessment... I'm just confused, why it doesn't work, while it works locally for me)

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Dec 4, 2017

Ahh... could it be, that the empty commits are removed by this (the rebase)?

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Dec 4, 2017

Works now with a non-empty commit! :-)

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Dec 4, 2017

How does it work with multiline commit messages ? Maybe we want to keep those ones ?

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Dec 4, 2017

How does it work with multiline commit messages ? Maybe we want to keep those ones ?

As long as contributors stick to the rules for those (i.e. start the detailed description after one empty line after the summary) the check will only complain about the summary (--pretty=format:"%s" makes sure of that). There are already commits in the history who ignore this rule. Naturally, Git then thinks the detailed description belongs to the summary and will complain about this in this check.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Dec 4, 2017

As you can see in https://travis-ci.org/RIOT-OS/RIOT/builds/311342120: 79cfa95 doesn't show up, but d268896 still does.

Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright needs to be updated. I find the test script a bit difficult to read but maybe because I'm missing experience on shell scripts.

@@ -0,0 +1,48 @@
#!/bin/sh

# Copyright 2015 Oliver Hahm <oliver.hahm@inria.fr>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right copyright ?

@jnohlgard
Copy link
Copy Markdown
Member

I like 👍

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Dec 6, 2017

Ping?

Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No particular comments on the check scripts and it seems to work on CIs, so ACK

Please remove the wrong commits and extra file, then squash.

@miri64 miri64 force-pushed the dist/tools/commit-msg-checker branch from 14aacd0 to 83930a0 Compare December 6, 2017 11:04
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Dec 6, 2017

Done. Did a minor indentation fix to the script ;-)

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Dec 6, 2017

All green, go!

@aabadie aabadie merged commit 02d91e1 into RIOT-OS:master Dec 6, 2017
@aabadie aabadie added this to the Release 2018.01 milestone Jan 18, 2018
@kaspar030
Copy link
Copy Markdown
Contributor

I'm not sure I like this... all my commit descriptions are longer than 50 characters... ;)

@miri64 miri64 deleted the dist/tools/commit-msg-checker branch February 2, 2018 19:17
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Feb 2, 2018

I'm not sure I like this... all my commit descriptions are longer than 50 characters... ;)

Well it is BCP for git, so you better should adapt your git behavior ;-). Also: those only get a warning, the 72 ones are the ones that cause errors ;-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants