Bugzilla – Full Text Bug Listing
| Summary: | Workspace#validFiltered results in I/O via fetchFileInfo => stat() | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | James Blackburn <jamesblackburn+eclipse> | ||||||
| Component: | Resources | Assignee: | Malgorzata Janczarska <malgorzata.tomczyk> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | john.arthorne, malgorzata.tomczyk, Szymon.Brandys | ||||||
| Version: | 3.7 | Flags: | Szymon.Brandys:
review+
|
||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
James Blackburn
From a quick look, the I/O doesn't appear to be strictly needed. It just checks if the file exists on disk, but if it doesn't it creates a dummy file info with just the name and directory attributes. Perhaps it could filter on a dummy file info all the time. I can see this being expensive because the loop walks up the parent hierarchy, so there are D native calls, where D is the depth of the resource in the workspace tree. Created attachment 182360 [details]
Allways creating the new file info
This patch does not fetch file info. Instead new file info is always created base on FileStore name. I've tested in on my local filesystem and is works exactly the same.
However I have one concern. The javadoc to IFileStore.getName() says that the name may differ in case for case insensitive filesystems. In order to make sure of the file case in filesystem you have to fetch the file info, so do exactly what we've been trying to eliminate. I'm not sure in what situations the difference in case may occur, but I made few tests on my windows case-insensitive file system and never came across. Do you know any resolution of this problem without performing I/O?
The case mismatch occurs when the local file system resource doesn't match the case of the resource handle. Steps: 1) Create IFile on local file "abc.txt" 2) In the file system, rename the file to "ABC.txt". -> Now the IFileInfo will return name of "ABC.txt" while resource name is "abc.txt". This difference is mainly interesting while performing refresh local so we can detect case mismatch properly. In this particular situation we can probably just live with the resource name. Any difference in case should get resolved when the next local refresh occurs, but until then the filter will just apply on the resource name in memory rather than the file name on disk. Thanks for clarification John. So I think this won't cause any inconsistency in the work of the validFiltered function, because the filter would not be applied to the resource until it's refreshed anyway. The patch looks good. (In reply to comment #4) > Thanks for clarification John. So I think this won't cause any inconsistency in > the work of the validFiltered function, because the filter would not be applied > to the resource until it's refreshed anyway. Gosia, could you attach a test for that? Created attachment 182621 [details]
Conains test for case sensitive search
Attached patch contains test for case sensitive search.
The test changes the name of the file to uppercase and then synchronizes. The validateFiltered function will reflect the file name change only after the file is synchronized. Before that fix was applied validateFiltered function reflected the file name change even before it was refreshed in workspace, so there is a small change here. However I think that the way it works now is better, as it is consistent with the way filters are actually applied.
Gosia's patch looks good, however there is an issue with it on Win. Steps: 1) On a project create a regex filter filtering 'a.txt' 2) Create 'a.txt' resource. It will be filtered out. Both old and new #validateFiltered would say the resource is filtered out. 3) Rename file 'a.txt' to 'A.txt' in the file system. 4) Old validateFiltered says that resource 'a.txt' is not filtered out. What is true, since resource 'a.txt' points to file 'A.txt' in the filesystem. 5) New validateFiltered says that resource 'a.txt' is still filtered out, what is not true in this case. Thus I'm not sure if we can get rid of the i/o call. (In reply to comment #1) > From a quick look, the I/O doesn't appear to be strictly needed. It just checks > if the file exists on disk, Well. it doesn't just check whether the file exist on disc. On Win it gets the real file name to check against filters. It doesn't matter on case sensitive OSes, but does on Win. I looked at it again today. Going through steps from comment 7 I found something confusing after file 'a.txt' is renamed to 'A.txt' (step 3). IResource#exists for resource 'a.txt' returns false and for 'A.txt' returns true, what is fine. However if you get IFileStores for both resources, IFileInfo#exists() for both return true, what confused me. IWorkspace#validateFiltered method indicates whether a resource is filtered out when it EXISTS. We should check filters against stores that are or would be used by existing resources. And in this particular case the store for existing resource 'a.txt' would match the filter and the resource would be filtered out on creation. Thus the patch is fine and is released to HEAD. |