check-whitespace: two fixes
cover check-whitespace: two fixes
I recently ran into a situation where a merge commit (which admittedly are undesirable in git/git and gitgitgadget/git Pull Requests, but are sometimes totally appropriate in the context of other forks such as git-for-windows/git) misled the logic in the check-whitespace workflow to look at upstream commits (and missing some patches that it should have looked at).
At the same time, I also realized that the feature where this workflow adds a PR comment in an attempt to be more helpful requires a read/write token (which weakens the overall security, I'd much rather stay with the read-only token configured e.g. in gitgitgadget/git and in git-for-windows/git).
This patch series addresses both issues.
Johannes Schindelin (2):
ci(check-whitespace): stop requiring a read/write token ci(check-whitespace): restrict to the intended commits
.github/workflows/check-whitespace.yml | 26 ++------------------------ 1 file changed, 2 insertions(+), 24 deletions(-)
base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-995%2Fdscho%2Ffix-check-whitespace-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-995/dscho/fix-check-whitespace-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/995
Signature · 1 line
gitgitgadget
PATCH 1/2 ci(check-whitespace): stop requiring a read/write token
From: Johannes Schindelin <[email protected]>
As part of some recent security tightening, GitHub introduced the ability to configure GitHub workflows to be run with a read-only token. This is much more secure, in particular when working in a public
repository: While the regular read/write token might be restricted to writing to the current branch, it is not necessarily restricted to access only the current Pull Request.
However, the check-whitespace workflow threw a wrench into this plan:
it _requires_ write access (because it wants to add a PR comment in case of a whitespace issue).
Let's just skip that PR comment. The user can always click through to the actual error, even if it is slightly less convenient.
--- .github/workflows/check-whitespace.yml | 16 ---------------- 1 file changed, 16 deletions(-)
.github/workflows/check-whitespace.yml +0 -16
@@ -51,21 +51,5 @@ jobs: if test -n "${log}" then - echo "::set-output name=checkout::"${log}"" exit 2 fi - - - name: Add Check Output as Comment - uses: actions/github-script@v3 - id: add-comment - env: - log: ${{ steps.check_out.outputs.checkout }} - with: - script: | - await github.issues.createComment({ - issue_number: context.issue.number, - owner: context.repo.owner, - repo: context.repo.repo, - body: `Whitespace errors found in workflow ${{ github.workflow }}:\n\n\`\`\`\n${process.env.log.replace(/\\n/g, "\n")}\n\`\`\`` - }) - if: ${{ failure() }}
Signature · 1 line
gitgitgadget
PATCH 2/2 ci(check-whitespace): restrict to the intended commits
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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).
--- .github/workflows/check-whitespace.yml | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
.github/workflows/check-whitespace.yml +2 -8
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH 1/2] ci(check-whitespace): stop requiring a read/write token
In reply to Johannes Schindelin via GitGitGadget"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 3 lines · writes:
However, the
check-whitespaceworkflow threw a wrench into this plan:
it _requires_ write access (because it wants to add a PR comment in case of a whitespace issue).
OK.
Re: [PATCH 2/2] ci(check-whitespace): restrict to the intended commits
In reply to Johannes Schindelin via GitGitGadget"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 7 lines · writes:
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 ofgit show --check <shallow>is _not_ pretty).
Makes sense.
As long as you have pull-request base, I suspect that you could shallow clone the base and incrementally fetch the rest to update, perhaps? But I do not know if it is worth doing so for a small project like ours.
Thanks.
Quoted 35 lines
Signed-off-by Johannes Schindelin <[email protected]>--- .github/workflows/check-whitespace.yml | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-).github/workflows/check-whitespace.yml +2
-8@@ -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
Re: [PATCH 2/2] ci(check-whitespace): restrict to the intended commits
Quoted 11 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 7 lines · writes:
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 ofgit show --check <shallow>is _not_ pretty).
Quoted 6 lines
Makes sense.
As long as you have pull-request base, I suspect that you could shallow clone the base and incrementally fetch the rest to update, perhaps? But I do not know if it is worth doing so for a small project like ours.
Agreed, and...
Quoted 4 lines
.github/workflows/check-whitespace.yml jobs: · line 17
Quoted 4 lines
- uses: actions/checkout@v2
with:.github/workflows/check-whitespace.yml +1
-1- fetch-depth: ${{ env.COMMIT_DEPTH }} + fetch-depth: 0
...I wondered whether "fetch-depth: 0" was the default and whether or not this hunk could have just removed "fetch-depth" entirely. But the default is "1", and "0" means "fetch everything". So we really do need it.
Thanks,
Taylor
Re: [PATCH 2/2] ci(check-whitespace): restrict to the intended commits
In reply to Junio C HamanoHi Junio,
Quoted 11 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 7 lines · writes:
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 ofgit show --check <shallow>is _not_ pretty).
Quoted 5 lines
Makes sense.
As long as you have pull-request base, I suspect that you could shallow clone the base and incrementally fetch the rest to update, perhaps?
Fetching into shallow clones is very expensive on the server. I want to avoid that whenever possible.
Quoted 1 line
But I do not know if it is worth doing so for a small project like ours.
And then there's that.
Ciao,
Dscho
Re: [PATCH 2/2] ci(check-whitespace): restrict to the intended commits
In reply to Taylor BlauHi Taylor,
Quoted 26 lines · Taylor Blau
Quoted 11 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 7 lines · writes:
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 ofgit show --check <shallow>is _not_ pretty).Quoted 6 lines
Makes sense.
As long as you have pull-request base, I suspect that you could shallow clone the base and incrementally fetch the rest to update, perhaps? But I do not know if it is worth doing so for a small project like ours.
Agreed, and...
Quoted 4 lines
.github/workflows/check-whitespace.yml jobs: · line 17
Quoted 4 lines
- uses: actions/checkout@v2
with:.github/workflows/check-whitespace.yml +1
-1- fetch-depth: ${{ env.COMMIT_DEPTH }} + fetch-depth: 0
Quoted 4 lines
...I wondered whether "fetch-depth: 0" was the default and whether or not this hunk could have just removed "fetch-depth" entirely. But the default is "1", and "0" means "fetch everything". So we really do need it.
Indeed. One of the things that makes action/checkout@v2 so much faster than @v1 is that it fetches shallow by default. But we can't have that here.
Ciao,
Dscho