Reject commit messages with ' r?'

RESOLVED FIXED

Status

Developer Services
Mercurial: hg.mozilla.org
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: poiru, Assigned: poiru)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Comment 1

3 years ago
Created attachment 8621109 [details] [diff] [review]
Reject commit messages with ' r?'

I didn't find false positives with this simple check (see link in comment 0).
Attachment #8621109 - Flags: review?(gps)

Comment 2

3 years ago
Comment on attachment 8621109 [details] [diff] [review]
Reject commit messages with ' r?'

Review of attachment 8621109 [details] [diff] [review]:
-----------------------------------------------------------------

::: hghooks/mozhghooks/commit-message.py
@@ +52,5 @@
>      if "[PATCH" in desc:
>          message("Rev {rev} contains git-format-patch \"[PATCH]\" cruft. Use git-format-patch -k to avoid this.")
>          return False
>  
> +    if " r?" in firstline:

This is slightly too greedy for my liking since it will flag "Fix bug with r? detection". We probably want the regexp 'r\?\w' to match "r?" followed by some characters.

Feel free to request review on an updated patch or submit this and file a follow-up bug.
Attachment #8621109 - Flags: review?(gps) → review+
(Assignee)

Comment 3

3 years ago
Created attachment 8621227 [details] [diff] [review]
Reject commit messages with 'r?'

(In reply to Gregory Szorc [:gps] from comment #2)
> This is slightly too greedy for my liking since it will flag "Fix bug with
> r? detection". We probably want the regexp 'r\?\w' to match "r?" followed by
> some characters.

Done.
Attachment #8621109 - Attachment is obsolete: true
Attachment #8621227 - Flags: review?(gps)

Comment 4

3 years ago
Comment on attachment 8621227 [details] [diff] [review]
Reject commit messages with 'r?'

Review of attachment 8621227 [details] [diff] [review]:
-----------------------------------------------------------------

::: hghooks/mozhghooks/commit-message.py
@@ +17,5 @@
>  
>  import re
>  from mercurial.node import hex
>  
> +INVALID_REVIEW_FLAG_RE = re.compile(r'[\s;.]r\?\w')

This may not be greedy enough! But it should be good for most scenarios. When it comes to potentially getting in the way of user workflows, I'd rather cast a narrow net before a wide one.
Attachment #8621227 - Flags: review?(gps) → review+
(Assignee)

Comment 5

3 years ago
http://hg.mozilla.org/hgcustom/version-control-tools/rev/f74a7a7f5e86
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Comment 6

3 years ago
Deployed to production.

Updated

3 years ago
Flags: sec-review?
Flags: needs-treeclosure?
Flags: needinfo?
Flags: cab-review?

Updated

3 years ago
Flags: sec-review?
Flags: needinfo?
Flags: needs-treeclosure?
Flags: cab-review?
You need to log in before you can comment on or make changes to this bug.