akiomik / akiomik/mado

The benchmark scripts are unquoted, and nothing runs shellcheck on them

Đang mở
#398 13 bình luận 0 reaction 0 người được giao Xem trên GitHub
ci
Ngôn ngữ chính
Rust
Star
407
Fork
12
Merge trung bình
5 giờ 45 phút
Pull request đã merge (30 ngày)
30

Mô tả

Split out of #396, which fixed the bugs that stopped `scripts/acceptance` from
working at all. What was left of it has since narrowed, and this is the current
state.

## Done: the BSD-only `sed -i ''`

`scripts/acceptance/test.sh` no longer edits in place. #396 replaced it with a
read-one-name, write-another form that behaves the same on BSD and GNU sed, and
said why in a comment beside it. The `> /dev/null` those lines carried went with
them. Nothing in `scripts/` runs `sed -i` today, and the macOS-only claim this
issue was opened under no longer holds.

## Left: the benchmark scripts are unquoted

`shellcheck` 0.10.0 reports ten findings across `scripts/`, all of them in
`scripts/benchmarks`. `scripts/acceptance` is clean — #396 rewrote it.

```
scripts/benchmarks/setup.sh:3:14: warning: Use 'cd ... || exit' or 'cd ... || return' in case cd fails. [SC2164]
scripts/benchmarks/setup.sh:3:17: warning: Quote this to prevent word splitting. [SC2046]
scripts/benchmarks/setup.sh:3:27: note: Double quote to prevent globbing and word splitting. [SC2086]
scripts/benchmarks/setup.sh:8:1: warning: Use 'cd ... || exit' or 'cd ... || return' in case cd fails. [SC2164]
scripts/benchmarks/setup.sh:8:4: note: Double quote to prevent globbing and word splitting. [SC2086]
scripts/benchmarks/setup.sh:10:1: warning: Use 'cd ... || exit' or 'cd ... || return' in case cd fails. [SC2164]
scripts/benchmarks/comparison.sh:146:11: note: Expressions don't expand in single quotes [SC2016]
scripts/benchmarks/comparison.sh:147:10: note: Expressions don't expand in single quotes [SC2016]
scripts/benchmarks/comparison.sh:148:19: note: Expressions don't expand in single quotes [SC2016]
scripts/benchmarks/comparison.sh:150:3: note: Expressions don't expand in single quotes [SC2016]
```

The two files are not the same job.

- **`comparison.sh`'s four are deliberate.** Those single-quoted strings are
what `hyperfine` labels its results with, and expanding them would print the
same label for every run — the comment above them says so. They want a
`# shellcheck disable=SC2016` with that reason attached, not a change.
- **`setup.sh`'s six are real**, and they are on the three lines #399 is about:
`cd $DATA_ROOT` and `cd gitlab` with nothing to stop the script when the
directory is not there, which is how `git sparse-checkout set doc` reaches
mado's own checkout and empties it. Quoting alone does not fix that — #399
needs a shape, not a `|| exit` — but the same lines are being rewritten there.
Do #399 first and this becomes a re-run of `shellcheck` over the result.

## And: nothing runs it

No workflow lints a shell script. Which linter, which version, and how it is
run is #397, which is also deciding it for `action/entrypoint.sh` — seven
findings there, and outside this issue. Whatever that settles should enforce
`scripts/` too, and pin its version: the runner image ships 0.9.0 where local is
0.10.0, and a linter that is not pinned is a gate that changes under the tree.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.