[CI][format] Explicitly pass extensions to git-clang-format (take 2)

[ldionne]

This patch ensures that the CI script controls which file extensions are
considered instead of letting git-clang-format apply its own filtering
rules. In particular, this properly handles libc++ extension-less headers
which were passed to git-clang-format, but then dropped by the tool as
having an unrecognized extension.

This is a second attempt to land 7620fe0, which was reverted in 9572388
because it caused formatting not to be enforced for several patches. The
problem was that we'd incorrectly pass the extensions with additional quoting
to git-clang-format. The incorrect quoting has been removed in this version
of the patch.

[llvmbot]

@llvm/pr-subscribers-libcxx
@llvm/pr-subscribers-github-workflow

@llvm/pr-subscribers-mlgo

Author: Louis Dionne (ldionne)

Changes

This is a second attempt to land 7620fe0, which was reverted in 9572388 because it caused formatting not to be enforced for several patches.

---

Full diff: https://github.com/llvm/llvm-project/pull/98227.diff

2 Files Affected:

• (modified) llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp (+2)
• (modified) llvm/utils/git/code-format-helper.py (+11)

diff --git a/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp b/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp
index 7f90457d720b4..536984e6c9a5a 100644
--- a/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp
+++ b/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp
@@ -10,6 +10,8 @@
 //
 //===----------------------------------------------------------------------===//
 
+// This line should never get through the formatting job clang-format please format this into something that is actually reasonable.
+
 #include "AllocationOrder.h"
 #include "RegAllocEvictionAdvisor.h"
 #include "RegAllocGreedy.h"
diff --git a/llvm/utils/git/code-format-helper.py b/llvm/utils/git/code-format-helper.py
index f1207026704e8..d60d4131bc94b 100755
--- a/llvm/utils/git/code-format-helper.py
+++ b/llvm/utils/git/code-format-helper.py
@@ -216,6 +216,17 @@ def format_run(self, changed_files: List[str], args: FormatArgs) -> Optional[str
             cf_cmd.append(args.start_rev)
             cf_cmd.append(args.end_rev)
 
+        # Gather the extension of all modified files and pass them explicitly to git-clang-format.
+        # This prevents git-clang-format from applying its own filtering rules on top of ours.
+        extensions = set()
+        for file in cpp_files:
+            _, ext = os.path.splitext(file)
+            extensions.add(
+                ext.strip(".")
+            )  # Exclude periods since git-clang-format takes extensions without them
+        cf_cmd.append("--extensions")
+        cf_cmd.append("'{}'".format(",".join(extensions)))
+
         cf_cmd.append("--")
         cf_cmd += cpp_files
 

[ldionne]

This PR is not ready for review, I'm trying to investigate why the CI doesn't detect format breakage.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[boomanaiden154]

One other thing to note (if you aren't already aware) is that the python scripts are pulled in from main rather than the branch being merged in, so you need to modify this step if you want to test changes in the branch to the python utilities:

llvm-project/.github/workflows/pr-code-format.yml

Line 37 in f35318e

- name: Fetch code formatting utils

[ldionne]

@boomanaiden154 I think I found the issue. I was passing 'cpp' as the extension. However, we're using subprocess.run, we're not inside a shell script. So the 'cpp' argument was passed as-is, and git-clang-format would basically think there's no file with an extension of 'cpp' (but there's a file with an extension of cpp).

I think removing the quoting should do it. I do need to test that the right thing happens when I have a file with no extension, especially when the only modified file is one without an extension (e.g. libcxx/include/vector).

[ldionne]

OK, sorry for the multiple updates here but I think this should be good to go. I tested with:

• a single .cpp file from LLVM
• a .cpp file from LLVM and an extension-less header from libc++
• a single extension-less header from libc++

and they all seemed to work now.

[boomanaiden154]

LGTM.

I'll try and keep an eye on this over the next couple days after this lands to see if something drastically changes, but if the basics work, we should be good.

At some point I would like proper testing for the CI infrastructure, but haven't figured out how to do pieces of it. Hopefully I can spend some time on that soon.