Adding "APPROVAL REQUIRED" to the CLOSED TREE hook.
Johnathan Nightingale
Back in November of last year, we added a hook to hg to prevent
checkins on a closed tree (unless people used the magic words, "CLOSED
TREE" as part of their checkin). People had differing opinions about
whether magic words were the best approach, but overall I think that
hook has been useful for us.
Sufficiently so, that recently someone asked if we could add a
restricted mode for branch trees like mozilla-1.9.2, to allow drivers
or sheriffs to restrict landings to those which have been explicitly
approved (bug 510211). I've augmented the hook to look for "a="
somewhere in each push comment (or topmost comment, for multi-
changeset pushes/merges) when the tinderbox reports the tree to be
"APPROVAL REQUIRED".
Remember, this doesn't impact OPEN trees in any way, it just gives
drivers/sheriffs a mechanism to put the tree into a state where
approvals are explicitly required for checkin.
It's been tested on the places repo successfully, my hope is that it's
not too onerous, and that we can push it live to our main trees this
week.
Yea, or Nay?
Johnathan
---
Johnathan Nightingale
Human Shield
joh...@mozilla.com
L. David Baron
> Sufficiently so, that recently someone asked if we could add a
> restricted mode for branch trees like mozilla-1.9.2, to allow drivers or
> sheriffs to restrict landings to those which have been explicitly
> approved (bug 510211). I've augmented the hook to look for "a="
> somewhere in each push comment (or topmost comment, for multi-changeset
> pushes/merges) when the tinderbox reports the tree to be "APPROVAL
> REQUIRED".
For what it's worth, I tend to write approvals as a1.9.1.4=dveditz
or a1.9.2=roc rather than a=dveditz or a=roc, since I think it
increases the chance that somebody will catch my mistake if I
accidentally push something to the wrong release branch or check
something in at the wrong time (e.g., when separate beta approval is
required). (I'm not intending to rely on others to catch such
mistakes, though.)
I'm happy to change back, but I wonder if it's worth allowing the
tree hook allow for formats other than a=.
-David
--
L. David Baron http://dbaron.org/
Mozilla Corporation http://www.mozilla.com/
Doug Turner
stable branches, I have been doing the following:
Download the changeset from hg:
wget http://hg.mozilla.org/mozilla-central/raw-rev/<changesetid>
Then simply calling:
hg import <changesetid>
This ensures that everything gets applied (including new files), and
uses the checkin comment used on the trunk.
Requiring extra strings to be added to the checkin line would add
another step to this process.
I am not really complaining -- as it usually takes a few hours to land
a patch given that one must watch the tree to ensure things stay
green. Instead, I am wondering if there is a better way to move
patches that landed on the trunk to the stable branch?
Doug
On Sep 2, 2009, at 6:38 AM, Johnathan Nightingale wrote:
> [followups to dev-tree-management, please]
>
> Back in November of last year, we added a hook to hg to prevent
> checkins on a closed tree (unless people used the magic words,
> "CLOSED TREE" as part of their checkin). People had differing
> opinions about whether magic words were the best approach, but
> overall I think that hook has been useful for us.
>
> Sufficiently so, that recently someone asked if we could add a
> restricted mode for branch trees like mozilla-1.9.2, to allow
> drivers or sheriffs to restrict landings to those which have been
> explicitly approved (bug 510211). I've augmented the hook to look
> for "a=" somewhere in each push comment (or topmost comment, for
> multi-changeset pushes/merges) when the tinderbox reports the tree
> to be "APPROVAL REQUIRED".
>
> Remember, this doesn't impact OPEN trees in any way, it just gives
> drivers/sheriffs a mechanism to put the tree into a state where
> approvals are explicitly required for checkin.
>
> It's been tested on the places repo successfully, my hope is that
> it's not too onerous, and that we can push it live to our main trees
> this week.
>
> Yea, or Nay?
>
> Johnathan
>
> ---
> Johnathan Nightingale
> Human Shield
> joh...@mozilla.com
>
>
>
> _______________________________________________
> dev-tree-management mailing list
> dev-tree-...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-tree-management
Mike Beltzner
> Yea, or Nay?
As a driver, I think I can speak for both Sam and myself when I say:
yay!
David's comments about making sure that we look for a[1-9\.]*= in the
regex seem valid, as different people use different notation here.
Should we also have a magic phrase, as we do with the CLOSED TREE
hook, that allows anyone to push past without having to seek approval?
cheers,
mike
Johnathan Nightingale
> On 02.09.2009 15:38, Johnathan Nightingale wrote:
>> Remember, this doesn't impact OPEN trees in any way
>
> 1.9.1 is "OPEN for approved patches". It would currently be
> considered open, right?
> hg import http://hg.mozilla.org/mozilla-central/raw-rev/foo has been
> my way to move stuff from trunk to branches for quite some time now,
> and I wouldn't want to change this.
[Shifting to dev-tree-management]
Yes, right now there is no hook, and hence no magic string, so "OPEN
for approved patches" wouldn't trigger the hook. I suspect that if
this were deployed, that text would be updated, since 1.9.1 is a tree
where I'd expect this to be useful.
As for your import workflow, yes, with this hook in place it would
likely get one step more complicated for any changeset whose original
commit message didn't include "a=...". I suspect it would turn into:
hg import http://hg.mozilla.org/mozilla-central/raw-rev/foo -m "Commit
message a=bar"
J
Mike Beltzner
On 2-Sep-09, at 10:13 AM, Dao wrote:On 02.09.2009 15:38, Johnathan Nightingale wrote:Remember, this doesn't impact OPEN trees in any way1.9.1 is "OPEN for approved patches". It would currently be considered open, right?hg import http://hg.mozilla.org/mozilla-central/raw-rev/foo has been my way to move stuff from trunk to branches for quite some time now, and I wouldn't want to change this.
As for your import workflow, yes, with this hook in place it would likely get one step more complicated for any changeset whose original commit message didn't include "a=...". I suspect it would turn into:
hg import http://hg.mozilla.org/mozilla-central/raw-rev/foo -m "Commit message a=bar"
Johnathan Nightingale
> On 2009-09-02, at 9:38 AM, Johnathan Nightingale wrote:
>
>> Yea, or Nay?
>
> As a driver, I think I can speak for both Sam and myself when I say:
> yay!
Huzzah!
> David's comments about making sure that we look for a[1-9\.]*= in
> the regex seem valid, as different people use different notation here.
Yeah, I agree. Ted also suggested adding a testing tree to the list of
known trees in the hook, so that we don't need to experiment on
places. :) I'll update the patch before pushing, once I see how the
rest of the discussion evolves here, if needed.
> Should we also have a magic phrase, as we do with the CLOSED TREE
> hook, that allows anyone to push past without having to seek approval?
Implicitly, I think there already is. "a=me" or "a=whatever" or
"a=because I said so" will work - we're still on honour system here,
just catching people who might not have realized that approvals were
now required on this tree.
Boris Zbarsky
> hg import http://hg.mozilla.org/mozilla-central/raw-rev/foo -m "Commit
> message a=bar"
I should just note that this requires separately looking up the old
commit message, which is a bit of a pain... Then again, I've been
adding a= anyway (via qimport/qref), so it's all the same to me.
-Boris
Mike Beltzner
> I should just note that this requires separately looking up the old
> commit message, which is a bit of a pain... Then again, I've been
> adding a= anyway (via qimport/qref), so it's all the same to me.
Well, you'd have to have approvals to get those things landed on
branch, yes? If they already had approvals, then they'd make it past
the hook on their own.
On 2009-09-02, at 10:41 AM, Dao wrote:
> Have there been accidental pushes to branches that would indicate
> that we need this new enforcement?
Sam indicated at the monday meeting that he's often had to remind
people. I think that because the status of the trees and branches
changes, it's actually helpful to developers to have these tools that
prevent people from making innocent mistakes.
I see this a lot like the CLOSED TREE hook, which is meant to prevent
simple errors, not actually block access (it's trivially defeated, f.e.)
> By the way, 1.9.2 doesn't require approval for blocking bugs and
> NPOTB landings.
Yes, and I would expect that a=NPOTB or a=test works fine under this
scheme.
cheers,
mike
Boris Zbarsky
>> I should just note that this requires separately looking up the old
>> commit message, which is a bit of a pain... Then again, I've been
>> adding a= anyway (via qimport/qref), so it's all the same to me.
>
> Well, you'd have to have approvals to get those things landed on branch,
> yes? If they already had approvals, then they'd make it past the hook on
> their own.
I'm not sure I follow....
-Boris
Mike Beltzner
Maybe it's me who's not following.
If something landed on trunk it may not have required approval to do
so, and so I'd expect it to not have an a= statement. However, once
approved for branch, it *would* require an a= statement, and indeed in
cases where it was approved by me for 1.9.2 and then Sam for 1.9.1
those approvers might be entirely different people.
I suppose there's a scenario - more common for someone like yourself
who works on security bugs - where you get approval for multiple
branches. I suppose my answer there is: suck it up, use that hg
command johnath wrote ;)
cheers,
mike
Boris Zbarsky
> I suppose there's a scenario - more common for someone like yourself who
> works on security bugs - where you get approval for multiple branches. I
> suppose my answer there is: suck it up, use that hg command johnath
> wrote ;)
My point was that to use that command you have to know the checkin
comment that was used for that changeset you're importing, since you
have to put it in the -m message before the a= part. Which means either
loading that url by hand and copy/pasting the comment or something.
Like I said, it's not that big a deal. My "something" has been to
qimport the changeset instead of just importing, qh to get the checkin
comment, qref to change the checkin comment to the one I copy/paste from
the qh output plus the a= thing.
-Boris
P.S. Getting approval for multiple branches is in the suck it up
category no matter what, since you have to watch all the trees, set all
the different flags, etc. ;)
Boris Zbarsky
> I suppose there's a scenario - more common for someone like yourself who
> works on security bugs - where you get approval for multiple branches. I
> suppose my answer there is: suck it up, use that hg command johnath
> wrote ;)
My point was that to use that command you have to know the checkin
L. David Baron
> You can use this slightly more complicated workflow:
>
> * hg qimport http://hg.mozilla.org/mozilla-central/raw-rev/<changesetid>
> * hg qpush
> * hg qref -e # add approval information
> * hg qfin .
> * hg push
For what it's worth, my merging workflow is a single hg command that
allows this:
hg transplant -s ../mozilla-central/ --filter vim <changesetid>
which then opens in vim so I can edit:
(a) the commit message and metadata
(b) the patch itself
The only thing I actually edit is the commit message.
(This requires the transplant extension be enabled.)
Benjamin Smedberg
> If something landed on trunk it may not have required approval to do so,
> and so I'd expect it to not have an a= statement. However, once approved
> for branch, it *would* require an a= statement, and indeed in cases
> where it was approved by me for 1.9.2 and then Sam for 1.9.1 those
> approvers might be entirely different people.
I have often been landing things on branch which were properly approved, but
not modifying the commit message with an extra a= annotation so that I could
just use `hg import`. I took "patches require approval" to mean you had to
have approval in the bug, not that you had to always mark it in the cset.
If we want to require people to use an explicit a= commit message, that's
fine (and I provided the MQ magic to do so in dev.planning), but that's not
what people have been doing, at least not consistently.
--BDS
Serge Gautherie
> David's comments about making sure that we look for a[1-9\.]*= in the
> regex seem valid, as different people use different notation here.
I use 'a-1.9.1.4='...
> Should we also have a magic phrase, as we do with the CLOSED TREE hook,
> that allows anyone to push past without having to seek approval?
Maybe always use 'APPROVED' or something, for that new hook?
Nick Thomas
> Should we also have a magic phrase, as we do with the CLOSED TREE hook,
> that allows anyone to push past without having to seek approval?
The release automation needs to push a version bump (eg 3.5.3pre to
3.5.3) whenever we start a new release process. We already use CLOSED
TREE in the commit message and can easily add "a=drivers" too, so
wouldn't require another magic word.
-Nick
Justin Dolske
> Like I said, it's not that big a deal. My "something" has been to
> qimport the changeset instead of just importing, qh to get the checkin
> comment, qref to change the checkin comment to the one I copy/paste from
> the qh output plus the a= thing.
Rob Arnold's qimportbz for FTW!
http://robarnold.org/hg-qimport-my-bugzilla-patch-redux/
When landing on branches, I just do:
hg qimportbz 123456
hg push
I don't think it currently adds "a=blocking192" to the commit message,
but that should be easy to fix.
Justin
Boris Zbarsky
> When landing on branches, I just do:
>
> hg qimportbz 123456
> hg push
>
> I don't think it currently adds "a=blocking192" to the commit message,
> but that should be easy to fix.
Won't work for patches that came from someone's mq (and hence have a
commit message already), for what it's worth.
Which I hope is the common case; the commit messages qimportbz generates
are the best it can do without some impressive AI, but pretty crappy as
commit messages go.
-Boris
Mark Banner
> Should we also have a magic phrase, as we do with the CLOSED TREE hook,
> that allows anyone to push past without having to seek approval?
NPOTB or NPOTFFB seems to be one we should definitely have as we the
approval requirements typically allow those patches.
Maybe TEST-ONLY as well.
Standard8
Mark Banner
ok, so I just read the a=NPOTB or a=test suggestions in the other part
of the thread. They would be fine.
Standard8
Robert Kaiser
> Maybe TEST-ONLY as well.
I've grown accustomed to state "a=test-only" for those in any case, just
so that it is clear what's up for everyone just skimming the pushlog.
Robert Kaiser