ronantakizawa/codereview-bench huggingface dataset:
Repository: GoogleContainerTools/jib
File: jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java (Java)

Code before:

  @Nested
  @Optional
  public SkaffoldParameters getSkaffold() {
    return skaffold;
  }

  @Input
  boolean getAllowInsecureRegistries() {
    if (System.getProperty(PropertyNames.ALLOW_INSECURE_REGISTRIES) != null) {
      return Boolean.getBoolean(PropertyNames.ALLOW_INSECURE_REGISTRIES);
    }
    return allowInsecureRegistries.get();
  }

  @Input
  @Optional
  public String getContainerizingMode() {
    String property = System.getProperty(PropertyNames.CONTAINERIZING_MODE);
    return property != null ? property : containerizingMode.get();
  }

  @Input
  @Optional
  public Property<String> getConfigurationName() {
    return configurationName;
  }

  public String readConfigurationName() {
    String property = System.getProperty(PropertyNames.CONFIGURATION_NAME);
    return property != null ? property : configurationName.get();
  }

  @Nested
  @Optional
  public ListProperty<ExtensionParameters> getPluginExtensions() {
    return pluginExtensions;
  }
}


Review comment (suggestion):
our of curiosity, can we just set the property to the system property value here?
```
public Property<String> getConfigurationName() {
  String property = System.getProperty(PropertyNames.CONFIGURATION_NAME);
  if (property != null) {
    configurationName.set(property);
  }
  return configurationName;
}
```

while this still looks a little odd, if it is possible, it matches more closely our previous patterns and it removes the ambiguous `GetconfigurationName/readConfigurationName` situation.

Code after:

  @Nested
  @Optional
  public SkaffoldParameters getSkaffold() {
    return skaffold;
  }

  @Input
  boolean getAllowInsecureRegistries() {
    if (System.getProperty(PropertyNames.ALLOW_INSECURE_REGISTRIES) != null) {
      return Boolean.getBoolean(PropertyNames.ALLOW_INSECURE_REGISTRIES);
    }
    return allowInsecureRegistries.get();
  }

  @Input
  @Optional
  public String getContainerizingMode() {
    String property = System.getProperty(PropertyNames.CONTAINERIZING_MODE);
    return property != null ? property : containerizingMode.get();
  }

  /**
   * Returns the configurationName property while setting it to the value of the system property if
   * present.
   *
   * @return The configurationName property
   */
  @Input
  @Optional
  public Property<String> getConfigurationName() {
    String property = System.getProperty(PropertyNames.CONFIGURATION_NAME);
    if (property != null) {
      configurationName.set(property);
    }
    return configurationName;
  }

  @Nested
  @Optional
  public ListProperty<ExtensionParameters> getPluginExtensions() {
    return pluginExtensions;
  }
}
Repository: GoogleContainerTools/jib
File: jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessors.java (Java)

Code before:
  @VisibleForTesting
  static Integer determineJavaMajorVersion(Path jarPath) throws IOException {
    try (JarFile jarFile = new JarFile(jarPath.toFile())) {
      Enumeration<JarEntry> jarEntries = jarFile.entries();
      while (jarEntries.hasMoreElements()) {
        String jarEntry = jarEntries.nextElement().toString();
        if (jarEntry.endsWith(".class") && !jarEntry.endsWith("module-info.class")) {
          try (URLClassLoader loader = new URLClassLoader(new URL[] {jarPath.toUri().toURL()});
              DataInputStream classFile =
                  new DataInputStream(loader.getResourceAsStream(jarEntry))) {

            // Check magic number
            if (classFile.readInt() != 0xCAFEBABE) {
              throw new IOException("Invalid class file format.");
            }

            // Skip over minor version
            classFile.skipBytes(2);

            int majorVersion = classFile.readUnsignedShort();
            int javaVersion = (majorVersion - 45) + 1;
            return javaVersion;
          }
        }
      }
      return VERSION_NOT_FOUND;
    }
  }
}


Review comment (suggestion):
Testing with a jar file containing a fake empty .class file  resulted in an EOFException with no error message. This change is to provide more clarity on why this exception is being thrown. 

