Deploy a global case folding detection hook

ASSIGNED
Assigned to

Status

ASSIGNED
6 years ago
a year ago

People

(Reporter: Ehsan, Assigned: gps)

Tracking

Details

Attachments

(1 attachment)

Comment hidden (empty)

Updated

6 years ago
Assignee: server-ops → server-ops-devservices
Component: Server Operations → Server Operations: Developer Services
QA Contact: jdow → shyam
From bug 797919 comment #5

You'll need to file a new IT bug to get this hook deployed. You want

[hooks]
pretxnchangegroup.renamecase = python:mozhghooks.prevent_case_only_renames.hook

deployed globally.

Updated

6 years ago
Assignee: server-ops-devservices → shyam
This is now in production as a global hg hook. Would be nice if someone can test it.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 3

6 years ago
(In reply to comment #2)
> This is now in production as a global hg hook. Would be nice if someone can
> test it.

We tested this today at some point, and it worked as expected.
I think this was backed out because of performance issues :

19:09:00#systems: <   cshields> | fox2mike: bkero I'm rolling back the hook that was added today, reports that it is blocking pushes
19:19:53#it: <    cshields> | I'll leave the hook disabled, vladamir had a leftover bookmarks file in his name, so I'm wondering if something with the hook caused that  /cc fox2mike
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ehsan, Ted : what's the progress here?
I haven't had any time to look at this again, sorry.
Timeout, reopen when you guys want this deployed.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → INCOMPLETE
Component: Server Operations: Developer Services → General
Product: mozilla.org → Developer Services
(Assignee)

Updated

4 years ago
Blocks: 1160129
(Assignee)

Comment 8

4 years ago
The lack of this hook bit us today.

I casually looked at the hook and know what the performance issue is. Let me try to rewrite it.
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
(Assignee)

Updated

4 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Updated

4 years ago
Assignee: smani → gps
(Assignee)

Comment 9

4 years ago
Created attachment 8599935 [details] [diff] [review]
hghooks: refactor case folding hook to have better performance

The old implementation of the case folding hook asked Mercurial for
rename information. This has poor performance because extracting renames
requires decoding two manifests and running "status" between them.

This patch changes the implementation to only examine the set of changed
files in each changeset.

As noted by the added inline comments, the detection of case collisions
isn't perfect. But it strikes a reasonable balance between features
and performance. Something is better than nothing after all. And this
something would have prevented the case collision on inbound this
morning (bug 1160129).

While I was here, I updated the output of the hook. I also refactored
the tests a bit. This version of the hook should be suitable to deploy
globally on hg.mozilla.org. There should be little performance concerns.
Attachment #8599935 - Flags: review?(mh+mozilla)
(Assignee)

Updated

4 years ago
Component: General → Mercurial: hg.mozilla.org
Summary: Deploy the hook implemented in bug 797919 → Deploy a global case folding detection hook
Comment on attachment 8599935 [details] [diff] [review]
hghooks: refactor case folding hook to have better performance

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

How did you generate this patch? It's -p0 instead of -p1.

(I guess this change was prompted by the recent rename of dom/animation/AnimationEffectReadOnly.cpp ?)

::: hghooks/mozhghooks/prevent_case_only_renames.py
@@ +23,5 @@
> +mercurial.store.basicstore and mercurial.store.fncachestore. On the
> +hg.mozilla.org master server, which is using NFS, the performance overhead
> +for this would add unacceptable latency to push operations. If the master
> +server is ever upgraded to have a fast I/O layer, we could potentially revisit
> +this decision.

While I laud the attempt at taking less resources, I can't help but notice the lack (in both the old and the new code) of handling a case that is about as dangerous:
- touch foo
- hg commit -A -m 'Add foo'
- touch Foo
- hg commit -A -m 'Add Foo'

This one requires looking at the whole manifest, unfortunately.

@@ +34,5 @@
> +MESSAGE = '''
> +One or more changesets contained case collisions!
> +
> +A case collision is when two or more filenames only differ in their casing.
> +e.g. UPPERCASE vs lowercase.

Would probably be less confusing if the only difference between UPPERCASE and lowercase was actually their case.

@@ +38,5 @@
> +e.g. UPPERCASE vs lowercase.
> +
> +Case collisions are problematic for users on case insensitive filesystems
> +(likely Windows and OS X). Although Mercurial can track case varying filenames
> +properly, this would confuse the filesystem and cause chaos for these users.

Doesn't mercurial handle this properly these days?
Attachment #8599935 - Flags: review?(mh+mozilla) → feedback-
(Assignee)

Comment 11

4 years ago
(In reply to Mike Hommey [:glandium] from comment #10)
> Comment on attachment 8599935 [details] [diff] [review]
> hghooks: refactor case folding hook to have better performance
> 
> Review of attachment 8599935 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How did you generate this patch? It's -p0 instead of -p1.

I have diff.noprefix=true in my global hgrc. If you don't think bzexport should upload diffs with noprefix (Splinter seemed to handle it properly), please file a new bug. Otherwise, bzexport will likely produce diffs using the user's settings instead of something reasonable.

> (I guess this change was prompted by the recent rename of
> dom/animation/AnimationEffectReadOnly.cpp ?)

Yes. This old bug got closed without proper resolution.

> ::: hghooks/mozhghooks/prevent_case_only_renames.py
> @@ +23,5 @@
> > +mercurial.store.basicstore and mercurial.store.fncachestore. On the
> > +hg.mozilla.org master server, which is using NFS, the performance overhead
> > +for this would add unacceptable latency to push operations. If the master
> > +server is ever upgraded to have a fast I/O layer, we could potentially revisit
> > +this decision.
> 
> While I laud the attempt at taking less resources, I can't help but notice
> the lack (in both the old and the new code) of handling a case that is about
> as dangerous:
> - touch foo
> - hg commit -A -m 'Add foo'
> - touch Foo
> - hg commit -A -m 'Add Foo'
> 
> This one requires looking at the whole manifest, unfortunately.

As I said in the comments, something is better than nothing. We can add functionality easier than taking it out. And I'd rather not introduce more full manifest reading to the hooks: performance is already problematic and I don't want to make it worse.

> @@ +38,5 @@
> > +e.g. UPPERCASE vs lowercase.
> > +
> > +Case collisions are problematic for users on case insensitive filesystems
> > +(likely Windows and OS X). Although Mercurial can track case varying filenames
> > +properly, this would confuse the filesystem and cause chaos for these users.
> 
> Doesn't mercurial handle this properly these days?

There is ui.portablefilenames. By default, it warns when a case folding collisions occurs. See http://selenic.com/repo/hg/file/e5b507efb36e/tests/test-casecollision.t. But AFAICT that only matters when doing commits on the working copy.

I believe Mercurial will abort when a case collision would occur for an `hg up` on case insensitive filesystems. Although, I'm not positive about what actually happens. Sounds like I should look into that...
(In reply to Gregory Szorc [:gps] from comment #11)
> (In reply to Mike Hommey [:glandium] from comment #10)
> > Comment on attachment 8599935 [details] [diff] [review]
> > hghooks: refactor case folding hook to have better performance
> > 
> > Review of attachment 8599935 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > How did you generate this patch? It's -p0 instead of -p1.
> 
> I have diff.noprefix=true in my global hgrc. If you don't think bzexport
> should upload diffs with noprefix (Splinter seemed to handle it properly),
> please file a new bug. Otherwise, bzexport will likely produce diffs using
> the user's settings instead of something reasonable.

hg qimport ; hg qpush fails on such a patch. So does hg import. The latter fortunately has a -p option to which you can give the level of the patch, but I wouldn't expect most people to know it's a patch level problem. qimport however doesn't have a -p option.
(Assignee)

Comment 13

4 years ago
I already filed http://bz.selenic.com/show_bug.cgi?id=4639. It sounds like someone will teach the import logic to automatically handle p0.

Updated

4 years ago
QA Contact: smani → hwine
(Assignee)

Comment 14

2 years ago
Bug 1297276 performed a case only rename and already landed on central this week. This is now back on my radar again.
QA Contact: hwine → klibby
(Assignee)

Updated

a year ago
Depends on: 1404847
You need to log in before you can comment on or make changes to this bug.