From ac97b16f8db22414ef31a505936349b8d907512c Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 22:31:50 +0000 Subject: [PATCH 1/3] test: a count grep never produced is not "present" (#929) #922 replaced roughly 28 `producer | grep -q PAT` tests with `[ "$(grep -c PAT ... || true)" != 0 ]`, which fixed a real EPIPE race (#486). The replacement answers PRESENT where the original answered ABSENT whenever grep produces no stdout, and a pattern that does not compile is the way to get there: grep -cE '[' file -> stdout is [] (empty, not "0") [ "" != 0 ] -> TRUE (a STRING comparison: "" is not "0") So the test reported the pattern present for a question it never managed to ask. NOTHING IS BROKEN TODAY -- every pattern in the tree is valid, so no site was wrong. The hazard is the DIRECTION of the next edit. A premise arm phrased to want `present`, and most are because a premise asserts the fixture really is in the state the test needs, turns GREEN when its pattern stops compiling. It passes BECAUSE the instrument broke, which is the failure this harness spends most of its effort refusing elsewhere. The old form failed red and loud. THE FIX IS A NUMERIC COMPARISON, and it is behaviour-preserving in every case that is not broken. Measured before touching a single site: input [ "$n" != 0 ] [ "$n" -ne 0 ] stderr a real count: 0 false false no a real count: 3 true true no EMPTY (grep usage error) TRUE false YES MEASURED POPULATION, AND IT WAS NOT THE NUMBER I FILED. The issue said 18 sites; the tree holds 58 matching `[ "$(grep -c`, of which **20** compare as a STRING and 29 and 3 compare numerically and were never affected. Both string spellings fail the same way: `= 0` is an ABSENCE claim, and on empty it is false, which does not assert absence -- the safe direction once it is loud. ALL 20 PASS EXACTLY ONE INPUT TO GREP, checked rather than assumed: 17 here-strings and 3 single files. That matters because `grep -c` over several files prints `file:count` lines, which a numeric comparison would reject where the string form tolerated it. No site does that, so the conversion cannot introduce a new failure. THE CLASS IS CLOSED, NOT THE 20 INSTANCES. `test/selftest/420-a-count-grep-never-produced.sh` sweeps for a string comparison on a `grep -c` and requires zero. It is heredoc-aware for the reason part 410's exit-0 sweep is -- the suites generate fixture scripts -- and it skips comments too. AND IT FLAGGED ITSELF FIRST. My initial version spelled the forbidden idiom inside a `printf` to plant it, so three of its own lines were offences and the arm reported 23 where the tree holds 20. The planted shapes are now ASSEMBLED, with the operator passed as an argument: the regex needs `!=` or `=` directly after the closing `)"`, and `%s` there is neither. A sweep flagging its own fixtures is the exact trap its own comment cites, arriving in the comment that cites it. A flat grep still counts 21 against the sweep's 20: the extra is the paragraph in this part that documents the idiom. That is why the sweep skips comments, and the difference is the guard not counting its own explanation. Gate: harness_selftest 601 checks, 601 passed + 0 failed + 0 unrunnable, PASSED pytest corpus 253 passed with a cluster on pg18a (lib.sh changed, and arms read it) native_groupagg.sh 72 checks, 72 passed, PASSED -- one converted suite end to end shellcheck -S error over test/*.sh and test/selftest/*.sh rc=0 every touched file re-parsed with bash -n WHAT I COULD NOT REPRODUCE, recorded in the issue and still true: an unreadable file does NOT trigger the inversion. `grep -c` on a file it cannot read prints `0`, so that case answers absent and agrees with the old form. The reachable trigger is a pattern that does not compile. My first attempt at that test was invalid anyway -- I ran it as root, which bypasses the mode bits. Fixes #929. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 40 ++++++++ test/arrow_import.sh | 2 +- test/concurrency.sh | 2 +- test/lib.sh | 4 +- test/native_groupagg.sh | 2 +- test/native_groupagg_batch.sh | 2 +- test/objstore_s3_read.sh | 2 +- test/parquet_import.sh | 2 +- test/run_all_versions.sh | 4 +- .../070-and-comm-s-two-inputs-must.sh | 4 +- .../080-no-suite-pipes-a-captured-string.sh | 2 +- .../300-a-test-script-must-be-runnable.sh | 2 +- .../320-a-check-that-could-not-run.sh | 2 +- .../340-the-binary-must-be-built-from.sh | 2 +- .../selftest/350-the-pytest-corpus-must-be.sh | 4 +- .../420-a-count-grep-never-produced.sh | 91 +++++++++++++++++++ test/unique_conc.sh | 2 +- test/update_conc.sh | 2 +- 18 files changed, 151 insertions(+), 20 deletions(-) create mode 100644 test/selftest/420-a-count-grep-never-produced.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a4ab100..89ad737b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -335,6 +335,46 @@ true until the next version shipped. ### Fixed +- A count `grep` never produced no longer reads as "present" (#929). + + #922 replaced roughly 28 `producer | grep -q PAT` tests with + `[ "$(grep -c PAT ... || true)" != 0 ]`, which fixed a real EPIPE race (#486). The + replacement answered **present** where the original answered **absent** whenever grep + produced no stdout, and a pattern that does not compile is the way to get there: + + grep -cE '[' file -> stdout is [] (empty, not "0") + [ "" != 0 ] -> TRUE (a STRING comparison: "" is not "0") + + So the test reported the pattern present for a question it never managed to ask. + + Every pattern in the tree is valid today, so no site was wrong. The hazard is the + DIRECTION of the next edit: a premise arm phrased to want `present` -- and most are, + because a premise asserts the fixture really is in the state the test needs -- turns + GREEN when its pattern stops compiling. It passes BECAUSE the instrument broke, which + is the failure this harness spends most of its effort refusing. The old form failed + red. + + **20 sites** compared a count as a string; they now compare numerically, which is + behaviour-preserving in every case that is not broken. Measured: + + input [ "$n" != 0 ] [ "$n" -ne 0 ] stderr + a real count: 0 false false no + a real count: 3 true true no + EMPTY (grep usage error) TRUE false YES + + Both spellings failed the same way. `= 0` is an ABSENCE claim, and on an empty value + it is false -- which does not assert absence, and is the safe direction once it is + loud. All 20 sites pass exactly one input to grep, so the value is always a bare + number and a numeric comparison cannot be confused by `file:count` output. + + The other 29 sites already compared numerically and were never affected. + + `test/selftest/420-a-count-grep-never-produced.sh` holds the arms and a heredoc-aware + sweep requiring zero string comparisons on a `grep -c`, so the class is closed rather + than the 20 instances. The sweep skips comments as well as heredocs: a flat grep + counts the paragraph that documents the idiom, which is how a guard comes to flag its + own explanation. + - The vacuity guard's PLACEMENT is now a checked property, because a guard in a teardown cannot fail the test it guards (#432). diff --git a/test/arrow_import.sh b/test/arrow_import.sh index b38afad0..7fb4f1eb 100755 --- a/test/arrow_import.sh +++ b/test/arrow_import.sh @@ -584,7 +584,7 @@ idx_plan_is_index_scan() { local _plan _plan="$(q "$IDX_SETUP EXPLAIN (COSTS OFF) SELECT count(*) FROM ix_tgt WHERE id BETWEEN 100 AND 199;")" - [ "$(grep -ci 'Index.*Scan' <<<"$_plan" || true)" != 0 ] && echo yes || echo no + [ "$(grep -ci 'Index.*Scan' <<<"$_plan" || true)" -ne 0 ] && echo yes || echo no } seq_count() { # force a sequential scan q "SET enable_indexscan = off; SET enable_bitmapscan = off; diff --git a/test/concurrency.sh b/test/concurrency.sh index 1bd2848c..74561727 100755 --- a/test/concurrency.sh +++ b/test/concurrency.sh @@ -71,7 +71,7 @@ port_is_free() { # port -> 0 if nothing is listening on it # EPIPE here answers "nothing is listening" for a port that IS taken, # and the suite then starts a cluster on an occupied port. _pif="$(ss -Htln "sport = :$1" 2>/dev/null || true)" - [ "$(grep -c ":$1" <<<"$_pif" || true)" = 0 ] + [ "$(grep -c ":$1" <<<"$_pif" || true)" -eq 0 ] else # fall back to a connect probe: a refused connection means free ! (exec 3<>"/dev/tcp/127.0.0.1/$1") 2>/dev/null diff --git a/test/lib.sh b/test/lib.sh index 8af23700..0edd36df 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -1258,7 +1258,7 @@ pgc_is_columnar_scan() { # query -> yes|no local _plan _plan="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ -d "$PGC_DB" -At -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null)" - [ "$(grep -c 'Columnar Projected Columns' <<<"$_plan" || true)" != 0 ] \ + [ "$(grep -c 'Columnar Projected Columns' <<<"$_plan" || true)" -ne 0 ] \ && echo yes || echo no } @@ -1290,7 +1290,7 @@ pgc_uses_row_fetch() { # setup query -> yes|no local _plan _plan="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ -d "$PGC_DB" -At -c "$1" -c "EXPLAIN (COSTS OFF) $2" 2>/dev/null)" - [ "$(grep -c 'Index Scan using' <<<"$_plan" || true)" != 0 ] \ + [ "$(grep -c 'Index Scan using' <<<"$_plan" || true)" -ne 0 ] \ && echo yes || echo no } diff --git a/test/native_groupagg.sh b/test/native_groupagg.sh index 85b3dbbf..6d39ecec 100755 --- a/test/native_groupagg.sh +++ b/test/native_groupagg.sh @@ -50,7 +50,7 @@ pgc_is_groupvec() { # query -> yes|no local _plan _plan="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ -d "$PGC_DB" -At -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null)" - [ "$(grep -c 'Columnar Vectorized Group Keys' <<<"$_plan" || true)" != 0 ] \ + [ "$(grep -c 'Columnar Vectorized Group Keys' <<<"$_plan" || true)" -ne 0 ] \ && echo yes || echo no } diff --git a/test/native_groupagg_batch.sh b/test/native_groupagg_batch.sh index 44662439..bc4642e8 100755 --- a/test/native_groupagg_batch.sh +++ b/test/native_groupagg_batch.sh @@ -63,7 +63,7 @@ is_groupvec() { # query -> yes|no # pgc_is_columnar_scan for the mechanism and the measurement. local _plan _plan="$(q "EXPLAIN (COSTS OFF) $1")" - [ "$(grep -c 'Columnar Vectorized Group Keys' <<<"$_plan" || true)" != 0 ] \ + [ "$(grep -c 'Columnar Vectorized Group Keys' <<<"$_plan" || true)" -ne 0 ] \ && echo yes || echo no } diff --git a/test/objstore_s3_read.sh b/test/objstore_s3_read.sh index 9753f49e..0228bce0 100755 --- a/test/objstore_s3_read.sh +++ b/test/objstore_s3_read.sh @@ -168,7 +168,7 @@ pg_restart_env "AWS_ENDPOINT_URL='https://127.0.0.1:$S3_PORT'" \ MOD_SO="$(pgc_pg "$PGC_BINDIR/pg_config --pkglibdir" | tail -1)/pgcolumnar_objstore.so" # grep -c, not grep -q; see lib.sh's pgc_is_columnar_scan. _ldd_out="$(pgc_pg "ldd '$MOD_SO'" 2>/dev/null || true)" -if [ "$(grep -c libssl <<<"$_ldd_out" || true)" != 0 ]; then +if [ "$(grep -c libssl <<<"$_ldd_out" || true)" -ne 0 ]; then HTTPS_WANT="08006" else HTTPS_WANT="0A000" diff --git a/test/parquet_import.sh b/test/parquet_import.sh index aed0fb3d..1429e1df 100755 --- a/test/parquet_import.sh +++ b/test/parquet_import.sh @@ -172,7 +172,7 @@ idx_plan_is_index_scan() { local _plan _plan="$(q "$IDX_SETUP EXPLAIN (COSTS OFF) SELECT count(*) FROM ix_tgt WHERE id BETWEEN 100 AND 199;")" - [ "$(grep -ci 'Index.*Scan' <<<"$_plan" || true)" != 0 ] && echo yes || echo no + [ "$(grep -ci 'Index.*Scan' <<<"$_plan" || true)" -ne 0 ] && echo yes || echo no } seq_count() { # force a sequential scan q "SET enable_indexscan = off; SET enable_bitmapscan = off; diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index cb3a09ce..4af868b9 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -953,8 +953,8 @@ pgc_log_shows_any_accounting() { # pgc_log_shows_any_accounting LOGFILE -> yes|n # the property that keeps the debt file from becoming a permission slip. local _log="$1" [ -f "$_log" ] || { echo no; return 0; } - if [ "$(grep -cE '^accounting: [0-9]+ passed \+ [0-9]+ failed \+ [0-9]+ unrunnable = [0-9]+$' "$_log" || true)" != 0 ] \ - || [ "$(grep -cE '^checks run: [0-9]+$' "$_log" || true)" != 0 ]; then + if [ "$(grep -cE '^accounting: [0-9]+ passed \+ [0-9]+ failed \+ [0-9]+ unrunnable = [0-9]+$' "$_log" || true)" -ne 0 ] \ + || [ "$(grep -cE '^checks run: [0-9]+$' "$_log" || true)" -ne 0 ]; then echo yes else echo no diff --git a/test/selftest/070-and-comm-s-two-inputs-must.sh b/test/selftest/070-and-comm-s-two-inputs-must.sh index 455d525d..679ca8fd 100644 --- a/test/selftest/070-and-comm-s-two-inputs-must.sh +++ b/test/selftest/070-and-comm-s-two-inputs-must.sh @@ -22,8 +22,8 @@ for _f in $_cm_files; do # The second test is grep -c on a captured value, not a pipe into grep -qv; # see selftest 080. The first reads a FILE and is not a pipeline at all. _cm_sorts="$(grep -E '\|[[:space:]]*sort' "$_f" || true)" - if [ "$(grep -cE '\|[[:space:]]*sort' "$_f" || true)" != 0 ] \ - && [ "$(grep -cv 'LC_ALL=C' <<<"$_cm_sorts" || true)" != 0 ]; then + if [ "$(grep -cE '\|[[:space:]]*sort' "$_f" || true)" -ne 0 ] \ + && [ "$(grep -cv 'LC_ALL=C' <<<"$_cm_sorts" || true)" -ne 0 ]; then _cm_unpinned="$_cm_unpinned $(basename "$_f")" fi done diff --git a/test/selftest/080-no-suite-pipes-a-captured-string.sh b/test/selftest/080-no-suite-pipes-a-captured-string.sh index f9f88bc8..7389c2fd 100644 --- a/test/selftest/080-no-suite-pipes-a-captured-string.sh +++ b/test/selftest/080-no-suite-pipes-a-captured-string.sh @@ -279,7 +279,7 @@ _epipe_hits="$( if [ -n "$_epipe_hd" ] && [ -n "$_epipe_hits" ]; then _epipe_hits="$(printf '%s\n' "$_epipe_hits" | while IFS= read -r _eh; do _ehk="${_eh%%:*}:$(printf '%s' "$_eh" | cut -d: -f2):" - [ "$(grep -cxF "$_ehk" <<<"$_epipe_hd" || true)" != 0 ] || printf '%s\n' "$_eh" + [ "$(grep -cxF "$_ehk" <<<"$_epipe_hd" || true)" -ne 0 ] || printf '%s\n' "$_eh" done)" fi _epipe_count="$(printf '%s' "$_epipe_hits" | grep -c . || true)" diff --git a/test/selftest/300-a-test-script-must-be-runnable.sh b/test/selftest/300-a-test-script-must-be-runnable.sh index c68d6835..8085ccbe 100644 --- a/test/selftest/300-a-test-script-must-be-runnable.sh +++ b/test/selftest/300-a-test-script-must-be-runnable.sh @@ -239,7 +239,7 @@ for _tsm_d in "${_tsm_dirs[@]}"; do _tsm_b="$(basename "$_tsm_d")" # grep -c on a here-string, not `printf | grep -q`; see selftest 080 and the # note in 350. The piped form reports a present name as absent under load. - [ "$(grep -c "^$_tsm_b/" <<<"$_tsm_named" || true)" != 0 ] \ + [ "$(grep -c "^$_tsm_b/" <<<"$_tsm_named" || true)" -ne 0 ] \ || _tsm_uncovered="$_tsm_uncovered $_tsm_b" done check "premise: and they name at least one command in every swept directory" \ diff --git a/test/selftest/320-a-check-that-could-not-run.sh b/test/selftest/320-a-check-that-could-not-run.sh index e6b56e12..439d919f 100644 --- a/test/selftest/320-a-check-that-could-not-run.sh +++ b/test/selftest/320-a-check-that-could-not-run.sh @@ -214,7 +214,7 @@ for _cnt_l in "${_cnt_sites[@]}"; do grep -q 'pgc_summary' "$_cnt_f" || continue # grep -c on a captured window, not a pipe into grep -q; see selftest 080. _cnt_win="$(sed -n "$((_cnt_ln > 3 ? _cnt_ln - 3 : 1)),$((_cnt_ln + 6))p" "$_cnt_f")" - if [ "$(grep -cE 'PGC_PASSED=|PGC_FAILED=|PGC_UNRUN=' <<<"$_cnt_win" || true)" = 0 ]; then + if [ "$(grep -cE 'PGC_PASSED=|PGC_FAILED=|PGC_UNRUN=' <<<"$_cnt_win" || true)" -eq 0 ]; then _cnt_n=$((_cnt_n + 1)) [ "$_cnt_n" -le 5 ] && _cnt_bad="$_cnt_bad ${_cnt_f##*/}:$_cnt_ln" fi diff --git a/test/selftest/340-the-binary-must-be-built-from.sh b/test/selftest/340-the-binary-must-be-built-from.sh index 584a01ca..78797aba 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -113,7 +113,7 @@ while IFS= read -r _bd_var; do [ -n "$_bd_name" ] || continue _bd_seen=$(( _bd_seen + 1 )) # grep -c on a here-string, not `printf | grep -qx`; see selftest 080. - [ "$(grep -cx "$_bd_name" <<<"$_bd_covered" || true)" != 0 ] || \ + [ "$(grep -cx "$_bd_name" <<<"$_bd_covered" || true)" -ne 0 ] || \ _bd_missing="$_bd_missing $_bd_name" done < "[]" or "[n: a b c]" # piped and 0 on a here-string. An independent run of the same shape in # isolation gave 10 in 40, so the rate is load- and size-dependent rather # than fixed -- the two measurements bracket it. - [ "$(grep -cxF "$name" <<<"$ondisk" || true)" != 0 ] && continue + [ "$(grep -cxF "$name" <<<"$ondisk" || true)" -ne 0 ] && continue n=$((n + 1)); [ "$n" -le 6 ] && bad="$bad $name" done < <(grep -oE '`test_[A-Za-z0-9_]*(\.py)?`' "$doc" 2>/dev/null \ | tr -d '`' | sort -u) @@ -319,7 +319,7 @@ _toc_unresolved() { # _toc_unresolved DOC -> "[]" or "[n: a b c]" while IFS= read -r anchor; do [ -n "$anchor" ] || continue # grep -cxF on a here-string, for the reason given in _dcv_absent above. - [ "$(grep -cxF "$anchor" <<<"$anchors" || true)" != 0 ] && continue + [ "$(grep -cxF "$anchor" <<<"$anchors" || true)" -ne 0 ] && continue n=$((n + 1)); [ "$n" -le 6 ] && bad="$bad $anchor" done < <(grep -oE '\]\(#[A-Za-z0-9_-]+\)' "$doc" 2>/dev/null \ | sed -e 's/^](#//' -e 's/)$//' | sort -u) diff --git a/test/selftest/420-a-count-grep-never-produced.sh b/test/selftest/420-a-count-grep-never-produced.sh new file mode 100644 index 00000000..70b5c5a8 --- /dev/null +++ b/test/selftest/420-a-count-grep-never-produced.sh @@ -0,0 +1,91 @@ +# ---- a count that grep never produced is not "present" (#929) ------------------ +# +# #922 replaced roughly 28 `producer | grep -q PAT` tests with +# `[ "$(grep -c PAT … || true)" != 0 ]`, which fixed a real EPIPE race (#486). The +# replacement answers PRESENT where the original answered ABSENT whenever grep +# produces no stdout, and a pattern that does not compile is the way to get there. +# +# grep -cE '[' file -> stdout is [] (empty, not "0") +# [ "" != 0 ] -> TRUE (a STRING comparison: "" is not "0") +# +# So the helper reported the pattern present for a question it never managed to ask. +# Every pattern in the tree is valid today, so no site was wrong -- the hazard is the +# DIRECTION of the next edit. A premise arm phrased to want `present`, and most are, +# turns GREEN when its pattern stops compiling: it passes BECAUSE the instrument +# broke. The old form failed the other way, red and loud. +# +# THE FIX IS A NUMERIC COMPARISON, and it is behaviour-preserving everywhere else. +# Measured: for a real count the two forms agree exactly; only the empty case differs, +# and there the numeric form is false AND writes to stderr. +# +# input [ "$n" != 0 ] [ "$n" -ne 0 ] stderr +# a real count: 0 false false no +# a real count: 3 true true no +# EMPTY (grep usage error) TRUE false YES +# +# Both spellings failed the same way. `= 0` is an ABSENCE claim, and on empty it is +# false -- which does not assert absence, and is the safe direction once it is loud. + +check "premise: a real count compares the same both ways, so the conversion is behaviour-preserving" \ + "$(_n=3; { [ "$_n" != 0 ] && [ "$_n" -ne 0 ]; } && echo same || echo differ)" "same" +check "premise: and a zero count does too" \ + "$(_n=0; { [ "$_n" != 0 ] || [ "$_n" -ne 0 ]; } && echo differ || echo same)" "same" +check "an empty count is not 'present' under a numeric comparison" \ + "$(_n=""; [ "$_n" -ne 0 ] 2>/dev/null && echo present || echo "not present")" "not present" +check "and the string comparison it replaces WOULD have said present" \ + "$(_n=""; [ "$_n" != 0 ] && echo present || echo "not present")" "present" +check "and the numeric form says so on stderr rather than silently" \ + "$(_n=""; { [ "$_n" -ne 0 ]; } 2>&1 >/dev/null | grep -c . || true)" "1" + +# The mechanism, against real grep rather than a hand-written empty string. +_c929="$PGC_WORKDIR/c929.txt" +printf 'hello\n' > "$_c929" +check "premise: grep -c prints nothing at all on a pattern that does not compile" \ + "$(grep -cE '[' "$_c929" 2>/dev/null | wc -c | tr -d ' ')" "0" +check "premise: while a valid pattern prints a number" \ + "$(grep -cE 'hello' "$_c929" 2>/dev/null)" "1" + +# ---- and the shape cannot come back ------------------------------------------ +# +# HEREDOC-AWARE, like the exit-0 sweep in part 410: the suites generate fixture +# scripts, and a forbidden idiom inside a generated script is the fixture rather than +# an offence. Measured: 20 sites before this change and 0 after. +_c929_sweep() { + awk ' + FNR == 1 { hd = "" } + hd != "" { if ($0 == hd || $0 ~ "^[ \t]*" hd "[ \t]*$") hd = ""; next } + /<<-?[ \t]*'\''?[A-Za-z_][A-Za-z0-9_]*'\''?[ \t]*$/ { + if ($0 !~ /^[ \t]*#/) { + t = $0; sub(/.*<<-?[ \t]*/, "", t); gsub(/'\''/, "", t) + sub(/[ \t]*$/, "", t); hd = t; next + } + } + /^[ \t]*#/ { next } + /\[ "\$\(grep -c[a-zA-Z]*[^)]*\)" (!=|=) / { printf "%s:%d\n", FILENAME, FNR } + ' "$@" +} +_c929_files=("$PGC_SRCDIR"/test/*.sh "$PGC_SRCDIR"/test/selftest/*.sh) +check "premise: the sweep has a corpus to read" \ + "$([ "${#_c929_files[@]}" -ge 250 ] && echo yes || echo "no (${#_c929_files[@]})")" "yes" +# THE PLANTED SHAPES ARE ASSEMBLED, never written out, and the first version of this +# part got that wrong: spelling the forbidden idiom inside a `printf` made three of +# its own lines offences, and the corpus sweep reported 23 where the tree holds 20. +# The sweep flagging its own fixtures is the exact trap its comment cites. Passing the +# OPERATOR as an argument is enough -- the regex needs `!=` or `=` directly after the +# closing `)"`, and `%s` there is not either of them. +_c929_plant() { # _c929_plant OP FILE -> a file holding the shape, assembled + printf 'if [ "$(grep -c x f || true)" %s 0 ]; then :; fi\n' "$1" > "$2" +} +check "premise: and it finds a planted string comparison on a grep -c" \ + "$(_c929_plant '!=' "$PGC_WORKDIR/p929.sh"; _c929_sweep "$PGC_WORKDIR/p929.sh" | grep -c .)" "1" +check "premise: and the = 0 spelling too, which fails the same way" \ + "$(_c929_plant '=' "$PGC_WORKDIR/p929b.sh"; _c929_sweep "$PGC_WORKDIR/p929b.sh" | grep -c .)" "1" +check "premise: while a numeric comparison is not an offence" \ + "$(_c929_plant '-ne' "$PGC_WORKDIR/p929c.sh"; _c929_sweep "$PGC_WORKDIR/p929c.sh" | grep -c .)" "0" +check "premise: nor is one inside a generated fixture script" \ + "$({ printf 'cat > /tmp/x <<%sSH%s\n' "'" "'" + printf 'if [ "$(grep -c x f || true)" %s 0 ]; then :; fi\n' '!=' + printf 'SH\n'; } > "$PGC_WORKDIR/p929d.sh" + _c929_sweep "$PGC_WORKDIR/p929d.sh" | grep -c .)" "0" +check "no count from grep -c is compared as a string, which answers present when grep could not answer" \ + "$(_c929_sweep "${_c929_files[@]}" | grep -c . || true)" "0" diff --git a/test/unique_conc.sh b/test/unique_conc.sh index 5f9fbab5..0c0fc30d 100755 --- a/test/unique_conc.sh +++ b/test/unique_conc.sh @@ -73,7 +73,7 @@ port_is_free() { # EPIPE here answers "nothing is listening" for a port that IS taken, # and the suite then starts a cluster on an occupied port. _pif="$(ss -Htln "sport = :$1" 2>/dev/null || true)" - [ "$(grep -c ":$1" <<<"$_pif" || true)" = 0 ] + [ "$(grep -c ":$1" <<<"$_pif" || true)" -eq 0 ] else ! (exec 3<>"/dev/tcp/127.0.0.1/$1") 2>/dev/null fi diff --git a/test/update_conc.sh b/test/update_conc.sh index a85e8b3d..592d2494 100755 --- a/test/update_conc.sh +++ b/test/update_conc.sh @@ -81,7 +81,7 @@ port_is_free() { # EPIPE here answers "nothing is listening" for a port that IS taken, # and the suite then starts a cluster on an occupied port. _pif="$(ss -Htln "sport = :$1" 2>/dev/null || true)" - [ "$(grep -c ":$1" <<<"$_pif" || true)" = 0 ] + [ "$(grep -c ":$1" <<<"$_pif" || true)" -eq 0 ] else ! (exec 3<>"/dev/tcp/127.0.0.1/$1") 2>/dev/null fi From a70c6d7913be16d11fbd04cacf8715b1acbb5473 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 22:34:01 +0000 Subject: [PATCH 2/3] test/selftest: renumber this part, because #925 claimed the number first #925 adds `410-a-check-must-have-been-red.sh` and `420-a-deleted-part-must-be-visible.sh`, and #923 under it adds `400-a-check-result-must-be-machine.sh`. Two of us picked the same numbers from the same free range, which is the residual #554's one-file-per-subject rule leaves open: the rule stops two additions touching the same LINE, and a number is not a line. #925 is approved and answering blockers, so it keeps the number and this part moves. The subjects never overlapped -- theirs are the red-check ledger and the part manifest -- so this is numbering and nothing else. A NOTE FOR WHOEVER MERGES SECOND. #925 also adds `test/selftest/parts.manifest` and a part comparing it against the glob, because a glob cannot notice a deletion. If #925 lands first, this part has to be added to that manifest or its arm reddens -- correctly, because that is the arm doing its job rather than a conflict. The reverse order needs nothing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 2 +- ...rep-never-produced.sh => 440-a-count-grep-never-produced.sh} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename test/selftest/{420-a-count-grep-never-produced.sh => 440-a-count-grep-never-produced.sh} (98%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89ad737b..5c78c7d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -369,7 +369,7 @@ true until the next version shipped. The other 29 sites already compared numerically and were never affected. - `test/selftest/420-a-count-grep-never-produced.sh` holds the arms and a heredoc-aware + `test/selftest/440-a-count-grep-never-produced.sh` holds the arms and a heredoc-aware sweep requiring zero string comparisons on a `grep -c`, so the class is closed rather than the 20 instances. The sweep skips comments as well as heredocs: a flat grep counts the paragraph that documents the idiom, which is how a guard comes to flag its diff --git a/test/selftest/420-a-count-grep-never-produced.sh b/test/selftest/440-a-count-grep-never-produced.sh similarity index 98% rename from test/selftest/420-a-count-grep-never-produced.sh rename to test/selftest/440-a-count-grep-never-produced.sh index 70b5c5a8..8622ad88 100644 --- a/test/selftest/420-a-count-grep-never-produced.sh +++ b/test/selftest/440-a-count-grep-never-produced.sh @@ -47,7 +47,7 @@ check "premise: while a valid pattern prints a number" \ # ---- and the shape cannot come back ------------------------------------------ # -# HEREDOC-AWARE, like the exit-0 sweep in part 410: the suites generate fixture +# HEREDOC-AWARE, like the exit-0 sweep in part 430 (#934): the suites generate fixture # scripts, and a forbidden idiom inside a generated script is the fixture rather than # an offence. Measured: 20 sites before this change and 0 after. _c929_sweep() { From c39c429ae5aac1760cf2d14a4de2f43036c4853c Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 10 Sep 2026 22:57:32 +0000 Subject: [PATCH 3/3] test/selftest: the sweep could not see three sites, and its own count hid that (#929) @jdatcmd's blocking finding, reproduced before fixing: `[^)]*` in the sweep stops at the first `)` -- which is inside the GREP PATTERN rather than at the end of the command substitution -- so any pattern containing a parenthesis hid its own site. Three were invisible, each spelling `grep -cE '^ *(->)? *(Incremental )?Sort'`: test/sorted_mark_rename.sh:183 test/sorted_pathkeys.sh:53 test/sorted_pathkeys.sh:456 All three are string-compared, all three are inside the sweep's own file list, and the sweep reported zero. They are converted, and the pattern is greedy now. AND MY OWN COUNT HID IT, which is the part worth keeping. The PR body said "58 sites, of which 20 string-compared and 32 numeric" -- and 20 + 32 is 52. Two instruments reported as one measurement: the 58 came from a broad grep, the 20 from the sweep's narrow one. I publish `inputs == sum(buckets)` reconciliations against other people's work and did not run one on my own. With the corrected pattern it reconciles: on main (cfe1fde9) 58 inputs = 23 string-compared + 35 numeric after this change 58 inputs = 0 string-compared + 58 numeric THE GUARD AGREED WITH ITS POPULATION BY CONSTRUCTION, because both came out of the same regex. That is the same shape as a gate whose input list is computed by the thing it gates. So one plant now carries a parenthesis and differs from the plain plant in nothing else -- all five of the original plants used `grep -c x f`, with no parenthesis, so not one of them could have failed on this. Gate: harness_selftest 602 checks, 602 passed + 0 failed + 0 unrunnable, PASSED pytest corpus 262 passed with a cluster on pg18a sorted_pathkeys.sh 113 checks, 113 passed, PASSED -- newly converted, end to end shellcheck -S error over test/*.sh and test/selftest/*.sh rc=0 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 23 +++++++++++++--- .../440-a-count-grep-never-produced.sh | 27 ++++++++++++++++++- test/sorted_mark_rename.sh | 2 +- test/sorted_pathkeys.sh | 4 +-- 4 files changed, 48 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c78c7d8..91dc7732 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -354,7 +354,7 @@ true until the next version shipped. is the failure this harness spends most of its effort refusing. The old form failed red. - **20 sites** compared a count as a string; they now compare numerically, which is + **23 sites** compared a count as a string; they now compare numerically, which is behaviour-preserving in every case that is not broken. Measured: input [ "$n" != 0 ] [ "$n" -ne 0 ] stderr @@ -364,14 +364,29 @@ true until the next version shipped. Both spellings failed the same way. `= 0` is an ABSENCE claim, and on an empty value it is false -- which does not assert absence, and is the safe direction once it is - loud. All 20 sites pass exactly one input to grep, so the value is always a bare + loud. All 23 sites pass exactly one input to grep, so the value is always a bare number and a numeric comparison cannot be confused by `file:count` output. - The other 29 sites already compared numerically and were never affected. + THE POPULATION RECONCILES, and the first version of this entry did not. It said + "58 sites, of which 20 string-compared and 32 numeric" -- and 20 + 32 is 52. The 58 + came from a broad grep and the 20 from the sweep's own narrower one, so two + instruments were reported as one measurement. With the sweep's pattern corrected: + + on main (cfe1fde9) 58 inputs = 23 string-compared + 35 numeric + after this change 58 inputs = 0 string-compared + 58 numeric `test/selftest/440-a-count-grep-never-produced.sh` holds the arms and a heredoc-aware sweep requiring zero string comparisons on a `grep -c`, so the class is closed rather - than the 20 instances. The sweep skips comments as well as heredocs: a flat grep + than the 23 instances. + + THE SWEEP'S FIRST PATTERN COULD NOT SEE THREE OF THEM. `[^)]*` stopped at the first + `)`, which is inside the GREP PATTERN rather than at the end of the substitution, so + any pattern containing a parenthesis hid its own site: `sorted_mark_rename.sh:183` and + `sorted_pathkeys.sh:53` and `:456`, each spelling + `grep -cE '^ *(->)? *(Incremental )?Sort'`. The guard and its population came out of + the same regex, so the guard agreed with the count by construction -- which is why one + plant now carries a parenthesis and differs from the plain one in nothing else. + Reported by @jdatcmd. The sweep skips comments as well as heredocs: a flat grep counts the paragraph that documents the idiom, which is how a guard comes to flag its own explanation. diff --git a/test/selftest/440-a-count-grep-never-produced.sh b/test/selftest/440-a-count-grep-never-produced.sh index 8622ad88..8f1cf116 100644 --- a/test/selftest/440-a-count-grep-never-produced.sh +++ b/test/selftest/440-a-count-grep-never-produced.sh @@ -47,6 +47,24 @@ check "premise: while a valid pattern prints a number" \ # ---- and the shape cannot come back ------------------------------------------ # +# GREEDY, NOT `[^)]*`, AND THAT IS THE WHOLE POINT. The first version of this sweep +# stopped at the first `)` -- which is inside the GREP PATTERN, not at the end of the +# substitution -- so any pattern containing a parenthesis hid its own site entirely. +# Three sites were invisible, all spelling `grep -cE '^ *(->)? *(Incremental )?Sort'`: +# sorted_mark_rename.sh:183 and sorted_pathkeys.sh:53 and :456. Found by @jdatcmd. +# +# AND THE COUNT HID IT TOO, because the count and the guard came out of the SAME regex. +# I published "58 sites, of which 20 string-compared and 32 numeric" -- and 20 + 32 is +# 52, not 58. Two instruments, never reconciled: the 58 came from a broad grep and the +# 20 from this narrow one. With the greedy form the split reconciles, which is the +# `inputs == sum(buckets)` rule this directory applies everywhere: +# +# on main (cfe1fde9) 58 inputs = 23 string-compared + 35 numeric +# after this change 58 inputs = 0 string-compared + 58 numeric +# +# A guard derived through the same pattern as its own population agrees with it by +# construction. The plant below carries a parenthesis for exactly that reason. +# # HEREDOC-AWARE, like the exit-0 sweep in part 430 (#934): the suites generate fixture # scripts, and a forbidden idiom inside a generated script is the fixture rather than # an offence. Measured: 20 sites before this change and 0 after. @@ -61,7 +79,7 @@ _c929_sweep() { } } /^[ \t]*#/ { next } - /\[ "\$\(grep -c[a-zA-Z]*[^)]*\)" (!=|=) / { printf "%s:%d\n", FILENAME, FNR } + /\[ "\$\(grep -c[a-zA-Z]*.*\)" (!=|=) / { printf "%s:%d\n", FILENAME, FNR } ' "$@" } _c929_files=("$PGC_SRCDIR"/test/*.sh "$PGC_SRCDIR"/test/selftest/*.sh) @@ -80,6 +98,13 @@ check "premise: and it finds a planted string comparison on a grep -c" \ "$(_c929_plant '!=' "$PGC_WORKDIR/p929.sh"; _c929_sweep "$PGC_WORKDIR/p929.sh" | grep -c .)" "1" check "premise: and the = 0 spelling too, which fails the same way" \ "$(_c929_plant '=' "$PGC_WORKDIR/p929b.sh"; _c929_sweep "$PGC_WORKDIR/p929b.sh" | grep -c .)" "1" +# THE PLANT THAT THE FIRST VERSION PASSED. Every other plant here uses `grep -c x f`, +# with no parenthesis, so none of them could fail on the `[^)]*` bug. This one differs +# from the plain plant in exactly one respect: the pattern contains `(a)`. +check "premise: and it finds one whose PATTERN contains a parenthesis, which the first version could not" \ + "$(printf 'if [ "$(grep -cE %s^(a)b%s f || true)" %s 0 ]; then :; fi\n' "'" "'" '!=' \ + > "$PGC_WORKDIR/p929e.sh" + _c929_sweep "$PGC_WORKDIR/p929e.sh" | grep -c .)" "1" check "premise: while a numeric comparison is not an offence" \ "$(_c929_plant '-ne' "$PGC_WORKDIR/p929c.sh"; _c929_sweep "$PGC_WORKDIR/p929c.sh" | grep -c .)" "0" check "premise: nor is one inside a generated fixture script" \ diff --git a/test/sorted_mark_rename.sh b/test/sorted_mark_rename.sh index bfc3c46c..43e259af 100755 --- a/test/sorted_mark_rename.sh +++ b/test/sorted_mark_rename.sh @@ -180,7 +180,7 @@ sorts() { local _plan _plan="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -At \ -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null)" - [ "$(grep -cE '^ *(->)? *(Incremental )?Sort' <<<"$_plan" || true)" != 0 ] \ + [ "$(grep -cE '^ *(->)? *(Incremental )?Sort' <<<"$_plan" || true)" -ne 0 ] \ && echo yes || echo no } psql_run "CREATE TABLE wp (k int, j int) PARTITION BY RANGE (k);" diff --git a/test/sorted_pathkeys.sh b/test/sorted_pathkeys.sh index aa4090cb..f948f50d 100755 --- a/test/sorted_pathkeys.sh +++ b/test/sorted_pathkeys.sh @@ -50,7 +50,7 @@ sorts() { # sorts QUERY -> yes|no local _plan _plan="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ -d "$PGC_DB" -At -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null)" - [ "$(grep -cE '^ *(->)? *(Incremental )?Sort' <<<"$_plan" || true)" != 0 ] \ + [ "$(grep -cE '^ *(->)? *(Incremental )?Sort' <<<"$_plan" || true)" -ne 0 ] \ && echo yes || echo no } inv() { # inversions on the lead column in the order the scan returns rows @@ -453,7 +453,7 @@ sorts_off() { _plan="$(env PATH="$PGC_BINDIR:$PATH" PGOPTIONS="-c pgcolumnar.enable_sorted_pathkeys=off" \ psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -At \ -c "EXPLAIN (COSTS OFF) $1" 2>/dev/null)" - [ "$(grep -cE '^ *(->)? *(Incremental )?Sort' <<<"$_plan" || true)" != 0 ] \ + [ "$(grep -cE '^ *(->)? *(Incremental )?Sort' <<<"$_plan" || true)" -ne 0 ] \ && echo yes || echo no } check "premise: the claim is live with the GUC on" "$(sorts 'SELECT k FROM c ORDER BY k')" "no"