Code after:
   */
  @VisibleForTesting
  static Integer determineJavaMajorVersion(Path jarPath) throws IOException {
    try (JarFile jarFile = new JarFile(jarPath.toFile())) {
      Enumeration<JarEntry> jarEntries = jarFile.entries();
      while (jarEntries.hasMoreElements()) {
        String jarEntry = jarEntries.nextElement().toString();
        if (jarEntry.endsWith(".class") && !jarEntry.endsWith("module-info.class")) {
          try (URLClassLoader loader = new URLClassLoader(new URL[] {jarPath.toUri().toURL()});
              DataInputStream classFile =
                  new DataInputStream(loader.getResourceAsStream(jarEntry))) {

            // Check magic number
            if (classFile == null || classFile.readInt() != 0xCAFEBABE) {
              throw new IllegalArgumentException(
                  "The class file (" + jarEntry + ") is of an invalid format.");
            }

            // Skip over minor version
            classFile.skipBytes(2);

            int majorVersion = classFile.readUnsignedShort();
            int javaVersion = (majorVersion - 45) + 1;
            return javaVersion;
          } catch (EOFException ex) {
            throw new IllegalArgumentException(
                "Reached end of class file ("
                    + jarEntry
                    + ") before being able to read the java major version. Make sure that the file is of the correct format.");
          }
        }
      }
      return VERSION_NOT_FOUND;
    }
  }
}
Repository: GoogleContainerTools/jib
File: jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibPluginTest.java (Java)

Code before:
  @Test
  public void testNonWebAppProject() {
    Project project = createProject("java", "com.google.cloud.tools.jib");

    TaskContainer tasks = project.getTasks();
    Exception exception = assertThrows(UnknownTaskException.class, () -> tasks.getByPath(":war"));
    assertThat(exception).hasMessageThat().isNotNull();
  }

  @Test
  public void testJibTaskGroupIsSet() {
    Project project = createProject("java", "com.google.cloud.tools.jib");

    TaskContainer tasks = project.getTasks();
    KNOWN_JIB_TASKS.forEach(
        taskName -> assertThat(tasks.getByPath(taskName).getGroup()).isEqualTo("Jib"));
  }

  @Test
  public void testLazyEvalForImageAndTags() {
    UnexpectedBuildFailure exception =
        assertThrows(
            UnexpectedBuildFailure.class,
            () -> testProject.build(JibPlugin.BUILD_IMAGE_TASK_NAME, "-Djib.console=plain"));

    String output = exception.getBuildResult().getOutput().trim();
    assertThat(output)
        .contains(
            "Containerizing application to updated-image, updated-image:updated-tag, updated-image:tag2");
  }

  private Project createProject(String... plugins) {
    Project project =
        ProjectBuilder.builder().withProjectDir(testProjectRoot.getRoot()).withName("root").build();
    Arrays.asList(plugins).forEach(project.getPluginManager()::apply);
    return project;
  }
}


Review comment (suggestion):
nit: while we're at it, since we use `assertThat().contains()`,
```suggestion
    String output = exception.getBuildResult().getOutput();
```

Code after:
  @Test
  public void testNonWebAppProject() {
    Project project = createProject("java", "com.google.cloud.tools.jib");

    TaskContainer tasks = project.getTasks();
    Exception exception = assertThrows(UnknownTaskException.class, () -> tasks.getByPath(":war"));
    assertThat(exception).hasMessageThat().isNotNull();
  }

  @Test
  public void testJibTaskGroupIsSet() {
    Project project = createProject("java", "com.google.cloud.tools.jib");

    TaskContainer tasks = project.getTasks();
    KNOWN_JIB_TASKS.forEach(
        taskName -> assertThat(tasks.getByPath(taskName).getGroup()).isEqualTo("Jib"));
  }

  @Test
  public void testLazyEvalForImageAndTags() {
    UnexpectedBuildFailure exception =
        assertThrows(
            UnexpectedBuildFailure.class,
            () -> testProject.build(JibPlugin.BUILD_IMAGE_TASK_NAME, "-Djib.console=plain"));

    String output = exception.getBuildResult().getOutput();
    assertThat(output)
        .contains(
            "Containerizing application to updated-image, updated-image:updated-tag, updated-image:tag2");
  }

  private Project createProject(String... plugins) {
    Project project =
        ProjectBuilder.builder().withProjectDir(testProjectRoot.getRoot()).withName("root").build();
    Arrays.asList(plugins).forEach(project.getPluginManager()::apply);
    return project;
  }
}
Repository: GoogleContainerTools/jib
File: jib-cli/src/test/java/com/google/cloud/tools/jib/cli/JibCliTest.java (Java)

Code before:
 * 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;

