ronantakizawa/codereview-bench huggingface dataset:
Repository: GoogleContainerTools/jib
File: jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java (Java)

Code before:
  /**
   * Jar Type.
   *
   * <ul>
   *   <li>{@code REGULAR} a regular jar.
   *   <li>{@code SPRINGBOOT} a springboot fat jar. *
   * </ul>
   */
  public enum JarType {
    REGULAR,
    SPRINGBOOT;
  }

  /**
   * Determines whether the jar is a spring-boot or regular jar, given a path to the jar.
   *
   * @param jarPath path to the jar.
   * @return the jar type.
   * @throws IOException if I/O error occurs when opening the file.
   */
  public static JarType determineJarType(Path jarPath) throws IOException {
    JarFile jarFile = new JarFile(jarPath.toString());
    if (jarFile.getEntry("BOOT-INF") != null) {
      return JarType.SPRINGBOOT;
    }
    return JarType.REGULAR;
  }
}


Review comment (suggestion):
May need to add more logic to verify that the jar is in fact a regular jar, otherwise throw an exception saying that non-springboot fat jars are not supported.  

Code after:
  /**
   * Jar Type.
   *
   * <ul>
   *   <li>{@code STANDARD} a regular jar.
   *   <li>{@code SPRING_BOOT} a spring boot fat jar.
   * </ul>
   */
  public enum JarType {
    STANDARD,
    SPRING_BOOT;
  }

  /**
   * Determines whether the jar is a spring boot or regular jar, given a path to the jar.
   *
   * @param jarPath path to the jar.
   * @return the jar type.
   * @throws IOException if I/O error occurs when opening the file.
   */
  public static JarType determineJarType(Path jarPath) throws IOException {
    JarFile jarFile = new JarFile(jarPath.toFile());
    if (jarFile.getEntry("BOOT-INF") != null) {
      return JarType.SPRING_BOOT;
    }
    return JarType.STANDARD;
  }
}
Repository: GoogleContainerTools/jib
File: jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java (Java)

Code before:
 * use this file except in compliance with the License. You may obtain a copy of
 * the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
 * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 * License for the specific language governing permissions and limitations under
 * the License.
 */

package com.google.cloud.tools.jib.cli.jar;

import java.io.IOException;
import java.nio.file.Path;
import java.util.jar.JarFile;

public class JarProcessor {

  /**
   * Jar Type.
   *
   * <ul>
   *   <li>{@code REGULAR} a regular jar.
   *   <li>{@code SPRINGBOOT} a springboot fat jar. *
   * </ul>
   */
  public enum JarType {
    REGULAR,
    SPRINGBOOT;
  }

  /**
   * Determines whether the jar is a spring-boot or regular jar, given a path to the jar.
   *
   * @param jarPath path to the jar.
   * @return the jar type.
   * @throws IOException if I/O error occurs when opening the file.
   */
  public static JarType determineJarType(Path jarPath) throws IOException {
    JarFile jarFile = new JarFile(jarPath.toString());
    if (jarFile.getEntry("BOOT-INF") != null) {
      return JarType.SPRINGBOOT;
    }
    return JarType.REGULAR;
  }
}


Review comment (suggestion):
```suggestion
   *   <li>{@code SPRING_BOOT} a springboot fat jar.
```

Code after:
 * use this file except in compliance with the License. You may obtain a copy of
 * the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
 * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 * License for the specific language governing permissions and limitations under
 * the License.
 */

package com.google.cloud.tools.jib.cli.jar;

import java.io.IOException;
import java.nio.file.Path;
import java.util.jar.JarFile;

public class JarProcessor {

  /**
   * Jar Type.
   *
   * <ul>
   *   <li>{@code STANDARD} a regular jar.
   *   <li>{@code SPRING_BOOT} a spring boot fat jar.
   * </ul>
   */
  public enum JarType {
    STANDARD,
    SPRING_BOOT;
  }

