859358 - Implement a string checker hook on mozilla-aurora,mozilla-beta and mozilla-release repos to refuse string changes without specific approval

Status: RESOLVED FIXED

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

Description

[bhavana bajaj [:bajaj]]

This can be possible by implementing a hook similar to Bug 813809 and tweaking it to check "the changes to properties/dtd files" . 

note : String changes includes case for removals as well.

Comment 1

[Francesco Lodolo [:flod]]

Hi, any update on this? Considering what just happened in bug 855570, this bug would definitely be of great help.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Someone needs to sign up to implement a hook based on the one in bug 813809, and get it reviewed+deployed. Any chance you are able to take a shot at doing that, Francesco?

Comment 3

[Francesco Lodolo [:flod]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> Any chance you are able to take a shot at doing that, Francesco?

No, unfortunately at the moment is completely outside of my knowledge :-(

Comment 4

[Francesco Lodolo [:flod]]

I've been thinking a bit more about this: checking if the changeset contains an "interesting" file is easy, using bug 813809 as reference.

Interesting = file with "/locales/en-US/" in the path, extension in ['.dtd', '.properties', '.ini'], ignore region.properties.

Problems come when you have to understand what's actually changed in those files, and I'm almost sure I'm taking too many things for granted (right now I'm doing simple tests with pre-commit on a local repo).

I could use patch.diff to get a difference between working dir and parent node, but the diff is not always useful. Example: DTD with declaration spanning on multiple lines, and the line changed is not the one containing the string ID.

Or I could compare the content (fctx.data) of the same file in changeset and parent.

In both cases I need a parser. I checked what compare-locales does, and it's not exactly simple
http://hg.mozilla.org/l10n/compare-locales/file/55c0d0a185ae/lib/Mozilla/Parser.py

Not sure if releng is happy with a hook running so much stuff.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

This hook doesn't need to be perfect. Just requiring the special approval marker on checkins that touch *.dtd or *.properties files would be a good start.

Comment 6

[Francesco Lodolo [:flod]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #5)
> This hook doesn't need to be perfect. Just requiring the special approval
> marker on checkins that touch *.dtd or *.properties files would be a good
> start.

From a developer's point of view, wouldn't it be too much rejecting all string changesets without marker? We're talking about tens of commits for every cycle, they're going to hate l10n ;-)

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Annoying hooks on beta/aurora are not uncommon, and are typically worth it if they're likely to catch enough common mistakes. Re-importing a patch to add a magic commit message token is not that difficult.

Comment 8

[Francesco Lodolo [:flod]]

Not feeling smart right now, after reading back the entire bug :-)

What I was thinking was a hook on mozilla-central to avoid changes to existing strings, not a "string frozen hook". I'll go back to work and do more tests locally, especially on merges. Assigning this bug to myself for now.

Comment 9

[Francesco Lodolo [:flod]]

Attachment: bug859358v1.patch

Pike, thoughts on this?

Some notes:
* one existing test fails for me (testTipDirRenameShouldFail, KeyError: 'force')
* this hook makes sense only as pretxncommit. If set as pretxnchangegroup, hell happens when I try to pull changes from other repos

Comment 10

[Francesco Lodolo [:flod]]

It's a edge case, but wondering if we should cover .inc for bookmarks as well.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Bookmarks change infrequently enough, and .inc is used for non-string-files enough, that I think that wouldn't be a good idea.

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 8367229 [details] [diff] [review]
bug859358v1.patch

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

I'm generally OK with the idea here, but I'm not sure which of the thousand ways to do this is technically the right way. We should clearly copy the tricks that let merge day go through, though.

Also, I think the approval shouldn't come from l10n-drivers, but from release management.

By now, all this code is in https://hg.mozilla.org/hgcustom/version-control-tools/file/default/hghooks, and has a new bread of tests, which might very well be easier to deal with.

