check-whitespace: two fixes #995

dscho · 2021-07-14T12:36:51Z

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.

Cc: Chris. Webster [email protected]
cc: Taylor Blau [email protected]

We introduced a couple of whitespace issues, and it this was sadly not caught by the `check-whitespace` workflow (the reason is a bit convoluted, see gitgitgadget#995 which also fixes it). Signed-off-by: Johannes Schindelin <[email protected]>

Fix some whitespace issues that were missed by the `check-whitespace` job (most likely because of issues that are being fixed in gitgitgadget#995). Signed-off-by: Johannes Schindelin <[email protected]>

webstech

Thanks. I probably did not understand the commit count value and if the log check was on the branch.

May not need to save the output now ~line 48?

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. Signed-off-by: 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). Signed-off-by: Johannes Schindelin <[email protected]>

dscho · 2021-07-14T20:44:13Z

May not need to save the output now ~line 48?

Good point!

dscho · 2021-07-14T22:08:32Z

/submit

gitgitgadget · 2021-07-14T22:09:49Z

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-995/dscho/fix-check-whitespace-v1

To fetch this version to local tag pr-995/dscho/fix-check-whitespace-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-995/dscho/fix-check-whitespace-v1

gitgitgadget · 2021-07-14T22:23:29Z

.github/workflows/check-whitespace.yml

@@ -51,21 +51,5 @@ jobs:



On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]> writes: > 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). OK.

gitgitgadget · 2021-07-14T22:26:37Z

.github/workflows/check-whitespace.yml

@@ -12,15 +12,9 @@ jobs:
  check-whitespace:


On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]> 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 of `git 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. > Signed-off-by: Johannes Schindelin <[email protected]> > --- > .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

On the Git mailing list, Taylor Blau wrote (reply to this):

On Wed, Jul 14, 2021 at 03:25:15PM -0700, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <[email protected]> > 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 of `git 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. Agreed, and... > > - uses: actions/checkout@v2 > > with: > > - 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

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio, On Wed, 14 Jul 2021, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <[email protected]> > 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 of `git 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? Fetching into shallow clones is very expensive on the server. I want to avoid that whenever possible. > But I do not know if it is worth doing so for a small project like ours. And then there's that. Ciao, Dscho

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Taylor, On Wed, 14 Jul 2021, Taylor Blau wrote: > On Wed, Jul 14, 2021 at 03:25:15PM -0700, Junio C Hamano wrote: > > "Johannes Schindelin via GitGitGadget" <[email protected]> > > 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 of `git 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. > > Agreed, and... > > > > - uses: actions/checkout@v2 > > > with: > > > - 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. 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

gitgitgadget · 2021-07-14T22:40:51Z

User Taylor Blau <[email protected]> has been added to the cc: list.

gitgitgadget · 2021-07-14T22:55:38Z

This branch is now known as js/ci-check-whitespace-updates.

gitgitgadget · 2021-07-14T22:55:38Z

This patch series was integrated into seen via git@776d0c9.

gitgitgadget · 2021-07-16T00:15:12Z

This patch series was integrated into seen via git@3455777.

gitgitgadget · 2021-07-17T00:49:17Z

This patch series was integrated into seen via git@7cf83c9.

gitgitgadget · 2021-07-19T21:16:14Z

This patch series was integrated into seen via git@9fd0fbb.

gitgitgadget · 2021-07-19T23:14:06Z

There was a status update in the "New Topics" section about the branch js/ci-check-whitespace-updates on the Git mailing list:

CI update.

Will merge to 'next'.

gitgitgadget · 2021-07-20T15:50:40Z

This patch series was integrated into seen via git@4c922d2.

gitgitgadget · 2021-07-20T21:24:13Z

This patch series was integrated into seen via git@b796683.

gitgitgadget · 2021-07-20T22:25:49Z

This patch series was integrated into seen via git@b00d3eb.

gitgitgadget · 2021-07-21T18:02:32Z

This patch series was integrated into seen via git@ef98f70.

gitgitgadget · 2021-07-22T02:29:10Z

There was a status update in the "Cooking" section about the branch js/ci-check-whitespace-updates on the Git mailing list:

CI update.

Will merge to 'next'.

gitgitgadget · 2021-07-22T22:08:57Z

This patch series was integrated into seen via git@17248f4.

gitgitgadget · 2021-07-22T22:08:58Z

This patch series was integrated into next via git@cdc9aa0.

gitgitgadget · 2021-07-26T23:10:10Z

This patch series was integrated into seen via git@fb06ead.

gitgitgadget · 2021-07-27T05:00:00Z

There was a status update in the "Cooking" section about the branch js/ci-check-whitespace-updates on the Git mailing list:

CI update.

