[Github] Simplify Getting Changed Files in Code Formatting Workflow

[boomanaiden154]

This patch changes getting changed files in the pr code format job to just checking out the previous two commits (the merge commit and its porent, the current commit latest in main), which allows us to just diff the merge commit. This means we do not have to checkout the ref through the merge base, which should save approximately a minute per job (or much more in some cases where the PR is particularly out of date).

[llvmbot]

@llvm/pr-subscribers-github-workflow

Author: Aiden Grossman (boomanaiden154)

Changes

This patch changes getting changed files in the pr code format job to just checking out the previous two commits (the merge commit and its porent, the current commit latest in main), which allows us to just diff the merge commit. This means we do not have to checkout the ref through the merge base, which should save approximately a minute per job (or much more in some cases where the PR is particularly out of date).

---

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

1 Files Affected:

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

diff --git a/.github/workflows/pr-code-format.yml b/.github/workflows/pr-code-format.yml
index b08e5c21ca62e..2c06c8bdddea6 100644
--- a/.github/workflows/pr-code-format.yml
+++ b/.github/workflows/pr-code-format.yml
@@ -21,14 +21,7 @@ jobs:
       - name: Fetch LLVM sources
         uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
         with:
-          ref: ${{ github.event.pull_request.head.sha }}
-
-      - name: Checkout through merge base
-        uses: rmacklin/fetch-through-merge-base@bfe4d03a86f9afa52bc1a70e9814fc92a07f7b75 # v0.3.0
-        with:
-          base_ref: ${{ github.event.pull_request.base.ref }}
-          head_ref: ${{ github.event.pull_request.head.sha }}
-          deepen_length: 500
+          fetch-depth: 2
 
       - name: Get changed files
         id: changed-files
@@ -36,6 +29,8 @@ jobs:
         with:
           separator: ","
           skip_initial_fetch: true
+          base_sha: 'HEAD~1'
+          sha: 'HEAD'
 
       # We need to pull the script from the main branch, so that we ensure
       # we get the latest version of this script.
@@ -89,8 +84,8 @@ jobs:
             --write-comment-to-file \
             --token ${{ secrets.GITHUB_TOKEN }} \
             --issue-number $GITHUB_PR_NUMBER \
-            --start-rev $(git merge-base $START_REV $END_REV) \
-            --end-rev $END_REV \
+            --start-rev HEAD~1 \
+            --end-rev HEAD \
             --changed-files "$CHANGED_FILES"
 
       - uses: actions/upload-artifact@26f96dfa697d77e81fd5907df203aa23a56210a8 #v4.3.0

[vbvictor]

Hi, could you please tell why the script works if a person creates a PR with more than 1 commit, e.g:

1. Person creates PR with 3 commits:
   main -> A -> B -> C

2. CI premerge check starts and makes git diff C B and only check formatting between those 2 commits. But what we want is git diff C main to properly check formatting.

I'm trying to make a clang-tidy CI job similar to this one and this part doesn't work for me: I only get diff C B which produces not proper diff to analyze.

[boomanaiden154]

Github tests PRs as if they were merged into main. So you end up with main and then a merge commit at HEAD containing all of the changes in the PR. So you only need to diff the merge commit with its base.

Anything clang-tidy should also probably be integrated into the code format job. Using a separate job means we need extra things like an additional clone of LLVM every time.

[vbvictor]

Thank you! I guess in my local runs I have slightly different setup.

Anything clang-tidy should also probably be integrated into the code format job. Using a separate job means we need extra things like an additional clone of LLVM every time.

I'm afraid that code formatting job will become too bloated with all these changes.
For clang-tidy CI, we need to additionally cmake configure, ninja build tablegen and run clang-tidy. I'll look into integration with formatting job.

[boomanaiden154]

Ah, forgot that clang-tidy needs to see a fully preprocessed file. Keeping it separate might be better then.