  /**
   * Determines whether the jar is a spring boot or regular jar, given a path to the jar.
   *
   * @param jarPath path to the jar.
   * @return the jar type.
   * @throws IOException if I/O error occurs when opening the file.
   */
  public static JarType determineJarType(Path jarPath) throws IOException {
    JarFile jarFile = new JarFile(jarPath.toFile());
    if (jarFile.getEntry("BOOT-INF") != null) {
      return JarType.SPRING_BOOT;
    }
    return JarType.STANDARD;
  }
}
Repository: GoogleContainerTools/jib
File: jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java (Java)

Code before:
import java.util.jar.JarFile;

public class JarProcessor {

  /**
   * Jar Type.
   *
   * <ul>
   *   <li>{@code REGULAR} a regular jar.
   *   <li>{@code SPRINGBOOT} a springboot fat jar. *
   * </ul>
   */
  public enum JarType {
    REGULAR,
    SPRINGBOOT;
  }

  /**
   * Determines whether the jar is a spring-boot or regular jar, given a path to the jar.
   *
   * @param jarPath path to the jar.
   * @return the jar type.
   * @throws IOException if I/O error occurs when opening the file.
   */
  public static JarType determineJarType(Path jarPath) throws IOException {
    JarFile jarFile = new JarFile(jarPath.toString());
    if (jarFile.getEntry("BOOT-INF") != null) {
      return JarType.SPRINGBOOT;
    }
    return JarType.REGULAR;
  }
}


Review comment (suggestion):
nit: it doesn't matter at all here, but generally I try to avoid dealing with path as a string.
```suggestion
    JarFile jarFile = new JarFile(jarPath.toFile());
```

Code after:
import java.util.jar.JarFile;

public class JarProcessor {

  /**
   * Jar Type.
   *
   * <ul>
   *   <li>{@code STANDARD} a regular jar.
   *   <li>{@code SPRING_BOOT} a spring boot fat jar.
   * </ul>
   */
  public enum JarType {
    STANDARD,
    SPRING_BOOT;
  }

  /**
   * Determines whether the jar is a spring boot or regular jar, given a path to the jar.
   *
   * @param jarPath path to the jar.
   * @return the jar type.
   * @throws IOException if I/O error occurs when opening the file.
   */
  public static JarType determineJarType(Path jarPath) throws IOException {
    JarFile jarFile = new JarFile(jarPath.toFile());
    if (jarFile.getEntry("BOOT-INF") != null) {
      return JarType.SPRING_BOOT;
    }
    return JarType.STANDARD;
  }
}
Repository: GoogleContainerTools/jib
File: jib-cli/src/integration-test/java/com/google/cloud/tools/jib/cli/cli2/JarCommandTest.java (Java)

Code before:

  @After
  public void tearDown() {
    System.setOut(originalOut);
    System.setErr(originalErr);
  }

  @Test
  public void testErrorLogging_fileDoesNotExist() {
    Integer actual =
        new CommandLine(new JibCli())
            .execute("--target", "docker://jib-cli-image", "jar", "unknown.jar");

    assertThat(actual).isEqualTo(FAILURE_CODE);
    assertThat(err.toString())
        .contains("[ERROR] The file path provided does not exist: unknown.jar");
  }

  @Test
  public void testErrorLogging_directoryGiven() throws URISyntaxException {
    Path jarFile = Paths.get(Resources.getResource("emptyDir").toURI());
    Integer actual =
        new CommandLine(new JibCli())
            .execute("--target", "docker://jib-cli-image", "jar", jarFile.toString());

    assertThat(actual).isEqualTo(FAILURE_CODE);
    assertThat(err.toString())
        .contains(
            "[ERROR] The file path provided is for a directory. Please provide a path to a jar file: "
                + jarFile.toString());
  }

  @Test
  public void testJar_toDocker() throws IOException, InterruptedException, URISyntaxException {
    Path jarFile = Paths.get(Resources.getResource("simpleJar.jar").toURI());
    Integer actual =
        new CommandLine(new JibCli())
            .execute("--target", "docker://jib-cli-image", "jar", jarFile.toString());
    String output = new Command("docker", "run", "--rm", "jib-cli-image").run();

    assertThat(actual).isEqualTo(SUCCESS_CODE);
    assertThat(output).isEqualTo("Hello World");
  }
}


