Closed Bug 943486 Opened 11 years ago Closed 10 years ago

Create an HG hook to block commits that touch Australis files without "Australis" in the commit message

Categories

(Developer Services :: Mercurial: hg.mozilla.org, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jaws, Assigned: emorley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [re-hg][Australis:P1])

Attachments

(2 files, 3 obsolete files)

Until Australis merges to mozilla-aurora, we need to have a simple way to back out Australis changesets when they get merged over to the Holly branch.

If a patch touches any files with "customizableui" in the file path, then the changeset summary must also include the word "Australis" (surrounding square brackets aren't necessary).

We are only expecting to need this for m-c28 and m-c29.
Tweaking summary since the hook will be created in this bug, but need to be switched on per tree in another product/component (server ops iirc). When that time comes, I suggest we do so for {mozilla-central,mozilla-inbound,fx-team,b2g-inbound} and maybe even all trunk trees.

Files that would be caught by this:
http://mxr.mozilla.org/mozilla-central/find?string=customizableui&tree=mozilla-central

Presuming we'd also want an exemption string? (maybe "IGNORE BAD COMMIT MESSAGES" to keep parity with https://hg.mozilla.org/hgcustom/hghooks/file/15e5831ab26b/mozhghooks/commit-message.py#l76 ?).

Seems like we could just steal bits from an existing hook used on aurora:
https://hg.mozilla.org/hgcustom/hghooks/file/15e5831ab26b/mozhghooks/prevent_uuid_changes.py
Summary: Add an HG hook to block commits to mozilla-inbound and fx-team that touch Australis files without the "Australis" in commit message → Create an HG hook to block commits that touch Australis files without "Australis" in the commit message
This hook/patch will need review by releng since it may be deployed on m-a & m-b. Some of our tooling needs to know the magic phrase to do things during release builds.

I don't think that will be the case here, but it's worth being extra careful. Please include me in the review loop.
Whiteboard: [re-hg]
We'll only be wanting this on trunk at most :-)
Gah - Never mind comment 2

I misread the list of repos - thanks for pointing that out, Ed.
Assignee: nobody → emorley
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Prevents changes to files containing "/customizableui/" in their filepath, unless the commit for that change contained "australis" (case insensitive) or else that commit (or one closer to tip than it) contains the string "OVERRIDE HOOK" (case sensitive).

gps, this is intended for trunk only & is only going to be used for < ~12 weeks.

jaws, are you happy allowing "australis" to be any case? Is "OVERRIDE HOOK" ok? Are the list of files affected (see MXR link in comment 1) acceptable? Any more to add?
Attachment #8338912 - Flags: review?(gps)
Attachment #8338912 - Flags: feedback?(jaws)
Comment on attachment 8338912 [details] [diff] [review]
Patch v1

Review of attachment 8338912 [details] [diff] [review]:
-----------------------------------------------------------------

Only nits.

::: mozhghooks/prevent_unlabelled_australis_changes.py
@@ +1,4 @@
> +#!/usr/bin/python
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

Anything that imports mercurial.* must be GPL: that's how Mercurial rolls.

@@ +9,5 @@
> +from mercurial.node import short
> +
> +def printError(c):
> +    print "\n\n************************** ERROR ****************************"
> +    print "Rev %s appears to make australis changes, but does" % short(c.node())

Nit: Changeset (not rev).

@@ +20,5 @@
> +    rev = repo.changectx(node).rev()
> +    tip = repo.changectx("tip").rev()
> +    rejecting = False
> +
> +    for i in reversed(xrange(rev, tip + 1)):

for i in repo.changelog.revs(repo[node].rev()):

@@ +33,5 @@
> +            # This commit is labelled correctly, proceed to the next.
> +            continue
> +
> +        for file in c.files():
> +            if "/customizableui/" in file:

Should this be prefixed in case this directory name appears elsewhere in the tree?
Attachment #8338912 - Flags: review?(gps) → review+
Comment on attachment 8338912 [details] [diff] [review]
Patch v1

Review of attachment 8338912 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozhghooks/prevent_unlabelled_australis_changes.py
@@ +28,5 @@
> +        if "OVERRIDE HOOK" in desc:
> +            # Skip all earlier commits in this push.
> +            break
> +
> +        if "australis" in desc.lower():

Case-insensitive comparison of "australis" is fine. Thanks!

@@ +33,5 @@
> +            # This commit is labelled correctly, proceed to the next.
> +            continue
> +
> +        for file in c.files():
> +            if "/customizableui/" in file:

Let's also add one more directory here that can problematic or a source for confusion without a commit hook. We can also be more specific for the customizableui folder:
/browser/themes/
/browser/components/customizableui/
Attachment #8338912 - Flags: feedback?(jaws) → feedback+
(In reply to Gregory Szorc [:gps] from comment #6)
> @@ +20,5 @@
> > +    rev = repo.changectx(node).rev()
> > +    tip = repo.changectx("tip").rev()
> > +    rejecting = False
> > +
> > +    for i in reversed(xrange(rev, tip + 1)):
> 
> for i in repo.changelog.revs(repo[node].rev()):

The changesets need to be in reversed order so that the 'override for all earlier changesets' string works.

I'm not that familiar with the mercurial API (and the mercurial website barely ever works for me), so tried just using reversed() on what you suggested, but got:
TypeError: argument to reversed() must be a sequence

I'm guessing repo.changelog.revs() is missing the __reversed__() method, if http://docs.python.org/2/library/functions.html#reversed is to be believed.

What would you recommend using instead?
Flags: needinfo?(gps)
reversed() accepts any iterable as input. It should work with the output of repo.changelog.revs().

Instead of relying on the docs on the Mercurial website, I'd look at the raw source. You are interested in localrepo.py for defining the localrepository class and revlog.py for defining the API for revlogs (of which the changelog is one).
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #9)
> reversed() accepts any iterable as input. It should work with the output of
> repo.changelog.revs().

Unless I'm missing something, it doesn't since I get:
TypeError: argument to reversed() must be a sequence
Ed's signature says that he will be away from Nov 30th to Dec 11th. Is there someone who can pick this up for him?
Flags: needinfo?(hwine)
Whiteboard: [re-hg] → [re-hg][Australis:P1]
Flags: needinfo?(philringnalda)
Whether you're looking for a Python expert, or an hg expert, or an hg hook expert, or someone who has modified our hooks before, not it. (Well, apparently I changed some Tinderbox URLs in 2011, but I don't remember having done it, so it doesn't count.)
Flags: needinfo?(philringnalda)
Can anyone describe what testing has been done with this hook? Without that, I'm reluctant to move forward until Ed returns. (As in, all the testing work would need to be recreated.)
Flags: needinfo?(hwine)
This bug is waiting on a response to:

(In reply to Ed Morley (Away 30th Nov until 11th Dec) [:edmorley UTC+0] from comment #10)
> (In reply to Gregory Szorc [:gps] from comment #9)
> > reversed() accepts any iterable as input. It should work with the output of
> > repo.changelog.revs().
> 
> Unless I'm missing something, it doesn't since I get:
> TypeError: argument to reversed() must be a sequence
Flags: needinfo?(gps)
(In reply to Hal Wine [:hwine] (use needinfo) from comment #13)
> Can anyone describe what testing has been done with this hook? Without that,
> I'm reluctant to move forward until Ed returns. (As in, all the testing work
> would need to be recreated.)

I've tested this locally and seems to work well. I would be satisfied to land once the review nits were resolved.
(In reply to Jared Wein [:jaws] from comment #11)
> Ed's signature says that he will be away from Nov 30th to Dec 11th. Is there
> someone who can pick this up for him?

Look's like Ed's signature isn't an absolute. I can coordinate getting the hook enabled once landed. However, it will also need to be announced on the lists so devs don't get surprised.

Can (or have) you put out a notice on this upcoming change?
Attached patch Patch v1.1 (obsolete) — Splinter Review
With all review nits addressed apart from that described in comment 8 (this was complete before I left, attaching here for reference; the remaining work is pending comment 10).

(In reply to Hal Wine [:hwine] (use needinfo) from comment #16)
> Can (or have) you put out a notice on this upcoming change?

I haven't, and likely won't have the time to whilst I'm abroad and on PTO, sorry.
Attachment #8338912 - Attachment is obsolete: true
Attached patch Patch v1.1 (obsolete) — Splinter Review
Sigh, correct version of the patch this time.
Attachment #8342871 - Attachment is obsolete: true
To test, add something like this to your hgrc (other than windows style paths):
[hooks]
pretxnchangegroup.test = python:c:\src\hghooks\mozhghooks\prevent_unlabelled_australis_changes.py:hook

Test cases:
* Modify a file in browser/components/customizableui/* or browser/themes/* and try committing with both "australis" and no australis in the commit message; former should succeed, latter fail.
* Modify a file outside of those locations and commit without australis in the commit message; should succeed.
* Modify a file in either of those two locations, commit the change, then make a second commit with the override string "OVERRIDE HOOK" in the commit message. This should make all older commits succeed, even if they didn't contain the string australis in the commit message.
* Do the same as for the previous test case, except now make a third commit (after the override one), which changes a file in those two locations, and doesn't contain "australis". This should not succeed, since the override is not supposed to affect commits closer to tip than it.

Note to test the cases above, you must make the commits to one clone of m-c and then pull that repo into a second local clone (which has the hook enabled), since this is a pretxnchangegroup hook (ie acts on the whole push transaction and not per commit).
Jared, could you make the announcement Hal mentioned in comment 16, probably best to at least dev.tree-management and also dev-apps-firefox, since I'm away? Thank you :-)
Flags: needinfo?(jaws)
The devtools CSS lives in browser/themes/*/devtools but is independent of Australis. Will this affect changes in those directories?
(In reply to Brandon Benvie [:benvie] from comment #22)
> The devtools CSS lives in browser/themes/*/devtools but is independent of
> Australis. Will this affect changes in those directories?

Yes, it will, but this patch should be changed to skip that directory as it is pretty isolated. Thank you for bringing attention to this.
Comment on attachment 8342872 [details] [diff] [review]
Patch v1.1

Review of attachment 8342872 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozhghooks/prevent_unlabelled_australis_changes.py
@@ +47,5 @@
> +            # This commit is labelled correctly, proceed to the next.
> +            continue
> +
> +        for file in c.files():
> +            if file.startswith("browser/themes/") or file.startswith("browser/components/customizableui/"):

if (file.startsWith("browser/themes/") and "devtools" not in file) or file.startswith("browser/components/customizableui/"):
Might be worth looking at the commits in these dirs in the release / 6-weeks preceeding Australis, to see if there are any other common things to exclude.
(In reply to Ed Morley [:edmorley UTC+0] from comment #8)
> (In reply to Gregory Szorc [:gps] from comment #6)
> > @@ +20,5 @@
> > > +    rev = repo.changectx(node).rev()
> > > +    tip = repo.changectx("tip").rev()
> > > +    rejecting = False
> > > +
> > > +    for i in reversed(xrange(rev, tip + 1)):
> > 
> > for i in repo.changelog.revs(repo[node].rev()):
> 
> The changesets need to be in reversed order so that the 'override for all
> earlier changesets' string works.
> 
> I'm not that familiar with the mercurial API (and the mercurial website
> barely ever works for me), so tried just using reversed() on what you
> suggested, but got:
> TypeError: argument to reversed() must be a sequence
> 
> I'm guessing repo.changelog.revs() is missing the __reversed__() method, if
> http://docs.python.org/2/library/functions.html#reversed is to be believed.
> 
> What would you recommend using instead?

reversed(xrange()) or reversed(repo.revs()) should work. I'm not sure why it isn't. I'm guessing you didn't paste the actual source that was failing.

__reversed__ only comes into play if you attempt to call reverse() on a class. If you are dealing with a list or an iterator, it should just work (I think). If not, cast whatever isn't working to a list and reverse that. e.g. reverse(list(repo.revs()))
Flags: needinfo?(gps)
This is a list of all of the files that have been touched within /browser/themes in the last 100 commits to mozilla-central: https://pastebin.mozilla.org/3804993

It looks like devtools is the only isolated subdirectory here. There have been Australis-related changes for the social directory, so that directory should be included in the commit-restrictions.
Back from vacation ping :)
Flags: needinfo?(emorley)
Attached patch Patch v1.2Splinter Review
Attachment #8342872 - Attachment is obsolete: true
(In reply to Gregory Szorc [:gps] from comment #26)
> I'm guessing you didn't paste the actual source that was failing.

I copied and pasted exactly what was suggested in comment 6.

Anyway, I've just resorted to casting, this is only a temporary hook and if we leave it much longer there will be little point in landing it.

(In reply to Jared Wein [:jaws] (Away 20 Dec to 2 Jan) from comment #24)
> Comment on attachment 8342872 [details] [diff] [review]
> Patch v1.1
> 
> if (file.startsWith("browser/themes/") and "devtools" not in file) or
> file.startswith("browser/components/customizableui/"):

Done :-)
Flags: needinfo?(emorley)
https://hg.mozilla.org/hgcustom/hghooks/rev/422eb2b595dd

(Will request the deployment in another bug)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 956320
Documentation (will email this to dev.platform & dev.tree-management):

Each commit that touches a file in:
  browser/themes/* (excluding those that contain devtools in the filepath)
or
  browser/components/customizableui/*

...will need to contain the string "australis" in the commit message (case insensitive), or else the push will be rejected. This hook is enabled on mozilla-central, mozilla-inbound, b2g-inbound, fx-team.

To override, make a commit containing the string "OVERRIDE HOOK" (case sensitive) to ignore that changeset and all earlier commits in that push.
somehow this adds now new work for sheriffs

Example.

I did today a merge from m-c to inbound with a change included from gijs for australis. This resulted in the push with a message

remote: ************************** ERROR ****************************
remote: Changeset ea6e6ce90b22 appears to make australis changes, but does
remote: not contain "australis" in the commit message! (See bug 943486)
remote:  -> "Merge mozilla-central to mozilla-inbound"
remote: *************************************************************

what fixed this was modifying the commit message to Merge mozilla-central to mozilla-inbound including australis

i have also no clue what ea6e6ce90b22 is a revision for, at least seems not on m-c or inbound ?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Carsten Book [:Tomcat] from comment #33)
> i have also no clue what ea6e6ce90b22 is a revision for, at least seems not
> on m-c or inbound ?

The merge cset hash? But yeah, erroring out on a merge cset is...dumb. I would think the commit hook could ignore commit messages with "merge" in them. We already do similar things for other hooks with "backout" and the like.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #34)
> The merge cset hash?

Note that the hash depends on the commit message too, so when you added australis to the commit message, the hash will have changed as well :)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #35)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #34)
> > The merge cset hash?
> 
> Note that the hash depends on the commit message too, so when you added
> australis to the commit message, the hash will have changed as well :)

yeah gijs pointed that out too :) so the issue is not the cset its more that merges get into this commit hook issue :)
I think something like this should do for merge csets themselves. Ed, does this look right to you?
Attachment #8363998 - Flags: review?(emorley)
Comment on attachment 8363998 [details] [diff] [review]
don't capture merges in australis hook,

Review of attachment 8363998 [details] [diff] [review]:
-----------------------------------------------------------------

No need to create a parents variable that's used once. But this is how you'd do it!
Attachment #8363998 - Flags: review?(emorley) → review+
lgtm; thank you :-)

I'd intentionally not skipped backouts in this hook, since without annotating them, there will be conflicts on Holly - but I'd overlooked merges (which whilst will still cause conflicts when merging to Holly, should probably be dealt with separately anyway, so we shouldn't require the annotation).
http://hg.mozilla.org/hgcustom/hghooks/rev/3ea78865a148

I moved the block after the break, so that you can fix broken merges (ie, if you merged from a repo without this hook, so you had changes that trigger the hook to stop you from committing, you can still use the OVERRIDE_HOOK on the merge cset to make things work).

I don't know what's required to get this into production, however. Looks like Chris committed something as well, so I don't know if this can be deployed straight away (or how - I guess I get to file a bug?). Chris?
Flags: needinfo?(catlee)
Attachment #8363998 - Flags: checked-in+
The thing to do is to file a bug with IT to deploy the hook; e.g. bug 959753
Flags: needinfo?(catlee)
Depends on: 963577
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
This hook has no tests (https://ci.mozilla.org/job/version-control-tools/coveragepy/?) and I don't think it is deployed any more given what the hook does. Can we delete it from source control?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(emorley)
Yeah, it can be deleted from source control. We don't use it anymore and have no need for it in the future.
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1075318
(In reply to Gregory Szorc [:gps] from comment #42)
> This hook has no tests
> (https://ci.mozilla.org/job/version-control-tools/coveragepy/?) and I don't
> think it is deployed any more given what the hook does. Can we delete it
> from source control?

sgtm :-)
Flags: needinfo?(emorley)
Product: Release Engineering → Developer Services
You need to log in before you can comment on or make changes to this bug.