hook to prevent accidentally pushing without a bug number

classic Classic list List threaded Threaded
31 messages Options
12
Reply | Threaded
Open this post in threaded view
|

hook to prevent accidentally pushing without a bug number

Bugzilla from jruderman@gmail.com
Commits without bug numbers mess with regression hunting, regression
filing, backouts, and patch attribution.  I also rely on being able to
get a list of bugs from hg in order to write The Burning Edge,
identify fixes that need fuzzing or security review, and identify
fixes that need Feature Pages.

I think we should have a push hook to prevent accidentally pushing
without a bug number. For example, it could require that each commit
has at least one of the following:

* Have "bug" or "b=" followed by a bug number
* Have "no bug"
* Are backing out a specified changeset:
    (back out|backing out|backed out|backout)( of)?( revs?|
changesets?|)? [0-9a-f]{12}
* Start with "merge" and is a merge changeset
* Come from user "ffxbld"

I have been advised that rather than just asking RelEng to do this in
https://bugzilla.mozilla.org/show_bug.cgi?id=660705, I should see if I
can get consensus here first.
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Bugzilla from jruderman@gmail.com
To estimate the false positive rate, I looked through all 675 commits
that were pushed to mozilla-central in the last 2 weeks. I included
commits that were merged from project branches in this timeframe.

31 commits (4.5%) would have failed the proposed push hook. Of those,
9 (29%) were the kinds of bad commit messages the hook is intended to
catch.

I think that's an acceptable false positive rate given the amount of
pain caused by missing information in commit messages. Additionally,
I'd expect the false positive rate to fall as committers get in the
habit of writing "no bug" when it's appropriate.

Changesets that really should have had bug numbers:
http://hg.mozilla.org/mozilla-central/rev/6031f209f2df ("backout the
backout", gross)
http://hg.mozilla.org/mozilla-central/rev/259c61798455 (test
disablement that not only lacked a bug number in the commit message
but also lacked a bug number in the makefile itself, and used the
wrong ifdef)
http://hg.mozilla.org/mozilla-central/rev/ac8fceaec76c (bug 662001)
http://hg.mozilla.org/mozilla-central/rev/bcccd02b5294 (bug 661091)
http://hg.mozilla.org/mozilla-central/rev/bac5be017ca7 (sync)
http://hg.mozilla.org/mozilla-central/rev/124aef7f7e0b (sync)
http://hg.mozilla.org/mozilla-central/rev/1f3102c47d46 (sync)

Vague backouts, which should have had a changeset ID or bug number:
http://hg.mozilla.org/mozilla-central/rev/436671b3bee7
http://hg.mozilla.org/mozilla-central/rev/a7eb0646a1b2

Bustage fixes, for which "followup to bug N" would be best:
http://hg.mozilla.org/mozilla-central/rev/4e2b86b6025c
http://hg.mozilla.org/mozilla-central/rev/31ea40628d48
http://hg.mozilla.org/mozilla-central/rev/01459cfa4b93
http://hg.mozilla.org/mozilla-central/rev/0da3c586a67c

Small warning/comment/whitespace fixes, for which "no bug" may have
been appropriate:
http://hg.mozilla.org/mozilla-central/rev/d6321d3a7dd7
http://hg.mozilla.org/mozilla-central/rev/eab02b0b7c7d
http://hg.mozilla.org/mozilla-central/rev/26967952b740
http://hg.mozilla.org/mozilla-central/rev/80231df12d57
http://hg.mozilla.org/mozilla-central/rev/0cd7128926db

Test fixes, for which "no bug" may have been appropriate:
http://hg.mozilla.org/mozilla-central/rev/4e334541e521
http://hg.mozilla.org/mozilla-central/rev/7479151a437a
http://hg.mozilla.org/mozilla-central/rev/d799c168c98d
http://hg.mozilla.org/mozilla-central/rev/eb0ab9014998
http://hg.mozilla.org/mozilla-central/rev/3cc54a498f86
http://hg.mozilla.org/mozilla-central/rev/533acc569efc
http://hg.mozilla.org/mozilla-central/rev/87291fa946cb
http://hg.mozilla.org/mozilla-central/rev/e89ff3fdbec0
http://hg.mozilla.org/mozilla-central/rev/91dd5412cd4f

Version bump for nanojit-import-rev stamp:
http://hg.mozilla.org/mozilla-central/rev/8b82f76719c6

Version bump for sync:
http://hg.mozilla.org/mozilla-central/rev/4adf0486b692

A temporary diagnostic that was backed out two csets later:
http://hg.mozilla.org/mozilla-central/rev/6f4ca81b13d5

