What is wrong
applyGatewayCoverageHelp (internal/commands/gateway_coverage.go:296) walks all 1759 command nodes and calls gatewayGroupCoverageHelp on each. That calls everyLeafRefused, which re-walks that node's whole subtree. So the pass is O(n·depth) where one post-order pass would do.
Measured
Benchmarked against NewRootCmd (1759 commands):
|
ns/op |
allocs |
applyGatewayCoverageHelp |
144,000 |
838 |
| a bottom-up equivalent |
99,000 |
838 |
| bare post-order walk, 1759 nodes |
4,000 |
0 |
About 45 µs saved, roughly 45% of the walk. For honest scale: the whole NewRootCmd tree build is ~2.5 ms and 36k allocations, and process time for a completion round trip is ~36 ms with ~4 ms of run-to-run spread. This is not a user-visible win. File it as the simplification it is, not as a performance fix.
The change
Compute the tally in applyGatewayCoverageHelp's own post-order pass and delete everyLeafRefused. Each node returns {ops, refused} summed from its children plus itself; the caveat fires on ops > 0 && ops == refused.
Equivalence was checked across all 1759 nodes — ops=1438 refused=73, zero disagreements with everyLeafRefused.
Two things to get right, both found the hard way
A runnable node is an operation in its own right. everyLeafRefused decides leaf-against-group on child count alone, so a runnable parent's own verdict is never read. The tree holds exactly one runnable parent today, pro backup, and it carries no gateway annotation — so both failure directions are unreachable and nothing changes on the live tree. A generalisation was written for this in PR #346 and then reverted, because it cost 128 lines to change nothing, made this walk 22% slower, and left the test-side copy of the same walk on the old rule. The bottom-up rewrite gets it right for free, which is the reason to prefer it over that patch.
guardUnknownSubcommands makes every group parent runnable. So "runnable" only reads as "is an operation" because applyGatewayCoverageHelp runs at root.go:954 and that guard at root.go:959. That ordering is load-bearing and nothing asserts it. Either exclude a node carrying jamfcli/group-parent, or assert the order.
Also fix the duplicate while in there
refusedLeafCount (internal/commands/gateway_coverage_test.go:686) is a second copy of the same walk, backing TestGatewaySuccessorsNameCommandsTheBinaryShips. It should call the production walk rather than restate its rule, or the two drift the moment markGatewayCoverage stamps a runnable parent — at which point the successor test would report a refused parent as served and advise deleting a correct entry.
Found during a /simplify pass over #346, #359 and #360.
What is wrong
applyGatewayCoverageHelp(internal/commands/gateway_coverage.go:296) walks all 1759 command nodes and callsgatewayGroupCoverageHelpon each. That callseveryLeafRefused, which re-walks that node's whole subtree. So the pass is O(n·depth) where one post-order pass would do.Measured
Benchmarked against
NewRootCmd(1759 commands):applyGatewayCoverageHelpAbout 45 µs saved, roughly 45% of the walk. For honest scale: the whole
NewRootCmdtree build is ~2.5 ms and 36k allocations, and process time for a completion round trip is ~36 ms with ~4 ms of run-to-run spread. This is not a user-visible win. File it as the simplification it is, not as a performance fix.The change
Compute the tally in
applyGatewayCoverageHelp's own post-order pass and deleteeveryLeafRefused. Each node returns{ops, refused}summed from its children plus itself; the caveat fires onops > 0 && ops == refused.Equivalence was checked across all 1759 nodes —
ops=1438 refused=73, zero disagreements witheveryLeafRefused.Two things to get right, both found the hard way
A runnable node is an operation in its own right.
everyLeafRefuseddecides leaf-against-group on child count alone, so a runnable parent's own verdict is never read. The tree holds exactly one runnable parent today,pro backup, and it carries no gateway annotation — so both failure directions are unreachable and nothing changes on the live tree. A generalisation was written for this in PR #346 and then reverted, because it cost 128 lines to change nothing, made this walk 22% slower, and left the test-side copy of the same walk on the old rule. The bottom-up rewrite gets it right for free, which is the reason to prefer it over that patch.guardUnknownSubcommandsmakes every group parent runnable. So "runnable" only reads as "is an operation" becauseapplyGatewayCoverageHelpruns atroot.go:954and that guard atroot.go:959. That ordering is load-bearing and nothing asserts it. Either exclude a node carryingjamfcli/group-parent, or assert the order.Also fix the duplicate while in there
refusedLeafCount(internal/commands/gateway_coverage_test.go:686) is a second copy of the same walk, backingTestGatewaySuccessorsNameCommandsTheBinaryShips. It should call the production walk rather than restate its rule, or the two drift the momentmarkGatewayCoveragestamps a runnable parent — at which point the successor test would report a refused parent as served and advise deleting a correct entry.Found during a
/simplifypass over #346, #359 and #360.