fix(arcup): accept checksum files without a trailing newline - #243
Conversation
verify_checksum_file() decided emptiness from the exit status of `read -r expected_checksum expected_name < "$checksum_path"`. `read` returns non-zero when it reaches EOF before a newline, even though it has already populated the variables. A .sha256 whose single line lacks a trailing newline is valid, but arcup rejected it with "Checksum file is empty", aborting a correct install. Decide emptiness from the parsed hash instead of read's exit status. A genuinely empty file still yields an empty hash and is still rejected. Adds regression tests covering the no-trailing-newline and empty cases.
|
Reviewed and ran this locally. The diagnosis is right: Verified the tests actually pin the bug: restoring the pre-fix I also probed the nearby edge cases manually. A newline-only file and a For sequencing context: these regression tests do not run in CI today because |
|
Thanks for the thorough pass — restoring the pre-fix script against the new tests to confirm they actually pin the bug is exactly the verification this kind of change needs, and I appreciate you probing the newline-only/spaces-only edges manually. Took the coverage nit as actionable rather than leaving it a nit: pushed # A wrong hash without a trailing newline must still fail the comparison,
# not slip through the emptiness check.
printf '%s %s' "0000...0000" "$archive_name" > "$checksum_file"
expect_fail "mismatched checksum without trailing newline fails" verify_checksum_file ...As you observed, it was never a behavioral gap — the value flows to the normal comparison and fails there — but it's the one cell where a future regression in the emptiness check could theoretically have flipped an invalid file to accepted, which is the dangerous direction for a checksum verifier (the original bug only broke in the safe direction, rejecting valid files). Now all four newline×validity combinations are pinned: valid+newline (pre-existing), valid+no-newline and empty (this PR's originals), mismatch+no-newline (new). Suite is green at 28 assertions locally. Agreed on sequencing with #247 — independent merges, and the coverage here becomes continuously enforced the moment that CI wiring lands. From this side there's nothing left pending; ready for maintainer review. |
|
Nice — the asymmetry argument is the better framing; the original bug only |
What
verify_checksum_file()decided whether a checksum file was empty from the exit status of:readreturns a non-zero status when it reaches EOF before encountering a newline — even though it has already assigned the variables. So a.sha256file whose single line has no trailing newline (valid, and produced by several non-GNU checksum tools or by manual editing) was rejected withChecksum file is empty, aborting an otherwise-correct install.Fix
Decide emptiness from the parsed hash rather than from
read's exit status:A genuinely empty file still produces an empty
$expected_checksumand is still rejected, so the empty-file guard is preserved.Repro
Tests
Extends
test_checksum_validationinarcup/test_arcup.shwith two cases:Both fail on the current
arcupand pass with this change. Fullbash arcup/test_arcup.shsuite is green.Notes
Current release
.sha256assets do end in a newline, so this is a latent-robustness fix in the checksum-verification path rather than an active break — butarcupshouldn't refuse a checksum file it verifies correctly simply because of a missing trailing newline.