diff --git a/.code-review/run-review.sh b/.code-review/run-review.sh index 47c45de..5fb4e61 100755 --- a/.code-review/run-review.sh +++ b/.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" diff --git a/action/run-review.sh b/action/run-review.sh index 47c45de..5fb4e61 100755 --- a/action/run-review.sh +++ b/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" diff --git a/tests/launcher/test_install_ci.py b/tests/launcher/test_install_ci.py index 9ded4c2..9c2fa74 100644 --- a/tests/launcher/test_install_ci.py +++ b/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