diff mbox series

[PULL,5/8] gitlab: work harder to avoid false positives in checkpatch

Message ID 20210607143303.28572-6-alex.bennee@linaro.org
State New
Headers show
Series testing and misc updates | expand

Commit Message

Alex Bennée June 7, 2021, 2:33 p.m. UTC
This copies the behaviour of patchew's configuration to make the diff
algorithm generate a minimal diff.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

Message-Id: <20210602153247.27651-1-alex.bennee@linaro.org>

-- 
2.20.1

Comments

Daniel P. Berrangé June 7, 2021, 2:50 p.m. UTC | #1
On Mon, Jun 07, 2021 at 03:33:00PM +0100, Alex Bennée wrote:
> This copies the behaviour of patchew's configuration to make the diff

> algorithm generate a minimal diff.

> 

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

> Message-Id: <20210602153247.27651-1-alex.bennee@linaro.org>

> 

> diff --git a/.gitlab-ci.d/static_checks.yml b/.gitlab-ci.d/static_checks.yml

> index 8e30872164..7e685c6a65 100644

> --- a/.gitlab-ci.d/static_checks.yml

> +++ b/.gitlab-ci.d/static_checks.yml

> @@ -3,7 +3,11 @@ check-patch:

>    image: $CI_REGISTRY_IMAGE/qemu/centos8:latest

>    needs:

>      job: amd64-centos8-container

> -  script: .gitlab-ci.d/check-patch.py

> +  script:

> +    - git config --local diff.renamelimit 0

> +    - git config --local diff.renames True

> +    - git config --local diff.algorithm histogram

> +    - .gitlab-ci.d/check-patch.py


No objection to merging this patch as is, but I wonder if we ought to
make scripts/checkpatch.pl set these options explicitly when it runs
git, eg

   git -c diff.renamelimit=0 -c diff.renames=True ...etc show

so that everyone who runs checkpatch.pl benefits from the improvement.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Alex Bennée June 7, 2021, 4:14 p.m. UTC | #2
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Jun 07, 2021 at 03:33:00PM +0100, Alex Bennée wrote:

>> This copies the behaviour of patchew's configuration to make the diff

>> algorithm generate a minimal diff.

>> 

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>> Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

>> Message-Id: <20210602153247.27651-1-alex.bennee@linaro.org>

>> 

>> diff --git a/.gitlab-ci.d/static_checks.yml b/.gitlab-ci.d/static_checks.yml

>> index 8e30872164..7e685c6a65 100644

>> --- a/.gitlab-ci.d/static_checks.yml

>> +++ b/.gitlab-ci.d/static_checks.yml

>> @@ -3,7 +3,11 @@ check-patch:

>>    image: $CI_REGISTRY_IMAGE/qemu/centos8:latest

>>    needs:

>>      job: amd64-centos8-container

>> -  script: .gitlab-ci.d/check-patch.py

>> +  script:

>> +    - git config --local diff.renamelimit 0

>> +    - git config --local diff.renames True

>> +    - git config --local diff.algorithm histogram

>> +    - .gitlab-ci.d/check-patch.py

>

> No objection to merging this patch as is, but I wonder if we ought to

> make scripts/checkpatch.pl set these options explicitly when it runs

> git, eg

>

>    git -c diff.renamelimit=0 -c diff.renames=True ...etc show

>

> so that everyone who runs checkpatch.pl benefits from the improvement.


Sure - I've generally held off messing with checkpatch directly because
of the general desire to keep it in-sync with upstream. Maybe that's
becoming less of a concern as time goes on?

[AJB says that with a totally straight face despite another patch in
this PR doing exactly that...]

>

> Regards,

> Daniel



-- 
Alex Bennée
Daniel P. Berrangé June 7, 2021, 4:19 p.m. UTC | #3
On Mon, Jun 07, 2021 at 05:14:35PM +0100, Alex Bennée wrote:
> 

> Daniel P. Berrangé <berrange@redhat.com> writes:

> 

> > On Mon, Jun 07, 2021 at 03:33:00PM +0100, Alex Bennée wrote:

> >> This copies the behaviour of patchew's configuration to make the diff

> >> algorithm generate a minimal diff.

> >> 

> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> >> Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

> >> Message-Id: <20210602153247.27651-1-alex.bennee@linaro.org>

> >> 

> >> diff --git a/.gitlab-ci.d/static_checks.yml b/.gitlab-ci.d/static_checks.yml

> >> index 8e30872164..7e685c6a65 100644

> >> --- a/.gitlab-ci.d/static_checks.yml

> >> +++ b/.gitlab-ci.d/static_checks.yml

> >> @@ -3,7 +3,11 @@ check-patch:

> >>    image: $CI_REGISTRY_IMAGE/qemu/centos8:latest

> >>    needs:

> >>      job: amd64-centos8-container

> >> -  script: .gitlab-ci.d/check-patch.py

> >> +  script:

> >> +    - git config --local diff.renamelimit 0

> >> +    - git config --local diff.renames True

> >> +    - git config --local diff.algorithm histogram

> >> +    - .gitlab-ci.d/check-patch.py

> >

> > No objection to merging this patch as is, but I wonder if we ought to

> > make scripts/checkpatch.pl set these options explicitly when it runs

> > git, eg

> >

> >    git -c diff.renamelimit=0 -c diff.renames=True ...etc show

> >

> > so that everyone who runs checkpatch.pl benefits from the improvement.

> 

> Sure - I've generally held off messing with checkpatch directly because

> of the general desire to keep it in-sync with upstream. Maybe that's

> becoming less of a concern as time goes on?


The automatic extraction of patches from git is a feature that's
custom to QEMU in checkpatch, as I implemented that a few years
back :-)


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
diff mbox series

Patch

diff --git a/.gitlab-ci.d/static_checks.yml b/.gitlab-ci.d/static_checks.yml
index 8e30872164..7e685c6a65 100644
--- a/.gitlab-ci.d/static_checks.yml
+++ b/.gitlab-ci.d/static_checks.yml
@@ -3,7 +3,11 @@  check-patch:
   image: $CI_REGISTRY_IMAGE/qemu/centos8:latest
   needs:
     job: amd64-centos8-container
-  script: .gitlab-ci.d/check-patch.py
+  script:
+    - git config --local diff.renamelimit 0
+    - git config --local diff.renames True
+    - git config --local diff.algorithm histogram
+    - .gitlab-ci.d/check-patch.py
   variables:
     GIT_DEPTH: 1000
   rules: