[Github] Fetch through merge base in code formatting action

[boomanaiden154]

This commit adds another step to the Github workflow that runs the code formatting check to fetch through the merge base. This ensures that the necessary history is present to find the changed files and also to run clang-format over. This change massively increases the speed of the action (~10 minutes down to ~2 minutes in most cases from my testing) and also increases the reliability significantly.

[llvmbot]

@llvm/pr-subscribers-github-workflow

Author: Aiden Grossman (boomanaiden154)

Changes

This commit adds another step to the Github workflow that runs the code formatting check to fetch through the merge base. This ensures that the necessary history is present to find the changed files and also to run clang-format over. This change massively increases the speed of the action (~10 minutes down to ~2 minutes in most cases from my testing) and also increases the reliability significantly.

---

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

1 Files Affected:

• (modified) .github/workflows/pr-code-format.yml (+24-5)

diff --git a/.github/workflows/pr-code-format.yml b/.github/workflows/pr-code-format.yml
index 4fa5c42bca22b02..b7673009aa4f6a9 100644
--- a/.github/workflows/pr-code-format.yml
+++ b/.github/workflows/pr-code-format.yml
@@ -11,14 +11,33 @@ jobs:
       - name: Fetch LLVM sources
         uses: actions/checkout@v4
         with:
-          fetch-depth: 2 # Fetches only the last 2 commits
+          ref: ${{ github.event.pull_request.head.ref }}
+
+      - name: Checkout through merge base
+        uses: rmacklin/fetch-through-merge-base@v0
+        with:
+          base_ref: ${{ github.event.pull_request.base.ref }}
+          head_ref: ${{ github.event.pull_request.head.ref }}
+          deepen_length: 500
 
       - name: Get changed files
         id: changed-files
         uses: tj-actions/changed-files@v39
         with:
           separator: ","
-          fetch_depth: 2000 # Fetches only the last 2000 commits
+          skip_initial_fetch: true
+
+      # We need to make sure that we aren't executing/using any code from the
+      # PR for security reasons as we're using pull_request_target. Checkout
+      # the target branch with the necessary files.
+      - name: Fetch code formatting utils
+        uses: actions/checkout@v4
+        with:
+          sparse-checkout: |
+            llvm/utils/git/requirements_formatting.txt
+            llvm/utils/git/code-format-helper.py
+          sparse-checkout-cone-mode: false
+          path: code-format-tools
 
       - name: "Listed files"
         run: |
@@ -35,10 +54,10 @@ jobs:
         with:
           python-version: '3.11'
           cache: 'pip'
-          cache-dependency-path: 'llvm/utils/git/requirements_formatting.txt'
+          cache-dependency-path: 'code-format-tools/llvm/utils/git/requirements_formatting.txt'
 
       - name: Install python dependencies
-        run: pip install -r llvm/utils/git/requirements_formatting.txt
+        run: pip install -r code-format-tools/llvm/utils/git/requirements_formatting.txt
 
       - name: Run code formatter
         env:
@@ -47,7 +66,7 @@ jobs:
           END_REV: ${{ github.event.pull_request.head.sha }}
           CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }}
         run: |
-          python llvm/utils/git/code-format-helper.py \
+          python ./code-format-tools/llvm/utils/git/code-format-helper.py \
             --token ${{ secrets.GITHUB_TOKEN }} \
             --issue-number $GITHUB_PR_NUMBER \
             --start-rev $START_REV \

[boomanaiden154]

The new step essentially does what @llvm-beanz suggested in #70946. I'm not sure why this is so much faster than having the changed-files action fetch the history itself as they both do the same thing, but the results are definitely clear. This should avoid the issues with being unable to run on PRs with merge commits in #71131.

I still don't believe this accounts for the case where the author merges main back into their branch to update it and main has modified files that the author also touches with code formatting errors. I believe the action would currently pick them up even though they're not in the commit in question as they will show up between the start revision and the end revision due to the merge. That is a separate issue though and can be addressed in a follow up patch.

[tru]

Ah this is great! Thanks so much that you are working on this and fix it.

[boomanaiden154]

Thanks for the review! I'm going to land this and keep a close eye on it over the weekend to make sure there aren't any significant regressions before usage picks up on Monday.