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

4 messages, 3 authors, 2021-07-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2021-07-14 22:25:21 

"Johannes Schindelin via GitGitGadget" [off-list ref]
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.
quoted hunk
Signed-off-by: Johannes Schindelin <redacted>
---
 .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

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

From: Taylor Blau <hidden>
Date: 2021-07-14 22:29:49 

On Wed, Jul 14, 2021 at 03:25:15PM -0700, Junio C Hamano wrote:
"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:
quoted
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...
quoted
     - 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

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

From: Johannes Schindelin <hidden>
Date: 2021-07-15 12:59:23 

Hi Junio,

On Wed, 14 Jul 2021, Junio C Hamano wrote:

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:
quoted
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

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

From: Johannes Schindelin <hidden>
Date: 2021-07-15 13:00:17 

Hi Taylor,

On Wed, 14 Jul 2021, Taylor Blau wrote:

On Wed, Jul 14, 2021 at 03:25:15PM -0700, Junio C Hamano wrote:
quoted
"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:
quoted
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...
quoted
quoted
     - 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help