Automated rollback of commit 6a54339bb943702bd7dffc43f85267dac98dc355.

*** Reason for rollback ***

b/71442447

*** Original change description ***

Call through to Path#createDirectoryAndParents from FileUtils.

This CL removes a method that due to its implementation causes threading difficulties for Path#createDirectory.

The tests for the method are brought across to FileSystemTests since the methods are now implemented natively by the FileSystem classes. The tests were also cleaned up.

The test revealed an edge case bug in JavaIoFileSystem, so fix this.

In two cases some code was using the return value from the old method. Returning "f...

***

ROLLBACK_OF=179864042
PiperOrigin-RevId: 180946251

5 files changed, 108 insertions, 54 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
index adeb9c847c..e42abdfd8a 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
@@ -616,15 +616,46 @@ public class FileSystemUtils {
   }
 
   /**
-   * Attempts to create a directory with the name of the given path, creating ancestors as
-   * necessary.
+   * Attempts to create a directory with the name of the given path, creating
+   * ancestors as necessary.
    *
-   * <p>Deprecated. Prefer to call {@link Path#createDirectoryAndParents()} directly.
+   * <p>Postcondition: completes normally iff {@code dir} denotes an existing
+   * directory (not necessarily canonical); completes abruptly otherwise.
+   *
+   * @return true if the directory was successfully created anew, false if it
+   *   already existed (including the case where {@code dir} denotes a symlink
+   *   to an existing directory)
+   * @throws IOException if the directory could not be created
    */
-  @Deprecated
   @ThreadSafe