Review comment (suggestion):
Let's just go for
```suggestion
    assertThat(exitCode).isEqualTo(1);
```

Code after:
    jibCli.setErr(new PrintWriter(stringWriter));

    Path jarFile = Paths.get("/");
    Integer exitCode =
        jibCli.execute("--target", "docker://jib-cli-image", "jar", jarFile.toString());

    assertThat(exitCode).isEqualTo(1);
    assertThat(stringWriter.toString())
        .isEqualTo(
            "[ERROR] The file path provided is for a directory. Please provide a path to a jar file: "
                + jarFile.toString()
                + "\n");
  }

  @Test
  public void testJar_toDocker() throws IOException, InterruptedException, URISyntaxException {
    Path jarFile = Paths.get(Resources.getResource("simpleJar.jar").toURI());
    Integer exitCode =
        new CommandLine(new JibCli())
            .execute("--target", "docker://jib-cli-image", "jar", jarFile.toString());
    String output = new Command("docker", "run", "--rm", "jib-cli-image").run();

    assertThat(exitCode).isEqualTo(0);
    assertThat(output).isEqualTo("Hello World");
  }
}
Repository: GoogleContainerTools/jib
File: jib-core/src/main/java/com/google/cloud/tools/jib/api/Containerizer.java (Java)

Code before:
  }

  Path getBaseImageLayersCacheDirectory() {
    return baseImageLayersCacheDirectory;
  }

  Path getApplicationLayersCacheDirectory() throws CacheDirectoryCreationException {
    return getApplicationLayersCacheDirectory(
        () -> Paths.get(System.getProperty("java.io.tmpdir")));
  }

  @VisibleForTesting
  Path getApplicationLayersCacheDirectory(Supplier<Path> tmpDirSupplier)
      throws CacheDirectoryCreationException {
    if (applicationLayersCacheDirectory == null) {
      // Create a directory in temp if application layers cache directory is not set.
      try {
        Path systemTmp = tmpDirSupplier.get();
        Path jibTempCache = systemTmp.resolve(DEFAULT_APPLICATION_CACHE_DIRECTORY_NAME);
        if (Files.exists(jibTempCache)) {
          if (!Files.isDirectory(jibTempCache)) {
            throw new CacheDirectoryCreationException(
                jibTempCache.toString() + " already exists and is not a directory");
          }
        } else {
          Files.createDirectory(jibTempCache);
        }
        applicationLayersCacheDirectory = jibTempCache;

      } catch (IOException ex) {
        throw new CacheDirectoryCreationException(ex);
      }
    }
    return applicationLayersCacheDirectory;
  }

  EventHandlers buildEventHandlers() {
    return eventHandlersBuilder.build();
  }

  boolean getAllowInsecureRegistries() {
    return allowInsecureRegistries;
  }

  boolean isOfflineMode() {
    return offline;
  }

  String getToolName() {
    return toolName;
  }

Review comment (performance):
Actually think I can clean up all this logic and just use Files.createDirectories ? Or just lets the cache writer handle this?

Code after:
  Path getBaseImageLayersCacheDirectory() {
    return baseImageLayersCacheDirectory;
  }

  Path getApplicationLayersCacheDirectory() throws CacheDirectoryCreationException {
    if (applicationLayersCacheDirectory == null) {
      // Create a directory in temp if application layers cache directory is not set.
      try {
        Path tmp = Paths.get(System.getProperty("java.io.tmpdir"));
        applicationLayersCacheDirectory = tmp.resolve(DEFAULT_APPLICATION_CACHE_DIRECTORY_NAME);
        Files.createDirectories(applicationLayersCacheDirectory);
      } catch (IOException ex) {
        throw new CacheDirectoryCreationException(ex);
      }
    }
    return applicationLayersCacheDirectory;
  }

  EventHandlers buildEventHandlers() {
    return eventHandlersBuilder.build();
  }

  boolean getAllowInsecureRegistries() {
    return allowInsecureRegistries;
  }

  boolean isOfflineMode() {
    return offline;
  }

  String getToolName() {
    return toolName;
  }

  @Nullable
  String getToolVersion() {
    return toolVersion;
  }

  boolean getAlwaysCacheBaseImage() {
    return alwaysCacheBaseImage;
  }

  String getDescription() {
    return description;
  }

  ImageConfiguration getImageConfiguration() {
    return imageConfiguration;
  }