import static com.google.common.truth.Truth.assertThat;

import java.util.logging.ConsoleHandler;
import java.util.logging.Handler;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.junit.Test;

public class JibCliTest {

  @Test
  public void testConfigureHttpLogging() {
    Logger logger = JibCli.configureHttpLogging(Level.ALL);
    assertThat(logger.getName()).isEqualTo("com.google.api.client.http.HttpTransport");
    assertThat(logger.getLevel()).isEqualTo(Level.ALL);

    assertThat(logger.getHandlers()).asList().hasSize(1);
    Handler handler = logger.getHandlers()[0];
    assertThat(handler).isInstanceOf(ConsoleHandler.class);
    assertThat(handler.getLevel()).isEqualTo(Level.ALL);
  }
}


Review comment (suggestion):
nit:
```suggestion
    assertThat(logger.getHandlers()).hasLength(1);
```

Code after:
 * 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;

import static com.google.common.truth.Truth.assertThat;

import java.util.logging.ConsoleHandler;
import java.util.logging.Handler;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.junit.Test;

public class JibCliTest {

  @Test
  public void testConfigureHttpLogging() {
    Logger logger = JibCli.configureHttpLogging(Level.ALL);
    assertThat(logger.getName()).isEqualTo("com.google.api.client.http.HttpTransport");
    assertThat(logger.getLevel()).isEqualTo(Level.ALL);

    assertThat(logger.getHandlers()).hasLength(1);
    Handler handler = logger.getHandlers()[0];
    assertThat(handler).isInstanceOf(ConsoleHandler.class);
    assertThat(handler.getLevel()).isEqualTo(Level.ALL);
  }
}
Repository: GoogleContainerTools/jib
File: jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/UpdateChecker.java (Java)

Code before:
        lastUpdateCheckTemp.toFile().deleteOnExit();
        Files.write(lastUpdateCheckTemp, Instant.now().toString().getBytes(StandardCharsets.UTF_8));
        Files.move(lastUpdateCheckTemp, lastUpdateCheck, StandardCopyOption.REPLACE_EXISTING);

        if (currentVersion.equals(version.latest)) {
          return Optional.empty();
        }
        return Optional.of(version.latest);
      } finally {
        httpClient.shutDown();
      }

    } catch (IOException ex) {
      log.accept(LogEvent.debug("Update check failed; " + ex.getMessage()));
    }

    return Optional.empty();
  }

  /**
   * Returns the latest Jib version available if the check succeeded and the current version is
   * outdated, or returns {@code Optional.empty()} if the check was interrupted or did not determine
   * that a later version was available.
   *
   * @param updateMessageFuture the {@link Future} returned by {@link UpdateChecker#checkForUpdate}
   * @return the {@link Optional} message to upgrade Jib if a later version was found, else {@code
   *     Optional.empty()}.
   */
  public static Optional<String> finishUpdateCheck(Future<Optional<String>> updateMessageFuture) {
    if (updateMessageFuture.isDone()) {
      try {
        return updateMessageFuture.get();
      } catch (InterruptedException | ExecutionException ignored) {
        // Fail silently;
      }
    }
    updateMessageFuture.cancel(true);
    return Optional.empty();
  }

  private UpdateChecker() {}
}


Review comment (suggestion):
```suggestion
   * @return the latest version, if found
```

Code after:
        lastUpdateCheckTemp.toFile().deleteOnExit();
        Files.write(lastUpdateCheckTemp, Instant.now().toString().getBytes(StandardCharsets.UTF_8));
        Files.move(lastUpdateCheckTemp, lastUpdateCheck, StandardCopyOption.REPLACE_EXISTING);

        if (currentVersion.equals(version.latest)) {
          return Optional.empty();
        }
        return Optional.of(version.latest);
      } finally {
        httpClient.shutDown();
      }

    } catch (IOException ex) {
      log.accept(LogEvent.debug("Update check failed; " + ex.getMessage()));
    }

    return Optional.empty();
  }

  /**
   * Returns the latest Jib version available if the check succeeded and the current version is
   * outdated, or returns {@code Optional.empty()} if the check was interrupted or did not determine
   * that a later version was available.
   *
   * @param updateMessageFuture the {@link Future} returned by {@link UpdateChecker#checkForUpdate}
   * @return the latest version, if found, else {@code Optional.empty()}.
   */
  public static Optional<String> finishUpdateCheck(Future<Optional<String>> updateMessageFuture) {
    if (updateMessageFuture.isDone()) {
      try {
        return updateMessageFuture.get();
      } catch (InterruptedException | ExecutionException ignored) {
        // Fail silently;
      }
    }
    updateMessageFuture.cancel(true);
    return Optional.empty();
  }

  private UpdateChecker() {}
}
Repository: GoogleContainerTools/jib
File: jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ContainerParameters.java (Java)

