Bug 7552 audit of <copy> and <move> - really make sure state is reset,

even if exceptions have been thrown.

Make the single file copy operation only work for a single nested
fileset that contains a single file (would have accepted multiple
included files and silently used the randomly chosen first one).

Remove private shadows of Copy members from Move.


git-svn-id: https://svn.apache.org/repos/asf/ant/core/trunk@272365 13f79535-47bb-0310-9956-ffa450edef68

src/main/org/apache/tools/ant/taskdefs/Copy.java

@@ -92,6 +92,8 @@ import java.util.Enumeration;
  *
  * @version $Revision$
  *
+ * @since Ant 1.2
+ *
  * @ant.task category="filesystem"
  */
 public class Copy extends Task {
@@ -300,61 +302,78 @@ public class Copy extends Task {
      * Performs the copy operation.
      */
     public void execute() throws BuildException {
+        File savedFile = file; // may be altered in validateAttributes
+        File savedDestFile = destFile;
+        File savedDestDir = destDir;
+        FileSet savedFileSet = null;
+        if (file == null && destFile != null && filesets.size() == 1) {
+            // will be removed in validateAttributes
+            savedFileSet = (FileSet) filesets.elementAt(0);
+        }
+        
         // make sure we don't have an illegal set of options
         validateAttributes();
 
-        // deal with the single file
-        if (file != null) {
-            if (file.exists()) {
-                if (destFile == null) {
-                    destFile = new File(destDir, file.getName());
-                }
+        try {
+            
+            // deal with the single file
+            if (file != null) {
+                if (file.exists()) {
+                    if (destFile == null) {
+                        destFile = new File(destDir, file.getName());
+                    }
 
-                if (forceOverwrite ||
-                    (file.lastModified() > destFile.lastModified())) {
-                    fileCopyMap.put(file.getAbsolutePath(), destFile.getAbsolutePath());
-                } else {
-                    log(file + " omitted as " + destFile + " is up to date.",
-                        Project.MSG_VERBOSE);
-                }
-            } else {
-                String message = "Warning: Could not find file "
-                                 + file.getAbsolutePath() + " to copy.";
-                if(!failonerror) {
-                    log(message);
+                    if (forceOverwrite ||
+                        (file.lastModified() > destFile.lastModified())) {
+                        fileCopyMap.put(file.getAbsolutePath(), 
+                                        destFile.getAbsolutePath());
+                    } else {
+                        log(file + " omitted as " + destFile 
+                            + " is up to date.", Project.MSG_VERBOSE);
+                    }
                 } else {
-                    throw new BuildException(message);
+                    String message = "Warning: Could not find file "
+                        + file.getAbsolutePath() + " to copy.";
+                    if(!failonerror) {
+                        log(message);
+                    } else {
+                        throw new BuildException(message);
+                    }
                 }
             }
-        }
 
-        // deal with the filesets
-        for (int i=0; i<filesets.size(); i++) {
-            FileSet fs = (FileSet) filesets.elementAt(i);
-            DirectoryScanner ds = fs.getDirectoryScanner(project);
-            File fromDir = fs.getDir(project);
-
-            String[] srcFiles = ds.getIncludedFiles();
-            String[] srcDirs = ds.getIncludedDirectories();
-            boolean isEverythingIncluded = ds.isEverythingIncluded();
-            if (isEverythingIncluded
-                && !flatten && mapperElement == null) {
-                completeDirMap.put(fromDir, destDir);
+            // deal with the filesets
+            for (int i=0; i<filesets.size(); i++) {
+                FileSet fs = (FileSet) filesets.elementAt(i);
+                DirectoryScanner ds = fs.getDirectoryScanner(project);
+                File fromDir = fs.getDir(project);
+                
+                String[] srcFiles = ds.getIncludedFiles();
+                String[] srcDirs = ds.getIncludedDirectories();
+                boolean isEverythingIncluded = ds.isEverythingIncluded();
+                if (isEverythingIncluded
+                    && !flatten && mapperElement == null) {
+                    completeDirMap.put(fromDir, destDir);
+                }
+                scan(fromDir, destDir, srcFiles, srcDirs);
+            }
+            
+            // do all the copy operations now...
+            doFileOperations();
+        } finally {
+            // clean up again, so this instance can be used a second
+            // time
+            file = savedFile;
+            destFile = savedDestFile;
+            destDir = savedDestDir;
+            if (savedFileSet != null) {
+                filesets.insertElementAt(savedFileSet, 0);
             }
-            scan(fromDir, destDir, srcFiles, srcDirs);
-        }
-
-        // do all the copy operations now...
-        doFileOperations();
 
-        // clean up destDir again - so this instance can be used a second
-        // time without throwing an exception
-        if (destFile != null) {
-            destDir = null;
+            fileCopyMap.clear();
+            dirCopyMap.clear();
+            completeDirMap.clear();
         }
-        fileCopyMap.clear();
-        dirCopyMap.clear();
-        completeDirMap.clear();
     }
 
 //************************************************************************
@@ -368,11 +387,13 @@ public class Copy extends Task {
      */
     protected void validateAttributes() throws BuildException {
         if (file == null && filesets.size() == 0) {
-            throw new BuildException("Specify at least one source - a file or a fileset.");
+            throw new BuildException("Specify at least one source "
+                                     + "- a file or a fileset.");
         }
 
         if (destFile != null && destDir != null) {
-            throw new BuildException("Only one of tofile and todir may be set.");
+            throw new BuildException("Only one of tofile and todir "
+                                     + "may be set.");
         }
 
         if (destFile == null && destDir == null) {
@@ -392,23 +413,26 @@ public class Copy extends Task {
                 DirectoryScanner ds = fs.getDirectoryScanner(project);
                 String[] srcFiles = ds.getIncludedFiles();
 
-                if (srcFiles.length > 0) {
+                if (srcFiles.length == 0) {
+                    throw new BuildException(
+                        "Cannot perform operation from directory to file.");
+                } else if (srcFiles.length == 1) {
                     if (file == null) {
                         file = new File(ds.getBasedir(), srcFiles[0]);
                         filesets.removeElementAt(0);
                     } else {
-                        throw new BuildException(
-                            "Cannot concatenate multiple files into a single file.");
+                        throw new BuildException("Cannot concatenate multiple "
+                                                 + "files into a single file.");
                     }
                 } else {
-                    throw new BuildException(
-                        "Cannot perform operation from directory to file.");
+                    throw new BuildException("Cannot concatenate multiple "
+                                             + "files into a single file.");
                 }
             }
         }
 
         if (destFile != null) {
-            destDir = new File(destFile.getParent());   // be 1.1 friendly
+            destDir = fileUtils.getParentFile(destFile);
         }
 
     }
@@ -417,7 +441,8 @@ public class Copy extends Task {
      * Compares source files to destination files to see if they should be
      * copied.
      */
-    protected void scan(File fromDir, File toDir, String[] files, String[] dirs) {
+    protected void scan(File fromDir, File toDir, String[] files, 
+                        String[] dirs) {
         FileNameMapper mapper = null;
         if (mapperElement != null) {
             mapper = mapperElement.getImplementation();
@@ -482,16 +507,21 @@ public class Copy extends Task {
                 try {
                     log("Copying " + fromFile + " to " + toFile, verbosity);
 
-                    FilterSetCollection executionFilters = new FilterSetCollection();
+                    FilterSetCollection executionFilters = 
+                        new FilterSetCollection();
                     if (filtering) {
-                        executionFilters.addFilterSet(project.getGlobalFilterSet());
+                        executionFilters
+                            .addFilterSet(project.getGlobalFilterSet());
                     }
-                    for (Enumeration filterEnum = filterSets.elements(); filterEnum.hasMoreElements();) {
-                        executionFilters.addFilterSet((FilterSet)filterEnum.nextElement());
+                    for (Enumeration filterEnum = filterSets.elements(); 
+                         filterEnum.hasMoreElements();) {
+                        executionFilters
+                            .addFilterSet((FilterSet)filterEnum.nextElement());
                     }
-                    fileUtils.copyFile(fromFile, toFile, executionFilters, filterChains,
-                                       forceOverwrite, preserveLastModified,
-                                       encoding, project);
+                    fileUtils.copyFile(fromFile, toFile, executionFilters, 
+                                       filterChains, forceOverwrite, 
+                                       preserveLastModified, encoding, 
+                                       project);
                 } catch (IOException ioe) {
                     String msg = "Failed to copy " + fromFile + " to " + toFile
                         + " due to " + ioe.getMessage();
@@ -507,7 +537,8 @@ public class Copy extends Task {
                 File d = new File((String)e.nextElement());
                 if (!d.exists()) {
                     if (!d.mkdirs()) {
-                        log("Unable to create directory " + d.getAbsolutePath(), Project.MSG_ERR);
+                        log("Unable to create directory " 
+                            + d.getAbsolutePath(), Project.MSG_ERR);
                     } else {
                         count++;
                     }

src/main/org/apache/tools/ant/taskdefs/Move.java

@@ -84,13 +84,12 @@ import java.util.Vector;
  * @author <a href="mailto:umagesh@rediffmail.com">Magesh Umasankar</a>
  * @version $Revision$
  *
+ * @since Ant 1.2
+ *
  * @ant.task category="filesystem"
  */
 public class Move extends Copy {
 
-    private Vector filterSets = null;
-    private Vector filterChains = null;
-
     public Move() {
         super();
         forceOverwrite = true;
@@ -101,9 +100,6 @@ public class Move extends Copy {
 //************************************************************************
 
     protected void doFileOperations() {
-        filterSets = getFilterSets();
-        filterChains = getFilterChains();
-
         //Attempt complete directory renames, if any, first.
         if (completeDirMap.size() > 0) {
             Enumeration e = completeDirMap.keys();
@@ -155,23 +151,32 @@ public class Move extends Copy {
 
                     if (!moved) {
                         try {
-                            log("Moving " + fromFile + " to " + toFile, verbosity);
+                            log("Moving " + fromFile + " to " + toFile, 
+                                verbosity);
 
-                            FilterSetCollection executionFilters = new FilterSetCollection();
+                            FilterSetCollection executionFilters = 
+                                new FilterSetCollection();
                             if (filtering) {
-                                executionFilters.addFilterSet(project.getGlobalFilterSet());
+                                executionFilters
+                                    .addFilterSet(project.getGlobalFilterSet());
                             }
-                            for (Enumeration filterEnum = getFilterSets().elements(); filterEnum.hasMoreElements();) {
-                                executionFilters.addFilterSet((FilterSet)filterEnum.nextElement());
+                            for (Enumeration filterEnum = 
+                                     getFilterSets().elements(); 
+                                 filterEnum.hasMoreElements();) {
+                                executionFilters
+                                    .addFilterSet((FilterSet)filterEnum
+                                                  .nextElement());
                             }
-                            getFileUtils().copyFile(f, d, executionFilters, filterChains,
+                            getFileUtils().copyFile(f, d, executionFilters, 
+                                                    getFilterChains(), 
                                                     forceOverwrite,
                                                     getPreserveLastModified(),
                                                     getEncoding(), project);
 
                             f = new File(fromFile);
                             if (!f.delete()) {
-                                throw new BuildException("Unable to delete file "
+                                throw new BuildException("Unable to delete "
+                                                         + "file "
                                                          + f.getAbsolutePath());
                             }
                         } catch (IOException ioe) {
@@ -192,7 +197,8 @@ public class Move extends Copy {
                 File d = new File((String)e.nextElement());
                 if (!d.exists()) {
                     if (!d.mkdirs()) {
-                        log("Unable to create directory " + d.getAbsolutePath(), Project.MSG_ERR);
+                        log("Unable to create directory " 
+                            + d.getAbsolutePath(), Project.MSG_ERR);
                     } else {
                         count++;
                     }
@@ -200,7 +206,8 @@ public class Move extends Copy {
             }
 
             if (count > 0) {
-                log("Moved " + count + " empty directories to " + destDir.getAbsolutePath());
+                log("Moved " + count + " empty directories to " 
+                    + destDir.getAbsolutePath());
             }
         }
 
@@ -224,7 +231,7 @@ public class Move extends Copy {
     protected boolean okToDelete(File d) {
         String[] list = d.list();
         if (list == null) {
-          return false;
+            return false;
         }     // maybe io error?
 
         for (int i = 0; i < list.length; i++) {
@@ -232,7 +239,7 @@ public class Move extends Copy {
             File f = new File(d, s);
             if (f.isDirectory()) {
                 if (!okToDelete(f)) {
-                  return false;
+                    return false;
                 }
             } else {
                 return false;   // found a file
@@ -248,7 +255,7 @@ public class Move extends Copy {
     protected void deleteDir(File d) {
         String[] list = d.list();
         if (list == null) {
-          return;
+            return;
         }      // on an io error list() can return null
 
         for (int i = 0; i < list.length; i++) {
@@ -257,13 +264,16 @@ public class Move extends Copy {
             if (f.isDirectory()) {
                 deleteDir(f);
             } else {
-                throw new BuildException("UNEXPECTED ERROR - The file " + f.getAbsolutePath() + " should not exist!");
+                throw new BuildException("UNEXPECTED ERROR - The file " 
+                                         + f.getAbsolutePath() 
+                                         + " should not exist!");
             }
         }
         log("Deleting directory " + d.getAbsolutePath(), verbosity);
         if (!d.delete()) {
-           throw new BuildException("Unable to delete directory " + d.getAbsolutePath());
-       }
+            throw new BuildException("Unable to delete directory " 
+                                     + d.getAbsolutePath());
+        }
     }
 
     /**
@@ -281,8 +291,8 @@ public class Move extends Copy {
         throws IOException, BuildException {
 
         boolean renamed = true;
-        if ((filterSets != null && filterSets.size() > 0) ||
-            (filterChains != null && filterChains.size() > 0)) {
+        if ((getFilterSets() != null && getFilterSets().size() > 0) ||
+            (getFilterChains() != null && getFilterChains().size() > 0)) {
             renamed = false;
         } else {
             if (!filtering) {
@@ -298,8 +308,8 @@ public class Move extends Copy {
 
                 if (destFile.exists()) {
                     if (!destFile.delete()) {
-                        throw new BuildException("Unable to remove existing file "
-                                                 + destFile);
+                        throw new BuildException("Unable to remove existing "
+                                                 + "file " + destFile);
                     }
                 }
                 renamed = sourceFile.renameTo(destFile);