fix(arcup): version_gt ignores prerelease tags in comparisons - #255
fix(arcup): version_gt ignores prerelease tags in comparisons#255batuhankocyigit wants to merge 1 commit into
Conversation
osr21
left a comment
There was a problem hiding this comment.
The bug is real and your analysis of it is accurate — but this ground has been claimed for three weeks. #212 (danilaverbena, opened July 20) fixes this exact bug — it's the PR attached to issue #205, which your description restates without citing. #212 has been through two review rounds and is strictly ahead of this implementation on the cases where the two differ. Details:
1. Build metadata: #212 handles it, this PR errors on it. SemVer allows +build suffixes, and version_gt here never strips them. Ran your branch directly:
version_gt "1.0.1+build.5" "1.0.0" → returns "not newer" (wrong), with:
arcup: line 286: [: 1+build.5: integer expression expected
version_gt "1.0.0+build-123" "1.0.0" → same class of failure
The +build survives into the patch field, the -gt integer test throws, and both comparisons fall through to "not newer". Worse, build metadata containing a hyphen (1.0.0-rc.1+build-123) gets the hyphen-split treatment and the metadata leaks into the prerelease identifier comparison. #212 hit this same class of issue in its first review round and fixed it by stripping +… up front (v="${v%%+*}") before the prerelease split — that ordering handles both cases correctly, and #212's test file asserts it (assert_le "1.0.0-rc.1+build-123" "1.0.0-rc.1" and friends).
2. Test placement: #212's composes better with the CI wiring in flight. This PR adds assertions inline to arcup/test_arcup.sh — the file that #223 (+235/-2) and #243 already modify, joining a three-way rebase cluster. #212 put its 25 assertions in a new arcup/version_gt_test.sh, which (a) touches nothing anyone else is editing, and (b) already matches the discovery glob in #247's make test-arcup, so it gets CI coverage automatically the moment that lands.
3. Small factual nit, same one I've flagged on two other PRs now: the suite on main has 25 assertions, not 24 ("28/28" should be 29 — your own branch prints 29 ok lines, which I confirmed by running it).
To be clear about what this PR gets right: the core precedence logic is sound for hyphen-only versions (verified 0.3.0 > 0.3.0-rc.1, rc-vs-rc numeric ordering works), the numeric-vs-lexical identifier split is correct semver, and the writeup is honest about scope. If #212 didn't exist this would be a solid start needing one hardening round. But #212 does exist, is further along, covers a superset of cases, and belongs to the contributor who filed the issue. The fair move is to close this in favor of #212 — and if you want this effort to count, reviewing #212's prerelease-identifier loop against your own implementation's edge cases (e.g. what each does with a purely-numeric first identifier like 1.0.0-1.2) would be genuinely additive: a second implementer is exactly who spots the cases both might have missed.
|
Thanks for the thorough review, @osr21 — verified all three points against my |
|
Appreciate the fast, graceful close, @batuhankocyigit — verifying against your own branch before responding is exactly the right instinct. To close the loop on the cross-review I suggested: I went ahead and ran the edge cases where your implementation and #212 could plausibly diverge, directly against #212's current branch. All correct per semver.org §11: Its own suite is also green ( If you're looking for genuinely open ground in this repo: as far as I've mapped it, the arcup/CI cluster is fully claimed (#212, #223, #243, #247, #249, #254 all pending merge), so fresh issues from actually running the node/tooling — rather than another pass over |
fix(arcup):
version_gtignores prerelease tags entirely, so it can't tell a release from its own release candidateThe bug
version_gt()inarcup/arcupstrips everything after the first-beforecomparing versions:
This means the prerelease suffix (
-rc.1,-rc.2, ...) is discardedbefore any comparison happens — two versions that only differ in their
prerelease tag are indistinguishable to this function, in both directions:
The existing test suite already asserts the other direction is correct
(
version_gt "0.3.0-rc.1" "0.3.0"→ false, i.e. "same prerelease base isnot newer" — this still passes), which shows prerelease correctness was
already a design concern here; the reverse direction and rc-vs-rc just
weren't covered.
Where this bites:
check_installer_up_to_dateandupdate_arcupbothcall
version_gtto decide whether a newerarcupis available. If a useris running a prerelease build of
arcupand the true latest stable releasehas the same
major.minor.patch, they'll never be prompted to update,since the function reports "not newer" in both directions.
The fix
Keep the numeric
major.minor.patchcomparison as-is (unchanged), but oncethat's equal, apply semver prerelease precedence instead of discarding the
suffix:
(
1.2.3 > 1.2.3-rc.1)..):numeric identifiers compare numerically (
rc.10 > rc.9, not a stringcomparison), others compare lexically; a prerelease that's a prefix of
the other (fewer identifiers) is lower.
No change to the plain
major.minor.patchpath — every previously-passingcomparison keeps the same result.
Testing
arcup/test_arcup.sh's existingtest_version_comparison(same bash TAP-style harness already used inthis file): stable-vs-own-rc, rc.2-vs-rc.1, rc.1-vs-rc.2, and
rc.10-vs-rc.9 (numeric, not lexical, ordering).
bash arcup/test_arcup.sh— 28/28 passing (24pre-existing + 4 new), including the pre-existing "same prerelease base
is not newer" case, confirming no regression.
bash -n arcup/arcup— syntax OK.shellcheck arcup/arcup— no warnings.Notes for reviewers
version_gt; no other function touched.normalize_version's regex or accepted formats — onlyhow two already-valid version strings are ordered.