Merge pull request #27 from winstonli/empty_file_in_dotgit

Empty file in dotgit: direct response to Git Bridge Fails to Clone With IllegalStateException (#3705)

services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/bridge/repo/FSGitRepoStore.java

@@ -76,13 +76,13 @@ public class FSGitRepoStore implements RepoStore {
             String projectName,
             long[] sizePtr
     ) throws IOException {
-        Preconditions.checkArgument(Project.isValidProjectName(projectName));
+        Project.checkValidProjectName(projectName);
         return Tar.bz2.zip(getDotGitForProject(projectName), sizePtr);
     }
 
     @Override
     public void remove(String projectName) throws IOException {
-        Preconditions.checkArgument(Project.isValidProjectName(projectName));
+        Project.checkValidProjectName(projectName);
         FileUtils.deleteDirectory(new File(rootDirectory, projectName));
     }
 
@@ -91,13 +91,22 @@ public class FSGitRepoStore implements RepoStore {
             String projectName,
             InputStream dataStream
     ) throws IOException {
-        Preconditions.checkArgument(Project.isValidProjectName(projectName));
-        Preconditions.checkState(getDirForProject(projectName).mkdirs());
+        Preconditions.checkArgument(
+                Project.isValidProjectName(projectName),
+                "[%s] invalid project name: ",
+                projectName
+        );
+        Preconditions.checkState(
+                getDirForProject(projectName).mkdirs(),
+                "[%s] directories for " +
+                        "evicted project already exist",
+                projectName
+        );
         Tar.bz2.unzip(dataStream, getDirForProject(projectName));
     }
 
     private File getDirForProject(String projectName) {
-        Preconditions.checkArgument(Project.isValidProjectName(projectName));
+        Project.checkValidProjectName(projectName);
         return Paths.get(
                 rootDirectory.getAbsolutePath()
         ).resolve(
@@ -106,7 +115,7 @@ public class FSGitRepoStore implements RepoStore {
     }
 
     private File getDotGitForProject(String projectName) {
-        Preconditions.checkArgument(Project.isValidProjectName(projectName));
+        Project.checkValidProjectName(projectName);
         return Paths.get(
                 rootDirectory.getAbsolutePath()
         ).resolve(
@@ -119,7 +128,12 @@ public class FSGitRepoStore implements RepoStore {
     private File initRootGitDirectory(String rootGitDirectoryPath) {
         File rootGitDirectory = new File(rootGitDirectoryPath);
         rootGitDirectory.mkdirs();
-        Preconditions.checkArgument(rootGitDirectory.isDirectory());
+        Preconditions.checkArgument(
+                rootGitDirectory.isDirectory(),
+                "given root git directory " +
+                        "is not a directory: %s",
+                rootGitDirectory.getAbsolutePath()
+        );
         return rootGitDirectory;
     }
 

services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/bridge/swap/job/SwapJobImpl.java

@@ -158,7 +158,7 @@ public class SwapJobImpl implements SwapJob {
      */
     @Override
     public void evict(String projName) throws IOException {
-        Preconditions.checkNotNull(projName);
+        Preconditions.checkNotNull(projName, "projName was null");
         Log.info("Evicting project: {}", projName);
         try (LockGuard __ = lock.lockGuard(projName)) {
             long[] sizePtr = new long[1];

services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/util/BiConsumerT.java

@@ -0,0 +1,11 @@
+package uk.ac.ic.wlgitbridge.util;
+
+/**
+ * BiConsumer interface that allows checked exceptions.
+ */
+@FunctionalInterface
+public interface BiConsumerT<T, U, E extends Throwable> {
+
+    void accept(T t, U u) throws E;
+
+}

services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/util/FunctionT.java

@@ -0,0 +1,14 @@
+package uk.ac.ic.wlgitbridge.util;
+
+/**
+ * Function interface that allows checked exceptions.
+ * @param <T>
+ * @param <R>
+ * @param <E>
+ */
+@FunctionalInterface
+public interface FunctionT<T, R, E extends Throwable> {
+
+    R apply(T t) throws E;
+
+}

services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/util/Project.java

@@ -1,5 +1,7 @@
 package uk.ac.ic.wlgitbridge.util;
 
+import com.google.common.base.Preconditions;
+
 /**
  * Created by winston on 23/08/2016.
  */
@@ -10,4 +12,9 @@ public class Project {
                 && !projectName.startsWith(".");
     }
 
+    public static void checkValidProjectName(String projectName) {
+        Preconditions.checkArgument(isValidProjectName(projectName),
+                "[%s] invalid project name", projectName);
+    }
+
 }

services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/util/ResourceUtil.java

@@ -0,0 +1,32 @@
+package uk.ac.ic.wlgitbridge.util;
+
+import org.apache.commons.io.FileUtils;
+
+import java.io.File;
+import java.io.IOException;
+
+public class ResourceUtil {
+
+    /**
+     * Creates a copy of a resource folder. Mainly used for testing to prevent
+     * the original folder from being mangled.
+     *
+     * It will have the same name as the original.
+     * @param resource the resource name, e.g. "/uk/ac/ic/wlgitbridge/file.txt"
+     * @param folderProvider function used to create the folder.
+     *                       E.g. TemporaryFolder from junit
+     * @return
+     * @throws IOException
+     */
+    public static File copyOfFolderResource(
+            String resource,
+            FunctionT<String, File, IOException> folderProvider
+    ) throws IOException {
+        File original
+                = new File(ResourceUtil.class.getResource(resource).getFile());
+        File tmp = folderProvider.apply(original.getName());
+        FileUtils.copyDirectory(original, tmp);
+        return tmp;
+    }
+
+}

services/git-bridge/src/main/java/uk/ac/ic/wlgitbridge/util/Tar.java

@@ -100,10 +100,7 @@ public class Tar {
                 continue;
             }
             long size = e.getSize();
-            Preconditions.checkArgument(
-                    size > 0 && size < Integer.MAX_VALUE,
-                    "file too big: tarTo should have thrown an IOException"
-            );
+            checkFileSize(size);
             try (OutputStream out = new FileOutputStream(f)) {
                 /* TarInputStream pretends each
                    entry's EOF is the stream's EOF */
@@ -112,6 +109,14 @@ public class Tar {
         }
     }
 
+    private static void checkFileSize(long size) {
+        Preconditions.checkArgument(
+                size >= 0 && size <= Integer.MAX_VALUE,
+                "file too big (" + size + " B): " +
+                        "tarTo should have thrown an IOException"
+        );
+    }
+
     private static void addTarEntry(
             TarArchiveOutputStream tout,
             Path base,
@@ -150,13 +155,19 @@ public class Tar {
             Path base,
             File file
     ) throws IOException {
-        Preconditions.checkArgument(file.isFile());
+        Preconditions.checkArgument(
+                file.isFile(),
+                "given file" +
+                " is not file: %s", file);
+        checkFileSize(file.length());
         String name = base.relativize(
                 Paths.get(file.getAbsolutePath())
         ).toString();
         ArchiveEntry entry = tout.createArchiveEntry(file, name);
         tout.putArchiveEntry(entry);
-        tout.write(FileUtils.readFileToByteArray(file));
+        try (InputStream in = new FileInputStream(file)) {
+            IOUtils.copy(in, tout);
+        }
         tout.closeArchiveEntry();
     }
 

services/git-bridge/src/test/java/uk/ac/ic/wlgitbridge/util/TarTest.java

@@ -1,6 +1,5 @@
 package uk.ac.ic.wlgitbridge.util;
 
-import org.apache.commons.io.FileUtils;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
@@ -8,8 +7,6 @@ import org.junit.rules.TemporaryFolder;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
-import java.nio.file.Path;
-import java.nio.file.Paths;
 
 import static org.junit.Assert.assertTrue;
 
@@ -18,37 +15,62 @@ import static org.junit.Assert.assertTrue;
  */
 public class TarTest {
 
+    private static final String RESOURCE_DIR
+            = "/uk/ac/ic/wlgitbridge/util/TarTest";
+
     private File testDir;
+    private File dirWithEmptyFile;
     private File tmpDir;
 
     @Before
     public void setup() throws IOException {
         TemporaryFolder tmpFolder = new TemporaryFolder();
         tmpFolder.create();
-        testDir = tmpFolder.newFolder("testdir");
-        Path resdir = Paths.get(
-                "src/test/resources/uk/ac/ic/wlgitbridge/util/TarTest/testdir"
-        );
-        FileUtils.copyDirectory(resdir.toFile(), testDir);
+        testDir = ResourceUtil.copyOfFolderResource(
+                RESOURCE_DIR + "/testdir",
+                tmpFolder::newFolder);
+        dirWithEmptyFile = ResourceUtil.copyOfFolderResource(
+                RESOURCE_DIR + "/dir_with_empty_file",
+                tmpFolder::newFolder);
         tmpDir = tmpFolder.newFolder();
     }
 
+    /**
+     * Compresses inputDir and decompresses to outputDir. Checks equality
+     * between outputDir and inputDir.
+     * @param inputDir the directory to compress
+     * @param outputDir the output directory. Must be empty.
+     * @param compressFunction compression function
+     * @param decompressFunction decompression function
+     * @throws IOException
+     */
+    private static void assertCompDecompEqual(
+            File inputDir,
+            File outputDir,
+            FunctionT<File, InputStream, IOException> compressFunction,
+            BiConsumerT<InputStream, File, IOException> decompressFunction
+    ) throws IOException {
+        try (InputStream tarbz2 = compressFunction.apply(inputDir)) {
+            decompressFunction.accept(tarbz2, outputDir);
+            File unzipped = new File(outputDir, inputDir.getName());
+            assertTrue(Files.contentsAreEqual(inputDir, unzipped));
+        }
+    }
+
     @Test
     public void tarAndUntarProducesTheSameResult() throws IOException {
-        try (InputStream tar = Tar.tar(testDir)) {
-            Tar.untar(tar, tmpDir);
-            File untarred = new File(tmpDir, "testdir");
-            assertTrue(Files.contentsAreEqual(testDir, untarred));
-        }
+        assertCompDecompEqual(testDir, tmpDir, Tar::tar, Tar::untar);
     }
 
     @Test
     public void tarbz2AndUntarbz2ProducesTheSameResult() throws IOException {
-        try (InputStream tarbz2 = Tar.bz2.zip(testDir)) {
-            Tar.bz2.unzip(tarbz2, tmpDir);
-            File unzipped = new File(tmpDir, "testdir");
-            assertTrue(Files.contentsAreEqual(testDir, unzipped));
-        }
+        assertCompDecompEqual(testDir, tmpDir, Tar.bz2::zip, Tar.bz2::unzip);
+    }
+
+    @Test
+    public void tarbz2WorksOnDirectoriesWithAnEmptyFile() throws IOException {
+        assertCompDecompEqual(
+                dirWithEmptyFile, tmpDir, Tar.bz2::zip, Tar.bz2::unzip);
     }
 
-}
+}