Merge branch 'bash-hygiene'
commit 4e822d05d71f12dc35e3de0cdd703d8b17e05c8e · parent 1fcf6f35d8 · parent ee3f738e3a
· browse this tree
warpfork, 2026-10-24 00:00:29 UTC
diff --git a/lib/testmark.bash b/lib/testmark.bash index 5e80e44..f5789b5 100644 --- a/lib/testmark.bash +++ b/lib/testmark.bash @@ -18,10 +18,12 @@ source "${BASH_SOURCE[0]%/*}/testmark/document.bash" ## Runs an implementation function in a known environment, whatever the caller's settings. ## ## - `LC_ALL=C`: string operations work on bytes, and `[[:space:]]` is exactly ASCII whitespace. +## - `set -u`: a library bug that reads an unset variable exits the script loudly. `local -` restores the caller's setting. ## - `nocasematch` off: pattern matches are exact. It's restored afterwards, since `local -` doesn't cover shopts. ## - The `||` list: inside it, the caller's `set -e` and ERR trap can't fire on the implementation's ordinary nonzero statuses. _testmark_call() { - local LC_ALL=C _testmark_st=0 _testmark_nocase= + local - LC_ALL=C _testmark_st=0 _testmark_nocase= + set -u case ":$BASHOPTS:" in *:nocasematch:*) _testmark_nocase=1 && shopt -u nocasematch ;; esac "$@" || _testmark_st=$? [[ -z $_testmark_nocase ]] || shopt -s nocasematch diff --git a/lib/testmark/document.bash b/lib/testmark/document.bash index d6f8899..2fe6e98 100644 --- a/lib/testmark/document.bash +++ b/lib/testmark/document.bash @@ -10,8 +10,7 @@ # pid $BASHPID at parse, for refusing state changes from a subshell # So forgetting a handle is one `unset` per kind, and nothing else needs cleaning up. # Variable names are built only from handles that pass _testmark_handle, and kinds have no "_", so two handles' variables never meet. -# Hunk names come from documents and may contain shell syntax: on some bash versions, arithmetic, `[[ -v ]]`, `unset`, and `printf -v` on an element run `$(...)` from inside its key. -# So touch name-keyed elements only as `a[$k]=v`, `"${a[$k]}"`, `"${a[$k]-}"`, `${a[$k]+set}`, and `"${!a[@]}"`. +# Hunk names come from documents and may contain shell syntax, so name-keyed elements are touched only as "Using assoc arrays safely" in docs/hacking/bash-pitfalls.md allows. _testmark_kinds=(lines start end info names pid) _testmark_err() { @@ -20,7 +19,7 @@ _testmark_err() { ## Fails with a message unless $1 is a valid handle. _testmark_handle() { - [[ $1 =~ ^[A-Za-z0-9_]+$ ]] || { + [[ -n $1 && $1 != *[!A-Za-z0-9_]* ]] || { _testmark_err "invalid handle \"$1\": must match [A-Za-z0-9_]+" return 1 } @@ -34,14 +33,11 @@ _testmark_parse() { local h=$1 file=$2 kind _testmark_handle "$h" || return 1 for kind in "${_testmark_kinds[@]}"; do unset -v "_testmark_${kind}_$h"; done - # A directory would read as zero lines, so it's rejected here. - [[ -f $file && -r $file ]] || { - _testmark_err "$file: not a readable file" - return 1 - } local -a lines - mapfile lines <"$file" || { - _testmark_err "$file: read failed" + # See "use blocks to open an fd and check properties on it without TOCTOU" in docs/hacking/bash-pitfalls.md. + # The 2>/dev/null comes before the <, so it also hides bash's message when the open fails; ours is printed instead. + { [[ ! -d /dev/stdin ]] && mapfile lines; } 2>/dev/null <"$file" || { + _testmark_err "$file: not a readable file" return 1 } @@ -50,7 +46,7 @@ _testmark_parse() { local i line name names='' open='' k='' at='' for ((i = 0; i < ${#lines[@]}; i++)); do line=${lines[i]} - # mapfile ends a line at a NUL, dropping its "\n". So a line without one is the last line, or has a NUL; read -d '' tells which. + # A line without its "\n" is the last line, or was cut at a NUL; read -d '' tells which. See "NUL bytes vanish unless you check for them" in docs/hacking/bash-pitfalls.md. if [[ $line != *$'\n' ]] && IFS= read -r -d '' _ <"$file"; then _testmark_err "$file:$((i + 1)): contains a NUL byte" return 1 @@ -83,11 +79,14 @@ _testmark_parse() { elif [[ $line == '[testmark]:#'* ]]; then name=${line#'[testmark]:# ('} name=${name%')'} - if [[ $line != '[testmark]:# ('*')' || ! $name =~ ^[[:space:]]*([^[:space:]]+) ]]; then + # The name is the first word: skip leading whitespace, then stop at the next. + name=${name#"${name%%[![:space:]]*}"} + name=${name%%[[:space:]]*} + if [[ $line != '[testmark]:# ('*')' || -z $name ]]; then _testmark_err "$file:$((i + 1)): malformed annotation; want \"[testmark]:# (name)\"" return 1 fi - k=${BASH_REMATCH[1]} + k=$name [[ -z ${start[$k]+set} ]] || { _testmark_err "$file:$((i + 1)): duplicate hunk name \"$k\"" return 1 @@ -106,7 +105,7 @@ _testmark_parse() { } # Parsed without error; only now are globals written. - # Assoc kinds are declared first, since a nameref to an undeclared name makes an indexed array. + # Assoc kinds are declared first; see "assoc arrays must be declared as such" in docs/hacking/bash-pitfalls.md. declare -gA "_testmark_start_$h" "_testmark_end_$h" "_testmark_info_$h" local -n g_lines=_testmark_lines_$h g_start=_testmark_start_$h g_end=_testmark_end_$h g_info=_testmark_info_$h local -n g_names=_testmark_names_$h g_pid=_testmark_pid_$h @@ -152,7 +151,7 @@ _testmark_body() { _testmark_hunk "$1" "$2" || return 1 local -n lines=_testmark_lines_$1 start=_testmark_start_$1 end=_testmark_end_$1 local i=${start[$2]} last=${end[$2]} body= - for (( ; i < last; i++)); do body+=${lines[i]}; done + for (( ; i < last; i++)); do body+=${lines[i]}; done # not a slice; see "Quadratic operations" in docs/hacking/bash-pitfalls.md printf '%s' "$body" } diff --git a/tests/test-caller-env.sh b/tests/test-caller-env.sh index 1d6e317..d69f237 100755 --- a/tests/test-caller-env.sh +++ b/tests/test-caller-env.sh @@ -28,4 +28,11 @@ testmark_body doc nope 2>/dev/null || rc=$? expect_eq "errexit: an error returns, to a caller that checks" "$rc" 1 expect_eq "the ERR trap never fired" "$err_trap_fired" 0 +# The library turns on set -u for its own code only. +set +u +before=$- +testmark_body doc x >/dev/null +expect_eq "set -u: a caller without it still lacks it after a call" "$-" "$before" +set -u + exit "$RESULT"