Will merge to 'master'.

gitgitgadget · 2021-07-28T21:47:14Z

This patch series was integrated into seen via git@cdc9aa0.

gitgitgadget · 2021-07-28T22:18:47Z

This patch series was integrated into seen via git@1a8e7ea.

gitgitgadget · 2021-07-29T00:16:41Z

There was a status update in the "Cooking" section about the branch js/ci-check-whitespace-updates on the Git mailing list:

CI update.

Will merge to 'master'.

gitgitgadget · 2021-07-30T20:54:46Z

This patch series was integrated into seen via git@f73bc92.

gitgitgadget · 2021-08-02T05:47:21Z

There was a status update in the "Cooking" section about the branch js/ci-check-whitespace-updates on the Git mailing list:

CI update.

Will merge to 'master'.

gitgitgadget · 2021-08-02T23:06:33Z

This patch series was integrated into seen via git@8a49dfa.

gitgitgadget · 2021-08-02T23:06:34Z

This patch series was integrated into next via git@8a49dfa.

gitgitgadget · 2021-08-02T23:06:34Z

This patch series was integrated into master via git@8a49dfa.

gitgitgadget · 2021-08-02T23:06:36Z

Closed via 8a49dfa.

dscho requested a review from webstech July 14, 2021 12:36

dscho force-pushed the fix-check-whitespace branch from a79d415 to 45f5dcf Compare July 14, 2021 12:55

dscho mentioned this pull request Jul 14, 2021

Fix some whitespace issues microsoft/git#398

Merged

dscho mentioned this pull request Jul 14, 2021

README: fix whitespace issue git-for-windows/git#3320

Merged

webstech approved these changes Jul 14, 2021

View reviewed changes

dscho added 2 commits July 14, 2021 22:41

dscho force-pushed the fix-check-whitespace branch from 45f5dcf to b63a5bb Compare July 14, 2021 20:43

gitgitgadget bot reviewed Jul 14, 2021

View reviewed changes

gitgitgadget bot added the seen label Jul 14, 2021

gitgitgadget bot added the next label Jul 22, 2021

gitgitgadget bot added the master label Aug 2, 2021

gitgitgadget bot closed this Aug 2, 2021

dscho deleted the fix-check-whitespace branch August 2, 2021 23:16

dscho mentioned this pull request Aug 3, 2021

[TEST] Sparse-index: the upstream version microsoft/git#401

Closed

check-whitespace: two fixes #995

check-whitespace: two fixes #995

Conversation

dscho commented Jul 14, 2021 • edited by gitgitgadget bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webstech left a comment

Choose a reason for hiding this comment

Uh oh!

dscho commented Jul 14, 2021

Uh oh!

dscho commented Jul 14, 2021

Uh oh!

gitgitgadget bot commented Jul 14, 2021

Uh oh!

gitgitgadget bot Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jul 15, 2021

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jul 15, 2021

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot commented Jul 14, 2021

Uh oh!

gitgitgadget bot commented Jul 14, 2021

Uh oh!

gitgitgadget bot commented Jul 14, 2021

Uh oh!

gitgitgadget bot commented Jul 16, 2021

Uh oh!

gitgitgadget bot commented Jul 17, 2021

Uh oh!

gitgitgadget bot commented Jul 19, 2021

Uh oh!

gitgitgadget bot commented Jul 19, 2021

Uh oh!

gitgitgadget bot commented Jul 20, 2021

Uh oh!

gitgitgadget bot commented Jul 20, 2021

Uh oh!

gitgitgadget bot commented Jul 20, 2021

Uh oh!

gitgitgadget bot commented Jul 21, 2021

Uh oh!

gitgitgadget bot commented Jul 22, 2021

Uh oh!

gitgitgadget bot commented Jul 22, 2021

Uh oh!

gitgitgadget bot commented Jul 22, 2021

Uh oh!

gitgitgadget bot commented Jul 26, 2021

Uh oh!

gitgitgadget bot commented Jul 27, 2021

Uh oh!

gitgitgadget bot commented Jul 28, 2021

Uh oh!

gitgitgadget bot commented Jul 28, 2021

Uh oh!

gitgitgadget bot commented Jul 29, 2021

Uh oh!

gitgitgadget bot commented Jul 30, 2021

Uh oh!

gitgitgadget bot commented Aug 2, 2021

Uh oh!

gitgitgadget bot commented Aug 2, 2021

Uh oh!

gitgitgadget bot commented Aug 2, 2021

Uh oh!

gitgitgadget bot commented Aug 2, 2021

Uh oh!

gitgitgadget bot commented Aug 2, 2021

Uh oh!

dscho commented Jul 14, 2021 •

edited by gitgitgadget bot

Loading