From cc887a6d23a0ddb2afc27c28c46ad46d453f399e Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Fri, 10 Jul 2026 15:43:27 -0400 Subject: [PATCH] refactor(serve)!: remove JGit engine, make subprocess the only git backend Make the native `git` subprocess (`ProcessGitClient`) the sole git engine for `bazel-diff serve` and remove the in-process JGit integration entirely. - Delete JGitClient, its test, and the SLF4J binding test (the slf4j-nop dep existed only to silence JGit's NOP-logger warning); drop the org.eclipse.jgit, slf4j-nop, and slf4j-api maven deps and repin maven_install.json. - Remove the `--gitEngine` flag; `createGitClient()` always returns ProcessGitClient. `/metrics` `gitEngine` now reports the literal "subprocess" (field kept so the JSON contract is stable). - Update serve_harness.py to drop the engine dimension (its cross-engine parity gates no longer apply); update README/template prose, the /metrics example, and the regenerated `serve --help` block. This also resolves the partial-clone checkout failure JGit 5.13 could not handle (it has no promisor/lazy-fetch): native git fetches the missing blobs at checkout. Validated: binary builds, the serve/server unit tests pass, and E2E `testServeEndToEnd` (real git repo, no `--gitEngine`) exercises the subprocess path end-to-end and passes. BREAKING CHANGE: the `--gitEngine` flag is removed and a `git` binary must now be on PATH. Invocations passing `--gitEngine=subprocess` should drop the flag; `--gitEngine=jgit` (the previous default) is no longer available. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/serve-harness.yml | 4 +- MODULE.bazel | 9 - README.md | 30 +- cli/BUILD | 21 -- .../kotlin/com/bazel_diff/cli/ServeCommand.kt | 30 +- .../com/bazel_diff/server/JGitClient.kt | 177 ------------ .../com/bazel_diff/cli/ServeCommandTest.kt | 26 +- .../com/bazel_diff/log/Slf4jBindingTest.kt | 63 ---- .../bazel_diff/server/BazelDiffServerTest.kt | 2 +- .../com/bazel_diff/server/JGitClientTest.kt | 270 ------------------ .../bazel_diff/server/MetricsServiceTest.kt | 4 +- maven_install.json | 113 +------- tools/readme_template.md | 18 +- tools/serve_harness.py | 105 +++---- 14 files changed, 71 insertions(+), 801 deletions(-) delete mode 100644 cli/src/main/kotlin/com/bazel_diff/server/JGitClient.kt delete mode 100644 cli/src/test/kotlin/com/bazel_diff/log/Slf4jBindingTest.kt delete mode 100644 cli/src/test/kotlin/com/bazel_diff/server/JGitClientTest.kt diff --git a/.github/workflows/serve-harness.yml b/.github/workflows/serve-harness.yml index bc3125bc..fa8afd57 100644 --- a/.github/workflows/serve-harness.yml +++ b/.github/workflows/serve-harness.yml @@ -2,8 +2,8 @@ name: Serve Harness # Integration validation for the `bazel-diff serve` query service. Runs tools/serve_harness.py, # which builds //cli:bazel-diff and drives the real binary over HTTP against a live git:// remote -# (git daemon) across both git engines x full/shallow/partial clones -- coverage the in-process -# E2ETest cannot reach. See the harness module docstring for the scenario matrix. +# (git daemon) across full/shallow/partial clones -- coverage the in-process E2ETest cannot reach. +# See the harness module docstring for the scenario matrix. # Cron-only for now while we confirm the harness is stable on CI runners. Once it has a healthy # track record, add `pull_request` / `push` triggers (and/or make it a required status check) to diff --git a/MODULE.bazel b/MODULE.bazel index 78b9abdf..6ca9c7d2 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -50,9 +50,6 @@ maven.install( artifacts = [ "com.google.code.gson:gson:2.9.0@jar", "com.google.guava:guava:31.1-jre", - # JGit powers the `serve` command's in-process git engine. Pinned to the 5.13.x line, the - # last release series that still targets Java 8 (matching bazel-diff's documented minimum). - "org.eclipse.jgit:org.eclipse.jgit:5.13.3.202401111512-r", "com.willowtreeapps.assertk:assertk-jvm:0.25", "info.picocli:picocli:4.6.3@jar", "io.insert-koin:koin-core-jvm:3.1.6", @@ -61,12 +58,6 @@ maven.install( "org.apache.commons:commons-pool2:2.11.1", "org.jetbrains.kotlinx:kotlinx-coroutines-core:1.5.2", "org.mockito.kotlin:mockito-kotlin:5.4.0", - # SLF4J binding for JGit (serve command). JGit depends on slf4j-api but ships no binding, so - # without one SLF4J prints a "Failed to load class StaticLoggerBinder / Defaulting to NOP" - # warning to stderr on first use. slf4j-nop supplies the binding (silencing the warning) and - # discards JGit's internal logs, keeping the CLI's stderr clean. Version tracks the slf4j-api - # that JGit 5.13.x resolves (1.7.30). - "org.slf4j:slf4j-nop:1.7.30", ], fail_if_repin_required = True, lock_file = "//:maven_install.json", diff --git a/README.md b/README.md index f67e88f3..d305ea04 100644 --- a/README.md +++ b/README.md @@ -186,7 +186,7 @@ curl 'http://localhost:8080/metrics' "version": "31.4.0", "uptimeSeconds": 3600, "ready": true, - "gitEngine": "jgit", + "gitEngine": "subprocess", "trackDeps": false, "cache": {"directory": "/var/cache/bazel-diff", "entries": 128, "sizeBytes": 4823913, "sizeHuman": "4.6 MB"}, "jvm": {"usedBytes": 123456789, "maxBytes": 2147483648} @@ -204,17 +204,11 @@ Notes and current limitations: * The service checks out revisions inside `--workspacePath`, so point it at a dedicated clone, not a working tree you edit. All workspace-mutating work (git checkout + `bazel query`) is serialized, so a single instance answers one cold query at a time; the per-SHA cache absorbs the rest. -* Git operations run in-process via JGit by default (no `git` binary required). Pass - `--gitEngine=subprocess` to shell out to the `git` binary at `--gitPath` instead -- useful for - workspaces that depend on checkout filters or hooks that JGit does not run. Note that JGit only - moves the git plumbing in-process; the working tree is still materialized on disk for `bazel query`. - JGit cannot fetch some clone shapes native git handles fine -- notably shallow (`--depth`) and - partial (`--filter=blob:none`) clones, whose thin packs are delta-compressed against objects the - clone does not have ("Missing delta base"). When an in-process JGit fetch fails, the service - automatically falls back to the native `git` binary (at `--gitPath`) for that fetch and logs a - warning; point `--gitEngine=subprocess` at such workspaces to skip the in-process attempt (a - partial clone in particular needs the subprocess engine to *serve* queries, since JGit cannot - lazily fetch the missing blobs a checkout needs). +* Git operations (fetch and checkout) shell out to the `git` binary at `--gitPath` (default `git` + on the `PATH`), so a `git` binary must be available on the host. The working tree is checked out + on disk for `bazel query` to read. Because native git performs every fetch, all clone shapes are + supported -- including shallow (`--depth`) and partial (`--filter=blob:none`) clones, whose thin + packs are delta-compressed against objects the clone does not have. * Hashes are cached on local disk via `--cacheDir` and survive restarts. Left unbounded the cache grows by one entry per distinct commit SHA queried, so a long-running server can bound it with any combination of `--cacheMaxAge` (expire entries not read or written within a window, e.g. `7d`), @@ -498,8 +492,7 @@ Usage: bazel-diff serve [-hkvV] [--[no-]excludeExternalTargets] [--cqueryExpression=] [--excludeTargetsQuery=] [--fineGrainedHashExternalReposFile=] [--gitEngine=] - [--gitPath=] [--port=] + rnalReposFile>] [--gitPath=] [--port=] [--requestTimeout=] [-s=] -w= [-co=]... @@ -562,13 +555,8 @@ targets between two git revisions, caching generated hashes per commit SHA. A text file with a newline separated list of external repos. Mutually exclusive with --fineGrainedHashExternalRepos. - --gitEngine= - Git backend: 'jgit' (in-process, no git binary - required) or 'subprocess' (shells out to - --gitPath). Defaults to 'jgit'. - --gitPath= Path to the git binary, used only when - --gitEngine=subprocess. Defaults to 'git' on the - PATH. + --gitPath= Path to the git binary used for fetch/checkout + operations. Defaults to 'git' on the PATH. -h, --help Show this help message and exit. --ignoredRuleHashingAttributes= Attributes that should be ignored when hashing rule diff --git a/cli/BUILD b/cli/BUILD index f762976b..26f8845b 100644 --- a/cli/BUILD +++ b/cli/BUILD @@ -30,12 +30,6 @@ kt_jvm_library( name = "cli-lib", srcs = glob(["src/main/kotlin/**/*.kt"]), resources = [":version_file"], - # SLF4J binding for JGit. Not referenced by bazel-diff code (JGit uses it internally), so it is a - # pure runtime dep: it supplies org.slf4j.impl.StaticLoggerBinder, silencing the "Defaulting to - # NOP logger" warning SLF4J prints to stderr when no binding is on the classpath. - runtime_deps = [ - "@bazel_diff_maven//:org_slf4j_slf4j_nop", - ], deps = [ "//proto:analysis_v2_java_proto", "//proto:build_java_proto", @@ -44,7 +38,6 @@ kt_jvm_library( "@bazel_diff_maven//:info_picocli_picocli", "@bazel_diff_maven//:io_insert_koin_koin_core_jvm", "@bazel_diff_maven//:org_apache_commons_commons_pool2", - "@bazel_diff_maven//:org_eclipse_jgit_org_eclipse_jgit", "@bazel_diff_maven//:org_jetbrains_kotlinx_kotlinx_coroutines_core_jvm", ], ) @@ -193,12 +186,6 @@ kt_jvm_test( runtime_deps = [":cli-test-lib"], ) -kt_jvm_test( - name = "Slf4jBindingTest", - test_class = "com.bazel_diff.log.Slf4jBindingTest", - runtime_deps = [":cli-test-lib"], -) - kt_jvm_test( name = "BazelModServiceTest", test_class = "com.bazel_diff.bazel.BazelModServiceTest", @@ -273,12 +260,6 @@ kt_jvm_test( runtime_deps = [":cli-test-lib"], ) -kt_jvm_test( - name = "JGitClientTest", - test_class = "com.bazel_diff.server.JGitClientTest", - runtime_deps = [":cli-test-lib"], -) - kt_jvm_test( name = "HashServiceTest", jvm_flags = [ @@ -315,8 +296,6 @@ kt_jvm_library( "@bazel_diff_maven//:io_insert_koin_koin_test_jvm", "@bazel_diff_maven//:junit_junit", "@bazel_diff_maven//:org_mockito_kotlin_mockito_kotlin", - # Compile-time API for Slf4jBindingTest; the runtime binding (slf4j-nop) comes via cli-lib. - "@bazel_diff_maven//:org_slf4j_slf4j_api", ], ) diff --git a/cli/src/main/kotlin/com/bazel_diff/cli/ServeCommand.kt b/cli/src/main/kotlin/com/bazel_diff/cli/ServeCommand.kt index 08f91a5c..ab725213 100644 --- a/cli/src/main/kotlin/com/bazel_diff/cli/ServeCommand.kt +++ b/cli/src/main/kotlin/com/bazel_diff/cli/ServeCommand.kt @@ -18,7 +18,6 @@ import com.bazel_diff.server.HashCacheStorage import com.bazel_diff.server.HashProvider import com.bazel_diff.server.HashService import com.bazel_diff.server.ImpactedTargetsService -import com.bazel_diff.server.JGitClient import com.bazel_diff.server.LocalDiskHashCacheStorage import com.bazel_diff.server.MetricsService import com.bazel_diff.server.ProcessGitClient @@ -56,8 +55,6 @@ import picocli.CommandLine class ServeCommand : Callable { @CommandLine.ParentCommand private lateinit var parent: BazelDiff - @CommandLine.Spec lateinit var spec: CommandLine.Model.CommandSpec - @CommandLine.Option( names = ["-w", "--workspacePath"], description = ["Path to the Bazel workspace git clone the service checks out and queries."], @@ -77,18 +74,10 @@ class ServeCommand : Callable { names = ["--gitPath"], description = [ - "Path to the git binary, used only when --gitEngine=subprocess. Defaults to 'git' on the PATH."], + "Path to the git binary used for fetch/checkout operations. Defaults to 'git' on the PATH."], defaultValue = "git") var gitPath: String = "git" - @CommandLine.Option( - names = ["--gitEngine"], - description = - [ - "Git backend: 'jgit' (in-process, no git binary required) or 'subprocess' (shells out to --gitPath). Defaults to 'jgit'."], - defaultValue = "jgit") - var gitEngine: String = "jgit" - @CommandLine.Option( names = ["--port"], description = ["Port to listen on. Defaults to 8080."], @@ -304,19 +293,10 @@ class ServeCommand : Callable { } /** - * Builds the [GitClient] for the configured [gitEngine]. Defaults to the in-process JGit engine; - * `subprocess` falls back to shelling out to [gitPath] (matching C-git behavior exactly, useful - * for workspaces relying on checkout filters/hooks JGit does not run). + * Builds the [GitClient]. Git fetch/checkout operations shell out to the `git` binary at + * [gitPath], so a `git` binary must be available on the host. */ - fun createGitClient(): GitClient = - when (gitEngine.lowercase()) { - "jgit" -> JGitClient(workspacePath, gitPath) - "subprocess" -> ProcessGitClient(workspacePath, gitPath) - else -> - throw CommandLine.ParameterException( - spec.commandLine(), - "Unknown --gitEngine '$gitEngine' (expected 'jgit' or 'subprocess')") - } + fun createGitClient(): GitClient = ProcessGitClient(workspacePath, gitPath) /** * Wires the services, starts the HTTP server, and performs the initial git fetch + readiness @@ -341,7 +321,7 @@ class ServeCommand : Callable { MetricsService( version = VersionProvider().version.firstOrNull() ?: "unknown", startedAtMillis = System.currentTimeMillis(), - gitEngine = gitEngine, + gitEngine = "subprocess", trackDeps = trackDeps, cacheDir = cacheDir.toString(), storage = storage, diff --git a/cli/src/main/kotlin/com/bazel_diff/server/JGitClient.kt b/cli/src/main/kotlin/com/bazel_diff/server/JGitClient.kt deleted file mode 100644 index 3ec6ee61..00000000 --- a/cli/src/main/kotlin/com/bazel_diff/server/JGitClient.kt +++ /dev/null @@ -1,177 +0,0 @@ -package com.bazel_diff.server - -import com.bazel_diff.log.Logger -import java.io.File -import java.nio.file.Path -import org.eclipse.jgit.api.Git -import org.eclipse.jgit.errors.IncorrectObjectTypeException -import org.eclipse.jgit.errors.MissingObjectException -import org.eclipse.jgit.lib.Repository -import org.eclipse.jgit.revwalk.RevWalk -import org.eclipse.jgit.storage.file.FileRepositoryBuilder -import org.eclipse.jgit.transport.RemoteConfig -import org.koin.core.component.KoinComponent -import org.koin.core.component.inject - -/** - * [GitClient] backed by JGit (pure Java), so the service performs git operations in-process without - * forking a `git` subprocess and without requiring a `git` binary on the PATH. - * - * Note on "in memory": JGit removes the subprocess, but the working tree is still materialized on - * disk — `bazel query` reads real files, so each revision must exist as files. Only the git - * plumbing runs in-process, not the checkout itself. - * - * A fresh [Git]/[org.eclipse.jgit.lib.Repository] handle is opened per call and closed promptly; - * these are cheap relative to the `bazel query` that follows, and it keeps the client stateless and - * thread-safe (the workspace itself is still serialized by [HashService]'s lock). - */ -class JGitClient( - private val workspacePath: Path, - private val gitPath: String = "git", - private val nativeFetchFallback: Boolean = true, -) : GitClient, KoinComponent { - private val logger: Logger by inject() - - // Built lazily and only when a JGit fetch actually fails. Native git negotiates clone shapes - // JGit 5.13 cannot fetch (shallow, partial/promisor, thin packs whose delta base is absent), - // scoped to the same workspace and configured git binary. - private val nativeGit: ProcessGitClient by lazy { ProcessGitClient(workspacePath, gitPath) } - - @Volatile private var warnedCloneShape = false - - private fun open(): Git { - val repository = - FileRepositoryBuilder() - .setWorkTree(workspacePath.toFile()) - .readEnvironment() - .findGitDir(workspacePath.toFile()) - .build() - return Git(repository) - } - - override fun fetch() { - open().use { git -> - warnOnUnsupportedCloneShapeOnce(git.repository) - val remotes = RemoteConfig.getAllRemoteConfigs(git.repository.config) - if (remotes.isEmpty()) { - logger.i { "No git remotes configured; skipping fetch" } - return - } - for (remote in remotes) { - logger.i { "JGit fetch ${remote.name} in $workspacePath" } - try { - git.fetch().setRemote(remote.name).setRemoveDeletedRefs(true).call() - } catch (e: Exception) { - // JGit cannot fetch some clone shapes it otherwise reads fine -- notably shallow and - // partial (blob:none) clones, where the remote's thin pack is delta-compressed against a - // base object absent from this clone ("Missing delta base "). Native git negotiates - // these correctly, so fall back to it rather than lame-ducking the whole service on a - // fetch the machine is perfectly capable of. - fetchViaNativeGitOrThrow(remote.name, e) - return // native `git fetch --all` already covered every remote; don't re-attempt them. - } - } - } - } - - override fun fetchRevision(revision: String): Boolean { - // Fetching an object by SHA is a negotiation JGit 5.13 does not reliably perform (it resolves - // the ref spec against the remote's advertised refs, and a PR-head or force-pushed SHA is not - // advertised). Route it through native git -- the same reason fetch() falls back -- which - // supports `git fetch ` against servers that allow reachable-SHA fetches. - // With the fallback disabled the in-process engine cannot supply it, so report that honestly. - if (!nativeFetchFallback) { - logger.i { - "JGit cannot fetch revision '$revision' by SHA and the native fallback is disabled" - } - return false - } - return nativeGit.fetchRevision(revision) - } - - /** - * Retries a failed JGit fetch with the native `git` binary (unless [nativeFetchFallback] is off, - * in which case the original JGit failure is surfaced). If native git also fails the two failures - * are combined so neither cause is hidden. - */ - private fun fetchViaNativeGitOrThrow(remoteName: String, jgitError: Exception) { - if (!nativeFetchFallback) { - throw GitClientException("JGit fetch $remoteName failed: ${jgitError.message}", jgitError) - } - logger.w { - "JGit fetch $remoteName failed (${jgitError.message}); retrying with native git ('$gitPath'). " + - "This usually means the workspace is a shallow or partial clone, which JGit cannot fetch; " + - "pass --gitEngine=subprocess to skip the in-process attempt entirely." - } - try { - nativeGit.fetch() - } catch (nativeError: Exception) { - throw GitClientException( - "JGit fetch $remoteName failed (${jgitError.message}) and the native git fallback " + - "('$gitPath') also failed: ${nativeError.message}", - jgitError) - .apply { addSuppressed(nativeError) } - } - } - - /** - * Logs a one-time warning if the workspace is a shallow or partial (promisor) clone -- shapes - * JGit 5.13 cannot fetch, so every fetch will take the native-git fallback path. Surfacing it up - * front (rather than only on the first failed fetch) makes the misconfiguration obvious. - */ - private fun warnOnUnsupportedCloneShapeOnce(repo: Repository) { - if (warnedCloneShape) return - warnedCloneShape = true - val gitDir = repo.directory ?: return - val shallow = File(gitDir, "shallow").exists() - val partial = - repo.config.getString("extensions", null, "partialClone") != null || - repo.config.getSubsections("remote").any { - repo.config.getBoolean("remote", it, "promisor", false) - } - if (shallow || partial) { - val kinds = listOfNotNull("shallow".takeIf { shallow }, "partial".takeIf { partial }) - logger.w { - "workspace clone is ${kinds.joinToString("+")}; JGit cannot reliably fetch these " + - "(thin-pack delta bases may be absent). Fetches will fall back to native git " + - "('$gitPath'); consider --gitEngine=subprocess for this workspace." - } - } - } - - override fun resolveSha(revision: String): String { - open().use { git -> - val objectId = git.repository.resolve(revision) ?: throw MissingRevisionException(revision) - // Repository.resolve() turns a full 40-char SHA into an ObjectId by parsing the - // hex WITHOUT consulting the object database, so a commit absent from this clone - // still "resolves". Parse it through a RevWalk to confirm the object is present - // (peeling annotated tags to their commit), so an absent commit becomes a - // retryable MissingRevisionException here instead of an opaque "Missing unknown - // " failure deferred to the later checkout. - val commit = - try { - RevWalk(git.repository).use { walk -> walk.parseCommit(objectId) } - } catch (e: MissingObjectException) { - throw MissingRevisionException(revision, e) - } catch (e: IncorrectObjectTypeException) { - throw GitClientException("revision '$revision' does not refer to a commit", e) - } - return commit.name - } - } - - override fun checkout(revision: String) { - open().use { git -> - logger.i { "JGit checkout $revision in $workspacePath" } - try { - // Checkout by commit name leaves HEAD detached, so branch refs stay intact and a later - // resolveSha("main") still resolves the real branch tip. setForced(true) discards any - // conflicting working-tree state (the service only ever reads the tree, but bazel-created - // artifacts are untracked and unaffected). - git.checkout().setName(revision).setForced(true).call() - } catch (e: Exception) { - throw GitClientException("JGit checkout $revision failed: ${e.message}", e) - } - } - } -} diff --git a/cli/src/test/kotlin/com/bazel_diff/cli/ServeCommandTest.kt b/cli/src/test/kotlin/com/bazel_diff/cli/ServeCommandTest.kt index 1cafda4e..721bcc56 100644 --- a/cli/src/test/kotlin/com/bazel_diff/cli/ServeCommandTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/cli/ServeCommandTest.kt @@ -13,7 +13,6 @@ import com.bazel_diff.log.Logger import com.bazel_diff.server.GitClient import com.bazel_diff.server.GitClientException import com.bazel_diff.server.HashCacheStorage -import com.bazel_diff.server.JGitClient import com.bazel_diff.server.LocalDiskHashCacheStorage import com.bazel_diff.server.ProcessGitClient import com.bazel_diff.server.ServerMetrics @@ -178,34 +177,11 @@ class ServeCommandTest : KoinTest { } @Test - fun createGitClientDefaultsToJGit() { + fun createGitClientUsesSubprocessEngine() { val cmd = command().apply { workspacePath = temp.newFolder().toPath() } - assertThat(cmd.createGitClient()).isInstanceOf(JGitClient::class) - } - - @Test - fun createGitClientHonorsSubprocessEngine() { - val cmd = - command().apply { - workspacePath = temp.newFolder().toPath() - gitEngine = "subprocess" - } assertThat(cmd.createGitClient()).isInstanceOf(ProcessGitClient::class) } - @Test - fun createGitClientRejectsUnknownEngine() { - val cmd = ServeCommand() - // Wrapping in a CommandLine populates the @Spec field the ParameterException references. - picocli.CommandLine(cmd) - cmd.workspacePath = temp.newFolder().toPath() - cmd.cacheDir = temp.newFolder().toPath() - cmd.gitEngine = "bogus" - org.junit.Assert.assertThrows(picocli.CommandLine.ParameterException::class.java) { - cmd.createGitClient() - } - } - @Test fun buildAndStartServerBecomesHealthyAfterFetch() { val cmd = command() diff --git a/cli/src/test/kotlin/com/bazel_diff/log/Slf4jBindingTest.kt b/cli/src/test/kotlin/com/bazel_diff/log/Slf4jBindingTest.kt deleted file mode 100644 index f7556f14..00000000 --- a/cli/src/test/kotlin/com/bazel_diff/log/Slf4jBindingTest.kt +++ /dev/null @@ -1,63 +0,0 @@ -package com.bazel_diff.log - -import assertk.assertThat -import assertk.assertions.doesNotContain -import assertk.assertions.isNotNull -import java.io.ByteArrayOutputStream -import java.io.PrintStream -import org.junit.After -import org.junit.Before -import org.junit.Test -import org.slf4j.LoggerFactory - -/** - * Regression test for the SLF4J warning printed by `bazel-diff serve`: - * ``` - * SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder". - * SLF4J: Defaulting to no-operation (NOP) logger implementation - * ``` - * - * JGit (the serve command's in-process git engine) depends on `slf4j-api` but ships no binding. - * With no `org.slf4j.impl.StaticLoggerBinder` on the classpath, SLF4J falls back to a NOP logger - * and prints the warning above to stderr on first use. Shipping `slf4j-nop` as a runtime dep - * supplies the binding, silencing the warning while discarding JGit's internal logs (keeping the - * CLI stderr clean). - */ -class Slf4jBindingTest { - private val originalErr = System.err - private lateinit var captured: ByteArrayOutputStream - - @Before - fun setUp() { - captured = ByteArrayOutputStream() - System.setErr(PrintStream(captured)) - } - - @After - fun tearDown() { - System.setErr(originalErr) - } - - @Test - fun staticLoggerBinderIsOnTheClasspath() { - // This is the exact class SLF4J fails to load when no binding is present ("Failed to load class - // org.slf4j.impl.StaticLoggerBinder"). If it resolves, the NOP-fallback warning cannot be - // printed — a deterministic guard that survives SLF4J's once-per-JVM static init ordering. - val binder = Class.forName("org.slf4j.impl.StaticLoggerBinder") - assertThat(binder).isNotNull() - } - - @Test - fun obtainingAndUsingALoggerDoesNotPrintNopFallbackWarning() { - // Exercises the same path JGit takes: resolve a logger and emit a record. With a binding - // present - // this is silent; without one, SLF4J's static init prints the warning to stderr. - val logger = LoggerFactory.getLogger(Slf4jBindingTest::class.java) - assertThat(logger).isNotNull() - logger.info("bazel-diff slf4j binding smoke test") - - val err = captured.toString() - assertThat(err).doesNotContain("StaticLoggerBinder") - assertThat(err).doesNotContain("NOP logger") - } -} diff --git a/cli/src/test/kotlin/com/bazel_diff/server/BazelDiffServerTest.kt b/cli/src/test/kotlin/com/bazel_diff/server/BazelDiffServerTest.kt index 6582f2ab..d9d8e74d 100644 --- a/cli/src/test/kotlin/com/bazel_diff/server/BazelDiffServerTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/server/BazelDiffServerTest.kt @@ -46,7 +46,7 @@ class BazelDiffServerTest : KoinTest { version = "9.9.9", uptimeSeconds = 42, ready = true, - gitEngine = "jgit", + gitEngine = "subprocess", trackDeps = false, cache = CacheMetrics("/cache", 2, 2048, "2.0 KB"), jvm = JvmMetrics(1000, 2000), diff --git a/cli/src/test/kotlin/com/bazel_diff/server/JGitClientTest.kt b/cli/src/test/kotlin/com/bazel_diff/server/JGitClientTest.kt deleted file mode 100644 index 20a3f7e1..00000000 --- a/cli/src/test/kotlin/com/bazel_diff/server/JGitClientTest.kt +++ /dev/null @@ -1,270 +0,0 @@ -package com.bazel_diff.server - -import assertk.assertThat -import assertk.assertions.hasLength -import assertk.assertions.isEqualTo -import assertk.assertions.isFalse -import assertk.assertions.isNotEqualTo -import assertk.assertions.isTrue -import assertk.assertions.messageContains -import com.bazel_diff.SilentLogger -import com.bazel_diff.log.Logger -import java.io.File -import org.junit.Assert.assertThrows -import org.junit.Rule -import org.junit.Test -import org.junit.rules.TemporaryFolder -import org.koin.dsl.module -import org.koin.test.KoinTest -import org.koin.test.KoinTestRule - -/** - * Exercises the in-process [JGitClient] against a real throwaway git repository created with the - * `git` CLI (verifying JGit interoperates with a standard repo). - */ -class JGitClientTest : KoinTest { - @get:Rule - val koinTestRule = KoinTestRule.create { modules(module { single { SilentLogger } }) } - - @get:Rule val temp: TemporaryFolder = TemporaryFolder() - - /** Runs git for test setup, returning trimmed stdout; fails the test on a non-zero exit. */ - private fun git(vararg args: String): String = runGit(temp.root, *args) - - /** Like [git] but in an explicit working directory (for multi-repo fetch tests). */ - private fun runGit(dir: File, vararg args: String): String { - val proc = ProcessBuilder(listOf("git") + args).directory(dir).redirectErrorStream(true).start() - val output = proc.inputStream.readBytes().decodeToString() - val code = proc.waitFor() - check(code == 0) { "git ${args.joinToString(" ")} failed ($code): $output" } - return output.trim() - } - - private fun client() = JGitClient(temp.root.toPath()) - - private fun initRepoWithTwoCommits(): Pair { - git("init", "-q") - git("config", "user.email", "test@example.com") - git("config", "user.name", "test") - File(temp.root, "file.txt").writeText("one") - git("add", ".") - git("commit", "-q", "-m", "first") - val sha1 = client().resolveSha("HEAD") - File(temp.root, "file.txt").writeText("two") - git("add", ".") - git("commit", "-q", "-m", "second") - val sha2 = client().resolveSha("HEAD") - return sha1 to sha2 - } - - @Test - fun resolveShaReturnsFullSha() { - val (sha1, sha2) = initRepoWithTwoCommits() - assertThat(sha1).hasLength(40) - assertThat(sha2).hasLength(40) - assertThat(sha1).isNotEqualTo(sha2) - } - - @Test - fun checkoutSwitchesWorkingTree() { - val (sha1, sha2) = initRepoWithTwoCommits() - val client = client() - val file = File(temp.root, "file.txt") - - client.checkout(sha1) - assertThat(file.readText()).isEqualTo("one") - client.checkout(sha2) - assertThat(file.readText()).isEqualTo("two") - } - - @Test - fun checkoutLeavesBranchRefsIntact() { - val (sha1, _) = initRepoWithTwoCommits() - val client = client() - // Checking out a commit detaches HEAD; the branch tip must still resolve afterwards. - client.checkout(sha1) - val branchTip = client.resolveSha("HEAD") - assertThat(branchTip).isEqualTo(sha1) - } - - @Test - fun resolveShaThrowsOnUnknownRevision() { - initRepoWithTwoCommits() - assertThrows(GitClientException::class.java) { client().resolveSha("no-such-ref") } - } - - @Test - fun fetchDoesNotThrowWithoutRemotes() { - initRepoWithTwoCommits() - // No remotes configured -> fetch is a no-op, must not throw. - client().fetch() - } - - @Test - fun fetchWithRemoteExercisesTheRemoteLoop() { - initRepoWithTwoCommits() - // A self-pointing remote lets fetch() run the actual fetch-per-remote path (not just the - // no-remote skip) without standing up a second server. - git("remote", "add", "origin", temp.root.absolutePath) - client().fetch() - } - - @Test - fun checkoutThrowsOnUnknownRevision() { - initRepoWithTwoCommits() - assertThrows(GitClientException::class.java) { client().checkout("does-not-exist") } - } - - @Test - fun resolveShaThrowsMissingRevisionForAbsentFullSha() { - initRepoWithTwoCommits() - // A well-formed 40-char SHA that is not in the object database. Repository.resolve() parses the - // hex without a DB lookup, so resolveSha must detect the absence itself and flag it retryable. - val absent = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" - assertThrows(MissingRevisionException::class.java) { client().resolveSha(absent) } - } - - @Test - fun resolveShaThrowsWhenRevisionIsNotACommit() { - initRepoWithTwoCommits() - // A tree object is present but is not a commit; resolveSha must reject it as a hard error, not - // as a retryable MissingRevisionException (a refetch would never make a tree checkoutable). - val treeSha = git("rev-parse", "HEAD^{tree}") - val ex = assertThrows(GitClientException::class.java) { client().resolveSha(treeSha) } - assertThat(ex).messageContains("does not refer to a commit") - } - - @Test - fun resolveShaSeesCommitOnlyAfterFetch() { - // Reproduces the reported serve failure: a commit that lands on the remote AFTER the clone is - // absent locally, so resolveSha rejects it (rather than deferring an opaque checkout error) - // until an on-demand fetch brings it in. - val origin = File(temp.root, "origin").apply { mkdirs() } - runGit(origin, "init", "-q") - runGit(origin, "config", "user.email", "test@example.com") - runGit(origin, "config", "user.name", "test") - File(origin, "file.txt").writeText("one") - runGit(origin, "add", ".") - runGit(origin, "commit", "-q", "-m", "first") - - val workspace = File(temp.root, "workspace") - runGit(temp.root, "clone", "-q", origin.absolutePath, workspace.absolutePath) - - // A new commit lands in origin after the clone; the workspace has not fetched it yet. - File(origin, "file.txt").writeText("two") - runGit(origin, "add", ".") - runGit(origin, "commit", "-q", "-m", "second") - val sha2 = runGit(origin, "rev-parse", "HEAD") - - val client = JGitClient(workspace.toPath()) - assertThrows(MissingRevisionException::class.java) { client.resolveSha(sha2) } - client.fetch() - assertThat(client.resolveSha(sha2)).isEqualTo(sha2) - } - - @Test - fun fetchWithoutFallbackSurfacesJGitFailure() { - initRepoWithTwoCommits() - // origin points at a directory that is not a git repository, so the in-process JGit fetch - // fails. With the native fallback disabled the failure surfaces directly (legacy behaviour). - val bogus = File(temp.root, "not-a-repo").apply { mkdirs() } - git("remote", "add", "origin", bogus.absolutePath) - val client = JGitClient(temp.root.toPath(), gitPath = "git", nativeFetchFallback = false) - assertThrows(GitClientException::class.java) { client.fetch() } - } - - @Test - fun fetchFallbackErrorNamesNativeGitWhenBothFail() { - initRepoWithTwoCommits() - // JGit fetch fails (origin is not a repo) AND the native fallback fails (bogus git binary). - // The surfaced error must name the native fallback so neither cause is hidden. - val bogus = File(temp.root, "not-a-repo").apply { mkdirs() } - git("remote", "add", "origin", bogus.absolutePath) - val client = - JGitClient(temp.root.toPath(), gitPath = "/nonexistent/git", nativeFetchFallback = true) - val ex = assertThrows(GitClientException::class.java) { client.fetch() } - assertThat(ex).messageContains("native git fallback") - } - - @Test - fun fetchRevisionBringsInACommitNoBranchReachesViaNativeGit() { - // JGit routes by-SHA fetch through native git; a PR-head commit (reachable from no branch) is - // absent after a broad fetch and arrives only via the targeted fetchRevision. file:// forces a - // real pack transfer so the object is genuinely missing until then. - val origin = File(temp.root, "origin").apply { mkdirs() } - runGit(origin, "init", "-q") - runGit(origin, "config", "user.email", "test@example.com") - runGit(origin, "config", "user.name", "test") - runGit(origin, "config", "uploadpack.allowReachableSHA1InWant", "true") - File(origin, "file.txt").writeText("one") - runGit(origin, "add", ".") - runGit(origin, "commit", "-q", "-m", "first") - val branch = runGit(origin, "rev-parse", "--abbrev-ref", "HEAD") - - // A commit reachable only from refs/pull/7/head, not from any branch. - runGit(origin, "checkout", "-q", "--detach") - File(origin, "pr.txt").writeText("pr") - runGit(origin, "add", ".") - runGit(origin, "commit", "-q", "-m", "pr head") - val prHead = runGit(origin, "rev-parse", "HEAD") - runGit(origin, "update-ref", "refs/pull/7/head", prHead) - // Re-attach HEAD to the branch so the clone checks out the branch, not the detached PR commit. - runGit(origin, "checkout", "-q", branch) - - val workspace = File(temp.root, "workspace") - runGit(temp.root, "clone", "-q", "file://${origin.absolutePath}", workspace.absolutePath) - val client = JGitClient(workspace.toPath()) - - client.fetch() - assertThrows(MissingRevisionException::class.java) { client.resolveSha(prHead) } - assertThat(client.fetchRevision(prHead)).isTrue() - assertThat(client.resolveSha(prHead)).isEqualTo(prHead) - } - - @Test - fun fetchRevisionReturnsFalseWhenNativeFallbackDisabled() { - initRepoWithTwoCommits() - // With the native fallback off, the in-process engine will not attempt an unreliable JGit - // by-SHA fetch; it reports failure so the caller surfaces the revision as missing. - val client = JGitClient(temp.root.toPath(), gitPath = "git", nativeFetchFallback = false) - assertThat(client.fetchRevision("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")).isFalse() - } - - @Test - fun fetchBringsInNewCommitForShallowClone() { - // A shallow clone is a shape JGit 5.13 may be unable to fetch (the remote's thin pack can be - // delta-compressed against a base object below the shallow boundary). The client must fall - // back to native git so a just-landed commit still arrives; whichever path succeeds, the end - // state must be the new commit present and resolvable. - val origin = File(temp.root, "origin").apply { mkdirs() } - runGit(origin, "init", "-q") - runGit(origin, "config", "user.email", "test@example.com") - runGit(origin, "config", "user.name", "test") - File(origin, "file.txt").writeText("one") - runGit(origin, "add", ".") - runGit(origin, "commit", "-q", "-m", "first") - File(origin, "file.txt").writeText("two") - runGit(origin, "add", ".") - runGit(origin, "commit", "-q", "-m", "second") - - val workspace = File(temp.root, "workspace") - runGit( - temp.root, - "clone", - "-q", - "--depth=1", - "file://${origin.absolutePath}", - workspace.absolutePath) - - // A new commit lands after the shallow clone; the workspace has not fetched it yet. - File(origin, "file.txt").writeText("three") - runGit(origin, "add", ".") - runGit(origin, "commit", "-q", "-m", "third") - val sha3 = runGit(origin, "rev-parse", "HEAD") - - val client = JGitClient(workspace.toPath()) - assertThrows(MissingRevisionException::class.java) { client.resolveSha(sha3) } - client.fetch() - assertThat(client.resolveSha(sha3)).isEqualTo(sha3) - } -} diff --git a/cli/src/test/kotlin/com/bazel_diff/server/MetricsServiceTest.kt b/cli/src/test/kotlin/com/bazel_diff/server/MetricsServiceTest.kt index fed84594..ca6a216c 100644 --- a/cli/src/test/kotlin/com/bazel_diff/server/MetricsServiceTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/server/MetricsServiceTest.kt @@ -31,7 +31,7 @@ class MetricsServiceTest { MetricsService( version = "1.2.3", startedAtMillis = startedAtMillis, - gitEngine = "jgit", + gitEngine = "subprocess", trackDeps = true, cacheDir = "/var/cache/bazel-diff", storage = storage, @@ -47,7 +47,7 @@ class MetricsServiceTest { assertThat(snap.version).isEqualTo("1.2.3") assertThat(snap.uptimeSeconds).isEqualTo(6) // (10000 - 4000) / 1000 assertThat(snap.ready).isTrue() - assertThat(snap.gitEngine).isEqualTo("jgit") + assertThat(snap.gitEngine).isEqualTo("subprocess") assertThat(snap.trackDeps).isTrue() assertThat(snap.cache.directory).isEqualTo("/var/cache/bazel-diff") assertThat(snap.cache.entries).isEqualTo(3L) diff --git a/maven_install.json b/maven_install.json index aab0bd2f..30da09a9 100644 --- a/maven_install.json +++ b/maven_install.json @@ -9,10 +9,8 @@ "io.insert-koin:koin-test-junit4": 1815996592, "junit:junit": -744267592, "org.apache.commons:commons-pool2": -501602100, - "org.eclipse.jgit:org.eclipse.jgit": -1935467463, "org.jetbrains.kotlinx:kotlinx-coroutines-core": -542524036, "org.mockito.kotlin:mockito-kotlin": 1836434344, - "org.slf4j:slf4j-nop": 1480452039, "repositories": -1949687017 }, "__RESOLVED_ARTIFACTS_HASH": { @@ -26,7 +24,6 @@ "com.google.guava:guava": 1410177884, "com.google.guava:listenablefuture": 1079558157, "com.google.j2objc:j2objc-annotations": 880287147, - "com.googlecode.javaewah:JavaEWAH": -1970338061, "com.willowtreeapps.assertk:assertk-jvm": -1267698986, "com.willowtreeapps.opentest4k:opentest4k-jvm": -1883118923, "info.picocli:picocli": -1901351180, @@ -38,7 +35,6 @@ "net.bytebuddy:byte-buddy-agent": 1903046347, "org.apache.commons:commons-pool2": -1200630144, "org.checkerframework:checker-qual": -1034954841, - "org.eclipse.jgit:org.eclipse.jgit": 607222344, "org.hamcrest:hamcrest-core": 649657847, "org.jetbrains.kotlin:kotlin-reflect": -1150430934, "org.jetbrains.kotlin:kotlin-stdlib": 1585233745, @@ -52,9 +48,7 @@ "org.mockito.kotlin:mockito-kotlin": 2123507963, "org.mockito:mockito-core": -960687581, "org.objenesis:objenesis": 1798216877, - "org.opentest4j:opentest4j": -1584531193, - "org.slf4j:slf4j-api": -801231047, - "org.slf4j:slf4j-nop": -2006403554 + "org.opentest4j:opentest4j": -1584531193 }, "conflict_resolution": { "io.insert-koin:koin-core-jvm:3.1.6": "io.insert-koin:koin-core-jvm:4.0.0" @@ -120,12 +114,6 @@ }, "version": "1.3" }, - "com.googlecode.javaewah:JavaEWAH": { - "shasums": { - "jar": "4c0fda2b1d317750d7ea324e36c70b2bc48310c0aaae67b98df0915d696d7111" - }, - "version": "1.1.13" - }, "com.willowtreeapps.assertk:assertk-jvm": { "shasums": { "jar": "7f62d1be9642b346a9b61505646a613852f0d1770026bc7754f51ed402fe3d55" @@ -192,12 +180,6 @@ }, "version": "3.12.0" }, - "org.eclipse.jgit:org.eclipse.jgit": { - "shasums": { - "jar": "dacb74c28b089bc378f8c2a1dcda5110c20f52124f5a020aef2684d70ef7f1bb" - }, - "version": "5.13.3.202401111512-r" - }, "org.hamcrest:hamcrest-core": { "shasums": { "jar": "66fdef91e9739348df7a096aa384a5685f4e875584cce89386a7a47251c4d8e9" @@ -281,18 +263,6 @@ "jar": "58812de60898d976fb81ef3b62da05c6604c18fd4a249f5044282479fc286af2" }, "version": "1.2.0" - }, - "org.slf4j:slf4j-api": { - "shasums": { - "jar": "cdba07964d1bb40a0761485c6b1e8c2f8fd9eb1d19c53928ac0d7f9510105c57" - }, - "version": "1.7.30" - }, - "org.slf4j:slf4j-nop": { - "shasums": { - "jar": "2d550dcefaea23d223b72027dbc7cbdb7327676ccefdd9cfe49cf9ea8e9ac8e0" - }, - "version": "1.7.30" } }, "dependencies": { @@ -344,10 +314,6 @@ "junit:junit": [ "org.hamcrest:hamcrest-core" ], - "org.eclipse.jgit:org.eclipse.jgit": [ - "com.googlecode.javaewah:JavaEWAH", - "org.slf4j:slf4j-api" - ], "org.jetbrains.kotlin:kotlin-reflect": [ "org.jetbrains.kotlin:kotlin-stdlib" ], @@ -379,9 +345,6 @@ "net.bytebuddy:byte-buddy", "net.bytebuddy:byte-buddy-agent", "org.objenesis:objenesis" - ], - "org.slf4j:slf4j-nop": [ - "org.slf4j:slf4j-api" ] }, "packages": { @@ -440,13 +403,6 @@ "com.google.j2objc:j2objc-annotations": [ "com.google.j2objc.annotations" ], - "com.googlecode.javaewah:JavaEWAH": [ - "com.googlecode.javaewah", - "com.googlecode.javaewah.datastructure", - "com.googlecode.javaewah.symmetric", - "com.googlecode.javaewah32", - "com.googlecode.javaewah32.symmetric" - ], "com.willowtreeapps.assertk:assertk-jvm": [ "assertk", "assertk.assertions", @@ -606,58 +562,6 @@ "org.checkerframework.dataflow.qual", "org.checkerframework.framework.qual" ], - "org.eclipse.jgit:org.eclipse.jgit": [ - "org.eclipse.jgit.annotations", - "org.eclipse.jgit.api", - "org.eclipse.jgit.api.errors", - "org.eclipse.jgit.attributes", - "org.eclipse.jgit.blame", - "org.eclipse.jgit.diff", - "org.eclipse.jgit.dircache", - "org.eclipse.jgit.errors", - "org.eclipse.jgit.events", - "org.eclipse.jgit.fnmatch", - "org.eclipse.jgit.gitrepo", - "org.eclipse.jgit.gitrepo.internal", - "org.eclipse.jgit.hooks", - "org.eclipse.jgit.ignore", - "org.eclipse.jgit.ignore.internal", - "org.eclipse.jgit.internal", - "org.eclipse.jgit.internal.fsck", - "org.eclipse.jgit.internal.revwalk", - "org.eclipse.jgit.internal.storage.dfs", - "org.eclipse.jgit.internal.storage.file", - "org.eclipse.jgit.internal.storage.io", - "org.eclipse.jgit.internal.storage.pack", - "org.eclipse.jgit.internal.storage.reftable", - "org.eclipse.jgit.internal.submodule", - "org.eclipse.jgit.internal.transport.connectivity", - "org.eclipse.jgit.internal.transport.http", - "org.eclipse.jgit.internal.transport.parser", - "org.eclipse.jgit.internal.transport.ssh", - "org.eclipse.jgit.lib", - "org.eclipse.jgit.lib.internal", - "org.eclipse.jgit.logging", - "org.eclipse.jgit.merge", - "org.eclipse.jgit.nls", - "org.eclipse.jgit.notes", - "org.eclipse.jgit.patch", - "org.eclipse.jgit.revplot", - "org.eclipse.jgit.revwalk", - "org.eclipse.jgit.revwalk.filter", - "org.eclipse.jgit.storage.file", - "org.eclipse.jgit.storage.pack", - "org.eclipse.jgit.submodule", - "org.eclipse.jgit.transport", - "org.eclipse.jgit.transport.http", - "org.eclipse.jgit.transport.resolver", - "org.eclipse.jgit.treewalk", - "org.eclipse.jgit.treewalk.filter", - "org.eclipse.jgit.util", - "org.eclipse.jgit.util.io", - "org.eclipse.jgit.util.sha1", - "org.eclipse.jgit.util.time" - ], "org.hamcrest:hamcrest-core": [ "org.hamcrest", "org.hamcrest.core", @@ -892,15 +796,6 @@ ], "org.opentest4j:opentest4j": [ "org.opentest4j" - ], - "org.slf4j:slf4j-api": [ - "org.slf4j", - "org.slf4j.event", - "org.slf4j.helpers", - "org.slf4j.spi" - ], - "org.slf4j:slf4j-nop": [ - "org.slf4j.impl" ] }, "repositories": { @@ -915,7 +810,6 @@ "com.google.guava:guava", "com.google.guava:listenablefuture", "com.google.j2objc:j2objc-annotations", - "com.googlecode.javaewah:JavaEWAH", "com.willowtreeapps.assertk:assertk-jvm", "com.willowtreeapps.opentest4k:opentest4k-jvm", "info.picocli:picocli", @@ -927,7 +821,6 @@ "net.bytebuddy:byte-buddy-agent", "org.apache.commons:commons-pool2", "org.checkerframework:checker-qual", - "org.eclipse.jgit:org.eclipse.jgit", "org.hamcrest:hamcrest-core", "org.jetbrains.kotlin:kotlin-reflect", "org.jetbrains.kotlin:kotlin-stdlib", @@ -941,9 +834,7 @@ "org.mockito.kotlin:mockito-kotlin", "org.mockito:mockito-core", "org.objenesis:objenesis", - "org.opentest4j:opentest4j", - "org.slf4j:slf4j-api", - "org.slf4j:slf4j-nop" + "org.opentest4j:opentest4j" ] }, "services": { diff --git a/tools/readme_template.md b/tools/readme_template.md index a5533bb9..ba86adcc 100644 --- a/tools/readme_template.md +++ b/tools/readme_template.md @@ -186,7 +186,7 @@ curl 'http://localhost:8080/metrics' "version": "31.4.0", "uptimeSeconds": 3600, "ready": true, - "gitEngine": "jgit", + "gitEngine": "subprocess", "trackDeps": false, "cache": {"directory": "/var/cache/bazel-diff", "entries": 128, "sizeBytes": 4823913, "sizeHuman": "4.6 MB"}, "jvm": {"usedBytes": 123456789, "maxBytes": 2147483648} @@ -204,17 +204,11 @@ Notes and current limitations: * The service checks out revisions inside `--workspacePath`, so point it at a dedicated clone, not a working tree you edit. All workspace-mutating work (git checkout + `bazel query`) is serialized, so a single instance answers one cold query at a time; the per-SHA cache absorbs the rest. -* Git operations run in-process via JGit by default (no `git` binary required). Pass - `--gitEngine=subprocess` to shell out to the `git` binary at `--gitPath` instead -- useful for - workspaces that depend on checkout filters or hooks that JGit does not run. Note that JGit only - moves the git plumbing in-process; the working tree is still materialized on disk for `bazel query`. - JGit cannot fetch some clone shapes native git handles fine -- notably shallow (`--depth`) and - partial (`--filter=blob:none`) clones, whose thin packs are delta-compressed against objects the - clone does not have ("Missing delta base"). When an in-process JGit fetch fails, the service - automatically falls back to the native `git` binary (at `--gitPath`) for that fetch and logs a - warning; point `--gitEngine=subprocess` at such workspaces to skip the in-process attempt (a - partial clone in particular needs the subprocess engine to *serve* queries, since JGit cannot - lazily fetch the missing blobs a checkout needs). +* Git operations (fetch and checkout) shell out to the `git` binary at `--gitPath` (default `git` + on the `PATH`), so a `git` binary must be available on the host. The working tree is checked out + on disk for `bazel query` to read. Because native git performs every fetch, all clone shapes are + supported -- including shallow (`--depth`) and partial (`--filter=blob:none`) clones, whose thin + packs are delta-compressed against objects the clone does not have. * Hashes are cached on local disk via `--cacheDir` and survive restarts. Left unbounded the cache grows by one entry per distinct commit SHA queried, so a long-running server can bound it with any combination of `--cacheMaxAge` (expire entries not read or written within a window, e.g. `7d`), diff --git a/tools/serve_harness.py b/tools/serve_harness.py index adeb0185..b446695a 100755 --- a/tools/serve_harness.py +++ b/tools/serve_harness.py @@ -6,20 +6,16 @@ `bazel-diff` binary as a subprocess against a *live git remote* served over `git://`, across the combinations that actually break in production: - * both git engines -- `--gitEngine=jgit` (default) and `--gitEngine=subprocess` * three clone shapes -- full, shallow (`--depth=1`), partial (`--filter=blob:none`) * the on-demand refetch path -- commits/branches that land on the remote *after* startup * the targeted by-SHA path -- a commit reachable only via refs/pull/* that a broad fetch misses * the startup fetch path -- lame-duck vs. ready health semantics -It then asserts a *parity* invariant: for every scenario, the in-process JGit engine must -succeed wherever the native-git subprocess engine succeeds. A violation is exactly the class -of bug behind reports like: +The `serve` command runs every git operation by shelling out to the native `git` binary, so the +shallow and partial clones here -- whose thin packs are delta-compressed against objects the clone +does not have -- fetch and serve correctly rather than failing like: - 400 git error: JGit fetch origin failed: Missing delta base - -which happens when JGit fetches a thin pack whose delta base is not present in a shallow or -partial clone -- a case native git handles but JGit 5.13 does not. + 400 git error: revision ... is missing from the local clone Usage: tools/serve_harness.py # build, then run the full matrix @@ -340,7 +336,6 @@ class Serve: port: int workspace: Path stderr_path: Path - engine: str def stderr_tail(self, n: int = 25) -> str: try: @@ -362,7 +357,6 @@ def stop(self) -> None: def serve( workspace: Path, cache: Path, - engine: str, *, initial_fetch: bool, track_deps: bool, @@ -373,7 +367,7 @@ def serve( health is one of: "ready" (200), "lameduck" (up but stuck 503), "down" (never bound). """ port = free_port() - stderr_path = workspace.parent / f"serve.{engine}.stderr.log" + stderr_path = workspace.parent / f"serve.{workspace.name}.stderr.log" args = [ str(LAUNCHER), "serve", @@ -385,15 +379,13 @@ def serve( str(cache), "--port", str(port), - "--gitEngine", - engine, ] if not initial_fetch: args.append("--no-initial-fetch") if track_deps: args.append("--trackDeps") - vlog(f"launch serve[{engine}] on :{port} ws={workspace.name} initial_fetch={initial_fetch}") + vlog(f"launch serve on :{port} ws={workspace.name} initial_fetch={initial_fetch}") with open(stderr_path, "w") as errf: proc = subprocess.Popen( args, @@ -401,7 +393,7 @@ def serve( stdout=(None if VERBOSE else subprocess.DEVNULL), stderr=(errf if not VERBOSE else None), ) - s = Serve(proc=proc, port=port, workspace=workspace, stderr_path=stderr_path, engine=engine) + s = Serve(proc=proc, port=port, workspace=workspace, stderr_path=stderr_path) try: health = _await_health(s, ready_timeout) yield s, health @@ -505,8 +497,8 @@ def _stable(data) -> set: A generated file's hash equals its generating rule's hash, yet whether it surfaces as a distinct impacted target fluctuates with Bazel's incremental analysis state across the - service's sequential checkouts (observed identically under both git engines). Rules and - source files are stable, so scenario assertions compare only that deterministic subset. + service's sequential checkouts. Rules and source files are stable, so scenario assertions + compare only that deterministic subset. """ return {label for label in _iset(data) if not label.endswith(".out")} @@ -516,15 +508,13 @@ def _stable(data) -> set: # ---------------------------------------------------------------------------------------------- -def case_full(remote: Remote, clone: Path, root: Path, rep: Report, engine: str, track_deps: bool) -> None: - """The broad functional suite on a full clone, run identically for both git engines so any - divergence is attributable to the engine alone.""" - case = f"full/{engine}" + ("+deps" if track_deps else "") +def case_full(remote: Remote, clone: Path, root: Path, rep: Report, track_deps: bool) -> None: + """The broad functional suite on a full clone.""" + case = "full" + ("+deps" if track_deps else "") log(f"\n{C.BOLD}== case {case} =={C.RESET}") sh = remote.shas - cache = root / f"cache-full-{engine}-{int(track_deps)}" - track = track_deps - with serve(clone, cache, engine, initial_fetch=False, track_deps=track, ready_timeout=30) as (s, health): + cache = root / f"cache-{clone.name}" + with serve(clone, cache, initial_fetch=False, track_deps=track_deps, ready_timeout=30) as (s, health): if not rep.check(case, "health becomes ready", health == "ready", fail_detail=f"health={health}\n{s.stderr_tail()}"): return @@ -645,18 +635,16 @@ def worker(): ok_detail=f"codes={codes}", fail_detail=f"codes={codes} distinct_sets={len(sets)}") -def case_shallow(remote: Remote, clone: Path, root: Path, rep: Report, engine: str) -> None: - """Fetch-parity gate on a shallow (--depth=1) clone: a just-landed commit must be servable via - on-demand refetch under BOTH engines. This is the clone shape behind production - 'JGit fetch ... Missing delta base' failures (the remote's thin pack is delta-compressed against - an object below the shallow boundary). A cooperative local git-daemon negotiates it cleanly, so - in-process JGit usually succeeds here; the native-git fetch fallback is what keeps the default - engine at parity on the nastier real-world remotes where JGit's in-process fetch cannot.""" - case = f"shallow/{engine}" +def case_shallow(remote: Remote, clone: Path, root: Path, rep: Report) -> None: + """Fetch gate on a shallow (--depth=1) clone: a just-landed commit must be servable via + on-demand refetch. This is the clone shape behind production 'Missing delta base' failures (the + remote's thin pack is delta-compressed against an object below the shallow boundary); native git + negotiates it cleanly.""" + case = "shallow" log(f"\n{C.BOLD}== case {case} =={C.RESET}") sh = remote.shas - cache = root / f"cache-shallow-{engine}" - with serve(clone, cache, engine, initial_fetch=False, track_deps=False, ready_timeout=30) as (s, health): + cache = root / f"cache-{clone.name}" + with serve(clone, cache, initial_fetch=False, track_deps=False, ready_timeout=30) as (s, health): if not rep.check(case, "health becomes ready", health == "ready", fail_detail=f"health={health}\n{s.stderr_tail()}"): return @@ -671,17 +659,15 @@ def case_shallow(remote: Remote, clone: Path, root: Path, rep: Report, engine: s _bazel_shutdown(clone) -def case_partial(remote: Remote, clone: Path, root: Path, rep: Report, engine: str) -> None: - """Fetch-parity gate on a partial (--filter=blob:none) clone, exercised through the *startup* - fetch: with an initial fetch enabled the instance must reach ready under both engines. A partial - clone's thin pack can be delta-compressed against an absent promised blob, which JGit cannot - fetch in-process; the native-git fallback keeps it ready. Health-only (no requests) so JGit's - separate lack of lazy-blob checkout support is not conflated with the fetch path under test -- - a partial clone still needs --gitEngine=subprocess to actually *serve* queries.""" - case = f"partial/{engine}" +def case_partial(remote: Remote, clone: Path, root: Path, rep: Report) -> None: + """Fetch gate on a partial (--filter=blob:none) clone, exercised through the *startup* fetch: + with an initial fetch enabled the instance must reach ready. A partial clone's thin pack can be + delta-compressed against an absent promised blob; native git fetches it correctly. Health-only + (no requests), since the startup fetch path is what is under test here.""" + case = "partial" log(f"\n{C.BOLD}== case {case} =={C.RESET}") - cache = root / f"cache-partial-{engine}" - with serve(clone, cache, engine, initial_fetch=True, track_deps=False, ready_timeout=25) as (s, health): + cache = root / f"cache-{clone.name}" + with serve(clone, cache, initial_fetch=True, track_deps=False, ready_timeout=25) as (s, health): rep.check(case, "startup fetch on partial clone -> ready", health == "ready", ok_detail="ready", @@ -704,28 +690,23 @@ def _bazel_shutdown(clone: Path) -> None: "partial": ["--filter=blob:none"], } -# (name, variant, engine, track_deps). The full clone runs the 2x2 of engine x trackDeps so any -# divergence is attributable to a single variable; shallow/partial exercise the fetch paths. -# Native (subprocess) rows run before their jgit twins so the correct baseline prints first. +# (name, variant, track_deps). The full clone runs with and without --trackDeps; shallow and +# partial exercise the on-demand and startup fetch paths. CASES = [ - ("full/subprocess", "full", "subprocess", False), - ("full/jgit", "full", "jgit", False), - ("full/subprocess+deps", "full", "subprocess", True), - ("full/jgit+deps", "full", "jgit", True), - ("shallow/subprocess", "shallow", "subprocess", None), - ("shallow/jgit", "shallow", "jgit", None), - ("partial/subprocess", "partial", "subprocess", None), - ("partial/jgit", "partial", "jgit", None), + ("full", "full", False), + ("full+deps", "full", True), + ("shallow", "shallow", None), + ("partial", "partial", None), ] -def _run_case(name, variant, engine, td, remote, clone, root, rep) -> None: +def _run_case(name, variant, td, remote, clone, root, rep) -> None: if variant == "full": - case_full(remote, clone, root, rep, engine, td) + case_full(remote, clone, root, rep, td) elif variant == "shallow": - case_shallow(remote, clone, root, rep, engine) + case_shallow(remote, clone, root, rep) else: - case_partial(remote, clone, root, rep, engine) + case_partial(remote, clone, root, rep) def main() -> int: @@ -761,7 +742,7 @@ def main() -> int: # land, so those commits genuinely require an on-demand fetch. Each case gets its own clone # + cache so sequential checkouts never cross-contaminate. selected = [c for c in CASES if not args.only or args.only in c[0]] - for name, variant, engine, td in selected: + for name, variant, td in selected: slug = name.replace("/", "-").replace("+", "-") clones[name] = remote.clone(root / f"clone-{slug}", *VARIANTS[variant]) @@ -777,9 +758,9 @@ def main() -> int: remote.repack() # force new blobs to delta against the most similar (possibly out-of-clone) base vlog(f"shas: {remote.shas}") - for name, variant, engine, td in selected: + for name, variant, td in selected: try: - _run_case(name, variant, engine, td, remote, clones[name], root, rep) + _run_case(name, variant, td, remote, clones[name], root, rep) except Exception: rep.add(name, "case crashed", FAIL, traceback.format_exc().splitlines()[-1]) vlog(traceback.format_exc())