ronantakizawa/codereview-bench huggingface dataset:
Repository: square/okhttp
File: okhttp/src/commonMain/kotlin/okhttp3/Protocol.kt (Kotlin)

Code before:
/*
 * Copyright (C) 2014 Square, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not 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 okhttp3

import kotlin.jvm.JvmStatic

/**
 * Protocols that OkHttp implements for [ALPN][ietf_alpn] selection.
 *
 * ## Protocol vs Scheme
 *
 * Despite its name, [java.net.URL.getProtocol] returns the [scheme][java.net.URI.getScheme] (http,
 * https, etc.) of the URL, not the protocol (http/1.1, spdy/3.1, etc.). OkHttp uses the word
 * *protocol* to identify how HTTP messages are framed.
 *
 * [ietf_alpn]: http://tools.ietf.org/html/draft-ietf-tls-applayerprotoneg
 */
enum class Protocol(private val protocol: String) {
  /**
   * An obsolete plaintext framing that does not use persistent sockets by default.
   */
  HTTP_1_0("http/1.0"),

  /**
   * A plaintext framing that includes persistent connections.
   *
   * This version of OkHttp implements [RFC 7230][rfc_7230], and tracks revisions to that spec.
   *

Review comment (suggestion):
What to do with

```
  @Deprecated("OkHttp has dropped support for SPDY. Prefer {@link #HTTP_2}.")
  SPDY_3("spdy/3.1"),
```

Code after:
/*
 * Copyright (C) 2014 Square, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not 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 okhttp3

import kotlin.jvm.JvmStatic
import okio.IOException

/**
 * Protocols that OkHttp implements for [ALPN][ietf_alpn] selection.
 *
 * ## Protocol vs Scheme
 *
 * Despite its name, [java.net.URL.getProtocol] returns the [scheme][java.net.URI.getScheme] (http,
 * https, etc.) of the URL, not the protocol (http/1.1, spdy/3.1, etc.). OkHttp uses the word
 * *protocol* to identify how HTTP messages are framed.
 *
 * [ietf_alpn]: http://tools.ietf.org/html/draft-ietf-tls-applayerprotoneg
 */
enum class Protocol(private val protocol: String) {
  /**
   * An obsolete plaintext framing that does not use persistent sockets by default.
   */
  HTTP_1_0("http/1.0"),

