Bug 336986 - [native] Enable removal of redundant files and folders by RemoveAction

2 files changed, 127 insertions, 35 deletions

diff --git a/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/touchpoint/natives/RemoveActionTest.java b/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/touchpoint/natives/RemoveActionTest.java
index b3a96cae9..e291ef35a 100644
--- a/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/touchpoint/natives/RemoveActionTest.java
+++ b/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/touchpoint/natives/RemoveActionTest.java
@@ -7,6 +7,7 @@
  *
  * Contributors:
  *     Cloudsmith Inc. - initial API and implementation
+ *     SAP AG - Ongoing development
  *******************************************************************************/
 
 package org.eclipse.equinox.p2.tests.touchpoint.natives;
@@ -22,6 +23,12 @@ import org.eclipse.equinox.p2.tests.AbstractProvisioningTest;
 
 public class RemoveActionTest extends AbstractProvisioningTest {
 
+	private File testFolder;
+	private File testFile;
+
+	private Map parameters;
+	private RemoveAction action;
+
 	public RemoveActionTest(String name) {
 		super(name);
 	}
@@ -30,36 +37,39 @@ public class RemoveActionTest extends AbstractProvisioningTest {
 		super("");
 	}
 
-	public void testExecuteUndo() {
+	@Override
+	protected void tearDown() throws Exception {
+		super.tearDown();
+
+		if (testFolder.exists()) {
+			delete(testFolder);
+		}
+	}
+
+	@Override
+	protected void setUp() throws Exception {
+		super.setUp();
+
 		Properties profileProperties = new Properties();
 		File installFolder = getTempFolder();
 		profileProperties.setProperty(IProfile.PROP_INSTALL_FOLDER, installFolder.toString());
 		IProfile profile = createProfile("testExecuteUndo", profileProperties);
 
-		Map parameters = new HashMap();
+		parameters = new HashMap();
 		parameters.put(ActionConstants.PARM_PROFILE, profile);
 		NativeTouchpoint touchpoint = new NativeTouchpoint();
 		touchpoint.initializePhase(null, profile, "testExecuteUndo", parameters);
 
-		File testFolder = new File(installFolder, "testExecuteUndo");
-		File testFile = new File(testFolder, "data.txt");
+		testFolder = new File(installFolder, "testExecuteUndo");
+		testFile = new File(testFolder, "data.txt");
 
 		parameters.put(ActionConstants.PARM_PATH, testFolder.getAbsolutePath());
 		parameters = Collections.unmodifiableMap(parameters);
+	}
 
-		testFolder.mkdir();
-		assertTrue(testFolder.exists());
-		try {
-			writeToFile(testFile, "AA\nTestfile with AA on first line.");
-		} catch (IOException e1) {
-			fail("Could not write test data to test file");
-		}
-		assertFileContent("Should contain AA", testFile, "AA");
+	public void testExecuteUndo() {
 
-		RemoveAction action = new RemoveAction();
-		action.execute(parameters);
-		assertFalse(testFolder.exists());
-		assertFalse(testFile.exists());
+		executeRemoveActionOnNonEmptyDir();
 
 		action.undo(parameters);
 		IBackupStore store = (IBackupStore) parameters.get(NativeTouchpoint.PARM_BACKUP);
@@ -69,12 +79,63 @@ public class RemoveActionTest extends AbstractProvisioningTest {
 			} catch (IOException e) {
 				fail("Restore of backup failed");
 			}
-		assertTrue(testFolder.exists());
+		assertTrue("Test folder was not restored from backup", testFolder.exists());
 		assertFileContent("Should contain AA", testFile, "AA");
 		if (store != null)
 			store.discard();
 	}
 
+	public void testExecuteMultipleRemovesOnTheSameDir() {
+
+		executeRemoveActionOnNonEmptyDir();
+		executeRemoveActionOnNonEmptyDir();
+
+	}
+
+	public void testExecuteMultipleRemovesOnTheSameEmptyDir() {
+
+		executeRemoveActionOnEmptyDir("Test folder exists after executing RemoveAction for the first time");
+		executeRemoveActionOnEmptyDir("Test folder exists after executing RemoveAction for the second time");
+	}
+
+	public void testExecuteMultipleRemovesOnTheSameEmptyDir2() {
+
+		executeRemoveActionOnEmptyDir("Test folder exists after executing RemoveAction for the first time");
+		executeUncheckedRemoveActionOnNonEmptyDir();
+		executeRemoveActionOnEmptyDir("Test folder exists after executing RemoveAction for the third time");
+	}
+
+	private void writeTestFile() {
+		testFolder.mkdir();
+		assertTrue("Test folder was not created before removal", testFolder.exists());
+		try {
+			writeToFile(testFile, "AA\nTestfile with AA on first line.");
+		} catch (IOException e1) {
+			fail("Could not write test data to test file");
+		}
+		assertFileContent("Test file should contain AA", testFile, "AA");
+	}
+
+	private void executeRemoveActionOnNonEmptyDir() {
+		writeTestFile();
+		action = new RemoveAction();
+		action.execute(parameters);
+		assertFalse("Test file exists after executing RemoveAction", testFile.exists());
+		assertFalse("Test folder exists after executing RemoveAction", testFolder.exists());
+	}
+
+	private void executeRemoveActionOnEmptyDir(String failureMessage) {
+		executeUncheckedRemoveActionOnNonEmptyDir();
+		assertFalse(failureMessage, testFolder.exists());
+	}
+
+	private void executeUncheckedRemoveActionOnNonEmptyDir() {
+		testFolder.mkdir();
+		assertTrue("Test folder was not created before removal", testFolder.exists());
+		action = new RemoveAction();
+		action.execute(parameters);
+	}
+
 	private static void writeToFile(File file, String content) throws IOException {
 		file.getParentFile().mkdirs();
 		file.createNewFile();
@@ -83,5 +144,4 @@ public class RemoveActionTest extends AbstractProvisioningTest {
 		out.write(content);
 		out.close();
 	}
-
 }
\ No newline at end of file
diff --git a/bundles/org.eclipse.equinox.p2.touchpoint.natives/src/org/eclipse/equinox/internal/p2/touchpoint/natives/BackupStore.java b/bundles/org.eclipse.equinox.p2.touchpoint.natives/src/org/eclipse/equinox/internal/p2/touchpoint/natives/BackupStore.java
index 82a3503cf..b16b58e11 100644
--- a/bundles/org.eclipse.equinox.p2.touchpoint.natives/src/org/eclipse/equinox/internal/p2/touchpoint/natives/BackupStore.java
+++ b/bundles/org.eclipse.equinox.p2.touchpoint.natives/src/org/eclipse/equinox/internal/p2/touchpoint/natives/BackupStore.java
@@ -7,6 +7,7 @@
  *
  * Contributors:
  *     Cloudsmith Inc. - initial API and implementation
+ *     SAP AG - Ongoing development
  *******************************************************************************/
 
 package org.eclipse.equinox.internal.p2.touchpoint.natives;
@@ -225,26 +226,41 @@ public class BackupStore implements IBackupStore {
 		if (file.isDirectory())
 			return backupDirectory(file);
 		file = makeParentCanonical(file);
-		File buRoot = backupRoot;
-		// File buRoot = findBackupRoot(file);
-		File buDir = new File(buRoot, backupName);
-		// move the file
-		// create the relative path from root and use that in buDir
-		File buFile = new File(buDir, makeRelativeFromRoot(file).getPath());
+		File buFile = getBackupFile(file);
 		// already backed up, but was a directory = wrong usage
 		if (buFile.isDirectory())
 			throw new IllegalArgumentException(NLS.bind(Messages.BackupStore_directory_file_mismatch, buFile.getAbsolutePath()));
 		// has already been backed up - can only be done once with one BackupStore
-		if (buFile.exists())
+		if (buFile.exists()) {
+			// although backed up, the file can be still on the file system when, for example,
+			// two IUs are unzipping their contents to the same location and share a few common file,
+			// which have to be removed twice
+			if (file.exists() && !file.delete())
+				throw new IOException(NLS.bind(Messages.BackupStore_can_not_remove, file.getAbsolutePath()));
 			return false;
+		}
+
+		moveToBackup(file, buFile);
+
+		return true;
+	}
 
+	/**
+	 * Move/rename file to a backup file. Callers of the method must have ensured that the source file exists and the
+	 * backup file has not been created yet.
+	 * 
+	 * @param file source file to move; should already exist and must not be directory
+	 * @param buFile destination backup file to move to; should not exist and must be a directory
+	 * @throws IOException if the backup operation fails
+	 */
+	private void moveToBackup(File file, File buFile) throws IOException {
 		// make sure all of the directories exist / gets created
 		buFile.getParentFile().mkdirs();
 		if (buFile.getParentFile().exists() && !buFile.getParentFile().isDirectory())
 			throw new IllegalArgumentException(NLS.bind(Messages.BackupStore_file_directory_mismatch, buFile.getParentFile().getAbsolutePath()));
 		if (file.renameTo(buFile)) {
 			backupCounter++;
-			return true;
+			return;
 		}
 		// could not move - this can happen because source and target are on different volumes, or
 		// that source is locked "in use" on a windows machine. The copy will work across volumes,
@@ -256,8 +272,14 @@ public class BackupStore implements IBackupStore {
 		// need to remove the backed up file
 		if (!file.delete())
 			throw new IOException(NLS.bind(Messages.BackupStore_can_not_delete_after_copy_0, file));
+	}
 
-		return true;
+	private File getBackupFile(File file) {
+		File buRoot = backupRoot;
+		File buDir = new File(buRoot, backupName);
+		// create the relative path from root and use that in buDir
+		File buFile = new File(buDir, makeRelativeFromRoot(file).getPath());
+		return buFile;
 	}
 
 	/**
@@ -366,7 +388,7 @@ public class BackupStore implements IBackupStore {
 	 * A call to backup a directory is really only needed for empty directories as a restore
 	 * of a file will also restore all of its parent directories.
 	 * @param file - the (empty) directory to back up
-	 * @return true if the directory was moved to backup. false if the directory was already backed up and remains.
+	 * @return true if the directory was moved to backup. false if the directory was already backed up
 	 * @throws IllegalArgumentException if file is not a directory, or is not empty.
 	 * @throws IOException if directory can not be moved to the backup store, or if the directory is not writeable
 	 */
@@ -376,15 +398,25 @@ public class BackupStore implements IBackupStore {
 		file = makeParentCanonical(file);
 		if (file.list().length != 0)
 			throw new IllegalArgumentException(NLS.bind(Messages.BackupStore_directory_not_empty, file.getAbsolutePath()));
-		// the easiest is to create a dummy file and back that up (the dummy is simply ignored when restoring).
+		// the easiest way is to create a dummy file and back that up (the dummy is simply ignored when restoring).
 		File dummy = new File(file, dummyName);
-		if (!dummy.createNewFile())
-			throw new IOException(NLS.bind(Messages.BackupStore_can_not_create_dummy, dummy.getAbsolutePath()));
-		boolean result = backup(dummy);
-		// if already backed up - do not delete the directory
-		if (result && !file.delete())
+		dummy = makeParentCanonical(dummy);
+		File buFile = getBackupFile(dummy);
+		boolean backedUp = buFile.exists();
+		// backup only if the folder has not been already backed up;
+		// this can happen if, for example, two IUs unzip to the same folder and then want to delete it
+		if (!backedUp) {
+			if (closed)
+				throw new ClosedBackupStoreException("Can not perform backup()"); //$NON-NLS-1$
+			if (!dummy.createNewFile())
+				throw new IOException(NLS.bind(Messages.BackupStore_can_not_create_dummy, dummy.getAbsolutePath()));
+			moveToBackup(dummy, buFile);
+		}
+		// previous checks have verified that the directory exists
+		if (!file.delete())
 			throw new IOException(NLS.bind(Messages.BackupStore_can_not_remove, dummy.getAbsolutePath()));
-		return result;
+		// will return true if the directory was already backed up at the beginning of the operation and false otherwise
+		return !backedUp;
 	}
 
 	/**