846953 - Roll out prevent_broken_csets hook on all trees except try

Status: RESOLVED FIXED

(Developer Services :: General, task)

Description

[Justin Lebar (not reading bugmail)]

In bug 843606 we rolled out this hg hook on m-a.  It doesn't seem to have caused any mass-breakages.  So maybe it's time to get it elsewhere.

Comment 1

[Ben Kero [:bkero]]

Rolling this out to everything is very easy. Rolling it out to every repository except try will be a bit more difficult. It would involve modifying nearly several hundred repositories and making new entries manually for each new repository.

I think the easiest way of accomplishing this is to modify the hook to just return true if it detects that the name of the repository is try. After this we can add it to the globally run hooks.

Does this sound good to you?

Comment 2

[Justin Wood (:Callek)]

I'd also argue that we *don't* want this hook on user repo's as well as try, fwiw.

Comment 3

[Ben Kero [:bkero]]

I'm sure those can be excluded in a similar manner. I've yet to see what the environment is for writing these, but checking path should be all that's required (all user repos live in /repo/hg/mozilla/users/)

Comment 4

[Justin Lebar (not reading bugmail)]

(In reply to Justin Wood (:Callek) from comment #2)
> I'd also argue that we *don't* want this hook on user repo's as well as try,
> fwiw.

That's probably sane...

I can try to figure out how to check the path, but can you give me concrete examples of the paths I should be expecting for try, user repos, and other repos?  I have no way to test this locally, so I don't want to get it wrong.

Comment 5

[Ben Kero [:bkero]]

Some example concrete paths:

/repo/hg/mozilla/try
/repo/hg/mozilla/users/bkero_mozilla.com/foo
/repo/hg/mozilla/releases/gaia-l10n/gd
/repo/hg_ecma/mozilla/es4
/repo/hg_pvt/omgseekrits
/repo/hg_pvt/users/aravind_mozilla.com

Comment 6

[Justin Lebar (not reading bugmail)]

So to be clear, we want to turn this on for repos which start with "/repo/hg/mozilla" but which aren't equal to "/repo/hg/mozilla/try"?

Comment 7

[Ben Kero [:bkero]]

Correct, however additionally the /repo/hg_pvt/mozilla and /repo/hg_ecma/mozilla repositories should run the script.

So basically:

if (path = /repo/hg/mozilla/try OR path ~ ^/repo/hg/mozilla/users):
    exit 0
else
    run_your_check()

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Don't do the prevent_broken_csets hook on try and user repos.

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 720301 [details] [diff] [review]
Patch v1

I verified that we don't seem to need a trailing / on the repo paths.

Comment 11

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/hgcustom/hghooks/rev/d7d2b8ffdc42

Back to server ops to deploy the hook.

Comment 12

[Ed Morley [:emorley]]

This is pretty urgent - the longer we leave this unfixed the greater chance of more broken (and irreversible) commits being landed.

Comment 13

[Ed Morley [:emorley]]

(In reply to Justin Wood (:Callek) from comment #2)
> I'd also argue that we *don't* want this hook on user repo's as well as try,
> fwiw.

Why out of curiosity? I think we're at risk of having new repos created in the future without this hook, if we do anything but apply it everywhere (as proven by bug 853761).

Please can we just roll this out everywhere?

Comment 14

[Justin Lebar (not reading bugmail)]

Thanks for noticing that this hasn't been rolled out, Ed.  I'd forgotten about this bug.

I wrote code in the hook to limit it to everything except user repos and try.  I think the plan was to enable the hook for all repos, and rely on the hook to exclude those repos.  Does that sound OK to you?

Comment 15

[Justin Lebar (not reading bugmail)]

(The logic for not rolling it out on user repos is that if a user repos is corrupted locally, it's more important to reflect the user's local history than it is to force the user to clean up their history.)

Comment 16

[Ed Morley [:emorley]]

(In reply to Justin Lebar [:jlebar] from comment #14)
> I wrote code in the hook to limit it to everything except user repos and
> try.  I think the plan was to enable the hook for all repos, and rely on the
> hook to exclude those repos.  Does that sound OK to you?

sgtm (as long as we can add it to the global hgrc I'm all for it :-))

Comment 17

[Justin Lebar (not reading bugmail)]

Ping on this bug?  This has been sitting for almost a month now.

Comment 18

[Shyam Mani [:fox2mike]]

Ben, Can we get this rolled out asap?

Comment 19

[Justin Lebar (not reading bugmail)]

Weekly ping on this bug.

Comment 20

[Justin Lebar (not reading bugmail)]

By my count there have been ten busted changesets landed since we asked for this hook to be turned on, on Feb 25.

> 124071   f27dbd9ba370   2013-03-06 16:41 +0000   jcoppeard
>   Bug 848395 - GC: Move Rooted to JS namespace - Move js::Rooted to JS namespace r=terrence
> 
> 124375   7b3da1dae19a   2013-02-21 17:31 -0800   terrence
>   Bug 843907 - Move to manual barriering for MapObject and SetObject's key; r=billm
> 
> 124993   ea6b05238e68   2013-03-15 15:35 -0700   terrence
>   Bug 851107 - Skip invalid poisoning of inline chars in RegExpExecute; r=sfink
> 
> 126468   3af927a8260c   2013-03-19 17:59 -0700   terrence
>   Bug 852802 - Add incremental needsBarrier to the runtime and check it first; r=billm
> 
> 126876   36e1ab8c7de0   2013-03-25 11:48 -0700   terrence
>   Bug 854029 - Allow use of non-gcthing values with RelocatableValue; r=billm
> 
> 123742   444272873216   2013-03-01 21:49 -0800   jwalden
>   Bug 847521 - Allow CheckedInt<signed char> in addition to char/unsigned char.  char != signed char (but is g
> 
> 126872   be2f7045bf15   2013-04-01 11:33 -0700   mozilladev
>   Bug 837950 - Enhance jstests framework to support Test402 tests. r=terrence
> 
> 123564   1cd1a9fdf8fb   2013-03-02 14:23 +0000   neil
>   Bug 738952 Make it possible for a stream converter to propagate the actual MIME type to the document r=bz
> 
> 123504   008743f80cb1   2013-02-15 08:32 +0100   dirkjan
>   Bug 638219 - Move command construction into Test class method; r=terrence
> 
> 123638   260236599b04   2013-02-28 00:50 +1300   robert
>   Bug 829557. Part 1: When calling into plugin code, identify situations where it is safe (or unsafe) to reent

I hope you'll forgive me for starting to run out of patience here.

Comment 21

[Ben Kero [:bkero]]

I'm going to review the hook now and push it out.

Comment 22

[Ben Kero [:bkero]]

Looks good, pushing this out globally

Comment 23

[Ben Kero [:bkero]]

This should be enabled now. Sorry about the delay. If you experience any issues feel free to ping me on IRC.

Comment 24

[Gregory Szorc [:gps]]

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