nodogsplash: minor cleanup - #30287
Conversation
|
@BKPepe what do you think? |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit. All three changes check out:
IPT/WD_DIRare genuinely dead — no other reference anywhere undernet/nodogsplash/.&>→>/dev/null 2>&1inrestart.shis a real fix, not just style: withoutASH_BASH_COMPATthe old form parses asndsctl status &plus a bare> /dev/null, so theifwould always take the true branch.>&2on the unsupported-option warning matches how every other diagnostic in the init script is already routed.PKG_RELEASEbump to 2 is correct (scripts changed,PKG_VERSIONunchanged).
One optional nit inline; nothing here blocks a merge.
Generated by Claude Code
| config_get val "$cfg" "$option" | ||
| if [ -n "$val" ]; then | ||
| echo "Warning: nodogsplash does not support $option" | ||
| echo "Warning: nodogsplash does not support $option" >&2 |
There was a problem hiding this comment.
nit (optional, doesn't block): one bashism of the same class survives this cleanup — uci_name=${rule//-/_} at nodogsplash:77. Like &>, ${var//pat/repl} is a bash extension that busybox ash only provides via BUSYBOX_CONFIG_ASH_BASH_COMPAT (default y, but selectable off), so it silently fails on a trimmed build. POSIX equivalent: uci_name=$(echo "$rule" | tr - _). Worth folding into this commit while you're in the file, if you agree.
Generated by Claude Code
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed at 239187f. The only change since 1c2cbf4 is uci_name=${rule//-/_} → uci_name=$(echo "$rule" | tr - _), which is exactly the follow-up suggested on the previous round — verified equivalent, and tr is enabled by default in busybox (BUSYBOX_DEFAULT_TR default y), so nothing new is pulled in. PKG_RELEASE:=2 still covers the whole set of script changes.
CI on this head is green or still running (FormalityCheck passed, package builds in progress), so nothing to chase there.
Commit checks
- 239187f "nodogsplash: minor cleanup" — the diff matches the message; only a typo in the body (
bashismi,). Details inline.
Nothing here blocks a merge.
Generated by Claude Code
| for rule in authenticated-users preauthenticated-users users-to-router trusted-users trusted-users-to-router; do | ||
| # uci does not allow dashes | ||
| uci_name=${rule//-/_} | ||
| uci_name=$(echo "$rule" | tr - _) |
There was a problem hiding this comment.
nit (optional, doesn't block): the change itself is correct — same semantics as ${rule//-/_}, and a lone - is treated as an operand rather than an option by both GNU and busybox tr, so tr - _ is safe. tr is BUSYBOX_DEFAULT_TR=y, so no new dependency either.
Two text mismatches around it though, both worth a quick amend since this is the only commit:
- The commit body reads
replace bashismi, &> redirect and shell substitution—bashismi,looks like a typo forbashisms:. - The PR description still lists only
replace bashism &>and doesn't mention this second bashism removal.
Generated by Claude Code
A few minor cleanup changes: - redirect warning message that causes abort to stderr - replace bashism: &> redirect and shell substitution - remove unused variables IPT and WD_DIR Signed-off-by: Moritz Warning <moritzwarning@web.de>
2 participants
A few minor cleanup changes:
📦 Package Details
Maintainer: @mwarning
Description:
Followup from #30169
🧪 Run Testing Details
Not tested yet!
✅ Formalities
If your PR contains a patch: