Skip to content

Commit

Permalink
[FS] Always copy file-attributes using Eclipse's Native(File)Handler
Browse files Browse the repository at this point in the history 
Files.copy seems to copy file-attributes differently than Eclipse
NativeHandler (for files) do on Mac. In order to stay consistent always
use the Eclipse way.

Fixes eclipse-platform#1504
  • Loading branch information
@HannesWell
HannesWell committed Aug 19, 2024
1 parent 7fc0709 commit 4fda918
Show file tree
Hide file tree
Showing 7 changed files with 110 additions and 109 deletions.
11 changes: 4 additions & 7 deletions ...ndles/org.eclipse.core.filesystem/src/org/eclipse/core/filesystem/provider/FileStore.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2005, 2015 IBM Corporation and others.
* Copyright (c) 2005, 2024 IBM Corporation and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
Expand All @@ -25,6 +25,7 @@
import org.eclipse.core.internal.filesystem.FileCache;
import org.eclipse.core.internal.filesystem.Messages;
import org.eclipse.core.internal.filesystem.Policy;
import org.eclipse.core.internal.filesystem.local.LocalFile;
import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.IPath;
import org.eclipse.core.runtime.IProgressMonitor;
Expand Down Expand Up @@ -129,7 +130,7 @@ protected void copyDirectory(IFileInfo sourceInfo, IFileStore destination, int o
// create directory
destination.mkdir(EFS.NONE, subMonitor.newChild(1));
// copy attributes
transferAttributes(sourceInfo, destination);
LocalFile.transferAttributes(sourceInfo, destination);

if (children == null)
return;
Expand Down Expand Up @@ -167,7 +168,7 @@ protected void copyFile(IFileInfo sourceInfo, IFileStore destination, int option
OutputStream out = destination.openOutputStream(EFS.NONE, subMonitor.newChild(1));) {
in.transferTo(out);
subMonitor.worked(93);
transferAttributes(sourceInfo, destination);
LocalFile.transferAttributes(sourceInfo, destination);
subMonitor.worked(5);
} catch (IOException e) {
Policy.error(EFS.ERROR_WRITE, NLS.bind(Messages.failedCopy, sourcePath), e);
Expand Down Expand Up @@ -429,8 +430,4 @@ public String toString() {
@Override
public abstract URI toURI();

private void transferAttributes(IFileInfo sourceInfo, IFileStore destination) throws CoreException {
int options = EFS.SET_ATTRIBUTES | EFS.SET_LAST_MODIFIED;
destination.putInfo(sourceInfo, options, null);
}
}
19 changes: 14 additions & 5 deletions ...org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/local/LocalFile.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2005, 2016 IBM Corporation and others.
* Copyright (c) 2005, 2024 IBM Corporation and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
Expand Down Expand Up @@ -148,28 +148,37 @@ public void copy(IFileStore destFile, int options, IProgressMonitor monitor) thr
super.copy(destFile, options, monitor);
}

private static final CopyOption[] NO_OVERWRITE = {StandardCopyOption.COPY_ATTRIBUTES};
private static final CopyOption[] OVERWRITE_EXISTING = {StandardCopyOption.COPY_ATTRIBUTES, StandardCopyOption.REPLACE_EXISTING};
private static final int LARGE_FILE_SIZE_THRESHOLD = 1024 * 1024; // 1 MiB experimentally determined
private static final CopyOption[] NO_OVERWRITE = {};
private static final CopyOption[] OVERWRITE_EXISTING = {StandardCopyOption.REPLACE_EXISTING};
public static final int LARGE_FILE_SIZE_THRESHOLD = 1024 * 1024; // 1 MiB experimentally determined

@Override
protected void copyFile(IFileInfo sourceInfo, IFileStore destination, int options, IProgressMonitor monitor) throws CoreException {
if (sourceInfo.getLength() > LARGE_FILE_SIZE_THRESHOLD && destination instanceof LocalFile target) {
SubMonitor subMonitor = SubMonitor.convert(monitor, NLS.bind(Messages.copying, this), 100);
try {
boolean overwrite = (options & EFS.OVERWRITE) != 0;
Files.copy(this.file.toPath(), target.file.toPath(), overwrite ? OVERWRITE_EXISTING : NO_OVERWRITE);
subMonitor.worked(93);
transferAttributes(sourceInfo, destination);
subMonitor.worked(5);
} catch (FileAlreadyExistsException e) {
Policy.error(EFS.ERROR_EXISTS, NLS.bind(Messages.fileExists, target.filePath), e);
} catch (IOException e) {
Policy.error(EFS.ERROR_WRITE, NLS.bind(Messages.failedCopy, this.filePath, target.filePath), e);
} finally {
IProgressMonitor.done(monitor);
subMonitor.done();
}
} else {
super.copyFile(sourceInfo, destination, options, monitor);
}
}

public static final void transferAttributes(IFileInfo sourceInfo, IFileStore destination) throws CoreException {
int options = EFS.SET_ATTRIBUTES | EFS.SET_LAST_MODIFIED;
destination.putInfo(sourceInfo, options, null);
}

@Override
public void delete(int options, IProgressMonitor monitor) throws CoreException {
if (monitor == null)
Expand Down
2 changes: 2 additions & 0 deletions resources/tests/org.eclipse.core.tests.resources/META-INF/MANIFEST.MF
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ Import-Package: org.assertj.core.api,
org.junit.jupiter.api.extension,
org.junit.jupiter.api.function,
org.junit.jupiter.api.io,
org.junit.jupiter.params,
org.junit.jupiter.params.provider,
org.junit.platform.suite.api,
org.mockito
Bundle-ActivationPolicy: lazy
Expand Down
116 changes: 44 additions & 72 deletions ...org.eclipse.core.tests.resources/src/org/eclipse/core/tests/filesystem/FileStoreTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,9 @@
*******************************************************************************/
package org.eclipse.core.tests.filesystem;

import static org.assertj.core.api.Assertions.assertThat;
import static org.eclipse.core.tests.internal.localstore.LocalStoreTestUtil.createTree;
import static org.eclipse.core.tests.internal.localstore.LocalStoreTestUtil.getTree;
import static org.eclipse.core.tests.resources.ResourceTestUtil.createInFileSystem;
import static org.eclipse.core.tests.resources.ResourceTestUtil.createInputStream;
import static org.eclipse.core.tests.resources.ResourceTestUtil.createRandomString;
import static org.eclipse.core.tests.resources.ResourceTestUtil.createTestMonitor;
import static org.eclipse.core.tests.resources.ResourceTestUtil.getFileStore;
Expand All @@ -34,14 +32,15 @@

import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.URI;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Random;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Stream;
import org.eclipse.core.filesystem.EFS;
Expand All @@ -56,20 +55,23 @@
import org.eclipse.core.resources.IResource;
import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.IPath;
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.core.runtime.Platform;
import org.eclipse.core.tests.resources.util.WorkspaceResetExtension;
import org.eclipse.osgi.util.NLS;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

/**
* Basic tests for the IFileStore API
*/
@ExtendWith(WorkspaceResetExtension.class)
public class FileStoreTest {

private static final Random RANDOM = new Random();

private IFileStore createDir(IFileStore store, boolean clear) throws CoreException {
if (clear && store.fetchInfo().exists()) {
store.delete(EFS.NONE, null);
Expand All @@ -93,10 +95,12 @@ private static IFileStore randomUniqueNotExistingFileStore() throws IOException
return getFileStore(randomUniqueNotExistingPath());
}

private void createFile(IFileStore target, String content) throws CoreException, IOException {
try (OutputStream output = target.openOutputStream(EFS.NONE, null)) {
createInputStream(content).transferTo(output);
}
private void createFile(IFileStore target, String content) throws CoreException {
createFile(target, content.getBytes());
}

private void createFile(IFileStore target, byte[] content) throws CoreException {
target.write(content, EFS.NONE, null);
}

/**
Expand Down Expand Up @@ -146,9 +150,9 @@ private IFileStore[] getFileStoresOnTwoVolumes() {
/**
* Basically this is a test for the Windows Platform.
*/

@Test
public void testCopyAcrossVolumes() throws Throwable {
@ParameterizedTest(name = "File size {0} bytes")
@ValueSource(ints = { 10, LocalFile.LARGE_FILE_SIZE_THRESHOLD + 10 })
public void testCopyAcrossVolumes(int fileSize) throws Throwable {
IFileStore[] tempDirectories = getFileStoresOnTwoVolumes();

/* test if we are in the adequate environment */
Expand All @@ -166,7 +170,7 @@ public void testCopyAcrossVolumes() throws Throwable {

IFileStore target = tempSrc.getChild(subfolderName);
createDir(target, true);
createTree(getTree(target));
createTree(getTree(target), fileSize);

/* c:\temp\target -> d:\temp\target */
IFileStore destination = tempDest.getChild(subfolderName);
Expand Down Expand Up @@ -228,34 +232,31 @@ public void testCopyDirectoryParentMissing() throws Throwable {
assertTrue(!child.fetchInfo().exists());
}

@Test
public void testCaseInsensitive() throws Throwable {
IFileStore temp = createDir(randomUniqueNotExistingFileStore(), true);
@ParameterizedTest(name = "File size {0} bytes")
@ValueSource(ints = { 10, LocalFile.LARGE_FILE_SIZE_THRESHOLD + 10 })
public void testCaseInsensitive(int fileSize) throws Throwable {
IFileStore temp = createDir(randomUniqueNotExistingFileStore(), true);
boolean isCaseSensitive = temp.getFileSystem().isCaseSensitive();
assumeFalse("only relevant for platforms with case-sensitive file system", isCaseSensitive);

byte[] content = new byte[fileSize];
RANDOM.nextBytes(content);

// create a file
String content = "this is just a simple content \n to a simple file \n to test a 'simple' copy";
IFileStore fileWithSmallName = temp.getChild("filename");
fileWithSmallName.delete(EFS.NONE, null);
createFile(fileWithSmallName, content);
System.out.println(fileWithSmallName.fetchInfo().getName());
assertTrue(fileWithSmallName.fetchInfo().exists());
try (InputStream stream = fileWithSmallName.openInputStream(EFS.NONE, null)) {
assertThat(stream).hasContent(content);
}
assertTrue( Arrays.equals(content, fileWithSmallName.readAllBytes(EFS.NONE, null)));

IFileStore fileWithOtherName = temp.getChild("FILENAME");
System.out.println(fileWithOtherName.fetchInfo().getName());
// file content is already the same for both Cases:
try (InputStream stream = fileWithOtherName.openInputStream(EFS.NONE, null)) {
assertThat(stream).hasContent(content);
}
assertTrue(Arrays.equals(content, fileWithOtherName.readAllBytes(EFS.NONE, null)));
fileWithSmallName.copy(fileWithOtherName, IResource.DEPTH_INFINITE, null); // a NOP Operation
// file content is still the same for both Cases:
try (InputStream stream = fileWithOtherName.openInputStream(EFS.NONE, null)) {
assertThat(stream).hasContent(content);
}
assertTrue(Arrays.equals(content, fileWithOtherName.readAllBytes(EFS.NONE, null)));
assertTrue(fileWithOtherName.fetchInfo().exists());
assertTrue(fileWithSmallName.fetchInfo().exists());
fileWithOtherName.delete(EFS.NONE, null);
Expand All @@ -267,26 +268,25 @@ public void testCaseInsensitive() throws Throwable {
assertEquals(message, exception.getMessage());
}

@Test
public void testCopyFile() throws Throwable {
@ParameterizedTest(name = "File size {0} bytes")
@ValueSource(ints = { 10, LocalFile.LARGE_FILE_SIZE_THRESHOLD + 10 })
public void testCopyFile(int fileSize) throws Throwable {
/* build scenario */
IFileStore temp = createDir(randomUniqueNotExistingFileStore(), true);
byte[] content = new byte[fileSize];
RANDOM.nextBytes(content);

// create target
String content = "this is just a simple content \n to a simple file \n to test a 'simple' copy";
IFileStore target = temp.getChild("target");
target.delete(EFS.NONE, null);
createFile(target, content);
assertTrue(target.fetchInfo().exists());
try (InputStream stream = target.openInputStream(EFS.NONE, null)) {
assertThat(stream).hasContent(content);
}
assertTrue(Arrays.equals(content, target.readAllBytes(EFS.NONE, null)));

/* temp\target -> temp\copy of target */
IFileStore copyOfTarget = temp.getChild("copy of target");
target.copy(copyOfTarget, IResource.DEPTH_INFINITE, null);
try (InputStream stream = copyOfTarget.openInputStream(EFS.NONE, null)) {
assertThat(stream).hasContent(content);
}
assertTrue(Arrays.equals(content, copyOfTarget.readAllBytes(EFS.NONE, null)));
copyOfTarget.delete(EFS.NONE, null);

// We need to know whether or not we can unset the read-only flag
Expand All @@ -297,43 +297,22 @@ public void testCopyFile() throws Throwable {
setReadOnly(target, true);

target.copy(copyOfTarget, IResource.DEPTH_INFINITE, null);
try (InputStream stream = copyOfTarget.openInputStream(EFS.NONE, null)) {
assertThat(stream).hasContent(content);
}
assertTrue(Arrays.equals(content, copyOfTarget.readAllBytes(EFS.NONE, null)));
// reset read only flag for cleanup
setReadOnly(copyOfTarget, false);
copyOfTarget.delete(EFS.NONE, null);
// reset the read only flag for cleanup
setReadOnly(target, false);
target.delete(EFS.NONE, null);
}

/* copy a big file to test progress monitor */
StringBuilder sb = new StringBuilder();
for (int i = 0; i < 1000; i++) {
sb.append("asdjhasldhaslkfjhasldkfjhasdlkfjhasdlfkjhasdflkjhsdaf");
}
IFileStore bigFile = temp.getChild("bigFile");
createFile(bigFile, sb.toString());
assertTrue(bigFile.fetchInfo().exists());
try (InputStream stream = bigFile.openInputStream(EFS.NONE, null)) {
assertThat(stream).hasContent(sb.toString());
}
IFileStore destination = temp.getChild("copy of bigFile");
// IProgressMonitor monitor = new LoggingProgressMonitor(System.out);
IProgressMonitor monitor = createTestMonitor();
bigFile.copy(destination, EFS.NONE, monitor);
try (InputStream stream = destination.openInputStream(EFS.NONE, null)) {
assertThat(stream).hasContent(sb.toString());
}
destination.delete(EFS.NONE, null);
}

/**
* Basically this is a test for the Windows Platform.
*/
@Test
public void testCopyFileAcrossVolumes() throws Throwable {
@ParameterizedTest(name = "File size {0} bytes")
@ValueSource(ints = { 10, LocalFile.LARGE_FILE_SIZE_THRESHOLD + 10 })
public void testCopyFileAcrossVolumes(int fileSize) throws Throwable {
IFileStore[] tempDirectories = getFileStoresOnTwoVolumes();

/* test if we are in the adequate environment */
Expand All @@ -346,32 +325,27 @@ public void testCopyFileAcrossVolumes() throws Throwable {
/* get the destination folder */
IFileStore tempDest = tempDirectories[1];
// create target
String content = "this is just a simple content \n to a simple file \n to test a 'simple' copy";
byte[] content = new byte[fileSize];
RANDOM.nextBytes(content);
String subfolderName = "target_" + System.currentTimeMillis();

IFileStore target = tempSrc.getChild(subfolderName);
target.delete(EFS.NONE, null);
createFile(target, content);
assertTrue(target.fetchInfo().exists());
try (InputStream stream = target.openInputStream(EFS.NONE, null)) {
assertThat(stream).hasContent(content);
}
assertTrue(Arrays.equals(content, target.readAllBytes(EFS.NONE, null)));

/* c:\temp\target -> d:\temp\target */
IFileStore destination = tempDest.getChild(subfolderName);
target.copy(destination, IResource.DEPTH_INFINITE, null);
try (InputStream stream = destination.openInputStream(EFS.NONE, null)) {
assertThat(stream).hasContent(content);
}
assertTrue(Arrays.equals(content, destination.readAllBytes(EFS.NONE, null)));
destination.delete(EFS.NONE, null);

/* c:\temp\target -> d:\temp\copy of target */
String copyOfSubfoldername = "copy of " + subfolderName;
destination = tempDest.getChild(copyOfSubfoldername);
target.copy(destination, IResource.DEPTH_INFINITE, null);
try (InputStream stream = destination.openInputStream(EFS.NONE, null)) {
assertThat(stream).hasContent(content);
}
assertTrue(Arrays.equals(content, destination.readAllBytes(EFS.NONE, null)));
destination.delete(EFS.NONE, null);

/* c:\temp\target -> d:\temp\target (but the destination is already a file */
Expand All @@ -380,9 +354,7 @@ public void testCopyFileAcrossVolumes() throws Throwable {
createFile(destination, anotherContent);
assertTrue(!destination.fetchInfo().isDirectory());
target.copy(destination, IResource.DEPTH_INFINITE, null);
try (InputStream stream = destination.openInputStream(EFS.NONE, null)) {
assertThat(stream).hasContent(content);
}
assertTrue(Arrays.equals(content, destination.readAllBytes(EFS.NONE, null)));
destination.delete(EFS.NONE, null);

/* c:\temp\target -> d:\temp\target (but the destination is already a folder */
Expand Down
Loading

0 comments on commit 4fda918

Please sign in to comment.