Skip to content

docs: the deeper-subshell premise's repair is depth, not a looser count (#961) - #987

Merged
jdatcmd merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:docs/460-the-premise-repair-is-depth-not-looser
Sep 12, 2026
Merged

docs: the deeper-subshell premise's repair is depth, not a looser count (#961)#987
jdatcmd merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:docs/460-the-premise-repair-is-depth-not-looser

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

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.

@jdatcmd raised 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 the start-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

premise: exactly one `if (` at any indent in the extracted block

  refuses the nested-orphan case      <- why it exists
  refuses legitimate nesting too      <- it cannot tell them apart
  the repair someone reaches for:  -ge 1
  which reinstates the nested-orphan case EXACTLY

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 1 is 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

14 lines added
 0 non-comment lines changed
859 checks on this branch, 859 on main
ledger and budget untouched (so no census movement, no regeneration)
docs_style  rc=0  0 FAIL
shellcheck  clean

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, or lcov --list has 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.

… 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 jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 linuxhikerpm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jdatcmd
jdatcmd merged commit 827f280 into commandprompt:main Sep 12, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants