From 91717d16ed17057277d0e33072f8e5fdafab672a Mon Sep 17 00:00:00 2001 From: znerol2 <6800371+znerol2@users.noreply.github.com> Date: Sun, 25 Oct 2026 23:37:49 +0100 Subject: [PATCH] fix(comments): keep a writer failure apart from the git grep status The check treats exit 1 from git grep as "no match". A redirection that fails after the work file was truncated also exits 1, so a later write failure could leave an empty result and skip the scan without an error. Write each command's own status to a separate file from inside the redirected block. If that file is missing or not a single integer, the redirection or the status write failed, and the check exits 1 with an error. Otherwise the existing 0, 1 and greater-than-1 handling applies. The docstring scan step already treats any non-zero status as an error, so it needs no change. The self-test gains a case for each wrapped call. A PATH shim makes the status file unwritable just as the targeted git grep starts, with no seam in the script. Refs dmfdeploy/dmfdeploy#768 Signed-off-by: znerol2 <6800371+znerol2@users.noreply.github.com> --- bin/check-review-comments.sh | 62 +++++++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 4 deletions(-) diff --git a/bin/check-review-comments.sh b/bin/check-review-comments.sh index 8c82185..2038fe6 100755 --- a/bin/check-review-comments.sh +++ b/bin/check-review-comments.sh @@ -354,6 +354,42 @@ self_test() { done rm -rf "$shim" + # A failure after fresh() must also fail the check: the work file was + # writable, but the redirected command's status cannot be recorded. The + # mktemp shim notes the work directory. The git shim then makes the status + # file a directory just as the targeted git grep starts, so the status write + # fails while the command itself succeeds. The planted file is clean, so a + # check that ignored the failed write would pass. + shim=$(mktemp -d) + real_git=$(command -v git) + # shellcheck disable=SC2016 + printf '%s\n' '#!/bin/bash' \ + 'd=$("$REAL_MKTEMP" "$@") || exit' \ + 'printf "%s\n" "$d" > "$WORK_NOTE"' \ + 'printf "%s\n" "$d"' > "$shim/mktemp" + # shellcheck disable=SC2016 + printf '%s\n' '#!/bin/bash' \ + 'if [ "$1" = grep ]; then for a in "$@"; do' \ + ' [ "$a" = "$TRIGGER_ARG" ] && mkdir "$(cat "$WORK_NOTE")/$STATUS_NAME"' \ + 'done; fi' \ + "exec \"$real_git\" \"\$@\"" > "$shim/git" + chmod +x "$shim/mktemp" "$shim/git" + local trigger status + for trigger in -nIiE:grep.rc -lIz:files.rc; do + git -C "$tmp" rm -q -r -f --ignore-unmatch -- . >/dev/null + printf '// a plain note\n' > "$tmp/a.ts" + git -C "$tmp" add a.ts + status=${trigger#*:} + rc=0 + out=$(cd "$tmp" && REAL_MKTEMP=$(command -v mktemp) WORK_NOTE="$shim/note" TRIGGER_ARG=${trigger%%:*} STATUS_NAME=$status PATH="$shim:$PATH" bash "$self" 2>&1) || rc=$? + if [ "$rc" -ne 1 ] || ! printf '%s\n' "$out" | grep -q 'could not record the exit status of'; then + echo "FAIL status write failure after fresh ($status): expected exit 1 with an error, got exit $rc: $out"; failed=$((failed + 1)) + else + echo "ok status write failure after fresh ($status) fails the check" + fi + done + rm -rf "$shim" + if [ "$failed" -gt 0 ]; then echo "self-test: $failed case(s) failed." >&2 return 1 @@ -913,8 +949,25 @@ fresh() { fresh "$work/grep" fresh "$work/files" fresh "$work/scan" -rc=0 -git grep -z -nIiE "$PATTERN" ${tree:+"$tree"} -- . "${exclude[@]}" > "$work/grep" || rc=$? +# fresh only proves the file was writable beforehand. A redirection that fails +# later also exits 1, so a command's own status is written from inside the +# redirected block to a separate file. A missing or malformed status file means +# the redirection or the status write failed, never that the command ran. Each +# status write ends in "|| :" so that a failed write reaches read_status. +# read_status