Repository: GoogleContainerTools/jib
File: jib-core/src/test/java/com/google/cloud/tools/jib/api/ContainerizerTest.java (Java)

Code before:
  public void testGetImageConfiguration_dockerDaemonImage() throws InvalidImageReferenceException {
    Containerizer containerizer = Containerizer.to(DockerDaemonImage.named("docker/deamon/image"));

    ImageConfiguration imageConfiguration = containerizer.getImageConfiguration();
    Assert.assertEquals("docker/deamon/image", imageConfiguration.getImage().toString());
    Assert.assertEquals(0, imageConfiguration.getCredentialRetrievers().size());
  }

  @Test
  public void testGetImageConfiguration_tarImage() throws InvalidImageReferenceException {
    Containerizer containerizer =
        Containerizer.to(TarImage.at(Paths.get("output/file")).named("tar/image"));

    ImageConfiguration imageConfiguration = containerizer.getImageConfiguration();
    Assert.assertEquals("tar/image", imageConfiguration.getImage().toString());
    Assert.assertEquals(0, imageConfiguration.getCredentialRetrievers().size());
  }

  @Test
  public void testGetApplicationLayersCacheDirectory_defaults()
      throws InvalidImageReferenceException, CacheDirectoryCreationException, IOException {
    Containerizer containerizer = Containerizer.to(RegistryImage.named("registry/image"));
    Path applicationLayersCache = containerizer.getApplicationLayersCacheDirectory();
    Path expectedCacheDir =
        Paths.get(System.getProperty("java.io.tmpdir"))
            .resolve(Containerizer.DEFAULT_APPLICATION_CACHE_DIRECTORY_NAME);
    Assert.assertTrue(Files.isSameFile(expectedCacheDir, applicationLayersCache));
  }
}


Review comment (performance):
How about verifying the name in the test?
```suggestion
            .resolve("jib-core-application-layers-cache");
```

Code after:
    Assert.assertEquals("docker/deamon/image", imageConfiguration.getImage().toString());
    Assert.assertEquals(0, imageConfiguration.getCredentialRetrievers().size());
  }

  @Test
  public void testGetImageConfiguration_tarImage() throws InvalidImageReferenceException {
    Containerizer containerizer =
        Containerizer.to(TarImage.at(Paths.get("output/file")).named("tar/image"));

    ImageConfiguration imageConfiguration = containerizer.getImageConfiguration();
    Assert.assertEquals("tar/image", imageConfiguration.getImage().toString());
    Assert.assertEquals(0, imageConfiguration.getCredentialRetrievers().size());
  }

  @Test
  public void testGetApplicationLayersCacheDirectory_defaults()
      throws InvalidImageReferenceException, CacheDirectoryCreationException, IOException {
    Containerizer containerizer = Containerizer.to(RegistryImage.named("registry/image"));
    Path applicationLayersCache = containerizer.getApplicationLayersCacheDirectory();
    Path expectedCacheDir =
        Paths.get(System.getProperty("java.io.tmpdir"))
            .resolve("jib-core-application-layers-cache");
    Assert.assertTrue(Files.isSameFile(expectedCacheDir, applicationLayersCache));
  }
}
Repository: GoogleContainerTools/jib
File: jib-cli/src/test/java/com/google/cloud/tools/jib/cli/jar/JarProcessorsTest.java (Java)

