git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 2/2] ci(check-whitespace): restrict to the intended commits

From
Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Date
Jul 14, 2021, 22:09 UTC
Message-ID
<b63a5bbc63ba17449a91913ab28c268db5fa3650.1626300577.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.995.git.1626300577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>

During a run of the `check-whitespace` we want to verify that the commits introduced in the Pull Request have no whitespace issues. We only want to look at those commits, not the upstream commits (because the contributor cannot do anything about the latter).

However, by using the `-<count>` form in `git log --check`, we run the risk of looking at the wrong commits. The reason is that the `actions/checkout` step does _not_ check out the tip commit of the Pull Request's branch: Instead, it checks out a merge commit that merges that branch into the target branch. For that reason, we already adjust the commit count by incrementing it, but that is not enough: if the upstream branch has newer commits, they are traversed _first_. And obviously we will then miss some of the commits that we _actually_ wanted to look at.

Therefore, let's be careful to stop assuming a linear, up to date commit topology in the contributed commits, and instead specify the correct commit range.

Unfortunately, this means that we no longer can rely on a shallow clone: There is no way of knowing just how many commits the upstream branch advanced after the commit from which the PR branch branched off. So let's just go with a full clone instead, and be safe rather than sorry (if we have "too shallow" a situation, a commit range `@{u}..` may very well include a shallow commit itself, and the output of `git show --check <shallow>` is _not_ pretty).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .github/workflows/check-whitespace.yml | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml
index c53614d6033..8c4358d805c 100644
--- a/.github/workflows/check-whitespace.yml
+++ b/.github/workflows/check-whitespace.yml
@@ -12,15 +12,9 @@ jobs:
   check-whitespace:
     runs-on: ubuntu-latest
     steps:
-    - name: Set commit count
-      shell: bash
-      run: echo "COMMIT_DEPTH=$((1+$COMMITS))" >>$GITHUB_ENV
-      env:
-        COMMITS: ${{ github.event.pull_request.commits }}
-
     - uses: actions/checkout@v2
       with:
-        fetch-depth: ${{ env.COMMIT_DEPTH }}
+        fetch-depth: 0
 
     - name: git log --check
       id: check_out
@@ -47,7 +41,7 @@ jobs:
             echo "${dash} ${etc}"
             ;;
           esac
-        done <<< $(git log --check --pretty=format:"---% h% s" -${{github.event.pull_request.commits}})
+        done <<< $(git log --check --pretty=format:"---% h% s" ${{github.event.pull_request.base.sha}}..)
 
         if test -n "${log}"
         then
-- 
gitgitgadget
Previous: Junio C HamanoNext: Junio C Hamano
Message 4 of 8 in “check-whitespace: two fixes”
  1. 0/2 check-whitespace: two fixesJohannes Schindelin via GitGitGadget, Jul 14, 2021
  2. 1/2 ci(check-whitespace): stop requiring a read/write tokenJohannes Schindelin via GitGitGadget, Jul 14, 2021
  3. Junio C HamanoJul 14, 2021
  4. 2/2 ci(check-whitespace): restrict to the intended commitsJohannes Schindelin via GitGitGadget, Jul 14, 2021
  5. Junio C HamanoJul 14, 2021
  6. Taylor BlauJul 14, 2021
  7. Johannes SchindelinJul 15, 2021
  8. Johannes SchindelinJul 15, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.