Open
Bug 797962
Opened 13 years ago
Updated 6 years ago
Deploy a global case folding detection hook
Categories
(Developer Services :: Mercurial: hg.mozilla.org, defect)
Tracking
(Not tracked)
ASSIGNED
People
(Reporter: ehsan.akhgari, Assigned: gps)
References
Details
Attachments
(1 file)
11.10 KB,
patch
|
glandium
:
feedback-
|
Details | Diff | Splinter Review |
No description provided.
Updated•13 years ago
|
Assignee: server-ops → server-ops-devservices
Component: Server Operations → Server Operations: Developer Services
QA Contact: jdow → shyam
Comment 1•13 years ago
|
||
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•13 years ago
|
Assignee: server-ops-devservices → shyam
Comment 2•13 years ago
|
||
This is now in production as a global hg hook. Would be nice if someone can test it.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 3•13 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.
Comment 4•13 years ago
|
||
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 → ---
Comment 5•13 years ago
|
||
Ehsan, Ted : what's the progress here?
Comment 6•13 years ago
|
||
I haven't had any time to look at this again, sorry.
Comment 7•13 years ago
|
||
Timeout, reopen when you guys want this deployed.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → INCOMPLETE
Updated•11 years ago
|
Component: Server Operations: Developer Services → General
Product: mozilla.org → Developer Services
Assignee | ||
Comment 8•10 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•10 years ago
|
Status: REOPENED → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Assignee: smani → gps
Assignee | ||
Comment 9•10 years ago
|
||
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•10 years ago
|
Component: General → Mercurial: hg.mozilla.org
Summary: Deploy the hook implemented in bug 797919 → Deploy a global case folding detection hook
Comment 10•10 years ago
|
||
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•10 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...
Comment 12•10 years ago
|
||
(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•10 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•10 years ago
|
QA Contact: smani → hwine
Assignee | ||
Comment 14•9 years ago
|
||
Bug 1297276 performed a case only rename and already landed on central this week. This is now back on my radar again.
You need to log in
before you can comment on or make changes to this bug.
Description
•