Code before:

  @Test
  public void testFrom_springBootExploded() throws IOException, URISyntaxException {
    Path jarPath = Paths.get(Resources.getResource(SPRING_BOOT).toURI());
    JarProcessor processor =
        JarProcessors.from(jarPath, mockTemporaryDirectoryProvider, ProcessingMode.exploded);
    Mockito.verify(mockTemporaryDirectoryProvider).newDirectory();
    assertThat(processor).isInstanceOf(SpringBootExplodedProcessor.class);
  }

  @Test
  public void testFrom_incompatibleBaseImage() throws URISyntaxException {
    Path jarPath = Paths.get(Resources.getResource(JAVA_14_JAR).toURI());
    IllegalStateException exception =
        assertThrows(
            IllegalStateException.class,
            () ->
                JarProcessors.from(
                    jarPath, mockTemporaryDirectoryProvider, ProcessingMode.exploded));
    assertThat(exception)
        .hasMessageThat()
        .startsWith("The input JAR (" + jarPath + ") is compiled with Java " + 14);
  }

  @Test
  public void testGetVersion_versionNotFound() throws URISyntaxException, IOException {
    Path jarPath = Paths.get(Resources.getResource(STANDARD).toURI());
    Integer version = JarProcessors.getJavaMajorVersion(jarPath);
    assertThat(version).isEqualTo(JarProcessors.VERSION_NOT_FOUND);
  }
}


Review comment (suggestion):
```suggestion
  public void testGetJavaMajorVersion_versionNotFound() throws URISyntaxException, IOException {
```

Code after:

  @Test
  public void testFrom_springBootExploded() throws IOException, URISyntaxException {
    Path jarPath = Paths.get(Resources.getResource(SPRING_BOOT).toURI());
    JarProcessor processor =
        JarProcessors.from(jarPath, mockTemporaryDirectoryProvider, ProcessingMode.exploded);
    Mockito.verify(mockTemporaryDirectoryProvider).newDirectory();
    assertThat(processor).isInstanceOf(SpringBootExplodedProcessor.class);
  }

  @Test
  public void testFrom_incompatibleBaseImage() throws URISyntaxException {
    Path jarPath = Paths.get(Resources.getResource(JAVA_14_JAR).toURI());
    IllegalStateException exception =
        assertThrows(
            IllegalStateException.class,
            () ->
                JarProcessors.from(
                    jarPath, mockTemporaryDirectoryProvider, ProcessingMode.exploded));
    assertThat(exception)
        .hasMessageThat()
        .startsWith("The input JAR (" + jarPath + ") is compiled with Java 14");
  }

  @Test
  public void testGetMajorJavaVersion_versionNotFound() throws URISyntaxException, IOException {
    Path jarPath = Paths.get(Resources.getResource(STANDARD).toURI());
    Integer version = JarProcessors.getJavaMajorVersion(jarPath);
    assertThat(version).isEqualTo(0);
  }
}
Repository: GoogleContainerTools/jib
File: jib-core/src/main/java/com/google/cloud/tools/jib/tar/TarExtractor.java (Java)

Code before:
              ByteStreams.copy(tarArchiveInputStream, out);
            }
          }
        }
      }
    }
    preserveModificationTimes(destination, entries, enableReproducibleTimestamps);
  }

  /**
   * Preserve modification time of files and directories in a tar file. If a directory is not an
   * entry in the tar file and reproducible timestamps are enabled then it's modification timestamp
   * is set to a constant value.
   *
   * @param destination target root for unzipping
   * @param entries list of entries in tar file
   * @param enableReproducibleTimestamps whether or not reproducible timestamps should be used
   * @throws IOException when I/O error occurs
   */
  private static void preserveModificationTimes(
      Path destination, List<TarArchiveEntry> entries, boolean enableReproducibleTimestamps)
      throws IOException {
    if (enableReproducibleTimestamps) {
      FileTime epochPlusOne = FileTime.fromMillis(1000L);
      new DirectoryWalker(destination)
          .filter(path -> Files.isDirectory(path))
          .walk(path -> Files.setLastModifiedTime(path, epochPlusOne));
    }
    for (TarArchiveEntry entry : entries) {
      Files.setLastModifiedTime(
          destination.resolve(entry.getName()), FileTime.from(entry.getModTime().toInstant()));
    }
  }
}


Review comment (suggestion):
nit
```suggestion
          .filter(Files::isDirectory)
```

