Repository navigation
fix(xtest): make otdf-sdk-mgr follow tags moved upstream - #675
Draft
dmihalcik-virtru wants to merge 1 commit into
Draft
dmihalcik-virtru wants to merge 1 commit into
dmihalcik-virtru wants to merge 1 commit into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Oct 22, 2026
dmihalcik-virtru
added a commit
that referenced
this pull request
Oct 23, 2026
…pted js installs (#667) Part 2a of the [DSPX-4372-t1 plan](#657), split out of #626: fixes the benchmark needs, which every `otdf-sdk-mgr` install benefits from. **Based on `main`.** ### What it does - **Bare repos refresh their branches.** `git clone --bare` records no fetch refspec, so the `fetch --all --tags` before every checkout refreshed tags only. Every branch stayed where the first clone found it, and `install tip main` or `--platform main` built that stale commit. `checkout.BARE_REFRESH` now fetches origin's branches into `refs/remotes/origin/*` with `--prune`, for the SDK and platform bare repos alike, and `checkout.upstream_ref` maps a branch name onto them. Fetching into the bare repo's own `refs/heads/*` instead would move branches that other worktrees have checked out, leaving their HEAD ahead of their files, and would fail for good once upstream replaced a branch `foo` with `foo/bar`. - **Worktrees are detached** and reset to the refreshed ref. A branch origin no longer has is refused rather than built from the clone's stale copy, and `HEAD` means origin's default branch as of the clone; a platform `HEAD` worktree used to reset to itself. - **Go source rebuilds work a second time.** `checkout_go_from_platform` reset an existing worktree to a `FETCH_HEAD` fetched into the bare repo, which no worktree can see, so every rebuild after the first failed with `pathspec 'FETCH_HEAD' did not match`. It now resets to the refreshed ref, like the SDK and platform paths. - **`install tip` reports checkout failures as errors.** A refused branch, or a failed git command with git's stderr, now prints `Error: …` and exits 1 for go, js and java, as the platform path already did, instead of a traceback. - **An interrupted js install leaves nothing behind.** #663 already removes a partial `dist/` when npm fails. A Ctrl-C mid-install still left a directory holding only `cli.sh`, which `install_release` then skipped as installed from then on. Both js install paths now remove it unless the install finished. - `AGENTS.md` claimed the bare clone mapped `refs/heads/*`, which is what hid the first bug. It and the code comments also said `git pull` fails in a worktree; it merges origin's default branch into whatever is checked out. ### Validation - `uv run pytest` in `otdf-sdk-mgr`: 207 passed, and 321 at the top of this track (#642). ruff, ruff format and pyright are clean. - `test_checkout.py` runs real git against a local origin. Every shared behaviour is parametrized over the SDK, go and platform entry points: new and existing worktrees follow their branch, `HEAD` (including beside a tag named `main`), a deleted branch is refused, tags and SHAs are checked out and then reused, and a force-pushed `refs/pull/N/head` is followed. This replaces the stubbed command-string test for platform PR refs. SDK-only tests guard other worktrees, force pushes and `x` → `x/y` renames; platform-only tests cover the helper-scripts worktree. A `CliRunner` test covers the `install tip` error path. - Mutation-checked: `symbolic-ref --short`, treating every non-origin name as a deleted branch, dropping go's explicit PR-ref fetch or its `+`, swallowing go's refusal, and dropping the `install tip` catch each fail a test. - Two new tests interrupt each js install path. - `isolated_git` keeps those tests off the developer's git config, any inherited `GIT_DIR`, and the network (`GIT_ALLOW_PROTOCOL=file`). ### Follow-ups Older problems found while reviewing this PR are fixed in a stack of drafts based on it: #673 (don't reset a directory that isn't a worktree) → #674 (branch names vs versions) → #675 (tags moved upstream) → #676 (`checkout go` / `--all`). ### Stack ([DSPX-4372-t1 plan](#657)) Two parallel tracks from `main`; they share no imports and meet in stage 5. | Part | PR | Base | |---|---|---| | 2a: bare-repo and js install fixes | **#667 (this PR)** | `main` | | 2b: manifest contract | #668 | 2a | | 2c: `install benchmark` | #626 | 2b | | 2d: `otdf-local env` exports | #642 | 2c | | 3a: arms as a tuple | #669 | `main` | | 3b: two to four arms | #658 | 3a | | 3c: pytest reads the manifest | #670 | 3b | | 4: candidate-to-candidate conclusions | #659 | 3c | | 5, 6: CI and report | #660, #661 | not yet moved onto these stacks | <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * SDK and platform worktrees now follow updated branches and pull request refs, including after force-pushes. Missing branches produce a clear error instead of a traceback. * Interrupted JavaScript installations no longer leave partial distribution files behind. * **Documentation** * Updated platform installation guidance for branch refreshes, detached worktrees, and pull request refs. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
When upstream moves a tag -- deletes it and re-creates it on another commit, as after a botched release -- the next BARE_REFRESH fails. git 2.20+ won't update an existing tag on fetch without force: `! [rejected] v2 -> v2 (would clobber existing tag)`, exit 1. That fetch runs before every SDK, go and platform source install against the bare repo, so all of them failed until someone ran `git --git-dir=<bare> tag -d <tag>` by hand. The `fetch --all --tags` before #667 failed the same way. BARE_REFRESH now passes --force. The branch refspec already had a `+`, and BARE_REFRESH fetches no `refs/pull/*`, so only tags change: in a scratch repo, branches update and prune, and a fetched PR ref is left alone, exactly as before. A `+refs/tags/*:refs/tags/*` refspec would force tags too, but --prune would then also delete the tags origin dropped, and any made in the bare repo by hand; --tags exempts them. Even with the new tag fetched, an existing worktree for it counted as immutable and was reused at the old commit. SDK builds run `make` on whatever the worktree holds, so they built the old commit under the tag's name. checkout.needs_reset still resets anything is_mutable_target calls mutable, and now also any other worktree whose HEAD isn't `<target>^{commit}`. The bare repo was just refreshed, so the comparison needs no network, and a SHA's worktree never differs. The SDK, go and platform refreshes use it. Reading a worktree's HEAD climbs to the enclosing checkout just as a reset does, so require_worktree now runs before every reuse, not only before a reset: a tag's or SHA's directory that lost its `.git` is refused like a branch's rather than built. A target that names no commit counts as moved, so a version-named branch made after the clone and since deleted on origin now fails at the reset instead of reusing its worktree, which #674 left as not done. A commit made by hand on top of a tag's worktree is reset away, as one on a branch's already was; uncommitted edits in an unmoved tag's worktree are kept. install_platform_source still serves a cached dist for a pinned service/vX.Y.Z without touching the network, so a moved release tag keeps its old build. That is #674's deliberate offline tradeoff. AGENTS.md now says so, and that removing the dist directory rebuilds from the moved tag. The real-git tests move a lightweight and an annotated tag on origin and check out again through every entry point: the bare repo's tag and the worktree follow, and an unmoved tag's worktree is untouched. Others cover a tag's worktree that lost its `.git`, the deleted version-named branch, rebuilding a moved release once its platform dist is removed, and edits surviving in an unmoved annotated tag's worktree. 13 of the 16 new cases failed before the fix. The last passes either way; it guards against comparing an annotated tag's object instead of its commit. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-4372-sdkmgr-branch-refs
branch
from
October 23, 2026 14:02
d6e7b99 to
bd2cb95
Compare
dmihalcik-virtru
force-pushed
the
DSPX-4372-sdkmgr-moved-tags
branch
from
October 23, 2026 14:02
f0ea7c8 to
394fb42
Compare
X-Test Failure Report |
|
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.
Third follow-up from #667's review. Based on #674 (
DSPX-4372-sdkmgr-branch-refs).What it does
! [rejected] v2 -> v2 (would clobber existing tag), exit 1. Every SDK, go and platform source install against that bare repo then failed until someone rangit --git-dir=<bare> tag -d <tag>by hand. The pre-fix(xtest): refresh otdf-sdk-mgr bare-repo branches; clean up interrupted js installs #667fetch --all --tagsdid the same.BARE_REFRESHnow passes--force. The branch refspec already had a+andrefs/pull/*isn't fetched, so only tags change. A+refs/tags/*:refs/tags/*refspec would force tags too, but--prunewould then also delete tags origin dropped, and any made in the bare repo by hand;--tagsexempts them.makebuilt the old commit under the tag's name.checkout.needs_resetstill resets whateveris_mutable_targetcalls mutable, and now also any other worktree whose HEAD isn't<target>^{commit}. The bare repo was just refreshed, so that needs no network, and a SHA never differs from itself. The SDK, go and platform refreshes all use it.require_worktreeruns before reuse too. Reading a worktree's HEAD can climb to the tests checkout just as a reset can, so a tag's or SHA's directory that lost its.gitis now refused like a branch's instead of built. CI is unaffected:xtest/setup-cli-toolputsactions/checkoutclones at these paths but builds them withmakedirectly; the only otdf-sdk-mgr command it runs isinstall artifact.AGENTS.md's "Mutable refs auto-refresh" bullet covers moved tags and the platform cache caveat below.Validation
uv run pytestinotdf-sdk-mgr: 262 passed (16 new or newly parametrized). ruff, ruff format and pyright are clean.Real-git tests move a lightweight and an annotated tag on origin (
git tag -f v1.2.3 <other>) and check out again through the SDK, go and platform entry points. They assert no error, the bare repo's tag moved, the worktree is at the new commit, and an unmoved tag's worktree is untouched. Other tests cover a tag's worktree that lost its.git, the deleted version-named branch, and rebuilding a moved release once its platform dist is removed.Against fix(xtest): stop otdf-sdk-mgr mistaking branch names for versions #674's code, 13 of the 16 new cases fail. With only
--forceapplied, the moved-tag cases still fail on the worktree's HEAD, so both halves are needed. The other 3 (test_unmoved_tag_keeps_local_edits) pass before and after. Mutation-checked: dropping^{commit}from the comparison resets every annotated tag's worktree, and fails them.The refspec choice, checked in a scratch repo on git 2.56, after upstream moves tags
v2and annotatedv3, deletes taggoneand branchother, and force-pushes PR 1:--force+refs/tags/*refspecwould clobber)v2,v3^{commit}gone; tag made only in the bare repoorigin/main;origin/otherrefs/pull/1/headNot done here
service/vX.Y.Z) still serves its cached build after a tag move.install_platform_sourcereuses that dist without touching the network, the offline tradeoff fix(xtest): stop otdf-sdk-mgr mistaking branch names for versions #674 made on purpose.AGENTS.mdsays how to rebuild: removextest/platform/dist/vX.Y.Z(otdf-sdk-mgr cleancovers only the SDKs) and install again. A test checks that recipe.Stack
Follow-ups from #667's review, each based on the one before:
checkout go/checkout --all