hook to prevent accidentally pushing without a bug number
Jesse Ruderman
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.
Jesse Ruderman
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
Boris Zbarsky
> 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
Nicholas Nethercote
>
>> 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
Boris Zbarsky
> 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
Nicholas Nethercote
>
> 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
Justin Dolske
> 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
Robert Kaiser
> 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! :)
Mike Connor
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
Mounir Lamouri
> I think we should have a push hook to prevent accidentally pushing
> 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
Mike Connor
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
Kyle Huey
I managed to accidentally push a new head to cedar a couple weeks ago, so I
don't believe that we do this ...
- Kyle
Mounir Lamouri
And it happened to mozilla-inbound 24 hours after its creation ;)
--
Mounir
Boris Zbarsky
> 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
Christian Legnitto
FWIW the pulse hook should be as it is a global hook. If you see different please let me know.
Christian
John O'Duinn
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.
Ehsan Akhgari
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!
Ehsan Akhgari
>
> On 2011-06-15, at 9:25 AM, Robert Kaiser wrote:
>
>>
>> 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.
>
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
Paul Biggar
>> 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
pbi...@mozilla.com
@paulbiggar
Ehsan Akhgari
> On Tue, Jun 14, 2011 at 23:53, Justin Dolske<dol...@mozilla.com> 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
>
> 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
Jeff Muizelaar
On 2011-06-15, at 2:00 PM, Paul Biggar wrote:
> 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.
We have the same problem on the graphics branch.
> I also believe that bug numbers are not cheap, but that is an discussion for another thread.
+1
-Jeff
Robert Kaiser
I list of hardcoded IDs that are not clearly visible is always prone to
unexpected failure when something changes slightly, that's why I dislike
that approach.
I also think that looking at every single changeset is really something
that will make merging project branches fail too much unless they're all
guaranteed to have the same hook running. Let's assume a simple case of
someone forgetting a bug number on checkin, backing out, pushing again
with the bug number - on merge, this would still fail the hook... (And
on a project branch, esp. in an early stage of development of a new
project, I agree with e.g. pbiggar that not every commit necessarily
must have a bug number.)
Robert Kaiser
So if I work on a personal, possibly even local repo and I make a single
"mischeckin" that fails this hook (i.e. has no bug number), I'm never
ever able to merge that repo in any form to m-c, right? Not so sure if
that's a cool idea...
Boris Zbarsky
> So if I work on a personal, possibly even local repo and I make a single
> "mischeckin" that fails this hook (i.e. has no bug number), I'm never
> ever able to merge that repo in any form to m-c, right?
If it's a local repo you can in fact qimport and fix the mischeckin.
Though we could make it possible to enable this hook on local repos too.
If it's not local, then if it's hosted on hg.mozilla.org the hook was
there in the first place.
If you're doing something else, it's your responsibility to not screw
things up.
But yes, if you have changesets that don't have a bug number but should,
then you should not be able to merge them to m-c. That's the whole point!
-Boris
Justin Dolske
>>> I think we should have a push hook to prevent accidentally pushing
>>> without a bug number.
>>
>> I strongly concur. Bug numbers are cheap.
>
> I disagree.
Deathmatch! :)
> 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.
I strongly concur with your disagreement!
My assumption was that this hook would only be for mozilla-central (and
branches that want it, like say mozilla-incoming). Merges from project
branches would be exempted, and the occasional "b=not_needed" would be
fine too.
It's important for future archaeology (and the other reasons Jesse
mentioned) to be able to associate a change with a bug number for
additional context. I frequently find that, when investigating the
history of old code, the lack of a bug number makes it impossible to
understand why something was done a particular way.
These same concerns may very well apply to code coming in through
project branches, but at least to begin with I don't think we need to
worry about it.
Justin
Boris Zbarsky
> These same concerns may very well apply to code coming in through
> project branches
They do. We end up paying that cost all the time: those initial
project branch prototypes that have no bug numbers and no decent code
comments pan out, make it into the product, and then we have
hard-to-maintain code....
That's a tradeoff we have to make to move fast, sometimes. :(
-Boris
Joshua Cranmer
> On 6/15/11 2:48 PM, Robert Kaiser wrote:
>> So if I work on a personal, possibly even local repo and I make a single
>> "mischeckin" that fails this hook (i.e. has no bug number), I'm never
>> ever able to merge that repo in any form to m-c, right?
>
> If it's a local repo you can in fact qimport and fix the mischeckin.
> Though we could make it possible to enable this hook on local repos too.
well if you're trying to import a merge changeset or something before a
merge point...
Boris Zbarsky
> Can you always qimport a changeset? I doubt that qimport works all that
> well if you're trying to import a merge changeset or something before a
> merge point...
Ah, yes, that is a problem.
Sounds like we want to make it possible to apply this hook to local
repos, yes.
-Boris
Ehsan Akhgari
>
> On 2011-06-15, at 2:00 PM, Paul Biggar wrote:
>
> > 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.
>
> We have the same problem on the graphics branch.
>
We already have a solution for it proposed, so this is not a problem.
> > I also believe that bug numbers are not cheap, but that is an discussion
> for another thread.
>
>
> +1
>
Let's not diverge on other topics in this thread. If you feel strongly,
please start a new thread.
--
Ehsan
<http://ehsanakhgari.org/>
Justin Dolske
>>> I think we should have a push hook to prevent accidentally pushing
>>> without a bug number.
>>
>> I strongly concur. Bug numbers are cheap.
>
> I disagree.
Deathmatch! :)
> 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.
I strongly concur with your disagreement!
My assumption was that this hook would only be for mozilla-central (and
branches that want it, like say mozilla-incoming). Merges from project
branches would be exempted, and the occasional "b=not_needed" would be
fine too.
It's important for future archaeology (and the other reasons Jesse
mentioned) to be able to associate a change with a bug number for
additional context. I frequently find that, when investigating the
history of old code, the lack of a bug number makes it impossible to
understand why something was done a particular way.
These same concerns may very well apply to code coming in through
Boris Zbarsky
> On 6/15/11 3:15 PM, Justin Dolske wrote:
>> These same concerns may very well apply to code coming in through
>> project branches
>
> They do. We end up paying that cost all the time: those initial project
> branch prototypes that have no bug numbers and no decent code comments
> pan out, make it into the product, and then we have hard-to-maintain
> code....
And to be clear, some people's stated end goal is that _all_ changes
come through project branches. Just worth keeping in mind. ;)
-Boris
Marco Bonardo
> 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.
I don't just agree, I beg we do this asap!
I just wrongly pushed a commit without bug # and immediately had to
backout it to fix the commit message, I always check with hg outgoing,
but the bug # doesn't appear with great evidence and I missed it.
Without taking into account times when, as a sheriff, you have to
backout something to make the tree green and you don't know where to
annotate the backout.
-m