Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions .githooks/validate-spdx.sh
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,21 @@

[ -z "$FILES_TO_CHECK" ] && exit 0

for file in $FILES_TO_CHECK; do
# ⚠ NEWLINE-DELIMITED, NOT WORD-SPLIT. This was `for file in $FILES_TO_CHECK`,
# which splits on $IFS — so a path containing a space became two paths, each
# of which then failed `[ -f "$file" ]` and was skipped by the `continue`
# below. Silently: no error, and the file never counted toward $CHECKED, so
# the denominator under-reported too.
#
# That is not hypothetical here. The estate contains a literal-space directory
# `_RSR _SET`, so EVERY source file beneath it passed this validator without
# ever being read. A validator that skips what it cannot name is worse than no
# validator, because it reports a pass.
#
# `<<<` keeps the loop in the CURRENT shell; a `... | while read` pipeline
# would run it in a subshell and discard $ERRORS and $CHECKED.
while IFS= read -r file; do
[ -n "$file" ] || continue

Check failure on line 68 in .githooks/validate-spdx.sh

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.

See more on https://sonarcloud.io/project/issues?id=hyperpolymath_standards&issues=AaDIruCrPaL7ZnFUuWiH&open=AaDIruCrPaL7ZnFUuWiH&pullRequest=961
[ -f "$file" ] || continue
is_source_file "$file" || continue

Expand Down Expand Up @@ -82,7 +96,7 @@
echo "[validate-spdx] ERROR: $file missing SPDX header" >&2
ERRORS=$((ERRORS + 1))
fi
done
done <<< "$FILES_TO_CHECK"

# Always print the denominator: "0 errors" out of 0 files examined is a
# vacuous pass, and it must not read the same as a real one.
Expand Down
62 changes: 56 additions & 6 deletions scripts/tests/validate-spdx-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@
#
# Tests for .githooks/validate-spdx.sh, the pre-commit gate.
#
# ⚠ TEST 1 IS THE REASON THIS EXISTS, and it FAILS against the previous version.
# ⚠ TEST 1 IS THE REASON THIS EXISTS, and it FAILS against the previous version.
#
# The validator had two modes asking two different questions. Full-scan mode
# filtered by source extension inside `find`; staged mode did not filter at all:
#
# FILES_TO_CHECK=$STAGED_FILES # every staged path, whatever it is
#
# Only the staged branch is reachable in practice — this validator is invoked by
# .githooks/pre-commit and by no workflow — so the mode that HAD the filter never
# Only the staged branch is reachable in practice — this validator is invoked by
# .githooks/pre-commit and by no workflow — so the mode that HAD the filter never
# ran, and the mode that ran had none.
#
# The casualty was .github/workflows/actions.lock: machine-generated by
Expand All @@ -30,7 +30,38 @@
mkdir -p "$T/.github/workflows"
pass=0; fail=0