-  public static void createDirectoryAndParents(Path dir) throws IOException {
-    dir.createDirectoryAndParents();
+  public static boolean createDirectoryAndParents(Path dir) throws IOException {
+    // Optimised for minimal number of I/O calls.
+
+    // Don't attempt to create the root directory.
+    if (dir.getParentDirectory() == null) {
+      return false;
+    }
+
+    FileSystem filesystem = dir.getFileSystem();
+    if (filesystem instanceof UnionFileSystem) {
+      // If using UnionFS, make sure that we do not traverse filesystem boundaries when creating
+      // parent directories by rehoming the path on the most specific filesystem.
+      FileSystem delegate = ((UnionFileSystem) filesystem).getDelegate(dir);
+      dir = delegate.getPath(dir.asFragment());
+    }
+
+    try {
+      return dir.createDirectory();
+    } catch (IOException e) {
+      if (e.getMessage().endsWith(" (No such file or directory)")) { // ENOENT
+        createDirectoryAndParents(dir.getParentDirectory());
+        return dir.createDirectory();
+      } else if (e.getMessage().endsWith(" (File exists)") && dir.isDirectory()) { // EEXIST
+        return false;
+      } else {
+        throw e; // some other error (e.g. ENOTDIR, EACCES, etc.)
+      }
+    }
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java
index 0dac1c4725..4477275f1f 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java
@@ -261,15 +261,7 @@ public class JavaIoFileSystem extends AbstractFileSystemWithCustomStat {
   @Override
   public void createDirectoryAndParents(Path path) throws IOException {
     java.nio.file.Path nioPath = getNioPath(path);
-    try {
-      Files.createDirectories(nioPath);
-    } catch (java.nio.file.FileAlreadyExistsException e) {
-      // Files.createDirectories will handle this case normally, but if the existing
-      // file is a symlink to a directory then it still throws. Swallow this.
-      if (!path.isDirectory()) {
-        throw e;
-      }
-    }
+    Files.createDirectories(nioPath);
   }
 
   private boolean linkExists(File file) {
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/UnionFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/UnionFileSystem.java
index d924e19d45..569357bcb2 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/UnionFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/UnionFileSystem.java
@@ -209,7 +209,7 @@ public class UnionFileSystem extends FileSystem {
   public void createDirectoryAndParents(Path path) throws IOException {
     checkModifiable(path);
     FileSystem delegate = getDelegate(path);
-    delegate.createDirectoryAndParents(adjustPath(path, delegate));
+    delegate.createDirectoryAndParents(path);
   }
 
   @Override
diff --git a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java
index e8896bbc0d..ba4b4e8c57 100644
--- a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java
+++ b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java
@@ -476,67 +476,47 @@ public abstract class FileSystemTest {
   }
 
   @Test
-  public void testCreateDirectoryAndParents() throws Exception {
+  public void testCreateDirectories() throws Exception {
     Path newPath = absolutize("new-dir/sub/directory");
     newPath.createDirectoryAndParents();
     assertThat(newPath.isDirectory()).isTrue();
   }
 
   @Test
-  public void testCreateDirectoryAndParentsCreatesEmptyDirectory() throws Exception {
+  public void testCreateDirectoriesIsNotFile() throws Exception {
     Path newPath = absolutize("new-dir/sub/directory");
     newPath.createDirectoryAndParents();
-    assertThat(newPath.getDirectoryEntries()).isEmpty();
+    assertThat(newPath.isFile()).isFalse();
   }
 
   @Test
-  public void testCreateDirectoryAndParentsIsOnlyChildInParent() throws Exception {
+  public void testCreateDirectoriesIsNotSymbolicLink() throws Exception {
     Path newPath = absolutize("new-dir/sub/directory");
     newPath.createDirectoryAndParents();
-    assertThat(newPath.getParentDirectory().getDirectoryEntries()).hasSize(1);
-    assertThat(newPath.getParentDirectory().getDirectoryEntries()).containsExactly(newPath);
+    assertThat(newPath.isSymbolicLink()).isFalse();
   }
 
   @Test
-  public void testCreateDirectoryAndParentsWhenAlreadyExistsSucceeds() throws Exception {
-    Path newPath = absolutize("new-dir");
-    newPath.createDirectory();
+  public void testCreateDirectoriesIsEmpty() throws Exception {
+    Path newPath = absolutize("new-dir/sub/directory");
     newPath.createDirectoryAndParents();
-    assertThat(newPath.isDirectory()).isTrue();
-  }
-
-  @Test
-  public void testCreateDirectoryAndParentsWhenAncestorIsFile() throws IOException {
-    Path path = absolutize("somewhere/deep/in");
-    path.getParentDirectory().createDirectoryAndParents();
-    FileSystemUtils.createEmptyFile(path);
-    Path theHierarchy = path.getChild("the-hierarchy");
-    MoreAsserts.assertThrows(IOException.class, theHierarchy::createDirectoryAndParents);
+    assertThat(newPath.getDirectoryEntries()).isEmpty();
   }
 
   @Test
-  public void testCreateDirectoryAndParentsWhenSymlinkToDir() throws IOException {
-    Path somewhereDeepIn = absolutize("somewhere/deep/in");
-    somewhereDeepIn.createDirectoryAndParents();
-    Path realDir = absolutize("real/dir");
-    realDir.createDirectoryAndParents();
-    assertThat(realDir.isDirectory()).isTrue();
-    Path theHierarchy = somewhereDeepIn.getChild("the-hierarchy");
-    theHierarchy.createSymbolicLink(realDir);
-    assertThat(theHierarchy.isDirectory()).isTrue();
-    theHierarchy.createDirectoryAndParents();
+  public void testCreateDirectoriesIsOnlyChildInParent() throws Exception {
+    Path newPath = absolutize("new-dir/sub/directory");
+    newPath.createDirectoryAndParents();
+    assertThat(newPath.getParentDirectory().getDirectoryEntries()).hasSize(1);
+    assertThat(newPath.getParentDirectory().getDirectoryEntries()).containsExactly(newPath);
   }
 
   @Test
-  public void testCreateDirectoryAndParentsWhenSymlinkEmbedded() throws IOException {
-    Path somewhereDeepIn = absolutize("somewhere/deep/in");
-    somewhereDeepIn.createDirectoryAndParents();
-    Path realDir = absolutize("real/dir");
-    realDir.createDirectoryAndParents();
-    Path the = somewhereDeepIn.getChild("the");
-    the.createSymbolicLink(realDir);
-    Path theHierarchy = somewhereDeepIn.getChild("hierarchy");
-    theHierarchy.createDirectoryAndParents();
+  public void testCreateAlreadyExistingDirectorySucceeds() throws Exception {
+    Path newPath = absolutize("new-dir");
+    newPath.createDirectory();
+    newPath.createDirectoryAndParents();
+    assertThat(newPath.isDirectory()).isTrue();
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java
index 656363333e..0e9b74ed3b 100644
--- a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java
@@ -18,6 +18,7 @@ import static com.google.devtools.build.lib.vfs.FileSystemUtils.appendWithoutExt
 import static com.google.devtools.build.lib.vfs.FileSystemUtils.commonAncestor;
 import static com.google.devtools.build.lib.vfs.FileSystemUtils.copyFile;
 import static com.google.devtools.build.lib.vfs.FileSystemUtils.copyTool;
+import static com.google.devtools.build.lib.vfs.FileSystemUtils.createDirectoryAndParents;
 import static com.google.devtools.build.lib.vfs.FileSystemUtils.deleteTree;
 import static com.google.devtools.build.lib.vfs.FileSystemUtils.moveFile;
 import static com.google.devtools.build.lib.vfs.FileSystemUtils.relativePath;
@@ -644,6 +645,56 @@ public class FileSystemUtilsTest {
   }
 
   @Test
+  public void testCreateDirectories() throws IOException {
+    Path mainPath = fileSystem.getPath("/some/where/deep/in/the/hierarchy");
+    assertThat(createDirectoryAndParents(mainPath)).isTrue();
+    assertThat(mainPath.exists()).isTrue();
+    assertThat(createDirectoryAndParents(mainPath)).isFalse();
+  }
+
+  @Test
+  public void testCreateDirectoriesWhenAncestorIsFile() throws IOException {
+    Path somewhereDeepIn = fileSystem.getPath("/somewhere/deep/in");
+    assertThat(createDirectoryAndParents(somewhereDeepIn.getParentDirectory())).isTrue();
+    FileSystemUtils.createEmptyFile(somewhereDeepIn);
+    Path theHierarchy = somewhereDeepIn.getChild("the-hierarchy");
+    try {
+      createDirectoryAndParents(theHierarchy);
+      fail();
+    } catch (IOException e) {
+      assertThat(e).hasMessage("/somewhere/deep/in (Not a directory)");
+    }
+  }
+
+  @Test
+  public void testCreateDirectoriesWhenSymlinkToDir() throws IOException {
+    Path somewhereDeepIn = fileSystem.getPath("/somewhere/deep/in");
+    assertThat(createDirectoryAndParents(somewhereDeepIn)).isTrue();
+    Path realDir = fileSystem.getPath("/real/dir");
+    assertThat(createDirectoryAndParents(realDir)).isTrue();
+
+    Path theHierarchy = somewhereDeepIn.getChild("the-hierarchy");
+    theHierarchy.createSymbolicLink(realDir);
+
+    assertThat(createDirectoryAndParents(theHierarchy)).isFalse();
+  }
+
+  @Test
+  public void testCreateDirectoriesWhenSymlinkEmbedded() throws IOException {
+    Path somewhereDeepIn = fileSystem.getPath("/somewhere/deep/in");
+    assertThat(createDirectoryAndParents(somewhereDeepIn)).isTrue();
+    Path realDir = fileSystem.getPath("/real/dir");
+    assertThat(createDirectoryAndParents(realDir)).isTrue();
+
+    Path the = somewhereDeepIn.getChild("the");
+    the.createSymbolicLink(realDir);
+
+    Path theHierarchy = somewhereDeepIn.getChild("hierarchy");
+    assertThat(createDirectoryAndParents(theHierarchy)).isTrue();
+  }
+
+
+  @Test
   public void testWriteIsoLatin1() throws Exception {
     Path file = fileSystem.getPath("/does/not/exist/yet.txt");
     FileSystemUtils.writeIsoLatin1(file, "Line 1", "Line 2", "Line 3");