843081 - Add hg push hook to check for csets which ruin hg blame

Status: RESOLVED FIXED

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

Description

[Justin Lebar (not reading bugmail)]

hg clients older than 2.5 have a bug [1] which causes them to occasionally generate csets which mess up hg blame.

Once such a cset is in m-c, there's no way to fix it.  So we need to catch the problem before it's committed.

There's apparently no way to have an hg server reject clients based on their version [2], so I think the only thing we can do here is to reject busted csets.

If we could provide you with a script to detect a busted cset, would you be willing to roll it out onto our servers as a pre-commit hook?

Comment 1

[Justin Lebar (not reading bugmail)]

[1] http://bz.selenic.com/show_bug.cgi?id=3833
[2] http://bz.selenic.com/show_bug.cgi?id=3833#c15

Comment 2

[Reed Loden [:reed]]

Write your hook, get it reviewed by somebody, and land in https://hg.mozilla.org/hgcustom/hghooks/. Then request IT to install it. :)

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Patch to hghooks, v1

I'm not 100% confident that this patch works right, because it's hard to test locally.  But I did test this pretty hard.

To test, I cloned my local m-i repository, installed the hook, and then did

> $ hg strip --rev 110000  # rev 110000 for me is e491269db27e.
> $ hg pull ../path/to/unstripped/m-i

My repo is past rev 120000, so I tested on more than 10k changesets.

