Simplify the clang format check

[Damyan Pepper (damyanp)]

This change runs the clang format check in a read-only pull request workflow.

It removes pull request comments and the option that applies formatting changes. The check shows the formatting diff in the job log and fails when formatting changes are required.

[Copilot]

🟢 Approval recommended

The streamlined workflow safely preserves formatting enforcement without requiring write permissions.

Pull request overview

Simplifies the clang-format check into a read-only pull request workflow.

Changes:

• Reports formatting diffs in job logs and fails when changes are required.
• Removes PR comments, automatic formatting, Python helpers, and dependencies.
• Adds minimal permissions, concurrency control, and a timeout.

File summaries

File	Description
.github/workflows/clang-format-checker.yml	Implements the read-only formatting check.
utils/git/code-format-helper.py	Removes the obsolete PR-comment helper.
utils/git/code-format-save-diff.py	Removes automatic patch application support.
utils/git/requirements_formatting.txt.in	Removes the PyGitHub dependency declaration.
utils/git/requirements_formatting.txt	Removes obsolete generated dependencies.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Justin Bogner (bogner)]

Is this necessary? Presumably if there isn't anything interesting to format git-clang-format will exit 0 and we don't need to worry about the actual file list at all.

[Damyan Pepper (damyanp)]

I think there's some value in having something to look at in the log to see why it exited with 0 rather than having to guess why it didn't do anything.

[Damyan Pepper (damyanp)]

oh, maybe I'm misunderstanding your feedback - it's not about the echo, but about the entire if?

[Damyan Pepper (damyanp)]

Without that message we would get nothing: https://github.com/damyanp/DirectXShaderCompiler/actions/runs/33570847315/job/100064255165#step:5:50

[Justin Bogner (bogner)]

Yeah, I'm suggesting we remove all of this:

          git diff --name-only --diff-filter=ACMR -z \
            "$START_REV...$END_REV" \
            -- >"$changed_files_file"
          mapfile -d '' -t changed_files <"$changed_files_file"

          cpp_files=()
          for path in "${changed_files[@]}"; do
            case "$path" in
              *.cpp|*.c|*.h|*.hpp|*.hxx|*.cxx) cpp_files+=("$path") ;;
            esac
          done

          if [[ "${#cpp_files[@]}" -eq 0 ]]; then
            echo "[clang-format] no modified files to format"
            exit 0
          fi

I'm not totally convinced it's important to differentiate between "no relevant files were changed at all" and "everything is already formatted correctly".

If we do want to differentiate, maybe we should just run git-clang-format with --verbose? It prints either "no modified files to format" or "clang-format did not modify any files" in that case. Though, annoyingly, it looks like it prints both the diff and the verbose messages to stdout rather than printing the messages to stderr as I would hope.

[Damyan Pepper (damyanp)]

Thanks! It's even simpler now!

[Alex Sepkowski (alsepkow)]

Claude has a concern:

One substantive concern: the workflow uses the merge base only when selecting filenames, but  git-clang-format  still compares the base tip directly with the PR head:

git-clang-format --diff "$START_REV" "$END_REV"

When a PR branch is behind and the target branch modified the same C++ file, this can inspect lines that were not changed by the PR and produce a false formatting failure. It should compute and use the merge base:

merge_base="$(git merge-base "$START_REV" "$END_REV")"

git-clang-format
--diff
--extensions=cpp,c,h,hpp,hxx,cxx
"$merge_base"
"$END_REV"

[Alex Sepkowski (alsepkow)]

LGTUS - Joshua Batista (@bob80905)
Assuming you were able to verify with a good/bad C++ file ?

[Joshua Batista (bob80905)]

nit: I think the timeout can be reduced from 30 minutes.

[Damyan Pepper (damyanp)]

LGTUS - Joshua Batista (Joshua Batista (@bob80905)) Assuming you were able to verify with a good/bad C++ file ?

• CLANG-FORMAT EXPERIMENT: NO CPP damyanp/DirectXShaderCompiler#17
• CLANG-FORMAT EXPERIMENT: FORMATTED CPP damyanp/DirectXShaderCompiler#18
• CLANG-FORMAT EXPERIMENT: UNFORMATTED CPP damyanp/DirectXShaderCompiler#19

[Copilot]

🟢 Approval recommended

The workflow consistently implements the described read-only check and removes all obsolete supporting components.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The workflow correctly performs a bounded, read-only formatting check and removes now-unused supporting code.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced