Right now, we allow e.g. "b=123" in place of "Bug 123". Because the vast majority use the latter format, we should stop allowing "b=N" in order to keep things consistent.

CCing people who have previously edited or reviewed this file / similar in the hghooks repo. I agree with this, and would say let's go one step further - and also: - r'bug\s+\#?[0-9]+', + r'bug [0-9]+', I think we should also: - message("Rev {rev} needs a bug number.") + message("Rev {rev} needs a bug number in the form \"bug [0-9]+\", " "or else \"no bug\" somewhere in the commit message.") Yes we can work around inconsistent bug numbers in every single tool (mcMerge, qimportbz, bzexport, TBPL, treeherder, ...) and yes we should probably have a library that handles them (/commit messages attributes in general) - however let's just enforce new commit messages are consistent and be done with it. If you could update the patch to include the changes above - I'll review that & we can see that the newly CCed people think. We'll obviously need to announce the change on dev.platform/dev.tree-management.

We were attempting to support all the various historical things people did and not be proscriptive. I honestly don't see huge value in forcing one format, it's still mostly free-form text. If we could force the bug number into structured data somewhere that would be useful.

Because of crap that results, like: https://hg.mozilla.org/webtools/tbpl/file/06af9dad7e20/mcmerge/js/Config.js 34 // The many ways bug numbers are specified 35 bugRE1: /b(?:ug)?=(\d{4,7})\b/i, // e.g. b=XXXXXX 36 bugRE2: /^fix(?:es)?\s*(?:for\s*)?(\d{4,7})\b/i, // Fix is sometimes used as a synonym for bug 37 bugRE3: /to\s+fix\s+bug\s+(\d{4,7})/i, 38 bugRE4: /\((\d{4,7}), r=/i, // e.g. JS-team style (XXXXXX, r=foo) 39 bugRE5: /but\s*(\d{4,7})/i, // The typo but XXXXXX happens quite often 40 bugRE6: /\bb(?:u?g(?:zilla)?)?:?\s*#?(\d{4,7})\b/i, // The catchall 41 bugRE7: /^(\d{4,7})\b/i, // e.g. XXXXXX, Note: mcMerge has to try and pick out the correct bug numbers from things like: Bug 111111 - Followup to bug 666666; r=foo Backout bug 111111 to fix bug 666666 Followup to fix foo (bug 123456) Fix topcrash introduced by bug 666666; b=123456 ...in order to correlate landings with backouts etc. This patch saves mcMerge's confusion over the final case above. I agree that structured data somewhere would be ideal, but given how few people use |b=|, I think we should just not permit it.

Ideally we'd also: - r'bug\s+\#?[0-9]+', + r'^bug [0-9]+',

I should add that of course this doesn't benefit tools that have to handle historical data, but for things like mcMerge, bzexport, qimportbz we're normally only handling new commits.

The version-control-tools repo contains Python modules for e.g. parsing commit messages. One of the goals of consolidating all the code in one repo is so hooks, extensions, etc could all share the same code path for things like parsing commit messages. If you update code under hghooks to use code in e.g. pylib/, our hghooks deployment procedure will need to update. Best to loop in bkero and fubar before you make that change. I fully support dropping the b=N syntax going forward.

(In reply to Ed Morley [:edmorley] from comment #2) > If you could update the patch to include the changes above - I'll review > that & we can see that the newly CCed people think. Done.

Comment on attachment 8473047 [details] [diff] [review] Disallow "b=N" and "Bug #N" in commit message hook Review of attachment 8473047 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thank you :-) We'll need to test on the last few months of commit messages just to check we don't have any bot commits that don't match (I'll have a look tomorrow hopefully) + also communicate out before deploying (including how to use the "IGNORE BAD COMMIT MESSAGES" workaround when merging a repo into another, that contains legacy commits).

(In reply to Ed Morley [:edmorley] from comment #9) > We'll need to test on the last few months of commit messages just to check > we don't have any bot commits that don't match (I'll have a look tomorrow > hopefully) + also communicate out before deploying (including how to use the > "IGNORE BAD COMMIT MESSAGES" workaround when merging a repo into another, > that contains legacy commits). The following command resulted in 4 non-bot email addresses: git log --format="%s <%ae>" --since 1.year.ago | grep -iE '^(bug\s+#|bug\s{2,}|b=)' | sed -E "s/.* <(.+)>$/\\1/g" | sort -u Should I go ahead and land this? I'll inform dev-platform (or is there some other list?) first.

(In reply to Ed Morley [:edmorley] from comment #4) > Note: mcMerge has to try and pick out the correct bug numbers from things > like: > Bug 111111 - Followup to bug 666666; r=foo > Backout bug 111111 to fix bug 666666 > Followup to fix foo (bug 123456) > Fix topcrash introduced by bug 666666; b=123456 > ...in order to correlate landings with backouts etc. > This patch saves mcMerge's confusion over the final case above. So wouldn't it be better to require b= to identify the indended bug number instead of trying to parse a language as poor as English?

(In reply to Birunthan Mohanathas [:poiru] from comment #10) > The following command resulted in 4 non-bot email addresses: > > git log --format="%s <%ae>" --since 1.year.ago | grep -iE > '^(bug\s+#|bug\s{2,}|b=)' | sed -E "s/.* <(.+)>$/\\1/g" | sort -u > > Should I go ahead and land this? I'll inform dev-platform (or is there some > other list?) first. Can you given some example commits? Don't forget merges etc aren't expected to have bug numbers. We'll also need to check the tests pass, but other than that, I think this is fine to land once that's done. (In reply to Karl Tomlinson (:karlt) from comment #11) > So wouldn't it be better to require b= to identify the indended bug number > instead of trying to parse a language as poor as English? Probably, but given 99% of commits use the "bug N" form, I think that's going to be a harder change to make.

(In reply to Ed Morley [:edmorley] from comment #4) > Fix topcrash introduced by bug 666666; b=123456 > ...in order to correlate landings with backouts etc. > This patch saves mcMerge's confusion over the final case above. I don't follow why that would be hard to parse, or why Fix topcrash introduced by bug 666666; bug 123456 would be easier to parse.

+1 on landing assuming the tests pass. Given that very few people use this form, I'm OK with silently deprecating it: email would just be spam for 99.5%.

Can we go ahead and land this?

(In reply to Tom Schuster [:evilpie] from comment #15) > Can we go ahead and land this? The tests need updating first, eg: https://hg.mozilla.org/hgcustom/version-control-tools/file/63274edfb8e6/hghooks/tests/test-commit-messages.t#l74

Test updated.

Comment on attachment 8504482 [details] [diff] [review] Disallow "b=N" and "Bug #N" in commit message hook Thank you :-)

Comment on attachment 8504482 [details] [diff] [review] Disallow "b=N" and "Bug #N" in commit message hook https://hg.mozilla.org/hgcustom/version-control-tools/rev/96535e603704

Tests are passing: https://ci.mozilla.org/job/version-control-tools/190/ So I've filed bug 1083177 to deploy this.

Deployed - thank you for the patch :-)