A typo, "Merginig":
http://hg.mozilla.org/mozilla-central/rev/1e3af440ce23
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Boris Zbarsky
In reply to this post by Bugzilla from jruderman@gmail.com
On 6/14/11 7:19 PM, Jesse Ruderman wrote:
> 31 commits (4.5%) would have failed the proposed push hook. Of those,
> 9 (29%) were the kinds of bad commit messages the hook is intended to
> catch.
...
> Changesets that really should have had bug numbers:

7 of these.

> Vague backouts, which should have had a changeset ID or bug number:

2 of these.

> Bustage fixes, for which "followup to bug N" would be best:

In my opinion, these should absolutely have a bug number.  4 of these.

> Small warning/comment/whitespace fixes, for which "no bug" may have
> been appropriate:

5 of these.

> Test fixes, for which "no bug" may have been appropriate:

9 of these.

> Version bump for nanojit-import-rev stamp:
> http://hg.mozilla.org/mozilla-central/rev/8b82f76719c6

Why doesn't this have a bug number?

> Version bump for sync:
> http://hg.mozilla.org/mozilla-central/rev/4adf0486b692

Likewise.

> A temporary diagnostic that was backed out two csets later:
> http://hg.mozilla.org/mozilla-central/rev/6f4ca81b13d5

"no bug" or the bug being investigated would be appropriate.

> A typo, "Merginig":

As far as I can tell, of the 31 changesets involved, 15 should have had
bug numbers but did not, 14 could/should have had "no bug", one could
have gone either way and 1 was the typo.

I agree that this would be a good hook to have, in case it's not clear.  ;)

-Boris

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Nicholas Nethercote
On Wed, Jun 15, 2011 at 9:46 AM, Boris Zbarsky <[hidden email]> wrote:
>
>> Version bump for nanojit-import-rev stamp:
>> http://hg.mozilla.org/mozilla-central/rev/8b82f76719c6
>
> Why doesn't this have a bug number?

https://developer.mozilla.org/en/NanojitMerge has the full gory
details of how Nanojit changes are merged to tracemonkey.  Basically,
it's not a normal merge, but a special tree splice (nb: "splice" may
not be the correct terminology).  It's done this way because both
Mozilla and Adobe use Nanojit.

Each splice contains one or more patches (that's why it doesn't have a
bug number), and then a changeset for the nanojit-import-rev stamp
update, which is necessary data to tell when the next splice should
begin.  The splice is done automatically by the 'update-nanojit' make
target.
The message can easily be changed to start with "merge" or something,
but it'll always be the same message, I hope that's ok because doing
otherwise is harder.

Nick
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Boris Zbarsky
In reply to this post by Boris Zbarsky
On 6/14/11 8:28 PM, Nicholas Nethercote wrote:
> The message can easily be changed to start with "merge" or something,
> but it'll always be the same message, I hope that's ok because doing
> otherwise is harder.

Yeah, that sounds just fine in this case.

-Boris
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Nicholas Nethercote
In reply to this post by Bugzilla from jruderman@gmail.com
On Wed, Jun 15, 2011 at 9:17 AM, Jesse Ruderman <[hidden email]> wrote:
>
> I think we should have a push hook to prevent accidentally pushing
> without a bug number.

Sounds good to me.  I've forgotten the bug number a couple of times
and then felt sheepish afterwards.

My only concern is that the failure message should be nice and clear
about what an acceptable log message looks like.

Nick
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Justin Dolske-2
In reply to this post by Bugzilla from jruderman@gmail.com
On 6/14/11 4:17 PM, Jesse Ruderman wrote:

> I think we should have a push hook to prevent accidentally pushing
> without a bug number. For example, it could require that each commit
> has at least one of the following:

I strongly concur. Bug numbers are cheap. Even if it's a simple change,
it's easy to file a bug and wouldn't be the first time a trivial change
had unexpected side effects. Bug numbers fit perfectly into our system
for tracking regressions, dependencies, and backouts.

Justin
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Robert Kaiser
In reply to this post by Bugzilla from jruderman@gmail.com
Jesse Ruderman schrieb:
> I think we should have a push hook to prevent accidentally pushing
> without a bug number.

I love that idea.

> * Come from user "ffxbld"

That's problematic for other releng teams than the Firefox one. Could we
make anything containing "CLOSED TREE" to still be accepted? We already
have that in all the automated messages from the release process, so
those would "just work".
Making an exception for a single user ID is always wonky, something else
is preferred.

Robert Kaiser


--
Note that any statements of mine - no matter how passionate - are never
meant to be offensive but very often as food for thought or possible
arguments that we as a community should think about. And most of the
time, I even appreciate irony and fun! :)
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Mike Connor-4

On 2011-06-15, at 9:25 AM, Robert Kaiser wrote:

>> * Come from user "ffxbld"
>
> That's problematic for other releng teams than the Firefox one. Could we make anything containing "CLOSED TREE" to still be accepted? We already have that in all the automated messages from the release process, so those would "just work".
> Making an exception for a single user ID is always wonky, something else is preferred.

