diff mbox series

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

Message ID b63a5bbc63ba17449a91913ab28c268db5fa3650.1626300577.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit a066a90db68da5262e81e74a50d18eaeddc6783f
Headers show
Series check-whitespace: two fixes | expand

Commit Message

Johannes Schindelin July 14, 2021, 10:09 p.m. UTC 
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(-)

Comments

Junio C Hamano July 14, 2021, 10:25 p.m. UTC | #1 
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
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 <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
Taylor Blau July 14, 2021, 10:29 p.m. UTC | #2 
On Wed, Jul 14, 2021 at 03:25:15PM -0700, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> 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
Johannes Schindelin July 15, 2021, 12:59 p.m. UTC | #3 
Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> 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
Johannes Schindelin July 15, 2021, 1 p.m. UTC | #4 
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" <gitgitgadget@gmail.com>
> > 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
diff mbox series

Patch

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