  /**
   * A plaintext framing that includes persistent connections.
   *
   * This version of OkHttp implements [RFC 7230][rfc_7230], and tracks revisions to that spec.
Repository: square/okhttp
File: okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt (Kotlin)

Code before:

      if (privateHost && !resolvePrivateAddresses) {
        throw UnknownHostException("private hosts not resolved")
      }

      if (!privateHost && !resolvePublicAddresses) {
        throw UnknownHostException("public hosts not resolved")
      }
    }

    return try {
      lookupHttps(hostname)
    } catch (bestFailure: UnknownHostException) {
      if (checkIsIpAddress(hostname)) {
        try {
          InetAddress.getAllByName(hostname).toList()
        } catch (e: UnknownHostException) {
          throw bestFailure
        }
      } else {
        throw bestFailure
      }
    }
  }

  private fun checkIsIpAddress(hostname: String): Boolean {
    if (ipv4Regex.matches(hostname)) {
      return true
    }

    if (hostname[0].digitToIntOrNull(16) != null || hostname[0] == ':' || hostname[0] == '[') {
      return true
    }

    return false
  }

  @Throws(UnknownHostException::class)
  private fun lookupHttps(hostname: String): List<InetAddress> {
    val networkRequests = ArrayList<Call>(2)
    val failures = ArrayList<Exception>(2)
    val results = ArrayList<InetAddress>(5)

    buildRequest(hostname, networkRequests, results, failures, DnsRecordCodec.TYPE_A)

    if (includeIPv6) {
      buildRequest(hostname, networkRequests, results, failures, DnsRecordCodec.TYPE_AAAA)
    }

    executeRequests(hostname, networkRequests, results, failures)


Review comment (suggestion):
Should use our existing function, `canParseAsIpAddress()` from `okhttp3.internal`. We definitely don’t want to functions to convert IP addresses to strings.

Code after:
        throw UnknownHostException("private hosts not resolved")
      }

      if (!privateHost && !resolvePublicAddresses) {
        throw UnknownHostException("public hosts not resolved")
      }
    }

    return lookupHttps(hostname)
  }

  @Throws(UnknownHostException::class)
  private fun lookupHttps(hostname: String): List<InetAddress> {
    val networkRequests = ArrayList<Call>(2)
    val failures = ArrayList<Exception>(2)
    val results = ArrayList<InetAddress>(5)

    buildRequest(hostname, networkRequests, results, failures, DnsRecordCodec.TYPE_A)

    if (includeIPv6) {
      buildRequest(hostname, networkRequests, results, failures, DnsRecordCodec.TYPE_AAAA)
    }

    executeRequests(hostname, networkRequests, results, failures)

    return results.ifEmpty {
      throwBestFailure(hostname, failures)
    }
  }

  private fun buildRequest(
    hostname: String,
    networkRequests: MutableList<Call>,
    results: MutableList<InetAddress>,
    failures: MutableList<Exception>,
    type: Int
  ) {
    val request = buildRequest(hostname, type)
    val response = getCacheOnlyResponse(request)

    response?.let { processResponse(it, hostname, results, failures) } ?: networkRequests.add(
        client.newCall(request))
  }

  private fun executeRequests(
    hostname: String,
    networkRequests: List<Call>,
    responses: MutableList<InetAddress>,
    failures: MutableList<Exception>
  ) {
    val latch = CountDownLatch(networkRequests.size)
Repository: square/okhttp
File: okhttp/src/commonMain/kotlin/okhttp3/internal/-UtilCommon.kt (Kotlin)

Code before:
/** Closes this, ignoring any checked exceptions. */
fun Closeable.closeQuietly() {
  try {
    close()
  } catch (rethrown: RuntimeException) {
    throw rethrown
  } catch (_: Exception) {
  }
}

/**
 * Returns true if file streams can be manipulated independently of their paths. This is typically
 * true for systems like Mac, Unix, and Linux that use inodes in their file system interface. It is
 * typically false on Windows.
 *
 * If this returns false we won't permit simultaneous reads and writes. When writes commit we need
 * to delete the previous snapshots, and that won't succeed if the file is open. (We do permit
 * multiple simultaneous reads.)
 *
 * @param file a file in the directory to check. This file shouldn't already exist!
 */
internal fun FileSystem.isCivilized(file: Path): Boolean {
  val sink: Sink = sink(file)
  return try {
    delete(file)
    true
  } catch (_: IOException) {
    false
  } finally {
    sink.close()
    delete(file)
  }
}

/** Delete file we expect but don't require to exist. */
internal fun FileSystem.deleteIfExists(path: Path) {
  try {
    delete(path)
  } catch (fnfe: FileNotFoundException) {
    return
  }
}

/** Tolerant delete, try to clear as many files as possible even after a failure. */
internal fun FileSystem.deleteContents(directory: Path) {
  var exception: IOException? = null
  val files = try {
    list(directory)
  } catch (fnfe: FileNotFoundException) {
    return
  }

Review comment (bug):
@swankjesse  This use seems broken, specifically the `return true` escapes the close in `use`.

Code after:
/** Closes this, ignoring any checked exceptions. */
fun Closeable.closeQuietly() {
  try {
    close()
  } catch (rethrown: RuntimeException) {
    throw rethrown
  } catch (_: Exception) {
  }
}

/**
 * Returns true if file streams can be manipulated independently of their paths. This is typically
 * true for systems like Mac, Unix, and Linux that use inodes in their file system interface. It is
 * typically false on Windows.
 *
 * If this returns false we won't permit simultaneous reads and writes. When writes commit we need
 * to delete the previous snapshots, and that won't succeed if the file is open. (We do permit
 * multiple simultaneous reads.)
 *
 * @param file a file in the directory to check. This file shouldn't already exist!
 */
internal fun FileSystem.isCivilized(file: Path): Boolean {
  sink(file).use {
    try {
      delete(file)
      return true
    } catch (_: IOException) {
    }
  }
  delete(file)
  return false
}

/** Delete file we expect but don't require to exist. */
internal fun FileSystem.deleteIfExists(path: Path) {
  try {
    delete(path)
  } catch (fnfe: FileNotFoundException) {
    return
  }
}

/** Tolerant delete, try to clear as many files as possible even after a failure. */
internal fun FileSystem.deleteContents(directory: Path) {
  var exception: IOException? = null
  val files = try {
    list(directory)
  } catch (fnfe: FileNotFoundException) {
    return
  }
  for (file in files) {
Repository: square/okhttp
File: okhttp/src/jvmMain/kotlin/okhttp3/internal/http/CallServerInterceptor.kt (Kotlin)

Code before:
      }
      if ("close".equals(response.request.header("Connection"), ignoreCase = true) ||
          "close".equals(response.header("Connection"), ignoreCase = true)) {
        exchange.noNewExchangesOnConnection()
      }
      if ((code == HTTP_NO_CONTENT || code == HTTP_RESET_CONTENT) && response.body.contentLength() > 0L) {
        throw ProtocolException(
            "HTTP $code had non-zero Content-Length: ${response.body.contentLength()}")
      }
      return response
    } catch (e: IOException) {
      if (sendRequestException != null) {
        sendRequestException.addSuppressed(e)
        throw sendRequestException
      }
      throw e
    }
  }