Code before:
  public void setPorts(List<String> ports) {
    this.ports = ports;
  }

  @Input
  @Optional
  public List<String> getVolumes() {
    if (System.getProperty(PropertyNames.CONTAINER_VOLUMES) != null) {
      return ConfigurationPropertyValidator.parseListProperty(
          System.getProperty(PropertyNames.CONTAINER_VOLUMES));
    }
    return volumes;
  }

  public void setVolumes(List<String> volumes) {
    this.volumes = volumes;
  }

  @Input
  @Optional
  public MapProperty<String, String> getLabels() {
    String labelsProperty = System.getProperty(PropertyNames.CONTAINER_LABELS);
    if (labelsProperty != null) {
      Map<String, String> parsedLabels =
          ConfigurationPropertyValidator.parseMapProperty(labelsProperty);
      labels.set(parsedLabels);
    }
    return labels;
  }

  @Input
  @Optional
  public String getAppRoot() {
    if (System.getProperty(PropertyNames.CONTAINER_APP_ROOT) != null) {
      return System.getProperty(PropertyNames.CONTAINER_APP_ROOT);
    }
    return appRoot;
  }

  public void setAppRoot(String appRoot) {
    this.appRoot = appRoot;
  }

  @Input
  @Nullable
  @Optional
  public String getUser() {
    if (System.getProperty(PropertyNames.CONTAINER_USER) != null) {
      return System.getProperty(PropertyNames.CONTAINER_USER);
    }
    return user;

Review comment (suggestion):
Sorry, this was missing in #3094. (I updated the issue to reflect this information.) We should call `.set()` only when the values differ. Using `.equals()` will do.
```suggestion
      if (!parsedLabels.equals(labels.get()) {
        labels.set(parsedLabels);
      }
```

Code after:
  public void setPorts(List<String> ports) {
    this.ports = ports;
  }

  @Input
  @Optional
  public List<String> getVolumes() {
    if (System.getProperty(PropertyNames.CONTAINER_VOLUMES) != null) {
      return ConfigurationPropertyValidator.parseListProperty(
          System.getProperty(PropertyNames.CONTAINER_VOLUMES));
    }
    return volumes;
  }

  public void setVolumes(List<String> volumes) {
    this.volumes = volumes;
  }

  @Input
  @Optional
  public MapProperty<String, String> getLabels() {
    String labelsProperty = System.getProperty(PropertyNames.CONTAINER_LABELS);
    if (labelsProperty != null) {
      Map<String, String> parsedLabels =
          ConfigurationPropertyValidator.parseMapProperty(labelsProperty);
      if (!parsedLabels.equals(labels.get())) {
        labels.set(parsedLabels);
      }
    }
    return labels;
  }

  @Input
  @Optional
  public String getAppRoot() {
    if (System.getProperty(PropertyNames.CONTAINER_APP_ROOT) != null) {
      return System.getProperty(PropertyNames.CONTAINER_APP_ROOT);
    }
    return appRoot;
  }

  public void setAppRoot(String appRoot) {
    this.appRoot = appRoot;
  }

  @Input
  @Nullable
  @Optional
  public String getUser() {
    if (System.getProperty(PropertyNames.CONTAINER_USER) != null) {
      return System.getProperty(PropertyNames.CONTAINER_USER);
Repository: GoogleContainerTools/jib
File: jib-core/src/main/java/com/google/cloud/tools/jib/blob/WritableContentsBlob.java (Java)

Code before:

package com.google.cloud.tools.jib.blob;

import com.google.cloud.tools.jib.hash.Digests;
import com.google.cloud.tools.jib.hash.WritableContents;
import java.io.IOException;
import java.io.OutputStream;

/** A {@link Blob} that holds {@link WritableContents}. */
class WritableContentsBlob implements Blob {

  private final WritableContents writableContents;

  WritableContentsBlob(WritableContents writableContents) {
    this.writableContents = writableContents;
  }

  @Override
  public BlobDescriptor writeTo(OutputStream outputStream) throws IOException {
    return Digests.computeDigest(writableContents, outputStream);
  }

  // in general it is since the underlying data is accessed in the lambda
  @Override
  public boolean isRetryable() {
    return true;
  }
}


Review comment (suggestion):
This should be false. `WritableContents` can be any lambda and doesn't have a guarantee that they are retriable. In fact, blob pulls (which is reading HTTP response input stream) from a registry is routinely wrapped as `WritableContents`, and such a stream is not a retriable one. Actually, It is confusing at this level whether it refers to 1) the resettable nature of certain streams for re-reading; or 2) the signal that you can retry an HTTP request that deals with this blob in a very particular way in Jib. The confusion I think is coming from the fact that this "retry" concept only makes sense at a higher-level layer handling this blob, which is not comprehensible only from this class. Please check the other comment.

Code after:

package com.google.cloud.tools.jib.blob;

import com.google.cloud.tools.jib.hash.Digests;
import com.google.cloud.tools.jib.hash.WritableContents;
import java.io.IOException;
import java.io.OutputStream;

/** A {@link Blob} that holds {@link WritableContents}. */
class WritableContentsBlob implements Blob {

  private final WritableContents writableContents;
  private final boolean retryable;

  WritableContentsBlob(WritableContents writableContents, boolean retryable) {
    this.writableContents = writableContents;
    this.retryable = retryable;
  }

  @Override
  public BlobDescriptor writeTo(OutputStream outputStream) throws IOException {
    return Digests.computeDigest(writableContents, outputStream);
  }

  @Override
  public boolean isRetryable() {
    return retryable;
  }
}
Repository: GoogleContainerTools/jib
File: jib-core/src/main/java/com/google/cloud/tools/jib/blob/Blob.java (Java)

Code before:
 * License for the specific language governing permissions and limitations under
 * the License.
 */

package com.google.cloud.tools.jib.blob;

import java.io.IOException;
import java.io.OutputStream;

/** Holds a BLOB source for writing to an {@link OutputStream}. */
public interface Blob {

  /**
   * Writes the BLOB to an {@link OutputStream}. Does not close the {@code outputStream}.
   *
   * @param outputStream the {@link OutputStream} to write to
   * @return the {@link BlobDescriptor} of the written BLOB
   * @throws IOException if writing the BLOB fails
   */
  BlobDescriptor writeTo(OutputStream outputStream) throws IOException;

  /**
   * Enables to notify to the HTTP client if the underlying request can be retried.
   * @return {@code true} if {@link #writeTo(OutputStream)} can be called multiple times, {@code false} otherwise.
   */
  default boolean isRetryable() {
    return false;
  }
}


Review comment (refactor):
In our codebase, there exists a hierarchy of layers (roughly divided into Java packages), and `com.google.cloud.tools.jib.blob` sits at the very bottom part of the hierarchy. We also maintain the hierarchy non-circular. Each layer should be insulated and should only know and use the abstractions established below its position in the hierarchy in general. There have been exceptions, but I think we've been doing good at maintaining this nice hierarchy especially better in the foundational part of the codebase.

The point is, this blob layer should not be coupled with or concerned with any behaviors conceivable at a higher abstraction, such as HTTP request retrying. In fact, the class should never know or talk about HTTP at all. In other words, if we were to take out the package `com.google.cloud.tools.jib.blob` (and packages below that in the hierarchy) into a separate, standalone dependency library JAR, it should be perfectly self-contained, isolated, usable, and still make sense.

Code after:
 * License for the specific language governing permissions and limitations under
 * the License.
 */

package com.google.cloud.tools.jib.blob;

import java.io.IOException;
import java.io.OutputStream;

/** Holds a BLOB source for writing to an {@link OutputStream}. */
public interface Blob {

  /**
   * Writes the BLOB to an {@link OutputStream}. Does not close the {@code outputStream}.
   *
   * @param outputStream the {@link OutputStream} to write to
   * @return the {@link BlobDescriptor} of the written BLOB
   * @throws IOException if writing the BLOB fails
   */
  BlobDescriptor writeTo(OutputStream outputStream) throws IOException;

  /**
   * Enables to notify if the underlying request can be retried (useful in the context of a
   * retryable HTTP request for ex).
   *
   * @return {@code true} if {@link #writeTo(OutputStream)} can be called multiple times, {@code
   *     false} otherwise.
   */
  boolean isRetryable();
}