1270872 - add a commit hook to reject commit messages containing diffstats

Status: NEW

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Description

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd2dae0c4020efec0263c28e2aacbc0a669cd0e

has a commit message that's just a diffstat of all the files in the commit.  These sorts of commit messages are unhelpful, and for tools that want to display commit messages, a diffstat just makes things very noisy.

Comment 1

[Gregory Szorc [:gps]]

I agree. This has been bothering me as well.

I'm not sure if I'll have time to hack on this. If anyone wants to code something up, go for it. The following can be used for inspiration:

https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/mozhghooks/commit-message.py
https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/tests/test-commit-messages.t

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Bug 1270872 - add a commit hook to reject commit messages containing diffstats

Review commit: https://reviewboard.mozilla.org/r/181162/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181162/

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I couldn't figure out how to strip out the previous test's commit, tonight. The strip extension doesn't seem to be enabled despite it being used earlier in the file. Am I missing something obvious?

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8909652 [details]
Bug 1270872 - add a commit hook to reject commit messages containing diffstats

https://reviewboard.mozilla.org/r/181162/#review188362

To answer your question about `hg strip`, the top of the test file echos some config additions to an hgrc file. You can enable the strip extension here.

::: hghooks/mozhghooks/commit-message.py:24
(Diff revision 1)
>  from mercurial.node import short
>  from mozautomation import commitparser
>  
>  INVALID_REVIEW_FLAG_RE = re.compile(r'[\s.;]r\?(?:\w|$)')
>  
> +DIFFSTATS_RE = re.compile(r'[0-9]+ files changed, [0-9]+ insertions\(\+\), [0-9]+ deletions\(\-\)')

This isn't comprehensive enough :(

Looking at commit messages in the wild, it appears the "N insertions" and "N deletions" messages are optional. So they should be optional in the regexp.

To avoid false positives, we should also anchor the regexp to the beginning of lines and add the leading space that appears to be present in all commit messages with this pattern.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8909652 [details]
Bug 1270872 - add a commit hook to reject commit messages containing diffstats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181162/diff/1-2/

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8909652 [details]
Bug 1270872 - add a commit hook to reject commit messages containing diffstats

https://reviewboard.mozilla.org/r/181162/#review188362

Seems that hgrc file at the top didn't affect my test at the bottom of the test file. I recreated it closer to my test at the bottom and it seemed to work there.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8909652 [details]
Bug 1270872 - add a commit hook to reject commit messages containing diffstats

https://reviewboard.mozilla.org/r/181162/#review188686

Almost. This just needs a leading space in the regexp and associated test changes.

::: hghooks/mozhghooks/commit-message.py:24
(Diff revision 2)
>  from mercurial.node import short
>  from mozautomation import commitparser
>  
>  INVALID_REVIEW_FLAG_RE = re.compile(r'[\s.;]r\?(?:\w|$)')
>  
> +DIFFSTATS_RE = re.compile(r'^[0-9]+ files changed(, [0-9]+ insertions\(\+\))?(, [0-9]+ deletions\(\-\))?', re.M)

This needs a space between `^` and `[0-9]+`.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8909652 [details]
Bug 1270872 - add a commit hook to reject commit messages containing diffstats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181162/diff/2-3/

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8909652 [details]
Bug 1270872 - add a commit hook to reject commit messages containing diffstats

https://reviewboard.mozilla.org/r/181162/#review189648

This seems reasonable.

Before deployed, someone should reach out to people who would have been hit by this in the past 1-2 months and verify they can easily modify their workflow so this doesn't disrupt them too much. We don't want to annoy people any more than necessary.