Community
Participate
Working Groups
I20081104-0916 IResource#move(..) and #delete(..) fail on locked files on the Mac. This makes our ValidateEditTests fail (bug 158105), but is also a problem in real-life scenarios when you try to delete or move locked files. On Windows and Linux, the read-only flag only makes the contents of the file read-only, but on the Mac, locked files also cannot be moved or deleted. I tracked this down into LocalFile, where both java.io.File#renameTo(File) and java.io.File#delete() fail on this platform. IMO, users of IResource should not have to care about this platform-dependency. The Eclipse file system implementation should hide this from clients by trying to make files writable if delete or move failed (and lock them again if move succeeded).
I would appreciate a fix in core.resources. If you don't fix this, we have to hack around it in the UI, but that would be quite ugly, since we would have to traverse the whole resources tree a second time to make sure nothing is locked.
Markus, do you mean that you need it for M4? Or later in 3.5 is fine?
No need to hurry, later in 3.5 is fine. I just wanted to make sure we have a fix for 3.5.
Note the general philosophy we have followed is to honor the underlying platform's behaviour (much like SWT). For example a Unix user would be surprised if deleting children of a read-only folder didn't fail, and Windows users would expect it to succeed. There is extensive discussion of this elsewhere, such as bug 21084, and bug 65893 comment 10.
I wouldn't expect the UI to have to write special-case code to handle different platforms either. If move/delete fails on the command line it should just fail in the GUI as well.
> I wouldn't expect the UI to have to write special-case code to handle different > platforms either. If move/delete fails on the command line it should just fail > in the GUI as well. Our concrete problem case is actually not in the UI. It's an automated test, where we just lock some files in a project and then deletion of the files fails on the Mac. If the tests should succeed, then we currently have to add special code for the Mac.
(In reply to comment #6) > > I wouldn't expect the UI to have to write special-case code to handle different > > platforms either. If move/delete fails on the command line it should just fail > > in the GUI as well. > > Our concrete problem case is actually not in the UI. It's an automated test, > where we just lock some files in a project and then deletion of the files fails > on the Mac. > > If the tests should succeed, then we currently have to add special code for the > Mac. Having in mind what John wrote in comment 4 and comment 5, the current behavior is correct. From my point of view confusing is how UI handles that issue. When a user tries to delete a read-only or locked file, he is prompted to continue. Now, when he continues, the delete can fail anyway. Users are not aware of it. Some could think that read only flag is removed or ignored. The message in "Confirm Delete of Read-Only Elements" should be something like "The selected elements contain read-only resources. You can try to delete them, but the attempt can fail. Proceed?" There also could be a check box to remove read-only flag from resources when a user agrees to proceed. If I have a locked file and the read-only flag is set to false, I can delete the file... at least on Leopard. I would suggest to do the same in the mentioned test. Set the read-only flag of the locked file to false and then delete it. I'm moving the bug to UI for a comment.
(In reply to comment #7) > There also could be a check box to remove read-only flag from resources when a > user agrees to proceed. If I have a locked file and the read-only flag is set > to false, I can delete the file... at least on Leopard. That would work for me, but the best implementation for this would still be a flag in the resources API as suggested in bug 65893 comment 10. It's easy to unset the read-only flag for single files, but when a client wants to delete a folder with nested locked files, he would has to traverse the whole resource tree again after delete(..) failed and do everything "by hand". > I'm moving the bug to UI for a comment. In UI, it would be a dup of bug 21084. I suggest to keep this in Platform/Resources to eventually add the new flag (especially since the dependent bug 158105 only depends on resources, not UI).
(In reply to comment #8) > That would work for me, but the best implementation for this would still be a > flag in the resources API as suggested in bug 65893 comment 10. We could introduce new flag. On the other side we could have a method on folders to recursively set attributes. Either way it looks like new API.
> On the other side we could have a method on > folders to recursively set attributes. That would require a second traversal of the resource tree, which is unnecessary in most cases but would be a performance hit all the time. The flag could just be checked in the catch clause that handles the failure and thus the read-only attribute would only have to be cleared in cases where it is really a problem.
Created attachment 152484 [details] Patch_v01 A POC patch. Just to give the idea of solution.
Created attachment 152568 [details] Patch_v02 A patch with complete solution. Szymon, could you please review it?
Patch_v02 introduces a new flag IResource.DELETE_READ_ONLY that unlocks files on Mac if it prevents them from being deleted. Since this flag might be not aligned with plans for resource attributes handling I'd like to mark it as non API.
I don't quite understand the value of this proposed new API. If clients have to set a special flag, isn't that equivalent to a client doing this: resource.setReadOnly(false); resource.move(...); Are we providing anything here that a client can't already do?
I agree it's not too interesting for single files (but even there, it would save an unnecessary attribute change in most cases). But to delete a directory, we would have to traverse the whole tree upfront to make everything writeable (again, this is unnecessary in most cases). > Are we providing anything here that a client can't already do? The client can do it but it needs too much code in each client and performs worse.
(In reply to comment #14) > I don't quite understand the value of this proposed new API. Actually we don't want to make it API, see Pawel's comment 13. This is just a convenient method for JDT UI tests. This isn't a fix for problems with deleting read-only files on Mac.
> This is just a > convenient method for JDT UI tests. This isn't a fix for problems with deleting > read-only files on Mac. It's not only a problem in our tests. See bug 21084 for cases where the UI should also use this.
(In reply to comment #17) > > This is just a > > convenient method for JDT UI tests. This isn't a fix for problems with deleting > > read-only files on Mac. > > It's not only a problem in our tests. See bug 21084 for cases where the UI > should also use this. Regarding bug 21084, I agree with John that we should respect the platform behavior. If OS prevents from deleting a file, I assume it does it on purpose and a user should intentionally clear those 'readOnly' or 'locked' flags and then proceed with deletion. Regarding this bug. The issue is that setting 'readOnly' flag on Mac sets 'locked' flag too. So the right fix for Mac would be to have finer granularity of file attributes and be able to mark a file 'readOnly' without setting 'locked'. That's why I think about the Pawel's fix as a temporary fix.
> Regarding bug 21084, I agree with John that we should respect the platform > behavior. If OS prevents from deleting a file, I assume it does it on purpose > and a user should intentionally clear those 'readOnly' or 'locked' flags and > then proceed with deletion. Mac OS 10.6 does not prevent deleting the file. It just asks the user whether the file should really be deleted, and if the user confirms, then the file gets deleted. Eclipse should offer the same behavior, which would best be done with this new API.
You mean, you can delete a 'locked' file on Mac OSX 10.6?
> You mean, you can delete a 'locked' file on Mac OSX 10.6? Yes, after answering a "Do you really want..." dialog. Moving a folder that contains 'locked' files to the trash does not give a dialog, but when I empty the trash, the OS asks me whether it should remove locked items as well.
(In reply to comment #21) > Yes, after answering a "Do you really want..." dialog. On Mac OS X 10.5.7 you can move files to trash but emptying it fails. It fails after the message that prompts whether to remove locked files. You have to hold alt when selecting "Empty the Trash" to make it work. Moreover in the console rm fails to delete such files even when force flag is passed. You have to explicitly clear the flag with chflags to make it work. The way we fail on delete in Resources is consistent with the shell behavior. Anyway, like Szymon said we had an internal discussion on possible improvements in attributes handling in Resources. That's why we are resistant to make this new flag API.
> Moreover in the console rm fails to delete such files even when force flag is > passed. You have to explicitly clear the flag with chflags to make it work. The > way we fail on delete in Resources is consistent with the shell behavior. Grr, rm still does not delete the file in 10.6.2. It asks, but does not work when I say "y": mk$ rm Imp.java override rw-r--r-- mk/mk uchg for Imp.java? y rm: Imp.java: Operation not permitted mk$ rm -f Imp.java rm: Imp.java: Operation not permitted
We should speed up the discussion. Markus, I think that 'delete' in Eclipse should be consistent with the shell behavior. Thus we should not force deleting 'locked' files from Eclipse UI. I understand that Mac OSX UI allows to remove files, but this is 'Move to Trash' not 'delete' operation. New flag is added just to speed up your tests, not to change the UI behavior. That's why I would mark it for >>internal usage only (not API)<<. Could you remind me how much this change improves your tests? I would like to mention once again, that we should not treat this fix as a fix for bug 21084 (see comment 17).
> Markus, I think that 'delete' in Eclipse should be consistent with the shell > behavior. Thus we should not force deleting 'locked' files from Eclipse UI. I agree that the default behavior should be consistent with the shell. > I understand that Mac OSX UI allows to remove files, but this is > 'Move to Trash' not 'delete' operation. 'Move to Trash' is a different issue (bug 70810). The closest we currently have in the UI is Delete, and that should behave similar to the OS's UI. > New flag is added just to speed up your tests, not to change the UI behavior. > That's why I would mark it for >>internal usage only (not API)<<. Could you > remind me how much this change improves your tests? I don't care that much about performance in our test. I could hack around the problem there. > I would like to mention once again, that we should not treat this fix as a fix > for bug 21084 (see comment 17). Why not? That was the main reason why I opened this bug. I could have hacked our tests a long time ago, but I realized that we have exactly the same problem in the UI (bug 65893 is exactly this problem and it's been marked a dup of bug 21084). My first thought was that core.resources should handle this completely under the hood, but I agree now that it would be better to have an explicit flag to force deletion of locked files. Of course, the UI should then not silently delete the files, but it should tell the user that there are locked files and offer to still remove them.
(In reply to comment #25) First of all introducing such a flag wouldn't improve performance in a significant way. Currently EFS code tries to delete a folder node with all its children and if it fails it traverses the tree and tries to delete each of its children recursively. Proposed workaround with DELETE_READ_ONLY flag clears the locked flag during the traversal. If we add API for settings attributes recursively this code would could be added on the client side. And this API would be far more general than one use case DELETE_READ_ONLY flag. Secondly we don't feel so much attached to the fact that setReadOnly locks files on Mac. If we provide a way to handle those two attributes separately the behavior could change. I feel the most appropriate way is to extend the attributes set Resources support. This is not only needed to handle locked and read only flags more appropriately but also improve Resources in cases like the one described on bug 259643. In such case isReadOnly could act as a convenience method that checks whether the owner of current process has write access to the file and if it's marked as locked. Calling setReadOnly(false) could clear locked flag and set write permission but for setReadOnly(true) it could be sufficient to clear write permission. UI could leverage this finer granularity by providing check boxes for each of the flags. In such case DELETE_READ_ONLY wouldn't be so useful (probably useless in JDT tests). Since the solution is yet to be decided I'm moving the bug to M5.
Fixing bug 297228 has extended a set of native file attributes we support. When bug 300502 providing UI support is fixed an user won't be able to set immutable flag unknowingly as we'll have "Locked" and "Owner write" separately. Markus, is it possible for the tests to use the new API and instead of calling setReadOnly(true) just clear the owner write permission?
> When > bug 300502 providing UI support is fixed an user won't be able to set immutable > flag unknowingly as we'll have "Locked" and "Owner write" separately. The UI on the Properties page is just one way to change attributes. There are many other possibilities for a user to have files locked, including - starting with an old workspace - using team plug-ins, such as CVS in watch/edit mode - changing settings in the OS For our tests, I'll piggyback on bug 21084 and use the same strategy as Platform UI to delete folders no matter how the files inside them are locked/read-only. As a client of the IResource APIs, I still think this would best be offered as an update flag, so that I can delete a folder with a one-liner, rather than having to write a try-catch block, a resource visitor, and the retry code.
(In reply to comment #28) > The UI on the Properties page is just one way to change attributes. There are > many other possibilities for a user to have files locked, including Eclipse used to do it silently though.
We'll implement API for changing resources recursively. Things like whether to follow linked resources (maybe FOLLOW_LINKED flag) have to be considered when introducing the change. Bug 297228 has been fixed providing EFS.ATTRIBUTE_IMMUTABLE. Thanks to this change UI is able to rewrite its visitor checking for read only resources and obtain information whether and locked resources exist in selection. If they do a prompt "Selection contains locked resources, do you wish to delete them anyway?" could be displayed followed by to-be-introduced API to reset the IMMUTABLE flag recursively. Removing 3.5 target, I'll simply leave 3.6.
(In reply to comment #30) > We'll implement API for changing resources recursively. Markus, will you consume such API in your test code? Other possibility is to test if a filesystem supports EFS.ATTRIBUTE_IMMUTABLE and in this case traverse the tree and clear the flag. If case of the former I'll change the target to 3.7 and release the change at the beginning of the cycle. In case of the latter I'll close the bug as WONTFIX.
(In reply to comment #31) > (In reply to comment #30) > > We'll implement API for changing resources recursively. > > Markus, will you consume such API in your test code? Other possibility is to > test if a filesystem supports EFS.ATTRIBUTE_IMMUTABLE and in this case traverse > the tree and clear the flag. If case of the former I'll change the target to > 3.7 and release the change at the beginning of the cycle. In case of the latter > I'll close the bug as WONTFIX. I thought the whole thing through again. The issue is about IMMUTABLE files on Mac only. I think that we should just decorate such files in navigators appropriately, so the user could see which files/folders are locked. Then he could manually remove the flag from those files, if he really wants to delete them. We could also think about showing only locked files somewhere in UI. This could be the properties dialog similarly to linked and virtual resources, or we can add a filter to navigators that filters out all non-locked files. I don't think we should provide API to recursively clear the IMMUTABLE flag. I think that users should clear this flag with caution, explicitly selecting files or folders that should have this flag cleared. This is WONTFIX for me.
Closing as WONTFIX, Markus please reopen in case you find it unacceptable.
