Follow-up from the independent review of #97 (which fixed the #89-reintroduction data-loss bug in actions/git-commit's push-retry conflict resolution).
Gap found (non-blocking for #97, but a real latent risk)
The conflict-resolution path's sed -i -E "s/^version = \"[0-9]+\.[0-9]+\.[0-9]+\"/version = \"$new_version\"/" (and the setup.cfg equivalent) requires an exact match: version = "X.Y.Z" with one space around = and double quotes for pyproject.toml. A pyproject.toml whose version line uses different spacing or quoting -- e.g. version="1.0.0" (no spaces, valid TOML) -- makes the sed a silent no-op: it exits 0, substitutes nothing, and the conflict resolution leaves the OLD version in place even though new_version extraction succeeded.
Since actions/git-commit is consumed @master by every wads-managed repo (~190+), and this code path only fires during the rare version-file-conflict race, a repo with non-standard formatting could hit exactly the problem #83 was written to prevent -- git recording an older version than what's actually live on PyPI -- with no error or warning logged anywhere.
Suggested fix
After the sed, diff the file (or compare old/new content) and log a loud ::warning:: (or fail outright) if the version line didn't actually change despite new_version being non-empty and differing from what's currently in the checked-out file -- so a formatting mismatch surfaces in the CI log instead of silently reintroducing the version-desync bug.
Also noted (UX-only, not data-loss)
If a version file is deleted on one side of the conflicting rebase, git checkout --ours -- "$path" fails (error: path '...' does not have our version), which aborts the step under set -eo pipefail with a cryptic native git error instead of the script's own ::error:: "resolve by hand" message. No data is lost (the step just fails loudly instead of gracefully), but worth a friendlier error path if this is ever touched again.
Found during the #97 review; not fixed there since it doesn't affect wads's own pyproject.toml (confirmed exact-format match) and is lower urgency than the live data-loss bug #97 fixes.
Follow-up from the independent review of #97 (which fixed the #89-reintroduction data-loss bug in
actions/git-commit's push-retry conflict resolution).Gap found (non-blocking for #97, but a real latent risk)
The conflict-resolution path's
sed -i -E "s/^version = \"[0-9]+\.[0-9]+\.[0-9]+\"/version = \"$new_version\"/"(and thesetup.cfgequivalent) requires an exact match:version = "X.Y.Z"with one space around=and double quotes forpyproject.toml. Apyproject.tomlwhose version line uses different spacing or quoting -- e.g.version="1.0.0"(no spaces, valid TOML) -- makes theseda silent no-op: it exits 0, substitutes nothing, and the conflict resolution leaves the OLD version in place even thoughnew_versionextraction succeeded.Since
actions/git-commitis consumed@masterby every wads-managed repo (~190+), and this code path only fires during the rare version-file-conflict race, a repo with non-standard formatting could hit exactly the problem #83 was written to prevent -- git recording an older version than what's actually live on PyPI -- with no error or warning logged anywhere.Suggested fix
After the
sed, diff the file (or compare old/new content) and log a loud::warning::(or fail outright) if the version line didn't actually change despitenew_versionbeing non-empty and differing from what's currently in the checked-out file -- so a formatting mismatch surfaces in the CI log instead of silently reintroducing the version-desync bug.Also noted (UX-only, not data-loss)
If a version file is deleted on one side of the conflicting rebase,
git checkout --ours -- "$path"fails (error: path '...' does not have our version), which aborts the step underset -eo pipefailwith a cryptic native git error instead of the script's own::error::"resolve by hand" message. No data is lost (the step just fails loudly instead of gracefully), but worth a friendlier error path if this is ever touched again.Found during the #97 review; not fixed there since it doesn't affect wads's own
pyproject.toml(confirmed exact-format match) and is lower urgency than the live data-loss bug #97 fixes.