  private fun shouldIgnoreAndWaitForRealResponse(code: Int, exchange: Exchange): Boolean = when {
    // Server sent a 100-continue even though we did not request one. Try again to read the
    // actual response status.
    code == HTTP_CONTINUE -> true

    // Processing & Early Hints (103) but not supported yet in OkHttp
    code in (HTTP_PROCESSING until HTTP_OK) -> true

    else -> false
  }
}


Review comment (suggestion):
Take it or leave it: I find the numbers much easier to remember vs. the symbolic names.

(This range is also weird, since it has nothing to do with HTTP_PROCESSING or HTTP_OK in particular and instead wants to express 1xx)

Code after:
      if ("close".equals(response.request.header("Connection"), ignoreCase = true) ||
          "close".equals(response.header("Connection"), ignoreCase = true)) {
        exchange.noNewExchangesOnConnection()
      }
      if ((code == 204 || code == 205) && response.body.contentLength() > 0L) {
        throw ProtocolException(
            "HTTP $code had non-zero Content-Length: ${response.body.contentLength()}")
      }
      return response
    } catch (e: IOException) {
      if (sendRequestException != null) {
        sendRequestException.addSuppressed(e)
        throw sendRequestException
      }
      throw e
    }
  }

  private fun shouldIgnoreAndWaitForRealResponse(code: Int, exchange: Exchange): Boolean = when {
    // Server sent a 100-continue even though we did not request one. Try again to read the
    // actual response status.
    code == 100 -> true

    // Handle Processing (102) & Early Hints (103) and any new codes without failing
    // 100 and 101 are the exceptions with different meanings
    // But Early Hints not currently exposed
    code in (102 until 200) -> true

    else -> false
  }
}
Repository: square/okhttp
File: okhttp/src/nonJvmMain/kotlin/okhttp3/internal/-HostnamesNonJvm.kt (Kotlin)

Code before:
      decodeIpv6(host, 0, host.length)
    }) ?: return null
    // TODO implement properly
    return inet6AddressToAscii(inetAddressByteArray)
  }

  try {
    val result = idnToAscii(host)
    if (result.isEmpty()) return null

    return if (result.containsInvalidHostnameAsciiCodes()) {
      // The IDN ToASCII result contains illegal characters.
      null
    } else if (result.containsInvalidLabelLengths()) {
      // The IDN ToASCII result contains invalid labels.
      null
    } else {
      result
    }
  } catch (_: IllegalArgumentException) {
    return null
  }
}

internal fun inet4AddressToAscii(address: ByteArray): String {
  return address.joinToString(".")
}

fun idnToAscii(host: String): String {
  // TODO implement properly
  return SYSTEM_NORMALIZER.normalizeNfc(host).lowercase()
}


Review comment (suggestion):
I think this might have printed `-1.-1.-1.-1` instead of `255.255.255.255` because we’re using signed bytes. Sigh.

