docs(fm-stat-lib): correct inverted GNU stat -f failure-mode comments - #132
Conversation
Six sites state the mechanism backwards. They claim GNU `stat -f <fmt> <path>`
exits 0 and that the `||` in a `stat -f ... || stat -c ...` chain therefore
never fires, leaving the correct call unrun.
Measured, GNU coreutils 9.7:
$ gstat -f %m /tmp ; echo rc=$?
File: "/tmp"
ID: 100000d0000001a Namelen: ? Type: apfs
Block size: 4096 Fundamental block size: 4096
rc=1
rc is 1. GNU takes the operands in order: the format string fails as an operand
and the path prints 223 bytes of filesystem dump to stdout, so the exit is
non-zero and the `||` does fire. The failure is not that the fallback never
runs - it is that the fallback runs and appends its correct integer to the dump
already sitting in the same command substitution. The caller receives a
multi-line non-integer at overall rc=0: invisible to error handling, fatal to
the arithmetic after it.
This matters beyond wording. The stated reason is why nobody may reintroduce
the chain, and bin/fm-stat-lib.sh is the single owner of that explanation, so
its header being wrong is the worst of the six. It also contradicted
bin/fm-busy-event.sh:110 on the same tree, which already said "so the `||` does
fire".
The probe order is unchanged and still correct, but the reason is restated:
`-c` goes first because GNU's `-f` pollutes stdout before failing, so a
`-f`-first probe would contribute junk even when it fails - not because the
`||` fails to fire.
tests/fm-remote-job.test.sh carried the error twice: once in prose, and once in
the GNU shim itself, whose -f arm printed the dump and exited 0. A fixture
labelled "GNU coreutils shape" that disagrees with the binary it stands in for
teaches the next reader the wrong mechanism. It now exits 1. The suite still
passes, which also confirms the correct code path never reaches that arm.
bin/fm-test-run.sh's chain is GNU-FIRST and therefore correct code; only its
justifying comment was wrong, so that site is comment-only.
Reported in robots-qyim, which #102 fixed on its branch (7e7bd9f, ffa1936);
#104 was what merged and it carried the pre-fix prose, plus a new instance in
fm-stat-lib.sh itself.
|
@coderabbitai review |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes align stat portability comments and test fixtures with GNU ChangesStat dialect alignment
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This change corrects documentation and aligns a test fixture with GNU stat behavior without changing production runtime logic; affected tests pass, and no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
What Changed
bin/fm-stat-lib.sh,bin/fm-bootstrap.sh,bin/fm-supervise-daemon.sh,bin/fm-test-run.sh,bin/fm-watch.sh, andbin/fm-x-lib.shthat described GNUstat -fas exiting 0 with a filesystem dump so the||fallback "never fires" — real GNU writes the dump to stdout and then exits 1, so the fallback does fire and its integer is appended to the dump, yielding a multi-line non-integer at overall rc=0.statfixture intests/fm-remote-job.test.shto exit 1 after printing the filesystem dump, matching the real binary's behavior instead of the previously documented (incorrect) exit-0 shape.tests/fm-busy-state.test.shto match the corrected failure mode and documented whyunameis deliberately not shimmed there.Risk Assessment
✅ Low: The change is comments-only plus a test-fixture exit-status correction that makes the fake match real GNU behavior; both prior findings are verified fixed in the current code, the fixture change cannot alter the code path the tests exercise (the library probes -c first and never runs -f under the GNU dialect), and a repo-wide grep confirms no inverted exit-status claims remain.
Testing
Empirically validated every corrected comment claim against real GNU coreutils 9.7 (stat -f dumps to stdout then exits 1; the || fallback fires and appends its integer at overall rc=0, with the cited 223-byte dump measured exactly), ran both affected test files which pass fully including the dialect-detection test exercising the exit-1 fixture change, and confirmed fm_stat_mtime returns a clean integer end-to-end with GNU stat resolved as
stat; no visual evidence applies since this is a shell-comment and test-fixture change with no UI surface.Evidence: GNU stat 9.7 behavior verification (exit status, dump bytes, ||-chain failure mode)
$ gstat -f %m /tmp; echo exit=$? File: "/tmp" ... Type: apfs ... (223 bytes on stdout) exit=1 $ v=$(gstat -f %m /tmp 2>/dev/null || gstat -c %m /tmp 2>/dev/null) overall rc=0 — fallback DID fire; its output is appended to the dumpEvidence: fm-stat-lib end-to-end with real GNU stat resolved as `stat`
stat (GNU coreutils) 9.7 fm_stat_mtime /tmp -> [1770268403] rc=0 clean integer: OKPipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-watch.sh:100- bin/fm-watch.sh:100 still asserts the inverted claim this branch exists to fix ("GNU's -f ... writes a filesystem dump to stdout and the||never fires"). Measured GNU coreutils 9.7 behavior: stat -f exits 1 after dumping, so the fallback DOES fire and appends its integer to the dump. The corrected fm-stat-lib.sh header now directly contradicts this comment while explicitly pointing readers at fm-watch.sh ("see the comment this replaces in fm-watch.sh"), recreating the exact hazard robots-qyim describes: a maintainer who tests the false 'exits 0' claim and finds it wrong may conclude the chain is safe. Fix: reword fm-watch.sh:99-102 to match the corrected wording used in fm-supervise-daemon.sh:232-234.bin/fm-bootstrap.sh:278- bin/fm-bootstrap.sh:277-278 carries the same inverted exit-status fact ("GNU-fis --file-system, so the Darwin branch would print an apfs dump at exit 0"). Measured: GNU exits 1 after the dump. The comment's conclusion (non-numeric token reaches the caller) is still correct, but the stated exit code is wrong in the same way the three ticket-named comments were. Worth correcting in the same sweep.🔧 Fix: correct remaining inverted GNU stat exit-status comments
1 info still open:
tests/fm-busy-state.test.sh:376- The newly added comment states the shim "would not catch a regression back touname-keyed dispatch." That claim is host-dependent: on a Darwin test host, a uname-keyed regression would select thestat -fbranch, hit the shim's dump-then-exit-1 behavior, and fail the stale-lock reap test — i.e. the regression WOULD be caught there. Only on a Linux host (uname→GNU branch→-cworks) does it slip through. The simplification errs toward caution (it undersells coverage rather than overselling safety), so it does not recreate the robots-qyim hazard; noting for precision only.✅ **Test** - passed
✅ No issues found.
Empirical:gstat -f %m /tmpon GNU coreutils 9.7 → exit 1 with 223 bytes of apfs dump on stdout, matching the corrected comment's cited measurementEmpirical:v=$(gstat -f %m /tmp 2>/dev/null || gstat -c %m /tmp 2>/dev/null)→ fallback fires, output appended to dump, overall rc=0 — the exact failure mode the corrected comments describebash tests/fm-remote-job.test.sh— all 23 tests pass, including "the mtime reader survives either stat dialect ahead of /usr/bin" which exercises the fixture changed to exit 1 on -fbash tests/fm-busy-state.test.sh— all tests pass, including the GNU-stat-shim dialect testEnd-to-end:fm_stat_mtime /tmpwith real GNU stat first on PATH asstat→ clean integer 1770268403, rc=0✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit
Documentation
statbehavior and detection logic.stat -fmay emit filesystem data before failing, potentially contaminating fallback output.Tests
statoutput and validate clean fallback behavior.