I would rather have a list of known releng IDs (there's what, three? four?) than overloading CLOSED TREE, since human committers use that as well, and I definitely want those checkins to tie back to a bug number.

-- Mike
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Mounir Lamouri-3
In reply to this post by Bugzilla from jruderman@gmail.com
On 06/15/2011 01:17 AM, Jesse Ruderman wrote:
> I think we should have a push hook to prevent accidentally pushing
> without a bug number. For example, it could require that each commit
> has at least one of the following:

The idea is great but can we make sure this hook will also be used on
all project branches and mozilla-inbound? Otherwise this could lead to
very annoying situations.

--
Mounir
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Mike Connor-4

On 2011-06-15, at 11:22 AM, Mounir Lamouri wrote:

> On 06/15/2011 01:17 AM, Jesse Ruderman wrote:
>> I think we should have a push hook to prevent accidentally pushing
>> without a bug number. For example, it could require that each commit
>> has at least one of the following:
>
> The idea is great but can we make sure this hook will also be used on all project branches and mozilla-inbound? Otherwise this could lead to very annoying situations.

Indeed.  We should make sure that any hooks designed to prevent bad behaviour on m-c (i.e. pushing new heads) are also enabled on project branches that will merge to m-c.  I think we already do this...

-- Mike
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Kyle Huey-2
On Wed, Jun 15, 2011 at 8:41 AM, Mike Connor <[hidden email]> wrote:

>
> On 2011-06-15, at 11:22 AM, Mounir Lamouri wrote:
>
> > On 06/15/2011 01:17 AM, Jesse Ruderman wrote:
> >> I think we should have a push hook to prevent accidentally pushing
> >> without a bug number. For example, it could require that each commit
> >> has at least one of the following:
> >
> > The idea is great but can we make sure this hook will also be used on all
> project branches and mozilla-inbound? Otherwise this could lead to very
> annoying situations.
>
> Indeed.  We should make sure that any hooks designed to prevent bad
> behaviour on m-c (i.e. pushing new heads) are also enabled on project
> branches that will merge to m-c.  I think we already do this...
>

I managed to accidentally push a new head to cedar a couple weeks ago, so I
don't believe that we do this ...

- Kyle
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Mounir Lamouri-3
On 06/15/2011 06:17 PM, Kyle Huey wrote:

> On Wed, Jun 15, 2011 at 8:41 AM, Mike Connor <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>
>     On 2011-06-15, at 11:22 AM, Mounir Lamouri wrote:
>
>      > On 06/15/2011 01:17 AM, Jesse Ruderman wrote:
>      >> I think we should have a push hook to prevent accidentally pushing
>      >> without a bug number. For example, it could require that each commit
>      >> has at least one of the following:
>      >
>      > The idea is great but can we make sure this hook will also be
>     used on all project branches and mozilla-inbound? Otherwise this
>     could lead to very annoying situations.
>
>     Indeed.  We should make sure that any hooks designed to prevent bad
>     behaviour on m-c (i.e. pushing new heads) are also enabled on
>     project branches that will merge to m-c.  I think we already do this...
>
>
> I managed to accidentally push a new head to cedar a couple weeks ago,
> so I don't believe that we do this ...

And it happened to mozilla-inbound 24 hours after its creation ;)

--
Mounir
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Boris Zbarsky
In reply to this post by Mounir Lamouri-3
On 6/15/11 11:41 AM, Mike Connor wrote:
> Indeed.  We should make sure that any hooks designed to prevent bad behaviour on m-c (i.e. pushing new heads) are also enabled on project branches that will merge to m-c.  I think we already do this...

Neither the multiple heads hook nor the closed tree hook are set up for
project branches.

-Boris
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Christian Legnitto
On Jun 15, 2011, at 9:40 AM, Boris Zbarsky wrote:

> On 6/15/11 11:41 AM, Mike Connor wrote:
>> Indeed.  We should make sure that any hooks designed to prevent bad behaviour on m-c (i.e. pushing new heads) are also enabled on project branches that will merge to m-c.  I think we already do this...
>
> Neither the multiple heads hook nor the closed tree hook are set up for project branches.
>
> -Boris

FWIW the pulse hook should be as it is a global hook. If you see different please let me know.

Christian

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

John O'Duinn
In reply to this post by Justin Dolske-2


On 6/14/11 11:53 PM, Justin Dolske wrote:
> On 6/14/11 4:17 PM, Jesse Ruderman wrote:
>
>> I think we should have a push hook to prevent accidentally pushing
>> without a bug number. For example, it could require that each commit
>> has at least one of the following:
>
> I strongly concur.
+1

