Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Address the directive-matching issues and add the missing stability-test regeneration rule.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates Argbash to preserve square brackets in user code before the first macro.
Changes:
- Adds quadrigraph protection for leading code and macro scans.
- Adds regression and stability tests.
- Updates documentation and release metadata.
File summaries
| File | Summary | Final review comments |
|---|---|---|
tests/regressiontests/test-leading-code.m4 |
Leading-code regression fixture | None |
tests/regressiontests/Makefile |
Test targets | None |
tests/regressiontests/make/tests/tests-base.m4 |
Test definitions | Critical (1 vote): Missing regeneration rule for test-leading-code2.sh. |
src/argbash.m4 |
Core protection logic | Moderate (3 votes): Restrict directive matching to actual prefixes. |
doc/usage.rst |
Template behavior documentation | None |
doc/index.rst |
Limitation updates | None |
ChangeLog |
Release note | None |
bin/argbash |
Generated executable update | Moderate (3 votes): Restrict directive matching to actual prefixes. |
AUTHORS |
Contributor attribution | None |
Review details
- Files reviewed: 8/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| grep -q '^_before=("\[one\]" "\[two\]")$$' $< | ||
| ]]) | ||
|
|
||
| ADD_SCRIPT([test-leading-code2]) |
There was a problem hiding this comment.
The file is produced by the generic %2.sh: %.sh pattern rule in resources/Makefile, which is how the existing stability test gets basic2.sh as well, so a clean build works (the suite runs green with this test). No change needed.
| protect_leading_user_code() | ||
| { | ||
| awk ' | ||
| !seen_directive && /^#[[:space:]]*(ARG|DEFINE_|INCLUDE_)/ { seen_directive = 1 } |
There was a problem hiding this comment.
Addressed: the detector now matches only the real macro prefixes (ARG_, ARGBASH_, DEFINE_, INCLUDE_), and the test template gained an # ARGUMENTS comment line before its bracketed code.
Square brackets are m4 quotes, so one level of them was stripped from
whatever user code preceded the first Argbash macro: "${arr[@]}" became
"${arr@}", "[[ -n $x ]]" became "[ -n $x ]", and every regeneration
made it worse.
The argbash script now escapes square brackets on the lines that precede
the first Argbash macro using m4sugar quadrigraphs, which autom4te turns
back into square brackets at the end of the run, so that code comes out
unchanged. Lines that contain m4 macros (and lines inside m4 quotes those
lines open) are left alone, so constructs such as the m4_ignore of
argbash-init templates keep working.
Fixes matejak#208
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017DxhC3HmTM7TQUzkBZnyH2
e074ad3 to
b68175f
Compare
Fixes #208
Square brackets are m4 quotes, so one level of them was stripped from whatever user code preceded the first Argbash macro:
"${arr[@]}"became"${arr@}",[[ -n $x ]]became[ -n $x ], and every regeneration made it worse.The
argbashscript now escapes square brackets on the lines that precede the first Argbash macro using m4sugar quadrigraphs (@<:@,@:>@), which autom4te turns back into square brackets at the end of the run, so that code comes out unchanged and does not have to have balanced brackets. Lines that contain m4 macros (and lines inside m4 quotes those lines open) are left alone, so constructs such as them4_ignoreofargbash-inittemplates and them4_definetraps of the test templates keep working.argbashscans the template forARGBASH_WRAPandINCLUDE_PARSING_CODEarguments.test-leading-codeandstability-leading-code.Tested with
make check(the only failures are the pre-existing shellcheck SC2154 ones in the wrapping tests that #207 addresses).🤖 Generated with Claude Code
https://claude.ai/code/session_017DxhC3HmTM7TQUzkBZnyH2