Code after:
              ByteStreams.copy(tarArchiveInputStream, out);
            }
          }
        }
      }
    }
    preserveModificationTimes(destination, entries, enableReproducibleTimestamps);
  }

  /**
   * Preserve modification timestamps of files and directories in a tar file. If a directory is not
   * an entry in the tar file and reproducible timestamps are enabled then its modification
   * timestamp is set to a constant value. Note that the modification timestamps of symbolic links
   * are not preserved even with reproducible timestamps enabled.
   *
   * @param destination target root for unzipping
   * @param entries list of entries in tar file
   * @param enableReproducibleTimestamps whether or not reproducible timestamps should be used
   * @throws IOException when I/O error occurs
   */
  private static void preserveModificationTimes(
      Path destination, List<TarArchiveEntry> entries, boolean enableReproducibleTimestamps)
      throws IOException {
    if (enableReproducibleTimestamps) {
      FileTime epochPlusOne = FileTime.fromMillis(1000L);
      new DirectoryWalker(destination)
          .filter(Files::isDirectory)
          .walk(path -> Files.setLastModifiedTime(path, epochPlusOne));
    }
    for (TarArchiveEntry entry : entries) {

      // Setting the symbolic link's modification timestamp will cause the modification timestamp of
      // the target to change
      if (!entry.isSymbolicLink()) {
        Files.setLastModifiedTime(
            destination.resolve(entry.getName()), FileTime.from(entry.getModTime().toInstant()));
      }
    }
  }
}
Repository: GoogleContainerTools/jib
File: jib-core/src/test/java/com/google/cloud/tools/jib/tar/TarExtractorTest.java (Java)

Code before:

    Path destination = temporaryFolder.getRoot().toPath();

    TarExtractor.extract(source, destination, true);

    assertThat(Files.getLastModifiedTime(destination.resolve("level-1")))
        .isEqualTo(FileTime.fromMillis(1000L));
    assertThat(Files.getLastModifiedTime(destination.resolve("level-1/level-2")))
        .isEqualTo(FileTime.fromMillis(1000L));
    assertThat(Files.getLastModifiedTime(destination.resolve("level-1/level-2/level-3")))
        .isEqualTo(FileTime.fromMillis(1000L));
    assertThat(Files.getLastModifiedTime(destination.resolve("level-1/level-2/level-3/file.txt")))
        .isEqualTo(FileTime.from(Instant.parse("2021-01-29T21:10:02Z")));
  }

  @Test
  public void testExtract_reproducibleTimestampsEnabled_invalidDestination() throws IOException {
    Path destination = temporaryFolder.getRoot().toPath();
    Files.createDirectory(destination.resolve("directory"));

    assertThrows(
        IllegalStateException.class,
        () -> TarExtractor.extract(Paths.get("ignore"), destination, true));
  }
}


Review comment (suggestion):
Let's assert the exception message too, since anyone can throw the very common `IllegalStateException`.

Code after:

    Path destination = temporaryFolder.getRoot().toPath();

    TarExtractor.extract(source, destination, true);

    assertThat(Files.getLastModifiedTime(destination.resolve("level-1")))
        .isEqualTo(FileTime.fromMillis(1000L));
    assertThat(Files.getLastModifiedTime(destination.resolve("level-1/level-2")))
        .isEqualTo(FileTime.fromMillis(1000L));
    assertThat(Files.getLastModifiedTime(destination.resolve("level-1/level-2/level-3")))
        .isEqualTo(FileTime.fromMillis(1000L));
    assertThat(Files.getLastModifiedTime(destination.resolve("level-1/level-2/level-3/file.txt")))
        .isEqualTo(FileTime.from(Instant.parse("2021-01-29T21:10:02Z")));
  }

  @Test
  public void testExtract_reproducibleTimestampsEnabled_destinationNotEmpty() throws IOException {
    Path destination = temporaryFolder.getRoot().toPath();
    temporaryFolder.newFile();

    IllegalStateException exception =
        assertThrows(
            IllegalStateException.class,
            () -> TarExtractor.extract(Paths.get("ignore"), destination, true));
    assertThat(exception).hasMessageThat().startsWith("Cannot enable reproducible timestamps");
  }
}