Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

case-sensitive file existence check #1984

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andrew-tram
Copy link

Check to address - #1866

Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A filesystem may be case sensitive. Mostly depends on OS

@andrew-tram
Copy link
Author

@jukzi , good call! I adjusted the checks to account for OS

jukzi
jukzi previously requested changes Jun 18, 2024
Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do not explicitly check for OS but use any existing function to compare files for equality.

@andrew-tram
Copy link
Author

Understood, hopefully this is closer...

	    // Use workspace and resource APIs to handle file comparison
	    IWorkspaceRoot root = ResourcesPlugin.getWorkspace().getRoot();
	    IPath newFilePath = getContainerFullPath().append(getFileName());
	    IFile newFileHandle = createFileHandle(newFilePath);

	    IFile[] files = root.findFilesForLocationURI(newFileHandle.getLocationURI());
	    for (IFile file : files) {
	        if (file.exists() && file.getName().equalsIgnoreCase(newFileHandle.getName()) &&
	                !file.getName().equals(newFileHandle.getName())) {
	            setErrorMessage(NLS.bind(IDEWorkbenchMessages.ResourceGroup_nameExistsDifferentCase, file.getName()));
	            return false;
	        }
	    }

@jukzi
Copy link
Contributor

jukzi commented Jun 18, 2024

You should not need to compare Strings yourself.

@merks
Copy link
Contributor

merks commented Jun 18, 2024

I’d expect to see IFile or IPath to be compared which know whether to do case-sensitive comparisons.

@andrew-tram
Copy link
Author

I believe this is closer to what yall are suggesting...

// Use workspace and resource APIs to handle file comparison
IPath newFilePath = getContainerFullPath().append(getFileName());
IFile newFileHandle = createFileHandle(newFilePath);

try {
	IContainer parentContainer = newFileHandle.getParent();
	IResource[] members = parentContainer.members();
	for (IResource member : members) {
		if (member.getType() == IResource.FILE) {
			IPath memberPath = member.getFullPath();
			if (memberPath.equals(newFilePath)) {
				continue; // Skip if it's the same path
			}
			if (memberPath.lastSegment().equalsIgnoreCase(newFileHandle.getName()) &&
					!memberPath.lastSegment().equals(newFileHandle.getName())) {
				setErrorMessage(NLS.bind(IDEWorkbenchMessages.ResourceGroup_nameExistsDifferentCase, member.getName()));
				return false;
			}
		}
	}
} catch (CoreException e) {
	setErrorMessage("Error while validating file name: " + e.getMessage());
	return false;
}

Will externalize the new string if all is good.

@merks
Copy link
Contributor

merks commented Jun 18, 2024

I think you should test it. I think memberPath.equals(newFilePath) will return true for two paths with different cases on a case-insensitive file system. I think you if want to check if two paths have different case, you should compare the strings. So if the paths are equal but the string representation are not equal, then what's specified is different from what's present. And that problem can be the case for the any segment of the path (project/folder), not just the final segment.

@andrew-tram
Copy link
Author

Ive tested this on Windows 11 and Mac 14.4.1 (default HFS+ or APFS in case-insensitive mode). The expected results look good where an error displays saying the file already exists with the same name, different case.

Trying Linux now.

@andrew-tram
Copy link
Author

So, I'm not entirely sure how it's feasible to do this without checking for at least if it's Linux.

@jwflicker
Copy link

jwflicker commented Jun 19, 2024

If this approach becomes a challenge, a smaller, targeted fix might be to not selectAndReveal the file and open it an editor if what is returned from mainPage.createNewFile() doesn't exist.

BasicNewFileResourceWizard:

	@Override
	public boolean performFinish() {
		IFile file = mainPage.createNewFile();
		if (file == null) {
			return false;
		}

mainPage.createNewFile() will trigger the display of the error message dialog showing "A resource exists with a different case", but it will return a non-null File object whose .exists() method would return false, but is not checked before invoking IDE.openEditor(page, file, true); just below

So perhaps just changing if (file == null) { to if (file == null || !file.exists()) { would dullen the bulk of the annoyance of #1866

@jukzi
Copy link
Contributor

jukzi commented Jun 19, 2024

You are supposed to find an existing method of signature
boolean equals(IFile, IFile) or boolean equals(IPath, IPath) which does the OS check for you. I dont know its place or name by heart but am sure you can find many of them.
Even java.nio.Path.equals(Path) probably does it if the Path is constructed using the appropriate FileSystem.

Copy link
Contributor

Test Results

 1 210 files   -   605   1 210 suites   - 605   1h 4m 3s ⏱️ - 31m 9s
 7 661 tests ±    0   7 430 ✅  -     3  231 💤 +  3  0 ❌ ±0 
16 096 runs   - 8 048  15 583 ✅  - 7 812  513 💤  - 236  0 ❌ ±0 

Results for commit 805586d. ± Comparison against base commit 954bc58.

This pull request skips 3 tests.
UiTestSuite org.eclipse.ui.tests.api.ApiTestSuite org.eclipse.ui.tests.api.WorkbenchPluginTest ‑ testGetImageRegistryFromAdditionalDisplay
org.eclipse.jface.text.tests.contentassist.ContextInformationTest ‑ testContextInfo_hide_focusOut
org.eclipse.urischeme.internal.registration.TestUnitWinRegistry ‑ testWinRegistry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants