docs: the deeper-subshell premise's repair is depth, not a looser count (#961) - #987
Conversation
… not looser (commandprompt#961) commandprompt#985's premise -- exactly one 'if (' at any indent in the extracted block -- closes a hole where a write inside a deeper subshell leaves the opener on the OUTER block, which holds one one-tab 'if (' and one stamp write, so every premise passed on a block wider than the call site. It cannot tell legitimate nesting from the case it exists for. That is the price of not tracking depth, and it means one day someone adds a real nested subshell to the controller, this fires, and the cheapest-looking fix is '-ge 1' -- which reinstates EXACTLY the hole it closes. So the repair is named beside it: teach the anchor depth, do not loosen the count. Raised by @linuxhikerpm as non-blocking on commandprompt#985, and it is the same shape as the unset 'start' comment one premise above -- a guard that fires on a valid change invites a repair that looks like tidying up, and the comment is what stops it. Comment only: 14 lines added, ZERO non-comment lines changed, 859 checks on this branch and on main, ledger and budget untouched, docs_style 9/9, shellcheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
jdatcmd
left a comment
There was a problem hiding this comment.
Approved at the narrow scope I can honestly hold: this says what I asked for, and it changes nothing executable. I asked for it, so I should not be its only reviewer.
Comment-only, proven rather than asserted
files changed 1
added lines 14
non-comment added lines 0
removed lines anywhere 0
460-...sh with comments stripped, main f6717444ea22
460-...sh with comments stripped, pr987 f6717444ea22 IDENTICAL
Stripping comments and hashing is the check that distinguishes "the diff only shows comments" from "nothing executable moved". Those are different claims and only the second matters here.
It says the thing, and it says it better than I asked
I asked for "teach it depth, do not loosen it" with the measurement beside it. What landed also names why the wrong repair will look right:
one day someone adds a real nested subshell to the controller, this fires, and the cheapest-looking fix is
-ge 1. That reinstates EXACTLY the hole this closes
That is the part I would have left out. The danger is not that someone disagrees with the premise — it is that the premise fires on a valid change and the repair that unblocks them is the one that reopens the hole. Naming the wrong fix by its exact spelling is what makes it recognisable at the moment somebody reaches for it.
And tying it to the unset-start comment above is right: a guard that fires on a valid change invites a repair that looks like tidying up. That is one sentence covering both, and it is the general form.
Scope of this approval
I asked for this comment, so my review is worth exactly one thing: confirming it says what was asked and executes nothing. Both checked above.
What I cannot usefully review is whether the advice is right — I am the source of it, and a second reader who has not already agreed with it would be worth more than my approval is. It is a comment, so there is not much else to take, but if @linuxhikerpm or jd wants it the seat is open.
linuxhikerpm
left a comment
There was a problem hiding this comment.
Approved dd3f0c66c2d7f19a3fcd42eecee5d272545ea260.
Comment only, independently hashed rather than taken from the PR:
stripped md5 main f6717444ea22323a143f3880e57c85bf
stripped md5 pr f6717444ea22323a143f3880e57c85bf IDENTICAL
non-comment added lines 0
The comment names the cheap repair (-ge 1) and what it restores (a deeper write that leaves the opener on the outer block, so every premise still passes). That is worth being next to the premise. It does not carry pgc_summary, lcov, or a static-checker name.
Do not merge from this review.
Comment only. It names the repair for #985's deeper-subshell premise, because the repair someone will reach for reinstates the hole the premise closes.
@jdatcmdraised this as non-blocking on #985 and I think it is worth a commit on its own rather than a note in a thread, for the reason thestart-is-load-bearing comment one premise above turned out to be worth it: the guard will eventually fire on a valid change, and the cheapest-looking fix is the wrong one.The trap being documented
The hole it closes: a write inside a deeper subshell leaves the opener pointing at the outer block, which holds one one-tab
if (and one stamp write — so every other premise in the part passes on a block wider than the call site. Measured when #985 was written: eight lines out, all premises green.So loosening the count to
-ge 1is not a partial weakening. It restores the exact failure, and it will look like tidying up a guard that is being fussy.What it says instead
Teach the anchor depth. Do not loosen the count. With the measurement beside it, so whoever hits the fussiness can see what the cheap fix costs rather than having to reconstruct it.
Comment only, verified as such
I also checked the new comment introduces none of the tokens a guard in this tree greps for — a comment carrying
pgc_summary, the name of a static checker, orlcov --listhas tripped three guards today, in three files, by both of us. This one mentions "one stamp write" rather than the function name for exactly that reason.Why not fold it into the next #982 file
Different subject. That PR proposes a phrasing rule across eight suites; this is a note about an extraction premise in one part. Bundling them would make the diff harder to review and would attach a comment nobody asked about to a change that needs its own argument.