New commit message restrictions

[Tom Schuster]

As the result of a discussion on dev.planning, Jesse implemented a new
Mercurial hook (same thing as when pushing on a CLOSED TREE)
that disallows certain commit messages. After I made some suggestions in
Bug 506949 he asked if I would like to take over and I happily did.
After some waiting, because all the
staff at IT working on Mercurial left, we now have finally deployed
the commit hook to mozilla-inbound,
thanks to Benjamin Kero. It's not going to actively block commits
until Monday 31th October 2011.
The hook just enforces best practices, so you likely won't notice
anything at all. Please read this section
"Commit Message Restrictions"
(https://developer.mozilla.org/En/Developer_Guide/Committing_Rules_and_Responsibilities#section_3)
for the details.

Here are some perfectly valid commit messages (all from this day):

Bug 696651 part 2 - Establish document.write() insertion position
before re-entrant calls can occur. r=Olli.Pettay.
Includes Bug #####, so good to go.
Ed Morley - Merge mozilla-central and mozilla-inbound
Starts with "Merge" and is actually a merge.
Paul O’Shannessy - Backout 78c921e2b56b (bug 640136) for causing bug 698162
Backout followed by 12 hex id.

Important note: If you need to merge a repository after this date, and
are hitting this new hook due to invalid commit messages, you can
put "IGNORE BAD COMMIT MESSAGES" into the tip commit message to
override all restrictions.

The plan is to deploy this on all repositories that merge to mozilla-central:
- mozilla-central
- integration/mozilla-inbound
- integration/fx-team/
- projects/jaegermonkey
- projects/build-system
- projects/devtools
- projects/places
- services/services-central

Cheers
Tom (evilpie)

[Benoit Jacob]

2011/10/29 Tom Schuster <t...@schuster.me>:

> Please read this section
> "Commit Message Restrictions"
> (https://developer.mozilla.org/En/Developer_Guide/Committing_Rules_and_Responsibilities#section_3)
> for the details.

This says:

> Also the reviewer is not checked at the moment, it could be at a future time and you are strongly encouraged to include it.

When I update our copy of the ANGLE library (gfx/angle) or of WebGL
conformance tests (content/canvas/test/webgl), I don't use a reviewer.
Nobody wants to review a 200kb patch that's just syncing us with some
upstream. So at least a way to opt out from the mandatory r= field is
needed. I can see that you discuss some exceptions, but see below
(about NanoJit):

> Please no fake reviewers like "my-dog" or "burning"

I can understand the concern about using one's dog as reviewer, but
what's the problem with "burning"? In practice, we often use
"r=bustage". This used to be considered OK and, as far as I can see,
necessary. Did that change?

> Special exceptions:
> Updating the NanoJit import in the js/src tree ("Update nanojit-import-rev stamp.")

Relying on a list of special exemptions like that means having to
update and maintain that list. Did you even know about my examples
(ANGLE and WebGL tests)? How many more exist? Maybe this would become
acceptable if this list of exemptions could be updated by any level 3
committer without having to wait for a central entity to do it, and if
that were easy to do and well documented.

Cheers,
Benoit

[Tom Schuster]

On Sun, Oct 30, 2011 at 12:04 AM, Benoit Jacob <jacob.b...@gmail.com> wrote:
> 2011/10/29 Tom Schuster <t...@schuster.me>:
>> Please read this section
>> "Commit Message Restrictions"
>> (https://developer.mozilla.org/En/Developer_Guide/Committing_Rules_and_Responsibilities#section_3)
>> for the details.
>
> This says:
>
>> Also the reviewer is not checked at the moment, it could be at a future time and you are strongly encouraged to include it.
>
> When I update our copy of the ANGLE library (gfx/angle) or of WebGL
> conformance tests (content/canvas/test/webgl), I don't use a reviewer.
> Nobody wants to review a 200kb patch that's just syncing us with some
> upstream. So at least a way to opt out from the mandatory r= field is
> needed. I can see that you discuss some exceptions, but see below
> (about NanoJit):
>
>> Please no fake reviewers like "my-dog" or "burning"
>
> I can understand the concern about using one's dog as reviewer, but
> what's the problem with "burning"? In practice, we often use
> "r=bustage". This used to be considered OK and, as far as I can see,
> necessary. Did that change?
>

We might just stretch this out for the moment, because this probably
not going to be anywhere soon.
I am going to remove this for the time being.

>> Special exceptions:
>> Updating the NanoJit import in the js/src tree ("Update nanojit-import-rev stamp.")
>
> Relying on a list of special exemptions like that means having to
> update and maintain that list. Did you even know about my examples
> (ANGLE and WebGL tests)? How many more exist? Maybe this would become
> acceptable if this list of exemptions could be updated by any level 3
> committer without having to wait for a central entity to do it, and if
> that were easy to do and well documented.
>

You are right this is probably going to be impossible to maintain. For
other strange we have the "ignore" message.
> Cheers,
> Benoit
>

[Nicholas Nethercote]

On Sun, Oct 30, 2011 at 9:04 AM, Benoit Jacob <jacob.b...@gmail.com> wrote:
>> Special exceptions:
>> Updating the NanoJit import in the js/src tree ("Update nanojit-import-rev stamp.")

Happily, Nanojit (and TraceMonkey) are on life-support and will be
removed from the tree fairly soon, probably during the Fx12 cycle.

Nick

[Chris Pearce]

On 30/10/2011 11:04 a.m., Benoit Jacob wrote:
> When I update our copy of the ANGLE library (gfx/angle) or of WebGL
> conformance tests (content/canvas/test/webgl), I don't use a reviewer.
> Nobody wants to review a 200kb patch that's just syncing us with some
> upstream. So at least a way to opt out from the mandatory r= field is
> needed. I can see that you discuss some exceptions, but see below
> (about NanoJit):

When updating the media libraries we still request review, but in
general I consider this to be a review of any build system changes, and
a review/confirmation of the decision to update the library, not a
review of the actual library code in general.

Chris Pearce.

[Justin Lebar]

> Important note: If you need to merge a repository after this date, and
> are hitting this new hook due to invalid commit messages, you can
> put "IGNORE BAD COMMIT MESSAGES" into the tip commit message to
> override all restrictions.

Is the hook checking just the tip commit in the push, or all commits
in a push? I thought the consensus from the earlier discussion was to
check only the tip, but this seems to suggest we're doing the latter.

-Justin

On Sat, Oct 29, 2011 at 5:16 PM, Tom Schuster <t...@schuster.me> wrote:
> As the result of a discussion on dev.planning, Jesse implemented a new
> Mercurial hook (same thing as when pushing on a CLOSED TREE)
> that disallows certain commit messages. After I made some suggestions in
> Bug 506949 he asked if I would like to take over and I happily did.
> After some waiting, because all the
> staff at IT working on Mercurial left, we now have finally deployed
> the commit hook to mozilla-inbound,
> thanks to Benjamin Kero.  It's not going to actively block commits
> until Monday 31th October 2011.
> The hook just enforces best practices, so you likely won't notice

> anything at all. Please read this section

> "Commit Message Restrictions"
> (https://developer.mozilla.org/En/Developer_Guide/Committing_Rules_and_Responsibilities#section_3)
> for the details.
>

> Here are some perfectly valid commit messages (all from this day):
>
> Bug 696651 part 2 - Establish document.write() insertion position
> before re-entrant calls can occur. r=Olli.Pettay.
>  Includes Bug #####, so good to go.
> Ed Morley - Merge mozilla-central and mozilla-inbound
>  Starts with "Merge" and is actually a merge.
> Paul O’Shannessy - Backout 78c921e2b56b (bug 640136) for causing bug 698162
>  Backout followed by 12 hex id.
>
> Important note: If you need to merge a repository after this date, and
> are hitting this new hook due to invalid commit messages, you can
> put "IGNORE BAD COMMIT MESSAGES" into the tip commit message to
> override all restrictions.
>
> The plan is to deploy this on all repositories that merge to mozilla-central:
>  - mozilla-central
>  - integration/mozilla-inbound
>  - integration/fx-team/
>  - projects/jaegermonkey
>  - projects/build-system
>  - projects/devtools
>  - projects/places
>  - services/services-central
>
> Cheers
> Tom (evilpie)

> _______________________________________________
> dev-planning mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-planning
>

[Tom Schuster]

On Sun, Oct 30, 2011 at 6:44 AM, Justin Lebar <justin...@gmail.com> wrote:
>> Important note: If you need to merge a repository after this date, and
>> are hitting this new hook due to invalid commit messages, you can
>> put "IGNORE BAD COMMIT MESSAGES" into the tip commit message to
>> override all restrictions.
>
> Is the hook checking just the tip commit in the push, or all commits
> in a push?  I thought the consensus from the earlier discussion was to
> check only the tip, but this seems to suggest we're doing the latter.
>

When we would do this, the hook would lose so much usefulness. And you
can already use ""IGNORE BAD COMMIT MESSAGES" and "no bug", so i
don't think we should check only the tip message.

[Axel Hecht]

I'd rather have DONTBUILD excempted, too. I do land changes to
all-locales and shipped-locales with that, and having yet another loop
to jump through is just going to be painful.

Axel

[Jesse Ruderman]

On Sat, Oct 29, 2011 at 10:44 PM, Justin Lebar <justin...@gmail.com> wrote:
>> Important note: If you need to merge a repository after this date, and
>> are hitting this new hook due to invalid commit messages, you can
>> put "IGNORE BAD COMMIT MESSAGES" into the tip commit message to
>> override all restrictions.
>

> Is the hook checking just the tip commit in the push, or all commits
> in a push?  I thought the consensus from the earlier discussion was to
> check only the tip, but this seems to suggest we're doing the latter.

If any commit in a push has the "IGNORE BAD COMMIT MESSAGES" mark, all
*earlier* commits have the restrictions lifted. This ensures that we
don't need to keep overriding every time we push a group of commits to
another repository.

[Jesse Ruderman]

> After some waiting, because all the
> staff at IT working on Mercurial left, we now have finally deployed
> the commit hook to mozilla-inbound,
>

> The plan is to deploy this on all repositories that merge to mozilla-central:
>  - mozilla-central
>  - integration/mozilla-inbound
>  - integration/fx-team/
>  - projects/jaegermonkey
>  - projects/build-system
>  - projects/devtools
>  - projects/places
>  - services/services-central

During the trial period when the hook is enabled only on inbound,
merges *to* inbound may require the override.

When we deploy it on the other repositories, we'll probably want to
bump DATE_HOOK_ENABLED again. (The purpose of DATE_HOOK_ENABLED is to
avoid errors when merging from one repo to another in the weeks
following the hook being enabled.)

[Dao]

On 30.10.2011 00:04, Benoit Jacob wrote:
>> Please no fake reviewers like "my-dog" or "burning"
>
> I can understand the concern about using one's dog as reviewer, but
> what's the problem with "burning"? In practice, we often use
> "r=bustage". This used to be considered OK and, as far as I can see,
> necessary. Did that change?

When and why was this necessary? I'd encourage people to leave out the
review tag if no review happened and they think that's OK. "bustage"
can't review patches. For a review-worthy bustage fix, you should
probably get ad-hoc review on IRC or back out the broken changeset and
take it back to bugzilla.

I'd also prefer if people always put the review tag on the first line in
the commit, so that it's clear what patches landed without review.

[Jeff Muizelaar]

On 2011-10-31, at 5:53 AM, Dao wrote:

> On 30.10.2011 00:04, Benoit Jacob wrote:
>>> Please no fake reviewers like "my-dog" or "burning"
>>
>> I can understand the concern about using one's dog as reviewer, but
>> what's the problem with "burning"? In practice, we often use
>> "r=bustage". This used to be considered OK and, as far as I can see,
>> necessary. Did that change?
>
> When and why was this necessary? I'd encourage people to leave out the review tag if no review happened and they think that's OK. "bustage" can't review patches. For a review-worthy bustage fix, you should probably get ad-hoc review on IRC or back out the broken changeset and take it back to bugzilla.

I agree. We shouldn't being forcing the content of our commit messages into a framework that it doesn't fit. I also push changes from upstream repositories that have not had review and would rather not and ugly tags to the commit message with to make a script happy.

Do we actually have a problem where people are omitting their reviewers? I'd rather this energy be put toward solving real problems like helping people avoid including the wrong bug number.

-Jeff

[Boris Zbarsky]

On 10/31/11 10:21 AM, Jeff Muizelaar wrote:
> Do we actually have a problem where people are omitting their reviewers?

It happens, yes. Whether it's a problem is an interesting question.

-Boris

[Gijs Kruitbosch]

Being naive, I would think that using the bugzilla API and the correct bug #, it
would be mostly trivial (only one non-obsolete patch; or one marked obsolete
with the HG changeset ID in a comment) to get the reviewer with eg. a
greasemonkey/user script/tbpl feature. Where there's a bug with multiple patches
which are unnumbered and/or the commit msg doesn't include an indicator which
patch was used, it'd be more tricky (maybe you could check which files are
touched?). Still, bug numbers mean there are paper, errr, digital trails.

Beside all that, I don't recall ever having needed the *reviewer* of the patch,
rather than either the author or the committer - but then, I've never committed
as regularly/much as some of the other people on this thread / in this forum.

~ Gijs

[Tom Schuster]

Did anybody have problems with the hook yet?

On Mon, Oct 31, 2011 at 3:21 PM, Jeff Muizelaar <jmuiz...@mozilla.com> wrote:
> On 2011-10-31, at 5:53 AM, Dao wrote:
>

>> On 30.10.2011 00:04, Benoit Jacob wrote:
>>>> Please no fake reviewers like "my-dog" or "burning"
>>>
>>> I can understand the concern about using one's dog as reviewer, but
>>> what's the problem with "burning"? In practice, we often use
>>> "r=bustage". This used to be considered OK and, as far as I can see,
>>> necessary. Did that change?
>>
>> When and why was this necessary? I'd encourage people to leave out the review tag if no review happened and they think that's OK. "bustage" can't review patches. For a review-worthy bustage fix, you should probably get ad-hoc review on IRC or back out the broken changeset and take it back to bugzilla.
>

> I agree. We shouldn't being forcing the content of our commit messages into a framework that it doesn't fit. I also push changes from upstream repositories that have not had review and would rather not and ugly tags to the commit message with to make a script happy.
>
> Do we actually have a problem where people are omitting their reviewers? I'd rather this energy be put toward solving real problems like helping people avoid including the wrong bug number.
>

As I said before this is not enforced and probably won't be in the near future.
But I agree we should figure out a way to check bug ids, also I think
it's hard to find the right way, if it's really the right bug. Maybe
we could check if the bug at least exists and isn't closed, and
assigned to that person. But it's then still going to happen. This
raises even more interesting problems, but what ever?
> -Jeff

[Boris Zbarsky]

On 10/31/11 10:33 AM, Gijs Kruitbosch wrote:
> Beside all that, I don't recall ever having needed the *reviewer* of the
> patch, rather than either the author or the committer

Usually you need that when the patch author is someone who doesn't
contribute very much, especially if no longer active. In that situation
the committer is generally completely unrelated to the patch and the
reviewer may be the go-to person for regressions.

This is a pretty rare case, though, and as long as the bug# is right you
can figure out who to get hold of.

-Boris

[Rafael Ávila de Espíndola]

> When and why was this necessary? I'd encourage people to leave out the
> review tag if no review happened and they think that's OK. "bustage"
> can't review patches. For a review-worthy bustage fix, you should
> probably get ad-hoc review on IRC or back out the broken changeset and
> take it back to bugzilla.

In another codebase what I have seen is the use of TBR (to be reviewed)
for things that have to go in with some urgency. So we could have

r=foo -> foo reviewed this already
tbr=foo -> foo is expected to review this. Would probably mean there is
a "r?(foo)" in the patch in bugzilla.

Cheers,
Rafael

[Rob Campbell]

Everybody makes mistakes. There are a few cases where a hook like this might save someone the hassle of backing out and recommitting and pushing a message fix. That can be costly, for sure, and I expect this is the origin of this discussion.

On 2011-10-31, at 11:21 , Jeff Muizelaar wrote:

> On 2011-10-31, at 5:53 AM, Dao wrote:
>

>> On 30.10.2011 00:04, Benoit Jacob wrote:
>>>> Please no fake reviewers like "my-dog" or "burning"
>>>
>>> I can understand the concern about using one's dog as reviewer, but
>>> what's the problem with "burning"? In practice, we often use
>>> "r=bustage". This used to be considered OK and, as far as I can see,
>>> necessary. Did that change?
>>
>> When and why was this necessary? I'd encourage people to leave out the review tag if no review happened and they think that's OK. "bustage" can't review patches. For a review-worthy bustage fix, you should probably get ad-hoc review on IRC or back out the broken changeset and take it back to bugzilla.
>

> I agree. We shouldn't being forcing the content of our commit messages into a framework that it doesn't fit. I also push changes from upstream repositories that have not had review and would rather not and ugly tags to the commit message with to make a script happy.

+1. This feels like it has the potential to make checking in more annoying for some or many.

> Do we actually have a problem where people are omitting their reviewers? I'd rather this energy be put toward solving real problems like helping people avoid including the wrong bug number.

This won't catch the class of commits where the committer pastes in the wrong bug number (this happened to me last week and I spent a good chunk of the day cleaning it up).

I could have saved myself that trouble/time/embarrassment and loss of tree time if I'd paid closer attention and not let distractions get in the way. Sadly, real life is hard.

On 2011-10-31, at 11:35 , Tom Schuster wrote:
> As I said before this is not enforced and probably won't be in the near future.
> But I agree we should figure out a way to check bug ids, also I think
> it's hard to find the right way, if it's really the right bug. Maybe
> we could check if the bug at least exists and isn't closed, and
> assigned to that person. But it's then still going to happen. This
> raises even more interesting problems, but what ever?

Check bug ids how? Verify that they exist? That won't solve the problem of someone checking in a patch with the wrong (but existent) bug number. Checking that the patch committed is the actual patch in the bug could also be problematic (e.g., what if there's more than one patch?)

If you're using a hash to verify that the patch is identical, it will reject patches that have been tweaked by the committer based on feedback in the bug. This happens quite frequently.

I think adding rules for this type of work is going to be problematic unless we hook up the ability to checkin directly from bugzilla itself. It feels to me like placing the logic in an HG hook is the wrong place to put it.

Anyway, I don't wanna be all Mr. Stop Energy. I think there's a kernel of a real problem here, I'm just concerned that adding rules to an already rule-laden system will create more pain for its users than solve problems.

(Note that most of the rules we have in place are enforced by the community and work Very Very Well. On a bad commit, you usually don't have to wait very long before you'll get a ping in #developers notifying you of a mistake. Or you'll get backed out. The System Works. /soapbox)

Rob

[Ehsan Akhgari]

2011/10/31 Rafael Ávila de Espíndola <respi...@mozilla.com>

> When and why was this necessary? I'd encourage people to leave out the
>> review tag if no review happened and they think that's OK. "bustage"
>> can't review patches. For a review-worthy bustage fix, you should
>> probably get ad-hoc review on IRC or back out the broken changeset and
>> take it back to bugzilla.
>>
>

> In another codebase what I have seen is the use of TBR (to be reviewed)
> for things that have to go in with some urgency. So we could have
>
> r=foo -> foo reviewed this already
> tbr=foo -> foo is expected to review this. Would probably mean there is a
> "r?(foo)" in the patch in bugzilla.
>

This assumes that we have a process for doing post-reviews, which we
currently don't.

I think we don't really need to discuss what the review tag should look
like for now, unless someone can demonstrate that enforcing them has a real
world value, and wants to drive that project forward. :-)

--
Ehsan
<http://ehsanakhgari.org/>

[Ehsan Akhgari]

On Mon, Oct 31, 2011 at 1:10 PM, Rob Campbell <rcam...@mozilla.com> wrote:

> I think adding rules for this type of work is going to be problematic
> unless we hook up the ability to checkin directly from bugzilla itself. It
> feels to me like placing the logic in an HG hook is the wrong place to put
> it.
>

Absolutely. This is something that RelEng has been working on, and we will
hopefully have this capability at some point in the future. Until then,
though, I think the commit message hook brings in more value than harm.
And as Tom mentioned, you shouldn't really notice it's existence at all
unless you're doing something strange, or wrong. :-)

--
Ehsan
<http://ehsanakhgari.org/>

[Tom Schuster]

So we are going to enable this on all other feeder repos, now. Any objections?

[Tom Schuster]

So hook has been put onto
- mozilla-central

- integration/fx-team/
- projects/jaegermonkey
- projects/build-system
- projects/devtools
- projects/places
- services/services-central

but won't be active till 2nd November