ck() { # name expected_exit staged_files

# ⚠ STAGED_FILES IS NEWLINE-DELIMITED, and these tests must say so.
#
# The real and only caller is .githooks/pre-commit, which builds the list with
# `git diff --cached --name-only --diff-filter=ACM` — one path per LINE. This
# suite used to pass multi-file cases SPACE-separated ("good.sh bad.sh"), which
# the validator only ever handled because its loop was an unquoted
# `for file in $FILES_TO_CHECK` that split on $IFS.
#
# So the suite encoded the BUG as its contract. It asked a different question
# than the consumer, and that is precisely why it could never catch #912: a
# path containing a space became two paths, each failed `[ -f ]`, and each was
# silently skipped — not counted, not reported. Every source file under the
# estate's literal-space `_RSR _SET` directory passed without being read.
#
# Multi-file cases therefore use $'a\nb', matching the caller exactly, and the
# SPACE IN A PATH controls below are the regression proof.
# Asserts the DENOMINATOR, not just the exit code. A skipped file and a checked
# file both exit 0; only the count tells them apart, which is the whole of #912.
ck_count() { # name expected_exit expected_checked staged_files

Check warning on line 52 in scripts/tests/validate-spdx-test.sh

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Add an explicit return statement at the end of the function.

See more on https://sonarcloud.io/project/issues?id=hyperpolymath_standards&issues=AaDIruOCPaL7ZnFUuWiM&open=AaDIruOCPaL7ZnFUuWiM&pullRequest=961
local out rc n
out="$(cd "$T" && INPUT_STAGED_FILES="$4" bash "$HOOK" 2>&1)"; rc=$?
n="$(printf '%s' "$out" | grep -oE '[0-9]+ (of [0-9]+ )?source files' | grep -oE '^[0-9]+|of [0-9]+' | tail -1 | grep -oE '[0-9]+')"
if [ "$rc" = "$2" ] && [ "${n:-}" = "$3" ]; then

Check warning on line 56 in scripts/tests/validate-spdx-test.sh

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Assign this positional parameter to a local variable.

See more on https://sonarcloud.io/project/issues?id=hyperpolymath_standards&issues=AaDIruOCPaL7ZnFUuWiL&open=AaDIruOCPaL7ZnFUuWiL&pullRequest=961

Check failure on line 56 in scripts/tests/validate-spdx-test.sh

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.

See more on https://sonarcloud.io/project/issues?id=hyperpolymath_standards&issues=AaDIruOCPaL7ZnFUuWiK&open=AaDIruOCPaL7ZnFUuWiK&pullRequest=961

Check failure on line 56 in scripts/tests/validate-spdx-test.sh

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.

See more on https://sonarcloud.io/project/issues?id=hyperpolymath_standards&issues=AaDIruOCPaL7ZnFUuWiI&open=AaDIruOCPaL7ZnFUuWiI&pullRequest=961

Check warning on line 56 in scripts/tests/validate-spdx-test.sh

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Assign this positional parameter to a local variable.

See more on https://sonarcloud.io/project/issues?id=hyperpolymath_standards&issues=AaDIruOCPaL7ZnFUuWiJ&open=AaDIruOCPaL7ZnFUuWiJ&pullRequest=961
printf ' ok %s (exit %s, checked %s)\n' "$1" "$rc" "$n"; pass=$((pass+1))
else
printf ' FAIL %s (expected exit %s checked %s, got exit %s checked %s) output=%s\n' \
"$1" "$2" "$3" "$rc" "${n:-<none>}" "${out:-<none>}"; fail=$((fail+1))
fi
}

ck() { # name expected_exit staged_files (NEWLINE-delimited)

Check warning on line 64 in scripts/tests/validate-spdx-test.sh

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Add an explicit return statement at the end of the function.

See more on https://sonarcloud.io/project/issues?id=hyperpolymath_standards&issues=AaDIruOCPaL7ZnFUuWiN&open=AaDIruOCPaL7ZnFUuWiN&pullRequest=961
local out rc
out="$(cd "$T" && INPUT_STAGED_FILES="$3" bash "$HOOK" 2>&1)"; rc=$?
if [ "$rc" = "$2" ]; then printf ' ok %s (exit %s)\n' "$1" "$rc"; pass=$((pass+1))
Expand All @@ -54,11 +85,11 @@

echo "validate-spdx.sh"
ck "PLANTED POSITIVE: headerless actions.lock must PASS" 0 ".github/workflows/actions.lock"
ck "negative control: headerless .sh must FAIL" 1 "good.sh bad.sh"
ck "negative control: headerless .sh must FAIL" 1 $'good.sh\nbad.sh'
ck "negative control: headerless .yml must FAIL" 1 ".github/workflows/bad.yml"
ck "valid header must PASS" 0 "good.sh"
ck "non-source README.md must PASS" 0 "README.md"
ck "lockfile alongside a valid source file must PASS" 0 ".github/workflows/actions.lock good.sh"
ck "lockfile alongside a valid source file must PASS" 0 $'.github/workflows/actions.lock\ngood.sh'

# PARITY: both modes must agree about the same tree. A tree whose only headerless
# files are non-source must pass a full scan exactly as it passes staged mode.
Expand All @@ -85,5 +116,24 @@
ck "trailing junk after expression must FAIL" 1 "junk.sh"
ck "OCaml (* *) terminator must PASS" 0 "good.ml"

# SPACE IN A PATH — the #912 regression. The estate contains a literal-space
# directory `_RSR _SET`, so this is a real path shape, not a contrived one.
#
# Under the old word-splitting loop `_RSR _SET/bad.sh` became TWO words, each
# failed `[ -f "$file" ]`, and each was silently `continue`d: no error, no
# increment of $CHECKED, exit 0. The validator reported a pass on a file it had
# never opened. Both directions are asserted, because only the pair distinguishes
# "read it and it was fine" from "never read it":
# - headerless under a space dir must FAIL (kills the mutant on rc)
# - headered under a space dir must PASS *and count 1* (kills it on the denominator)
mkdir -p "$T/_RSR _SET"
printf 'echo bad\n' > "$T/_RSR _SET/bad.sh"
printf '# SPDX-License-Identifier: MPL-2.0\necho ok\n' > "$T/_RSR _SET/good.sh"
ck "SPACE IN PATH: headerless under a space dir must FAIL" 1 "_RSR _SET/bad.sh"
ck_count "SPACE IN PATH: headered under a space dir must PASS" 0 1 "_RSR _SET/good.sh"
ck "SPACE IN PATH: mixed list, one headerless, must FAIL" 1 "$(printf '%s\n%s' '_RSR _SET/good.sh' '_RSR _SET/bad.sh')"
ck_scan "SPACE IN PATH: full-scan mode must see into a space dir" 1 "_RSR _SET"


printf '\n%s passed, %s failed\n' "$pass" "$fail"
[ "$fail" -eq 0 ] || exit 1
Loading