Disallow "b=N" (instead of "Bug N") in commit message hook

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, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8472033 [details] [diff] [review]
Disallow "b=N" (instead of "Bug N") in commit message hook
Attachment #8472033 - Flags: review?(emorley)

Comment 2

3 years ago
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.

Updated

3 years ago
Attachment #8472033 - Flags: review?(emorley) → feedback+
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.

Comment 4

3 years ago
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.

Comment 5

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

Comment 6

3 years ago
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.

Comment 7

3 years ago
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.
(Assignee)

Comment 8

3 years ago
Created attachment 8473047 [details] [diff] [review]
Disallow "b=N" and "Bug #N" in commit message hook

(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.
Attachment #8472033 - Attachment is obsolete: true
Attachment #8473047 - Flags: review?(emorley)

Comment 9

3 years ago
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).
Attachment #8473047 - Flags: review?(emorley) → review+
(Assignee)

Comment 10

3 years ago
(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.
Flags: needinfo?(emorley)
(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.
Flags: needinfo?(emorley)
(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%.
Product: Release Engineering → Developer Services
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
Flags: needinfo?(birunthan)
(Assignee)

Comment 17

3 years ago
Created attachment 8504482 [details] [diff] [review]
Disallow "b=N" and "Bug #N" in commit message hook

Test updated.
Attachment #8473047 - Attachment is obsolete: true
Attachment #8504482 - Flags: review?(emorley)
Flags: needinfo?(birunthan)
Comment on attachment 8504482 [details] [diff] [review]
Disallow "b=N" and "Bug #N" in commit message hook

Thank you :-)
Attachment #8504482 - Flags: review?(emorley) → review+
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
Attachment #8504482 - Flags: checked-in+

Updated

3 years ago
Depends on: 1083177
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 :-)
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.