{"thread":{"id":"56108","subject":"[PATCH 0/2] check-whitespace: two fixes","startedAt":"2021-07-14T22:09:42Z","lastAt":"2021-07-15T13:00:17Z","messageCount":8,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Taylor Blau","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"430138","messageId":"pull.995.git.1626300577.gitgitgadget@gmail.com","threadId":"56108","inReplyTo":null,"subject":"[PATCH 0/2] check-whitespace: two fixes","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-14T22:09:35Z","receivedAt":"2021-07-14T22:09:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I recently ran into a situation where a merge commit (which admittedly are\nundesirable in git/git and gitgitgadget/git Pull Requests, but are sometimes\ntotally appropriate in the context of other forks such as\ngit-for-windows/git) misled the logic in the check-whitespace workflow to\nlook at upstream commits (and missing some patches that it should have\nlooked at).\n\nAt the same time, I also realized that the feature where this workflow adds\na PR comment in an attempt to be more helpful requires a read/write token\n(which weakens the overall security, I'd much rather stay with the read-only\ntoken configured e.g. in gitgitgadget/git and in git-for-windows/git).\n\nThis patch series addresses both issues.\n\nJohannes Schindelin (2):\n ci(check-whitespace): stop requiring a read/write token\n ci(check-whitespace): restrict to the intended commits\n\n .github/workflows/check-whitespace.yml | 26 ++------------------------\n 1 file changed, 2 insertions(+), 24 deletions(-)\n\n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-995%2Fdscho%2Ffix-check-whitespace-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-995/dscho/fix-check-whitespace-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/995\n-- \ngitgitgadget\n"},{"id":"430139","messageId":"b54c137cb14527c14bd7c990f9cf32dca50ba5dc.1626300577.git.gitgitgadget@gmail.com","threadId":"56108","inReplyTo":"pull.995.git.1626300577.gitgitgadget@gmail.com","subject":"[PATCH 1/2] ci(check-whitespace): stop requiring a read/write token","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-14T22:09:36Z","receivedAt":"2021-07-14T22:09:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nAs part of some recent security tightening, GitHub introduced the\nability to configure GitHub workflows to be run with a read-only token.\nThis is much more secure, in particular when working in a public\nrepository: While the regular read/write token might be restricted to\nwriting to the current branch, it is not necessarily restricted to\naccess only the current Pull Request.\n\nHowever, the `check-whitespace` workflow threw a wrench into this plan:\nit _requires_ write access (because it wants to add a PR comment in case\nof a whitespace issue).\n\nLet's just skip that PR comment. The user can always click through to\nthe actual error, even if it is slightly less convenient.\n\nSigned-off-by: Johannes Schindelin \n---\n .github/workflows/check-whitespace.yml | 16 ----------------\n 1 file changed, 16 deletions(-)\n\ndiff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml\nindex f1483059c76..c53614d6033 100644\n--- a/.github/workflows/check-whitespace.yml\n+++ b/.github/workflows/check-whitespace.yml\n@@ -51,21 +51,5 @@ jobs:\n \n if test -n \"${log}\"\n then\n- echo \"::set-output name=checkout::\"${log}\"\"\n exit 2\n fi\n-\n- - name: Add Check Output as Comment\n- uses: actions/github-script@v3\n- id: add-comment\n- env:\n- log: ${{ steps.check_out.outputs.checkout }}\n- with:\n- script: |\n- await github.issues.createComment({\n- issue_number: context.issue.number,\n- owner: context.repo.owner,\n- repo: context.repo.repo,\n- body: `Whitespace errors found in workflow ${{ github.workflow }}:\\n\\n\\`\\`\\`\\n${process.env.log.replace(/\\\\n/g, \"\\n\")}\\n\\`\\`\\``\n- })\n- if: ${{ failure() }}\n-- \ngitgitgadget\n\n"},{"id":"430140","messageId":"b63a5bbc63ba17449a91913ab28c268db5fa3650.1626300577.git.gitgitgadget@gmail.com","threadId":"56108","inReplyTo":"pull.995.git.1626300577.gitgitgadget@gmail.com","subject":"[PATCH 2/2] ci(check-whitespace): restrict to the intended commits","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-14T22:09:37Z","receivedAt":"2021-07-14T22:09:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nDuring a run of the `check-whitespace` we want to verify that the\ncommits introduced in the Pull Request have no whitespace issues. We\nonly want to look at those commits, not the upstream commits (because\nthe contributor cannot do anything about the latter).\n\nHowever, by using the `-` form in `git log --check`, we run the\nrisk of looking at the wrong commits. The reason is that the\n`actions/checkout` step does _not_ check out the tip commit of the Pull\nRequest's branch: Instead, it checks out a merge commit that merges that\nbranch into the target branch. For that reason, we already adjust the\ncommit count by incrementing it, but that is not enough: if the upstream\nbranch has newer commits, they are traversed _first_. And obviously we\nwill then miss some of the commits that we _actually_ wanted to look at.\n\nTherefore, let's be careful to stop assuming a linear, up to date commit\ntopology in the contributed commits, and instead specify the correct\ncommit range.\n\nUnfortunately, this means that we no longer can rely on a shallow clone:\nThere is no way of knowing just how many commits the upstream branch\nadvanced after the commit from which the PR branch branched off. So\nlet's just go with a full clone instead, and be safe rather than sorry\n(if we have \"too shallow\" a situation, a commit range `@{u}..` may very\nwell include a shallow commit itself, and the output of `git show\n--check ` is _not_ pretty).\n\nSigned-off-by: Johannes Schindelin \n---\n .github/workflows/check-whitespace.yml | 10 ++--------\n 1 file changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml\nindex c53614d6033..8c4358d805c 100644\n--- a/.github/workflows/check-whitespace.yml\n+++ b/.github/workflows/check-whitespace.yml\n@@ -12,15 +12,9 @@ jobs:\n check-whitespace:\n runs-on: ubuntu-latest\n steps:\n- - name: Set commit count\n- shell: bash\n- run: echo \"COMMIT_DEPTH=$((1+$COMMITS))\" >>$GITHUB_ENV\n- env:\n- COMMITS: ${{ github.event.pull_request.commits }}\n-\n - uses: actions/checkout@v2\n with:\n- fetch-depth: ${{ env.COMMIT_DEPTH }}\n+ fetch-depth: 0\n \n - name: git log --check\n id: check_out\n@@ -47,7 +41,7 @@ jobs:\n echo \"${dash} ${etc}\"\n ;;\n esac\n- done <<< $(git log --check --pretty=format:\"---% h% s\" -${{github.event.pull_request.commits}})\n+ done <<< $(git log --check --pretty=format:\"---% h% s\" ${{github.event.pull_request.base.sha}}..)\n \n if test -n \"${log}\"\n then\n-- \ngitgitgadget\n"},{"id":"430142","messageId":"xmqq1r80v8p0.fsf@gitster.g","threadId":"56108","inReplyTo":"b54c137cb14527c14bd7c990f9cf32dca50ba5dc.1626300577.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] ci(check-whitespace): stop requiring a read/write token","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-14T22:20:43Z","receivedAt":"2021-07-14T22:20:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> However, the `check-whitespace` workflow threw a wrench into this plan:\n> it _requires_ write access (because it wants to add a PR comment in case\n> of a whitespace issue).\n\nOK.\n"},{"id":"430145","messageId":"xmqqwnpsttx0.fsf@gitster.g","threadId":"56108","inReplyTo":"b63a5bbc63ba17449a91913ab28c268db5fa3650.1626300577.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] ci(check-whitespace): restrict to the intended commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-14T22:25:15Z","receivedAt":"2021-07-14T22:25:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> Unfortunately, this means that we no longer can rely on a shallow clone:\n> There is no way of knowing just how many commits the upstream branch\n> advanced after the commit from which the PR branch branched off. So\n> let's just go with a full clone instead, and be safe rather than sorry\n> (if we have \"too shallow\" a situation, a commit range `@{u}..` may very\n> well include a shallow commit itself, and the output of `git show\n> --check ` is _not_ pretty).\n\nMakes sense.\n\nAs long as you have pull-request base, I suspect that you could\nshallow clone the base and incrementally fetch the rest to update,\nperhaps? But I do not know if it is worth doing so for a small\nproject like ours.\n\nThanks.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> .github/workflows/check-whitespace.yml | 10 ++--------\n> 1 file changed, 2 insertions(+), 8 deletions(-)\n>\n> diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml\n> index c53614d6033..8c4358d805c 100644\n> --- a/.github/workflows/check-whitespace.yml\n> +++ b/.github/workflows/check-whitespace.yml\n> @@ -12,15 +12,9 @@ jobs:\n> check-whitespace:\n> runs-on: ubuntu-latest\n> steps:\n> - - name: Set commit count\n> - shell: bash\n> - run: echo \"COMMIT_DEPTH=$((1+$COMMITS))\" >>$GITHUB_ENV\n> - env:\n> - COMMITS: ${{ github.event.pull_request.commits }}\n> -\n> - uses: actions/checkout@v2\n> with:\n> - fetch-depth: ${{ env.COMMIT_DEPTH }}\n> + fetch-depth: 0\n> \n> - name: git log --check\n> id: check_out\n> @@ -47,7 +41,7 @@ jobs:\n> echo \"${dash} ${etc}\"\n> ;;\n> esac\n> - done <<< $(git log --check --pretty=format:\"---% h% s\" -${{github.event.pull_request.commits}})\n> + done <<< $(git log --check --pretty=format:\"---% h% s\" ${{github.event.pull_request.base.sha}}..)\n> \n> if test -n \"${log}\"\n> then\n"},{"id":"430149","messageId":"YO9lWk4NVfe/yK8Y@nand.local","threadId":"56108","inReplyTo":"xmqqwnpsttx0.fsf@gitster.g","subject":"Re: [PATCH 2/2] ci(check-whitespace): restrict to the intended commits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-14T22:29:46Z","receivedAt":"2021-07-14T22:29:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 14, 2021 at 03:25:15PM -0700, Junio C Hamano wrote:\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > Unfortunately, this means that we no longer can rely on a shallow clone:\n> > There is no way of knowing just how many commits the upstream branch\n> > advanced after the commit from which the PR branch branched off. So\n> > let's just go with a full clone instead, and be safe rather than sorry\n> > (if we have \"too shallow\" a situation, a commit range `@{u}..` may very\n> > well include a shallow commit itself, and the output of `git show\n> > --check ` is _not_ pretty).\n>\n> Makes sense.\n>\n> As long as you have pull-request base, I suspect that you could\n> shallow clone the base and incrementally fetch the rest to update,\n> perhaps? But I do not know if it is worth doing so for a small\n> project like ours.\n\nAgreed, and...\n\n> > - uses: actions/checkout@v2\n> > with:\n> > - fetch-depth: ${{ env.COMMIT_DEPTH }}\n> > + fetch-depth: 0\n\n...I wondered whether \"fetch-depth: 0\" was the default and whether or\nnot this hunk could have just removed \"fetch-depth\" entirely. But the\ndefault is \"1\", and \"0\" means \"fetch everything\". So we really do need\nit.\n\nThanks,\nTaylor\n"},{"id":"430197","messageId":"nycvar.QRO.7.76.6.2107151458320.59@tvgsbejvaqbjf.bet","threadId":"56108","inReplyTo":"xmqqwnpsttx0.fsf@gitster.g","subject":"Re: [PATCH 2/2] ci(check-whitespace): restrict to the intended commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-07-15T12:59:15Z","receivedAt":"2021-07-15T12:59:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 14 Jul 2021, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > Unfortunately, this means that we no longer can rely on a shallow clone:\n> > There is no way of knowing just how many commits the upstream branch\n> > advanced after the commit from which the PR branch branched off. So\n> > let's just go with a full clone instead, and be safe rather than sorry\n> > (if we have \"too shallow\" a situation, a commit range `@{u}..` may very\n> > well include a shallow commit itself, and the output of `git show\n> > --check ` is _not_ pretty).\n>\n> Makes sense.\n>\n> As long as you have pull-request base, I suspect that you could\n> shallow clone the base and incrementally fetch the rest to update,\n> perhaps?\n\nFetching into shallow clones is very expensive on the server. I want to\navoid that whenever possible.\n\n> But I do not know if it is worth doing so for a small project like ours.\n\nAnd then there's that.\n\nCiao,\nDscho\n"},{"id":"430198","messageId":"nycvar.QRO.7.76.6.2107151500000.59@tvgsbejvaqbjf.bet","threadId":"56108","inReplyTo":"YO9lWk4NVfe/yK8Y@nand.local","subject":"Re: [PATCH 2/2] ci(check-whitespace): restrict to the intended commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-07-15T13:00:07Z","receivedAt":"2021-07-15T13:00:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Taylor,\n\nOn Wed, 14 Jul 2021, Taylor Blau wrote:\n\n> On Wed, Jul 14, 2021 at 03:25:15PM -0700, Junio C Hamano wrote:\n> > \"Johannes Schindelin via GitGitGadget\" \n> > writes:\n> >\n> > > Unfortunately, this means that we no longer can rely on a shallow clone:\n> > > There is no way of knowing just how many commits the upstream branch\n> > > advanced after the commit from which the PR branch branched off. So\n> > > let's just go with a full clone instead, and be safe rather than sorry\n> > > (if we have \"too shallow\" a situation, a commit range `@{u}..` may very\n> > > well include a shallow commit itself, and the output of `git show\n> > > --check ` is _not_ pretty).\n> >\n> > Makes sense.\n> >\n> > As long as you have pull-request base, I suspect that you could\n> > shallow clone the base and incrementally fetch the rest to update,\n> > perhaps? But I do not know if it is worth doing so for a small\n> > project like ours.\n>\n> Agreed, and...\n>\n> > > - uses: actions/checkout@v2\n> > > with:\n> > > - fetch-depth: ${{ env.COMMIT_DEPTH }}\n> > > + fetch-depth: 0\n>\n> ...I wondered whether \"fetch-depth: 0\" was the default and whether or\n> not this hunk could have just removed \"fetch-depth\" entirely. But the\n> default is \"1\", and \"0\" means \"fetch everything\". So we really do need\n> it.\n\nIndeed. One of the things that makes `action/checkout@v2` so much faster\nthan `@v1` is that it fetches shallow by default. But we can't have that\nhere.\n\nCiao,\nDscho\n"}]}