Repository navigation
Make a non-default Branch of ConfigTester resolve - #809
Merged
Merged
Conversation
The `Branch` override has never worked, so there was no way to run
`buildDependants` against an unmerged branch of a consumer repository —
which is exactly when one needs it. The feature that
`config-tester.gradle.kts` documents in a comment,
.addRepo(SpineRepos.coreJvm, Branch("grpc-concurrency-fixes"))
died with
org.eclipse.jgit.api.errors.RefNotFoundException: Ref <name> cannot
be resolved
`GitRepository.checkout` cloned, and then checked the branch out as
a second step. `Grgit.checkout` resolves a name against the clone's
local ref database, which right after a clone holds only the default
branch; every other branch is there solely as a remote-tracking ref. So
the commented-out example was the one thing in that file that could not
work, which is why it went unnoticed.
Pass the ref to the clone instead. `CloneOp.refToCheckout` maps onto
JGit's `CloneCommand.setBranch`, which resolves against the refs
advertised by the remote, links local `HEAD` to the branch and adds its
merge config. The branch thus lands as a proper local branch tracking
its remote counterpart.
This is preferred over adding `createBranch` and `startPoint` to the
`checkout` call, which would need an "is this already a local ref?"
special case to keep the default `Branch("master")` working. One code
path now serves both.
The added `close()` releases the JGit repository handle, which the
discarded `Grgit` of the two-step code leaked on every checkout.
A misspelled branch now fails faster and more clearly, with
`TransportException: Remote branch 'x' not found in upstream origin`.
It does not clone an empty working tree, which would have resurfaced
the confusing "does not contain a Gradle build" failure that #805
removed.
`GitRepositorySpec` covers the three cases without network access, by
cloning a throwaway repository over its `file://` URI. It asserts the
`HEAD` of the clone, along with the branch it tracks, because
`Branch("origin/<name>")` — the pre-fix workaround — holds the right
files too, but as a detached `HEAD`.
`Path.put()`, which both specs of the package need to lay out a folder,
moves from the companion of `ClonedRepoSpec` to a top-level extension
of its own, instead of being copied into the new spec.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
There was a problem hiding this comment.
🟢 Approved
The focused fix has targeted regression coverage and no identified blocking issues; tests were not executed in this review.
No open issues 🎉
What changed in this PR
Fixes non-default branch checkout in ConfigTester, extending the buildDependants repairs from #805 to support unmerged consumer branches.
Changes:
- Selects the branch during cloning and closes the Git repository handle.
- Adds local regression tests for default, non-default, and missing branches.
- Shares the test file-creation helper and clarifies branch documentation.
| File | Description |
|---|---|
buildSrc/src/test/kotlin/io/spine/gradle/PathExtensions.kt |
Extracts the shared file-creation helper. |
buildSrc/src/test/kotlin/io/spine/gradle/GitRepositorySpec.kt |
Tests branch selection, tracking, and missing-branch failure. |
buildSrc/src/test/kotlin/io/spine/gradle/ClonedRepoSpec.kt |
Uses the extracted helper. |
buildSrc/src/main/kotlin/io/spine/gradle/ConfigTester.kt |
Resolves branches during cloning, closes the handle, and updates documentation. |
Review effort: Balanced
`put()` says where the file goes but not what happens there, so the extension both specs of the package use to lay out a folder becomes `createFile()`, and its file becomes `PathExts.kt`. The name is an overload of `kotlin.io.path.createFile()` rather than a clash: that one takes file attributes, this one a relative path and the content. Neither spec imports the standard-library extension. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ix-config-tester-branch
…ix-config-tester-branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #805, which repaired four defects in
buildDependants. This is thefifth, found while using the repaired task to verify an unmerged consumer fix.
The defect
The
Branchoverride has never worked, so there was no way to runbuildDependantsagainst an unmerged branch of a consumer repository — which isexactly when one needs it.
config-tester.gradle.ktsdocuments the feature:Using it failed with:
GitRepository.checkoutcloned, and then checked the branch out as a secondstep.
Grgit.checkoutresolves a name against the clone's local refdatabase, which right after a clone holds only the default branch — every other
branch is there solely as a remote-tracking ref. So the commented-out example
was the one thing in that file that could not work, which is why it went
unnoticed for so long.
The fix
Pass the ref to the clone instead of checking it out afterwards:
CloneOp.refToCheckoutmaps onto JGit'sCloneCommand.setBranch, whichresolves against the refs advertised by the remote, links local
HEADtothe branch and adds its merge config. The branch thus lands as a proper local
branch tracking its remote counterpart.
This was preferred over adding
createBranch+startPointto thecheckoutcall, which would have needed an "is this already a local ref?" special case to
keep the default
Branch("master")working. One code path now serves both.The
.close()releases the JGit repository handle, which the discardedGrgitof the two-step code leaked on every checkout.
Two things worth knowing
Branch("origin/<name>")was the pre-fix workaround and still resolves, butas a detached
HEAD. So "the right files are present" does not prove abranch was checked out properly, and the spec asserts the clone's current
branch for that reason.
TransportException: Remote branch 'x' not found in upstream origin. It doesnot clone an empty working tree, which would have resurfaced the confusing
"does not contain a Gradle build" failure that Fix the
buildDependantstask #805 removed.Tests
GitRepositorySpec— 3 cases, no network access. ItGrgit.inits a throwawayrepository in
@TempDirwith two branches and clones it over itsfile://URI.JGit honors
init.defaultBranch, so the fixture reads the initial branch namerather than assuming
master.Against the unfixed
checkout,check out a branch other than the default onefails with the reported
RefNotFoundException, whilecheck out the default branchstill passes — the suite is vacuous in neither direction.Path.createFile(), which both specs of the package need to lay out a folder,moves out of the companion of
ClonedRepoSpecinto a top-level extension of itsown in
PathExts.kt, instead of being copied into the new spec. The name is anoverload of
kotlin.io.path.createFile(), not a clash: that one takes fileattributes, this one a relative path and the content.
Verification
./gradlew detektpasses, as does the wholebuildSrctest suite../gradlew clean buildDependantswas run against the real remotes, and everycheckout was inspected with
git branch -vv:HEADof the clonebasemastermaster->origin/masterbase-typesmastermaster->origin/mastercore-jvmmastermaster->origin/masterbase-typesfix-publishing-docsfix-publishing-docs->origin/fix-publishing-docsSo both paths resolve, and both land a local branch tracking its remote
counterpart rather than a detached
HEAD. The non-default run used a temporaryedit to
config-tester.gradle.ktsthat is not committed — the example therestays a comment.
Both consumer build failures are unrelated to this change, and both occur well
after a successful checkout:
core-jvmatmasterfailed onUnresolved reference 'OpenTelemetryKotlin',which Declare
OpenTelemetryKotlininbuildSrc#808 has since declared inbuildSrc.base-typesatfix-publishing-docsfails compiling its own build script onUnresolved reference 'pluginLib'— that branch predates the currentconfig. This is exactly the driftbuildDependantsexists to surface.An earlier run also confirmed the fast-fail path against a real remote: a branch
deleted after its PR merged was reported as
Remote branch '...' not found in upstream origin, rather than silently cloningan empty tree.
🤖 Generated with Claude Code