Code after:
/*
 * Copyright (C) 2022 Square, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not 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 okhttp3.internal

import com.squareup.okhttpicu.SYSTEM_NORMALIZER

internal actual fun idnToAscii(host: String): String {
  // TODO implement properly
  return SYSTEM_NORMALIZER.normalizeNfc(host).lowercase()
}
Repository: square/okhttp
File: settings.gradle.kts (Kotlin)

Code before:
rootProject.name = "okhttp-parent"

include(":mockwebserver")
project(":mockwebserver").name = "mockwebserver3"
include(":mockwebserver-deprecated")
project(":mockwebserver-deprecated").name = "mockwebserver"
include(":mockwebserver-junit4")
project(":mockwebserver-junit4").name = "mockwebserver3-junit4"
include(":mockwebserver-junit5")
project(":mockwebserver-junit5").name = "mockwebserver3-junit5"

val androidBuild: String by settings
val graalBuild: String by settings
val loomBuild: String by settings

if (androidBuild.toBoolean()) {
  include(":regression-test")
}

if (graalBuild.toBoolean()) {
  include(":native-image-tests")
}

include(":okcurl")
include(":okhttp")
include(":okhttp-android")
include(":okhttp-bom")
include(":okhttp-brotli")
include(":okhttp-dnsoverhttps")
include(":okhttp-hpacktests")
include(":okhttp-idna-mapping-table")
include(":okhttp-logging-interceptor")
project(":okhttp-logging-interceptor").name = "logging-interceptor"
include(":okhttp-sse")
include(":okhttp-testing-support")
include(":okhttp-tls")
include(":okhttp-coroutines")
include(":okhttp-urlconnection")
include(":samples:compare")
include(":samples:crawler")
include(":samples:guide")
include(":samples:simple-client")
include(":samples:slack")
include(":samples:static-server")
include(":samples:tlssurvey")
include(":samples:unixdomainsockets")
include(":android-test")

enableFeaturePreview("TYPESAFE_PROJECT_ACCESSORS")


Review comment (bug):
This function should probably be inverted. It’s surprising that Gradle builds run outside IntelliJ return true, given this function’s name.

Maybe it could be isKnownBrokenIntellij()

Code after:
include(":okhttp-logging-interceptor")
project(":okhttp-logging-interceptor").name = "logging-interceptor"
include(":okhttp-sse")
include(":okhttp-testing-support")
include(":okhttp-tls")
include(":okhttp-coroutines")
include(":okhttp-urlconnection")
include(":samples:compare")
include(":samples:crawler")
include(":samples:guide")
include(":samples:simple-client")
include(":samples:slack")
include(":samples:static-server")
include(":samples:tlssurvey")
include(":samples:unixdomainsockets")

if (isIdea20232OrHigher()) {
  include(":okhttp-android")
  include(":android-test")
}

enableFeaturePreview("TYPESAFE_PROJECT_ACCESSORS")

fun isIdea20232OrHigher(): Boolean {
  // No problem outside Idea
  val ideaVersionString = System.getProperty("idea.version") ?: return true

  return try {
    val (major, minor, _) = ideaVersionString.split(".", limit = 3)
    KotlinVersion(major.toInt(), minor.toInt()) >= KotlinVersion(2023, 2)
  } catch (e: Exception) {
    // Unknown version, presumably compatible
    true
  }
}
Repository: square/okhttp
File: okhttp/src/jvmMain/kotlin/okhttp3/Cookie.kt (Kotlin)

Code before:
        append("; httponly")
      }

      append("; samesite=").append(sameSite)

      return toString()
    }
  }

  fun newBuilder(): Builder = Builder(this)

  /**
   * Builds a cookie. The [name], [value], and [domain] values must all be set before calling
   * [build].
   */
  class Builder() {
    private var name: String? = null
    private var value: String? = null
    private var expiresAt = MAX_DATE
    private var domain: String? = null
    private var path = "/"
    private var secure = false
    private var httpOnly = false
    private var persistent = false
    private var hostOnly = false
    private var sameSite = "Lax"

    internal constructor(cookie: Cookie) : this() {
      this.name = cookie.name
      this.value = cookie.value
      this.expiresAt = cookie.expiresAt
      this.domain = cookie.domain
      this.path = cookie.path
      this.secure = cookie.secure
      this.httpOnly = cookie.httpOnly
      this.persistent = cookie.persistent
      this.hostOnly = cookie.hostOnly
      this.sameSite = cookie.sameSite
    }

    fun name(name: String) = apply {
      require(name.trim() == name) { "name is not trimmed" }
      this.name = name
    }

    fun value(value: String) = apply {
      require(value.trim() == value) { "value is not trimmed" }
      this.value = value
    }

    fun expiresAt(expiresAt: Long) = apply {

Review comment (suggestion):
... and I think the default should be null, not `Lax`



Code after:
        append("; httponly")
      }

      if (sameSite != null) {
        append("; samesite=").append(sameSite)
      }

      return toString()
    }
  }

  fun newBuilder(): Builder = Builder(this)

  /**
   * Builds a cookie. The [name], [value], and [domain] values must all be set before calling
   * [build].
   */
  class Builder() {
    private var name: String? = null
    private var value: String? = null
    private var expiresAt = MAX_DATE
    private var domain: String? = null
    private var path = "/"
    private var secure = false
    private var httpOnly = false
    private var persistent = false
    private var hostOnly = false
    private var sameSite: String? = null

    internal constructor(cookie: Cookie) : this() {
      this.name = cookie.name
      this.value = cookie.value
      this.expiresAt = cookie.expiresAt
      this.domain = cookie.domain
      this.path = cookie.path
      this.secure = cookie.secure
      this.httpOnly = cookie.httpOnly
      this.persistent = cookie.persistent
      this.hostOnly = cookie.hostOnly
      this.sameSite = cookie.sameSite
    }

    fun name(name: String) = apply {
      require(name.trim() == name) { "name is not trimmed" }
      this.name = name
    }

    fun value(value: String) = apply {
      require(value.trim() == value) { "value is not trimmed" }
      this.value = value
    }
Repository: square/okhttp
File: okhttp-testing-support/src/main/kotlin/okhttp3/internal/io/InMemoryFileSystem.kt (Kotlin)

Code before:
  @Throws(IOException::class)
  override fun delete(file: File) {
    files.remove(file)
  }

  override fun exists(file: File) = files.containsKey(file)

  override fun size(file: File) = files[file]?.size ?: 0L

  @Throws(IOException::class)
  override fun rename(from: File, to: File) {
    files[to] = files.remove(from) ?: throw FileNotFoundException()
  }

  @Throws(IOException::class)
  override fun deleteContents(directory: File) {
    val i = files.keys.iterator()
    while (i.hasNext()) {
      val file = i.next()
      if (file.isDescendentOf(directory)) i.remove()
    }
  }

  override fun toString() = "InMemoryFileSystem"
}