I modified the hook to show me every changeset which it would have rejected (I'll attach the data in a moment) and to list the errant files in those csets.  I then opened each of these csets in hgweb and verified that the errant files were actually unmodified by that cset.  I found no false-positives in this data.

There were a few wrinkles in the test results.  In a few instances, hgweb indicated that a changeset made a non-empty modification to a file, but hg in my console disagreed and attributed that modification to a different changeset.  As far as I can tell, hgweb is simply wrong in these instances.  (hgweb also contradicts itself in these instances: It shows the modification to the file only when you click on the file; in the overview diff, it does not show the modification.)  I've called them out in the data that I'll attach, so you can look if you want.

Additionally, I noticed one instance of my hook not catching a bad changeset.  (Actually, it caught the changeset, but it didn't flag one of the bad files in that cset.)  I think this is a result of an optimization I made to the hook.  The hook looks at files modified between the push "parent" and a leaf of the push.  But if you modify a file and then revert it, we won't look at that file.  I don't know how to get around this, because the hook was way too slow if I did |hg status| on every changeset in a push.

This doesn't worry me a lot, because I think this situation should be rare, and anyway it's not going to cause false positives.

Ted, do you want to do the honors here?

Comment 4

[Justin Lebar (not reading bugmail)]

> (This is sorted lexicographically by changeset hash.)
> 
> * Broken changeset: 08baa859a8d4 for file content/html/content/test/test_ol_attributes_reflection.html
> * Broken changeset: 0c9b624f525f for file content/html/content/test/test_ol_attributes_reflection.html
> * Broken changeset: 12f5029c12a5 for file toolkit/system/windowsproxy/Makefile.in
> * Broken changeset: 12f5029c12a5 for file toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp
> * Broken changeset: 1aa612b762f3 for file dom/browser-element/BrowserElementChild.js
> 
> O Broken changeset: 2ffeac43b95e for file content/media/MediaStreamGraph.cpp
>   A change is shown on hgweb, but the file was actually modified in 66c03536917a.
>   http://hg.mozilla.org/mozilla-central/diff/2ffeac43b95e/content/media/MediaStreamGraph.cpp
> 
> * Broken changeset: 2ffeac43b95e for file dom/telephony/Makefile.in
> * Broken changeset: 2ffeac43b95e for file dom/telephony/nsIDOMNavigatorTelephony.idl
> * Broken changeset: 3083e652374a for file layout/style/test/test_rem_unit.html
> * Broken changeset: 3ffefc46a8bd for file docshell/test/browser/browser_bug134911.js
> * Broken changeset: 4356739a6f44 for file image/test/reftest/generic/accept-image-catchall-ref.html
> * Broken changeset: 4356739a6f44 for file image/test/reftest/generic/accept-image-catchall.html
> * Broken changeset: 4356739a6f44 for file image/test/reftest/generic/check-header.sjs
> * Broken changeset: 4356739a6f44 for file image/test/reftest/generic/green.png
> 
> O Broken changeset: 4d180e495354 for file toolkit/components/passwordmgr/test/authenticate.sjs
>   A change is shown on hgweb, but the file was actually modified in 1be13ecfd80b.
>   http://hg.mozilla.org/mozilla-central/diff/4d180e495354/toolkit/components/passwordmgr/test/authenticate.sjs
> 
> O Broken changeset: 4f0b7a4daacf for file js/src/jit-test/jit_test.py
>   hgweb shows a mode change, but that actually occurred in 9de04b80715f.
>   http://hg.mozilla.org/mozilla-central/diff/4f0b7a4daacf/js/src/jit-test/jit_test.py
> 
> * Broken changeset: 4f0b7a4daacf for file layout/reftests/css-calc/background-image-gradient-1-ref.html
> * Broken changeset: 4f0b7a4daacf for file layout/reftests/css-calc/background-image-gradient-1.html
> * Broken changeset: 4f0b7a4daacf for file layout/reftests/css-calc/reftest.list
> * Broken changeset: 4f0b7a4daacf for file layout/reftests/text/auto-hyphenation-10.html
> * Broken changeset: 4f0b7a4daacf for file layout/reftests/text/auto-hyphenation-8.html
> * Broken changeset: 4f0b7a4daacf for file layout/reftests/text/auto-hyphenation-9.html
> * Broken changeset: 4f0b7a4daacf for file services/sync/SyncComponents.manifest
> * Broken changeset: 59692d8cd68e for file accessible/src/msaa/nsAccessNodeWrap.cpp
> * Broken changeset: 59692d8cd68e for file accessible/src/msaa/nsAccessNodeWrap.h
> * Broken changeset: 60b927d33f47 for file browser/themes/gnomestripe/downloads/downloads.css
> * Broken changeset: 60b927d33f47 for file browser/themes/winstripe/downloads/downloads.css
> * Broken changeset: 6593f27f1898 for file content/canvas/test/test_mozGetAsFile.html
> * Broken changeset: 6b4e13b0d1e4 for file mobile/android/config/tooltool-manifests/android/releng.manifest
> * Broken changeset: 712eca11a04e for file content/svg/content/src/DOMSVGPoint.cpp
> * Broken changeset: 7349207f0878 for file accessible/src/base/NotificationController.cpp
> * Broken changeset: 7349207f0878 for file accessible/src/base/NotificationController.h
> * Broken changeset: 78a71db31dca for file services/common/tests/unit/test_utils_json.js
> * Broken changeset: 8d09962f1e79 for file dom/encoding/TextDecoder.h
> * Broken changeset: 8d09962f1e79 for file dom/encoding/TextEncoder.h
> * Broken changeset: b1e281b6c558 for file python/mozbuild/mozbuild/base.py
> * Broken changeset: b1e281b6c558 for file toolkit/components/downloads/test/unit/test_private_resume.js
> * Broken changeset: b1e281b6c558 for file toolkit/components/downloads/test/unit/test_privatebrowsing.js
> * Broken changeset: b1e281b6c558 for file toolkit/components/downloads/test/unit/test_privatebrowsing_cancel.js
> * Broken changeset: c3268996719e for file dom/tests/mochitest/localstorage/frame_clear_browser_data.html
> * Broken changeset: c3268996719e for file extensions/cookie/test/test_app_uninstall_permissions.html
> * Broken changeset: c3268996719e for file services/sync/SyncComponents.manifest
> 
> X c3268996719e should flag toolkit/components/passwordmgr/test/test_privbrowsing.html as broken, but it doesn't.
>   In fact, we never check the debugindex for test_privbrowsing.html.  It doesn't show up in our hg status.
>   http://hg.mozilla.org/mozilla-central/diff/c3268996719e/toolkit/components/passwordmgr/test/test_privbrowsing.html
> 
> * Broken changeset: c4119fd2fb37 for file dom/bindings/BindingGen.py
> * Broken changeset: cde4cf36ca7c for file dom/browser-element/BrowserElementParent.js
> * Broken changeset: d5179738abfe for file image/test/reftest/generic/accept-image-catchall-ref.html
> * Broken changeset: d5179738abfe for file image/test/reftest/generic/accept-image-catchall.html
> * Broken changeset: d5179738abfe for file image/test/reftest/generic/check-header.sjs
> * Broken changeset: d5179738abfe for file image/test/reftest/generic/green.png
> * Broken changeset: dad25c17ccc7 for file content/svg/content/src/nsSVGFilters.cpp
> * Broken changeset: f5ec1efd39ee for file services/sync/SyncComponents.manifest
> * Broken changeset: fb87010ac009 for file docshell/test/browser/browser_bug134911.js
> * Broken changeset: fc50a2127738 for file toolkit/components/downloads/test/unit/test_private_resume.js
> * Broken changeset: fc883f5a1a08 for file dom/base/test/test_gsp-standards.html
> * Broken changeset: fd6d1d52a2a1 for file toolkit/components/places/tests/favicons/favicon-normal16.png

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 716351 [details] [diff] [review]
Patch to hghooks, v1

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

I don't completely understand the details of what this hook is checking, but I trust that you got it right. I guess we'll have to enable this on m-c/inbound first, and wait to roll it out to release branches until they've had the existing broken changesets pushed to them? (Perhaps the most recent busted changesets have already made it to Aurora and we can enable it there as well.)

::: mozhghooks/prevent_broken_csets.py
@@ +34,5 @@
> +
> +def dedent_and_fill(s):
> +    r'''This is like textwrap.fill(textwrap.dedent(s)) except it respects
> +    paragraphs in the text.  That is, " foo\n\n bar" is rendered as
> +    "foo\n\nbar", instead of "foo bar".

Is this really not something available in the stdlib? :-/

Comment 6

[Justin Lebar (not reading bugmail)]

> I guess we'll have to enable this on m-c/inbound first, and wait to roll it out to 
> release branches until they've had the existing broken changesets pushed to them?

Hm.  On merge day, how exactly do we push the new csets to Aurora?

If the m-c --> m-a "merge" (which may or may not actually be a merge) comes with its own cset, we could just put IGNORE BROKEN CHANGESETS in there.  Is that feasible?

> Is this really not something available in the stdlib? :-/

Not that I'm aware of, although I agree it's weird that it's not there.

Comment 7

[(not currently active) Ted Mielczarek]

You'd have to ask someone from Release Management, I don't know precisely how the hg mechanics work.

Comment 8

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/hgcustom/hghooks/rev/88aa117ff487

Now we just need someone to install this hook.  Anything I need to do to get this to happen?

We explicitly do not want this on try, but I think we want it everywhere else.  I'm OK if we want to be cautious and roll it out slowly, though.

Comment 9

[Justin Lebar (not reading bugmail)]

It looks like when we merge from m-c to m-a (and so on), we include magic words in the commit:

https://wiki.mozilla.org/Release_Management/Merge_Documentation#Tagging_and_closing_old_aurora

So we should just be able to update the docs to include the new magic words as well, I think.

Comment 10

[Shyam Mani [:fox2mike]]

(In reply to Justin Lebar [:jlebar] from comment #8)
> https://hg.mozilla.org/hgcustom/hghooks/rev/88aa117ff487
> 
> Now we just need someone to install this hook.  Anything I need to do to get
> this to happen?

Can you please file a bug under mozilla.org, Server Operations : Developer Services and we'll get this rolled out for you on the repos you mention.

> We explicitly do not want this on try, but I think we want it everywhere
> else.  I'm OK if we want to be cautious and roll it out slowly, though.

We've been bitten before by hooks being written and then causing damage and have had to rollback. Would be awesome if you've tested this well before we deploy it :)

Thanks!

Comment 11

[Justin Lebar (not reading bugmail)]

> Would be awesome if you've tested this well before we deploy it :)

I did my best, and documented it above.  But I totally agree with a path of caution here.

Comment 12

[Justin Lebar (not reading bugmail)]

I guess we can call this FIXED; the roll-out will be in bug 843606.

Thanks for the quick review, Ted!

Comment 13

[Ed Morley [:emorley]]

Is there any indication that Mercurial will at some point add something similar to this hook to Hg natively, and we could then just update the version on hg.m.o & avoid the need for this hook?

Comment 14

[Justin Lebar (not reading bugmail)]

It's probably worth asking, but istm from the hg bug that the hg devs aren't keen on doing this.

Comment 15

[Jonathan Watt [:jwatt]]

Thanks for working on this, Justin!