> Bug numbers are cheap. Even if it's a simple change,
> it's easy to file a bug and wouldn't be the first time a trivial change
> had unexpected side effects. Bug numbers fit perfectly into our system
> for tracking regressions, dependencies, and backouts.
The more project branches we use, the more important bug#s are for
tracking landings, backouts, and followup fixes across all the different
branches.

> Justin
tc
John.
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Ehsan Akhgari
In reply to this post by Boris Zbarsky
On 11-06-15 12:40 PM, Boris Zbarsky wrote:
> On 6/15/11 11:41 AM, Mike Connor wrote:
>> Indeed. We should make sure that any hooks designed to prevent bad
>> behaviour on m-c (i.e. pushing new heads) are also enabled on project
>> branches that will merge to m-c. I think we already do this...
>
> Neither the multiple heads hook nor the closed tree hook are set up for
> project branches.

That sucks.  It's now bug 664493.

In general, I think we should follow this simple rule: we should have
the same set of hooks on our code repositories, with the exception of
the try server and stable branches (which is currently only 1.9.2,
IINM), right?

Ehsan

PS. I fully support Jesse's suggestion, and I do think we have
consensus, so we can proceed with this!
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Ehsan Akhgari
In reply to this post by Mike Connor-4
On 11-06-15 9:46 AM, Mike Connor wrote:
>
> On 2011-06-15, at 9:25 AM, Robert Kaiser wrote:
>
>>> * Come from user "ffxbld"
>>
>> That's problematic for other releng teams than the Firefox one. Could we make anything containing "CLOSED TREE" to still be accepted? We already have that in all the automated messages from the release process, so those would "just work".
>> Making an exception for a single user ID is always wonky, something else is preferred.
>
> I would rather have a list of known releng IDs (there's what, three? four?) than overloading CLOSED TREE, since human committers use that as well, and I definitely want those checkins to tie back to a bug number.

On top of that, CLOSED TREE is checked on the topmost hook, while the
suggested hook is per-changeset, so we can't use CLOSED TREE at all.

Ehsan
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Paul Biggar-3
In reply to this post by Justin Dolske-2
On Tue, Jun 14, 2011 at 23:53, Justin Dolske <[hidden email]> wrote:
>> I think we should have a push hook to prevent accidentally pushing
>> without a bug number. For example, it could require that each commit
>> has at least one of the following:
>
> I strongly concur. Bug numbers are cheap. Even if it's a simple change, it's

I disagree. Consider the type-inference branch, currently on
http://hg.mozilla.org/projects/jaegermonkey. Many of the changesets do
not have a bug number, nor should they. Imposing that kind of overhead
on prototypes is counter-productive, and when those prototypes turn
into products, we don't want to just lose that history.

Perhaps when we merge, we should auto-rewrite those logs, but that has
problems of its own (breaks bidirectional merges, changes revision
SHAs and hence hg.m.o links, probably more).

Potential solutions:
- don't reject for changesets *dated* before July 1st, 2011 (or whatever)
  - we need plenty of notice so that future work can always have a bug number
  - prototypes can use a meta bug, or 'no bug'
- turn the hook off for specific merges
  - perhaps the head of the push could have a specific message to turn
the hook off?

I support the hook, on as many repos as possible), but don't want to
lose history, code, changeset links. I also believe that bug numbers
are not cheap, but that is an discussion for another thread.

Paul

--
Paul Biggar
Compiler Geek
[hidden email]
@paulbiggar
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: hook to prevent accidentally pushing without a bug number

Ehsan Akhgari
On 11-06-15 2:00 PM, Paul Biggar wrote:

> On Tue, Jun 14, 2011 at 23:53, Justin Dolske<[hidden email]>  wrote:
>>> I think we should have a push hook to prevent accidentally pushing
>>> without a bug number. For example, it could require that each commit
>>> has at least one of the following:
>>
>> I strongly concur. Bug numbers are cheap. Even if it's a simple change, it's
>
> I disagree. Consider the type-inference branch, currently on
> http://hg.mozilla.org/projects/jaegermonkey. Many of the changesets do
> not have a bug number, nor should they. Imposing that kind of overhead
> on prototypes is counter-productive, and when those prototypes turn
> into products, we don't want to just lose that history.
>
> Perhaps when we merge, we should auto-rewrite those logs, but that has
> problems of its own (breaks bidirectional merges, changes revision
> SHAs and hence hg.m.o links, probably more).
>
> Potential solutions:
> - don't reject for changesets *dated* before July 1st, 2011 (or whatever)
>    - we need plenty of notice so that future work can always have a bug number
>    - prototypes can use a meta bug, or 'no bug'
> - turn the hook off for specific merges
>    - perhaps the head of the push could have a specific message to turn
> the hook off?

I'd support having a trigger in the topmost changeset to turn checking
each individual commit off.

Ehsan
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
12