Follow-up to #852, agreed with jdatcmd on #853 rather than folded into it: 13
files of churn inside an approved PR is how a reviewer stops reading.
Measured on #853's head, ffbf775, which is the tree after #852 is fixed.
Two separate things, and the second is the one that matters
1. Fourteen files declare an interpreter and cannot be run
#852's rule is that a file whose first line says #! has declared it is meant
to be run, and a mode that forbids running it contradicts what the file already
says about itself. After #853 the whole repository still holds fourteen of those:
100644 bench/cb_guards.sh
100644 test/fixtures/iceberg/crosscheck_dv.py
100644 test/fixtures/iceberg/crosscheck_eq_duckdb.py
100644 test/fixtures/iceberg/crosscheck_nm.py
100644 test/fixtures/iceberg/gen_delete_fixture.py
100644 test/fixtures/iceberg/gen_iceberg_bucket.py
100644 test/fixtures/iceberg/gen_iceberg_datepart.py
100644 test/fixtures/iceberg/gen_iceberg_day.py
100644 test/fixtures/iceberg/gen_iceberg_fixture.py
100644 test/fixtures/iceberg/gen_iceberg_temporal.py
100644 test/fixtures/iceberg/gen_iceberg_trunc.py
100644 test/fixtures/iceberg/gen_iceberg_warehouse.py
100644 test/fixtures/iceberg/gen_malformed_fixture.py
100644 test/fixtures/iceberg/gen_name_mapping_fixture.py
against 270 that carry a shebang and the bit.
None of them is broken today, and that is stated first because it decides the
priority. bench/cb_guards.sh is sourced, by bench/run_clickbench.sh:176 and
test/bench_guards.sh:43, and its own header says "Sourced, not executed". Of
the thirteen under test/fixtures/iceberg/, three document an explicit
interpreter in their headers (V/bin/python test/fixtures/iceberg/crosscheck_dv.py
and its two siblings) and ten document no invocation at all — they are the
generators that record how a committed fixture was made, run by hand when one is
regenerated. An interpreter named on the command line does not consult the
execute bit, so every one of these works as documented.
What they are is the state #852's own guard calls self-contradictory, and the way
out that guard documents — drop the shebang and say why in the header — is taken
for none of them. bench/cb_guards.sh is the near miss: its header already gives
the reason, and it kept the shebang anyway.
2. The guard's population stops at test/, and bench/ has five documented bare commands
This is the part that can bite. test/selftest/300-a-test-script-must-be-runnable.sh
sweeps test/ recursively. bench/ is not swept at all, and docs/benchmarks.md
documents five commands that name a bench/ script with no interpreter:
docs/benchmarks.md:7 BENCH_DUCKDB=1 bench/run_bench.sh /path/to/pg17_nc/bin/pg_config
docs/benchmarks.md:8 bench/run_bench_fsst.sh /path/to/pg17_nc/bin/pg_config
docs/benchmarks.md:9 bench/run_bench_readstream.sh /path/to/pg18_uring/bin/pg_config
docs/benchmarks.md:748 BENCH_SCALE=20000000 bench/run_bench_join.sh /path/to/pg18n/bin/pg_config
docs/benchmarks.md:818 PGC_CB_ROWS=10000000 bench/run_clickbench.sh /path/to/pg18n/bin/pg_config
All five scripts are 100755 today, so all five work. They are correct by
habit, with nothing checking it — which is the exact state test/ was in
before #852, where 102 of the 103 files were born 100644 and one lost its bit
to a passing commit (56ae5f8eb). The next bench/ script added at 100644 and
documented as a bare command reproduces #852 in a directory the guard cannot see.
What I am not deciding here
Two coherent shapes for part 1, and the choice is not mine:
Part 2 is a smaller and more mechanical question: extend the sweep past test/,
or leave bench/ uncovered deliberately and say so where the exemption is
argued.
I am happy to implement whichever, test-first, on the same red-before-green shape
as #853. Filing rather than pushing because #853 is approved and this is not its
defect.
Measured with git ls-files -s plus a first-line read on every tracked file, at
ffbf775.
Follow-up to #852, agreed with jdatcmd on #853 rather than folded into it: 13
files of churn inside an approved PR is how a reviewer stops reading.
Measured on
#853's head,ffbf775, which is the tree after #852 is fixed.Two separate things, and the second is the one that matters
1. Fourteen files declare an interpreter and cannot be run
#852's rule is that a file whose first line says#!has declared it is meantto be run, and a mode that forbids running it contradicts what the file already
says about itself. After #853 the whole repository still holds fourteen of those:
against 270 that carry a shebang and the bit.
None of them is broken today, and that is stated first because it decides the
priority.
bench/cb_guards.shis sourced, bybench/run_clickbench.sh:176andtest/bench_guards.sh:43, and its own header says "Sourced, not executed". Ofthe thirteen under
test/fixtures/iceberg/, three document an explicitinterpreter in their headers (
V/bin/python test/fixtures/iceberg/crosscheck_dv.pyand its two siblings) and ten document no invocation at all — they are the
generators that record how a committed fixture was made, run by hand when one is
regenerated. An interpreter named on the command line does not consult the
execute bit, so every one of these works as documented.
What they are is the state #852's own guard calls self-contradictory, and the way
out that guard documents — drop the shebang and say why in the header — is taken
for none of them.
bench/cb_guards.shis the near miss: its header already givesthe reason, and it kept the shebang anyway.
2. The guard's population stops at
test/, andbench/has five documented bare commandsThis is the part that can bite.
test/selftest/300-a-test-script-must-be-runnable.shsweeps
test/recursively.bench/is not swept at all, anddocs/benchmarks.mddocuments five commands that name a
bench/script with no interpreter:All five scripts are
100755today, so all five work. They are correct byhabit, with nothing checking it — which is the exact state
test/was inbefore #852, where 102 of the 103 files were born
100644and one lost its bitto a passing commit (
56ae5f8eb). The nextbench/script added at100644anddocumented as a bare command reproduces #852 in a directory the guard cannot see.
What I am not deciding here
Two coherent shapes for part 1, and the choice is not mine:
test/fixtures/exemption stops hiding anything. Running a generator directlywould fail on a missing
pyicebergrather than on a permission, which is thehonest error.
documents as the way out.
bench/cb_guards.shis one line from this already.Part 2 is a smaller and more mechanical question: extend the sweep past
test/,or leave
bench/uncovered deliberately and say so where the exemption isargued.
I am happy to implement whichever, test-first, on the same red-before-green shape
as #853. Filing rather than pushing because #853 is approved and this is not its
defect.
Measured with
git ls-files -splus a first-line read on every tracked file, atffbf775.