Review comment (suggestion):
files.keys is a live view; `files.keys.toSet()` is a necessary defensive copy, if that’s what we want

Code after:
  @Throws(IOException::class)
  override fun delete(file: File) {
    files.remove(file)
  }

  override fun exists(file: File) = files.containsKey(file)

  override fun size(file: File) = files[file]?.size ?: 0L

  @Throws(IOException::class)
  override fun rename(from: File, to: File) {
    files[to] = files.remove(from) ?: throw FileNotFoundException()
  }

  @Throws(IOException::class)
  override fun deleteContents(directory: File) {
    val i = files.keys.iterator()
    while (i.hasNext()) {
      val file = i.next()
      if (file.isDescendentOf(directory)) i.remove()
    }
  }

  override fun toString() = "InMemoryFileSystem"
  fun allPaths(): MutableSet<File> {
    return files.keys
  }
}
Repository: square/okhttp
File: mockwebserver/src/main/kotlin/okhttp3/mockwebserver/MockResponse.kt (Kotlin)

Code before:
    promises.add(promise)
  }

  /**
   * When [protocols][MockWebServer.protocols] include [HTTP_2][okhttp3.Protocol], this pushes
   * [settings] before writing the response.
   */
  fun withSettings(settings: Settings) = apply {
    this.settings = settings
  }

  /**
   * Attempts to perform a web socket upgrade on the connection.
   * This will overwrite any previously set status or body.
   */
  fun withWebSocketUpgrade(listener: WebSocketListener) = apply {
    status = "HTTP/1.1 101 Switching Protocols"
    setHeader("Connection", "Upgrade")
    setHeader("Upgrade", "websocket")
    body = null
    webSocketListener = listener
  }

  override fun toString() = status

  fun addInformationalResponseInternal(informationalResponse: MockResponse): MockResponse = apply {
    informationalResponses += informationalResponse
  }

  companion object {
    private const val CHUNKED_BODY_HEADER = "Transfer-encoding: chunked"
  }
}


Review comment (suggestion):
This is not great, but seems better to have some limited support without the bigger change of backporting.

cc @swankjesse 

Code after:
    promises.add(promise)
  }

  /**
   * When [protocols][MockWebServer.protocols] include [HTTP_2][okhttp3.Protocol], this pushes
   * [settings] before writing the response.
   */
  fun withSettings(settings: Settings) = apply {
    this.settings = settings
  }

  /**
   * Attempts to perform a web socket upgrade on the connection.
   * This will overwrite any previously set status or body.
   */
  fun withWebSocketUpgrade(listener: WebSocketListener) = apply {
    status = "HTTP/1.1 101 Switching Protocols"
    setHeader("Connection", "Upgrade")
    setHeader("Upgrade", "websocket")
    body = null
    webSocketListener = listener
  }

  override fun toString() = status

  fun addInformationalResponse(informationalResponse: MockResponse): MockResponse = apply {
    informationalResponses += informationalResponse
  }

  companion object {
    private const val CHUNKED_BODY_HEADER = "Transfer-encoding: chunked"
  }
}