CI review: fail the run when git cannot read the skip marks

.code-review/run-review.sh

@@ -27,14 +27,20 @@ done < <(git -C "$repo" diff --no-ext-diff -z --name-only --diff-filter=ACMRD "$
 # Files .gitattributes marks linguist-generated or linguist-vendored (snapshots, generated output)
 # are skipped, as GitHub collapses them. The marks are read from the base, so a change cannot
 # mark itself unreviewed. check-attr answers two attributes per path, in order.
+# A git that cannot read them (check-attr --source needs 2.40) fails the run: skipping on an
+# error would pass a change nobody reviewed.
 paths=()
 if [ ${#changed[@]} -gt 0 ]; then
+  marks="$work/marks"
+  if ! printf '%s\0' "${changed[@]}" | git -C "$repo" check-attr -z --stdin --source "$base" \
+       linguist-generated linguist-vendored > "$marks"; then
+    echo "code-review: git check-attr could not read the skip marks (git 2.40 or newer is needed)" >&2
+    exit 2
+  fi
   while IFS= read -r -d '' path && IFS= read -r -d '' _ && IFS= read -r -d '' generated \
         && IFS= read -r -d '' _ && IFS= read -r -d '' _ && IFS= read -r -d '' vendored; do
     case "$generated $vendored" in set\ *|true\ *|*\ set|*\ true) ;; *) paths+=("--path=$path") ;; esac
-  done < <(printf '%s\0' "${changed[@]}" \
-           | git -C "$repo" check-attr -z --stdin --source "$base" linguist-generated \
-             linguist-vendored)
+  done < "$marks"
 fi
 if [ ${#paths[@]} -eq 0 ]; then
   echo "code-review: no changed files outside .code-review/ and the generated or vendored ones, nothing to review"

action/run-review.sh

@@ -27,14 +27,20 @@ done < <(git -C "$repo" diff --no-ext-diff -z --name-only --diff-filter=ACMRD "$
 # Files .gitattributes marks linguist-generated or linguist-vendored (snapshots, generated output)
 # are skipped, as GitHub collapses them. The marks are read from the base, so a change cannot
 # mark itself unreviewed. check-attr answers two attributes per path, in order.
+# A git that cannot read them (check-attr --source needs 2.40) fails the run: skipping on an
+# error would pass a change nobody reviewed.
 paths=()
 if [ ${#changed[@]} -gt 0 ]; then
+  marks="$work/marks"
+  if ! printf '%s\0' "${changed[@]}" | git -C "$repo" check-attr -z --stdin --source "$base" \
+       linguist-generated linguist-vendored > "$marks"; then
+    echo "code-review: git check-attr could not read the skip marks (git 2.40 or newer is needed)" >&2
+    exit 2
+  fi
   while IFS= read -r -d '' path && IFS= read -r -d '' _ && IFS= read -r -d '' generated \
         && IFS= read -r -d '' _ && IFS= read -r -d '' _ && IFS= read -r -d '' vendored; do
     case "$generated $vendored" in set\ *|true\ *|*\ set|*\ true) ;; *) paths+=("--path=$path") ;; esac
-  done < <(printf '%s\0' "${changed[@]}" \
-           | git -C "$repo" check-attr -z --stdin --source "$base" linguist-generated \
-             linguist-vendored)
+  done < "$marks"
 fi
 if [ ${#paths[@]} -eq 0 ]; then
   echo "code-review: no changed files outside .code-review/ and the generated or vendored ones, nothing to review"

tests/launcher/test_install_ci.py

@@ -375,3 +375,35 @@ def test_the_install_carries_every_module_the_wrapper_runs(workdir):
 
     for module in sorted(set(re.findall(r"-m (review_core[\w.]+)", script))):
         assert (dest / "src" / (module.replace(".", "/") + ".py")).is_file(), module
+
+
+def test_a_git_that_cannot_read_the_marks_fails_the_run_instead_of_skipping(workdir, tmp_path):
+    """Git before 2.40 has no `check-attr --source`: the run must fail, not report nothing to
+    review and pass (the change would go unreviewed)."""
+    bundle.install(workdir)
+    git = ["git", "-C", str(workdir), "-c", "user.name=t", "-c", "user.email=t@t"]
+    subprocess.run([*git, "add", "-A"], check=True, capture_output=True)
+    subprocess.run([*git, "commit", "-q", "-m", "install"], check=True, capture_output=True)
+    subprocess.run([*git, "tag", "installed"], check=True, capture_output=True)
+    (workdir / "src.py").write_text("y = 2\n")
+    subprocess.run([*git, "add", "-A"], check=True, capture_output=True)
+    subprocess.run([*git, "commit", "-q", "-m", "code"], check=True, capture_output=True)
+    shim = tmp_path / "bin"
+    shim.mkdir()
+    real = subprocess.run(["which", "git"], capture_output=True, text=True).stdout.strip()
+    (shim / "git").write_text(
+        "#!/bin/bash\n"
+        'for a in "$@"; do\n'
+        '  [ "$a" = check-attr ] && { echo "unknown option: --source" >&2; exit 129; }\n'
+        "done\n"
+        f'exec {real} "$@"\n'
+    )
+    (shim / "git").chmod(0o755)
+    env = {"PATH": f"{shim}:{os.environ['PATH']}", "HOME": str(tmp_path),
+           "RUNNER_TEMP": str(tmp_path / "rt"), "GITHUB_WORKSPACE": str(workdir),
+           "CR_BASE": "installed", "CR_HEAD": "HEAD"}  # fmt: skip
+    done = subprocess.run(["bash", str(workdir / ".code-review" / "run-review.sh")], env=env,
+                          capture_output=True, text=True)  # fmt: skip
+    assert "nothing to review" not in done.stdout
+    assert done.returncode == 2
+    assert "check-attr" in done.stderr