::: mozhghooks/string_frozen_repository.py
@@ +30,5 @@
> +        # Loop through each file for the current changeset
> +        for file in repo[change_id].files():
> +            # Interested only in files potentially used for l10n
> +            if (re.search('locales/en-US/', file) and
> +                not re.search('region.properties', file) and

Not sure I'd exclude region.properties, but if we want to, make this just be .endswith('region.properties').

Comment 13

[Sylvestre Ledru [:Sylvestre]]

(In reply to Axel Hecht [:Pike] from comment #12)

> Also, I think the approval shouldn't come from l10n-drivers, but from
> release management.
Clearly, we would be super happy to have an aurora/beta hook which could check for new string (and warning/reject a new patch).

Comment 14

[Gregory Szorc [:gps]]

Yes, please convert the tests to .t tests. See the existing files in hghooks/tests and consult http://mercurial.selenic.com/wiki/WritingTests#Writing_a_shell_script_test if you need more info.

Comment 15

[Francesco Lodolo [:flod]]

Thanks, I'll take a look hopefully in the next days (realized this patch became obsolete after reading the post on planet).

And I agree with comment 12, we don't probably want to exclude region.properties, given how many localizers go and follow en-US without any hesitation.

Comment 16

[Francesco Lodolo [:flod]]

Attachment: bug859358.patch

Updated patch: cleaned up output (lines of text were too long), switched to shell tests.
Is it just me or it's extremely hard to catch the results before the old tests start running and clutter the console? Also tested with --all-versions flag (much easier to read too).

Gregory, I hope you're the right reviewer for this.

I still have a gigantic question mark over my head (see comment 9). I did setup locally a couple of repositories and it works as expected, but I'm not sure this would work for Mozilla repositories.

Example: 
* This hook is enabled on all string frozen branches, and prevent devs from landing string changes without the approval keyword. On mozilla-central devs need to be free to land any changes they need, without asking for approval or setting keywords in the commit message.
* When I pull changes from mozilla-central into mozilla-aurora (I assume that's what happens on merge day), hook rejects changesets that were perfectly fine for m-c.

Am I looking at this all wrong? It could be, considering this the first time I'm working on Mercurial hooks.

Comment 17

[Axel Hecht [:Pike]]

I think https://hg.mozilla.org/hgcustom/version-control-tools/file/5ec7535e0c23/hghooks/mozhghooks/prevent_webidl_changes.py#l70 is trying to handle the merge push, I guess gps is the right guy to ask if that's the right way for it.

Comment 18

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #17)
> I think
> https://hg.mozilla.org/hgcustom/version-control-tools/file/5ec7535e0c23/
> hghooks/mozhghooks/prevent_webidl_changes.py#l70 is trying to handle the
> merge push, I guess gps is the right guy to ask if that's the right way for
> it.

Thanks, I should have definitely used the WebIDL hook as a starting point.

I'm not sure I should ignore backouts like WebIDL does: for example, when a feature needs to be disabled on Aurora, we usually ask for a patch that doesn't touch string files, so it seems good to reject a plain backout.

I'll turn the r? as NI?, since this patch is clearly not good enough.

Comment 19

[Francesco Lodolo [:flod]]

Attachment: bug859358v3.patch

Less broken patch while waiting for Gregory's feedback

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8477436 [details] [diff] [review]
bug859358v3.patch

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

This looks pretty good!

::: hghooks/mozhghooks/prevent_string_changes.py
@@ +42,5 @@
> +                        error += "* File used for localization (%s) altered in this changeset *\n" % file
> +    # Check if an error occurred
> +    if error != "":
> +        print "\n************************** ERROR ****************************\n"
> +        ui.warn(error)

It's weird to see print and ui.warn() both used here. They both go to the same place in hooks. So pick one and use it.

::: hghooks/tests/test-prevent-string-changes.t
@@ +57,5 @@
> +  adding manifests
> +  adding file changes
> +  added 1 changesets with 1 changes to 1 files
> +
> +Remove file (untracked extension, inside expected path), should work  

Nit: trailing whitespace

Comment 21

[Gregory Szorc [:gps]]

The hook is implemented all right from a Mercurial perspective.

Strictly speaking, ctx.files() can't be trusted (changed files may not be in the returned set). But for Mozilla's purposes it is good enough. Other hooks do this, so not your problem.

The spam from run-mercurial-tests.py is being fixed in bug 1054517.

You can run a single test via `run-mercurial-tests.py hghooks/tests/my-test.t` to make testing easier.

I can't speak for the l10n nature of what's being done. You should have a sheriff familiar with the uplift procedures, etc take a look and verify you aren't breaking any workflows.

I wouldn't be too worried about the test repository not resembling mozilla-central. Other tests aren't doing this and I would think people would be filing bugs about ineffective hooks if things were broken.

Comment 22

[Francesco Lodolo [:flod]]

Thanks Gregory. The mix print + ui.warn came from the hook I used as base (prevent_uuid_changes), didn't realize it was unnecessary.

Thanks also for the tip about running a single test, it definitely works but it doesn't seem to like the --all-version flag (I'm getting a ton of errors that I don't get running all tests together).

Comment 23

[Francesco Lodolo [:flod]]

Attachment: bug859358v4.patch

Carry-on f+ from :gps

Comment 24

[Francesco Lodolo [:flod]]

Comment on attachment 8478388 [details] [diff] [review]
bug859358v4.patch

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

Asking Ed for review, hoping he can cover also the uplift part.

Comment 25

[Ed Morley [:emorley]]

Comment on attachment 8478388 [details] [diff] [review]
bug859358v4.patch

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

on merge day, the topmost commit contains a=release (eg https://hg.mozilla.org/releases/mozilla-release/rev/a3cc3e46b571) so this seems fine for the merge day uplifts. As for the single bug cherry pick uplifts the commit message will just need editing to include the L10NOK string, but this is something that has to happen for things like the |ba=foo| hook anyway.

That said, I'm wondering if using a=l10n rather than L10NOK might be more consistent?

Comment 26

[Francesco Lodolo [:flod]]

(In reply to Ed Morley [:edmorley] from comment #25)
> That said, I'm wondering if using a=l10n rather than L10NOK might be more
> consistent?

Sounds good to me. Updating patch and tests to accept a=l10n, case insensitive.

Not sure what has changed, but tests for the attached patch now fail for a missing blank line.

Comment 27

[Francesco Lodolo [:flod]]

Attachment: bug859358v5.patch

Carrying over flags from v4.

Besides the output text, the only changed line in the hook is

- repo.changectx('tip').description()):
+ repo.changectx('tip').description().lower()):

Plus fixed tests.

What are the next steps? Not sure if checkin-needed keyword work for this repository, and I guess there's an implementation bug to file.

Comment 28

[Ed Morley [:emorley]]

(In reply to Francesco Lodolo [:flod] from comment #27)
> What are the next steps? Not sure if checkin-needed keyword work for this
> repository, and I guess there's an implementation bug to file.

Not sure what permissions the repo needs (due to bug 1059214), but try pushing and if that doesn't work I'll push for you. We can then file a server ops bug to get this deployed (something like bug 1053485).

Comment 29

[Francesco Lodolo [:flod]]

Tried out of curiosity but I get permission denied (not surprisingly, considering that I have only l10n access).

Comment 30

[Ed Morley [:emorley]]

Comment on attachment 8479804 [details] [diff] [review]
bug859358v5.patch

Landed with a couple of whitespace tweaks to fix these pep8 failures:
prevent_string_changes.py:25:1: E302 expected 2 blank lines, found 1
prevent_string_changes.py:38:5: E129 visually indented line with same indent as next logical line
prevent_string_changes.py:59:1: W391 blank line at end of file

Adjusted the commit message to shorten it slightly (removed the part about aurora etc, since the hook itself doesn't check the repo, it's whether it's added to the hgrc or not that makes the difference).

https://hg.mozilla.org/hgcustom/version-control-tools/rev/2fcfbaf6c91c

Comment 31

[Axel Hecht [:Pike]]

To reiterate my comment 12, I don't think the l10n-drivers should be doing the approvals here.

The case we're trying to prevent is people not filling out the strings section correctly when talking to relman.

How about 

This repository is string frozen. Please request explicit permission from release managers to break string freeze in your bug.
If you have that explicit permission, denote that by including L10NOK in your commit message.

Comment 32

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #31)
> To reiterate my comment 12, I don't think the l10n-drivers should be doing
> the approvals here.

Sorry, I read that comment as "I shouldn't be reviewing this", not as a comment about the rejection message.

> This repository is string frozen. Please request explicit permission from
> release managers to break string freeze in your bug.
> If you have that explicit permission, denote that by including L10NOK in
> your commit message.

Sounds good to me (s/L10NOK/a=l10n).

Comment 33

[Francesco Lodolo [:flod]]

Discussing with Pike over IRC, I'll have to do a follow-up patch to avoid confusions.

Comment 34

[Francesco Lodolo [:flod]]

Attachment: bug859358_followup.patch

Pike, are you OK with these changes?

* switched from a=l10n (which sounds like "approved by l10n" to "l10nok=")
* updated rejection message using your suggestion.

Comment 35

[Francesco Lodolo [:flod]]

Comment on attachment 8479861 [details] [diff] [review]
bug859358_followup.patch

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

Ed, sorry to be a PITA but there was some confusion on my side, asking to review the change to be sure.

Comment 36

[Ed Morley [:emorley]]

Comment on attachment 8479861 [details] [diff] [review]
bug859358_followup.patch

I think I'd prefer l10n= rather than l10nok=
(we don't have rok= or baok= for review or binary approval)

That said, I wonder how our various commit message flag handling regexes will cope with a number in the flag name..? Probably not worth worrying about though...

Comment 37

[Francesco Lodolo [:flod]]

Attachment: bug859358_followupv2.patch

Carry over f+ from Pike per IRC talk, switched to l10n= instead of l10nok= from previous patch.

Comment 38

[Ed Morley [:emorley]]

Comment on attachment 8479917 [details] [diff] [review]
bug859358_followupv2.patch

https://hg.mozilla.org/hgcustom/version-control-tools/rev/f528bfcf6c0a

Could you file the deployment bug? :-)

Comment 39

[Francesco Lodolo [:flod]]

Marking as fixed.