add a commit hook to reject commit messages containing diffstats

NEW
Assigned to

Status

Developer Services
Mercurial: hg.mozilla.org
2 years ago
2 months ago

People

(Reporter: froydnj, Assigned: KWierso)

Tracking

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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

2 years ago
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
QA Contact: hwine → klibby
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 months ago
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?
Assignee: nobody → wkocher

Comment 4

2 months ago
mozreview-review
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.
Attachment #8909652 - Flags: review?(gps) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 months ago
mozreview-review-reply
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

2 months ago
mozreview-review
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]+`.
Attachment #8909652 - Flags: review?(gps) → review-
Comment hidden (mozreview-request)

Comment 9

2 months ago
mozreview-review
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.
Attachment #8909652 - Flags: review?(gps) → review+
You need to log in before you can comment on or make changes to this bug.