[PATCH] checkpatch: Ignore removed lines in license check

[PATCH] checkpatch: Ignore removed lines in license check

[Nabih Estefan]

When running the license check, if we are updating a license it is
possible for the checkpatch script to test against old license lines
instead of newer ones, since the removal lines appear before the
addition lines in a .patch file.

Fix this by skipping over lines that start with "-" in the checkpatch
script.

Signed-off-by: Nabih Estefan <nabihestefan@google.com>
---
 scripts/checkpatch.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 833f20f555..c57a423f9f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1813,7 +1813,8 @@ sub process {
 		}
 
 # Check SPDX-License-Identifier references a permitted license
-		if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
+		if (($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) &&
+			$rawline !~ /^-/) {
 			$fileinfo->{facts}->{sawspdx} = 1;
 			&checkspdx($realfile, $1);
 		}
-- 
2.51.0.384.g4c02a37b29-goog

Re: [PATCH] checkpatch: Ignore removed lines in license check

[Alex Bennée]

Nabih Estefan <nabihestefan@google.com> writes:

> When running the license check, if we are updating a license it is
> possible for the checkpatch script to test against old license lines
> instead of newer ones, since the removal lines appear before the
> addition lines in a .patch file.
>
> Fix this by skipping over lines that start with "-" in the checkpatch
> script.
>
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] checkpatch: Ignore removed lines in license check

[Daniel P. Berrangé]

On Tue, Sep 16, 2025 at 04:59:28PM +0000, Nabih Estefan wrote:
> When running the license check, if we are updating a license it is
> possible for the checkpatch script to test against old license lines
> instead of newer ones, since the removal lines appear before the
> addition lines in a .patch file.

While we match the "SPDX-License-Identifier" text in any context,
the "file must have SDPX" validation is only performed against
files that are entirely new: 

  # Called at the end of processing a diff hunk for a file
  sub process_end_of_file {
        my $fileinfo = shift;

        if ($fileinfo->{action} eq "new" &&
            !exists $fileinfo->{facts}->{sawspdx}) {
             ...raise error ....

> Fix this by skipping over lines that start with "-" in the checkpatch
> script.

A new file cannot have any "-" lines present, so there isn't any
bug that needs fixing AFAICT.  Can you show any patch or commit
where this would have made a difference to what checkpatch.pl
reports ?

> 
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> ---
>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 833f20f555..c57a423f9f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1813,7 +1813,8 @@ sub process {
>  		}
>  
>  # Check SPDX-License-Identifier references a permitted license
> -		if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
> +		if (($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) &&
> +			$rawline !~ /^-/) {
>  			$fileinfo->{facts}->{sawspdx} = 1;
>  			&checkspdx($realfile, $1);
>  		}
> -- 
> 2.51.0.384.g4c02a37b29-goog
> 

With 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 :|

Re: [PATCH] checkpatch: Ignore removed lines in license check

[Nabih Estefan]

We ran it against an internal patch that we were updating, so I can't
show you the patch.

However, the difference on it being affected might be in how we're
running it? To check
against just the changes being done in the specific patch, instead of
the whole file, we
trigger it by running `./scripts/checkpatch.pl --branch HEAD...HEAD^`.
Could that be
why it's triggering against existing files?

Thanks,
Nabih

On Wed, Sep 17, 2025 at 2:24 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Sep 16, 2025 at 04:59:28PM +0000, Nabih Estefan wrote:
> > When running the license check, if we are updating a license it is
> > possible for the checkpatch script to test against old license lines
> > instead of newer ones, since the removal lines appear before the
> > addition lines in a .patch file.
>
> While we match the "SPDX-License-Identifier" text in any context,
> the "file must have SDPX" validation is only performed against
> files that are entirely new:
>
>   # Called at the end of processing a diff hunk for a file
>   sub process_end_of_file {
>         my $fileinfo = shift;
>
>         if ($fileinfo->{action} eq "new" &&
>             !exists $fileinfo->{facts}->{sawspdx}) {
>              ...raise error ....
>
> > Fix this by skipping over lines that start with "-" in the checkpatch
> > script.
>
> A new file cannot have any "-" lines present, so there isn't any
> bug that needs fixing AFAICT.  Can you show any patch or commit
> where this would have made a difference to what checkpatch.pl
> reports ?
>
> >
> > Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> > ---
> >  scripts/checkpatch.pl | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 833f20f555..c57a423f9f 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1813,7 +1813,8 @@ sub process {
> >               }
> >
> >  # Check SPDX-License-Identifier references a permitted license
> > -             if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
> > +             if (($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) &&
> > +                     $rawline !~ /^-/) {
> >                       $fileinfo->{facts}->{sawspdx} = 1;
> >                       &checkspdx($realfile, $1);
> >               }
> > --
> > 2.51.0.384.g4c02a37b29-goog
> >
>
> With 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 :|
>

Re: [PATCH] checkpatch: Ignore removed lines in license check

[Daniel P. Berrangé]

On Wed, Sep 17, 2025 at 08:16:53AM -0700, Nabih Estefan wrote:
> We ran it against an internal patch that we were updating, so I can't
> show you the patch.

If you can't share that private patch, perhaps you can create a
dummy patch with the same type of diff structure that shows the
problem ?

> However, the difference on it being affected might be in how we're
> running it? To check
> against just the changes being done in the specific patch, instead of
> the whole file, we
> trigger it by running `./scripts/checkpatch.pl --branch HEAD...HEAD^`.
> Could that be
> why it's triggering against existing files?

I don't think that's a problem. It is just a different way ot getting
a list of git commit hashes to analyse - it'll still operate against
a patch diff IIUC.

FWIW, I use   'checkpatch.pl master..' and/or 'git show| checkpatch.pl -'

> 
> Thanks,
> Nabih
> 
> On Wed, Sep 17, 2025 at 2:24 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Tue, Sep 16, 2025 at 04:59:28PM +0000, Nabih Estefan wrote:
> > > When running the license check, if we are updating a license it is
> > > possible for the checkpatch script to test against old license lines
> > > instead of newer ones, since the removal lines appear before the
> > > addition lines in a .patch file.
> >
> > While we match the "SPDX-License-Identifier" text in any context,
> > the "file must have SDPX" validation is only performed against
> > files that are entirely new:
> >
> >   # Called at the end of processing a diff hunk for a file
> >   sub process_end_of_file {
> >         my $fileinfo = shift;
> >
> >         if ($fileinfo->{action} eq "new" &&
> >             !exists $fileinfo->{facts}->{sawspdx}) {
> >              ...raise error ....
> >
> > > Fix this by skipping over lines that start with "-" in the checkpatch
> > > script.
> >
> > A new file cannot have any "-" lines present, so there isn't any
> > bug that needs fixing AFAICT.  Can you show any patch or commit
> > where this would have made a difference to what checkpatch.pl
> > reports ?
> >
> > >
> > > Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> > > ---
> > >  scripts/checkpatch.pl | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 833f20f555..c57a423f9f 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -1813,7 +1813,8 @@ sub process {
> > >               }
> > >
> > >  # Check SPDX-License-Identifier references a permitted license
> > > -             if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
> > > +             if (($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) &&
> > > +                     $rawline !~ /^-/) {
> > >                       $fileinfo->{facts}->{sawspdx} = 1;
> > >                       &checkspdx($realfile, $1);
> > >               }
> > > --
> > > 2.51.0.384.g4c02a37b29-goog
> > >
> >
> > With 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 :|
> >
> 

With 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 :|

Re: [PATCH] checkpatch: Ignore removed lines in license check

[Nabih Estefan]

On Wed, Sep 17, 2025 at 9:05 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Sep 17, 2025 at 08:16:53AM -0700, Nabih Estefan wrote:
> > We ran it against an internal patch that we were updating, so I can't
> > show you the patch.
>
> If you can't share that private patch, perhaps you can create a
> dummy patch with the same type of diff structure that shows the
> problem ?
>
The patch looks something like the following:

```
Subject: [PATCH] path/to/fake/file.c: Fix unacceptable license

These source files trigger the following error in ./scripts/checkpatch.pl:

ERROR: Saw unacceptable licenses 'GPL-2.0', valid choices for QEMU are:
GPL-2.0-or-later
---
path/to/fake/file.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/path/to/fake/file.c b/path/to/fake/file.c
index a7604f014e..043ead35 106892
--- a/path/to/fake/file.c
+++ b/path/to/fake/file.c
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0
+// SPDX-License-Identifier: GPL-2.0-or-later
/*
* Header of Fake file

```
It's just changing the file from an invalid  to a valid one, nothing else is
being done, which is why I'm confused how it's triggering this.

> > However, the difference on it being affected might be in how we're
> > running it? To check
> > against just the changes being done in the specific patch, instead of
> > the whole file, we
> > trigger it by running `./scripts/checkpatch.pl --branch HEAD...HEAD^`.
> > Could that be
> > why it's triggering against existing files?
>
> I don't think that's a problem. It is just a different way ot getting
> a list of git commit hashes to analyse - it'll still operate against
> a patch diff IIUC.
>
> FWIW, I use   'checkpatch.pl master..' and/or 'git show| checkpatch.pl -'
>
> >
> > Thanks,
> > Nabih
> >
> > On Wed, Sep 17, 2025 at 2:24 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > On Tue, Sep 16, 2025 at 04:59:28PM +0000, Nabih Estefan wrote:
> > > > When running the license check, if we are updating a license it is
> > > > possible for the checkpatch script to test against old license lines
> > > > instead of newer ones, since the removal lines appear before the
> > > > addition lines in a .patch file.
> > >
> > > While we match the "SPDX-License-Identifier" text in any context,
> > > the "file must have SDPX" validation is only performed against
> > > files that are entirely new:
> > >
> > >   # Called at the end of processing a diff hunk for a file
> > >   sub process_end_of_file {
> > >         my $fileinfo = shift;
> > >
> > >         if ($fileinfo->{action} eq "new" &&
> > >             !exists $fileinfo->{facts}->{sawspdx}) {
> > >              ...raise error ....
> > >
> > > > Fix this by skipping over lines that start with "-" in the checkpatch
> > > > script.
> > >
> > > A new file cannot have any "-" lines present, so there isn't any
> > > bug that needs fixing AFAICT.  Can you show any patch or commit
> > > where this would have made a difference to what checkpatch.pl
> > > reports ?
> > >
> > > >
> > > > Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> > > > ---
> > > >  scripts/checkpatch.pl | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > index 833f20f555..c57a423f9f 100755
> > > > --- a/scripts/checkpatch.pl
> > > > +++ b/scripts/checkpatch.pl
> > > > @@ -1813,7 +1813,8 @@ sub process {
> > > >               }
> > > >
> > > >  # Check SPDX-License-Identifier references a permitted license
> > > > -             if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
> > > > +             if (($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) &&
> > > > +                     $rawline !~ /^-/) {
> > > >                       $fileinfo->{facts}->{sawspdx} = 1;
> > > >                       &checkspdx($realfile, $1);
> > > >               }
> > > > --
> > > > 2.51.0.384.g4c02a37b29-goog
> > > >
> > >
> > > With 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 :|
> > >
> >
>
> With 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 :|
>

Re: [PATCH] checkpatch: Ignore removed lines in license check

[Daniel P. Berrangé]

On Wed, Sep 17, 2025 at 09:17:57AM -0700, Nabih Estefan wrote:
> On Wed, Sep 17, 2025 at 9:05 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Wed, Sep 17, 2025 at 08:16:53AM -0700, Nabih Estefan wrote:
> > > We ran it against an internal patch that we were updating, so I can't
> > > show you the patch.
> >
> > If you can't share that private patch, perhaps you can create a
> > dummy patch with the same type of diff structure that shows the
> > problem ?
> >
> The patch looks something like the following:

snip

> It's just changing the file from an invalid  to a valid one, nothing else is
> being done, which is why I'm confused how it's triggering this.

Ahhh, I was getting confused about which SPDX check was being
triggered.

The one that runs against "new" files only is a check for the
existence of a SPDX tag.

The one that runs against all files is a check for the declared
license check.

So yes, your patch here is correct & required so

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


> 
> > > However, the difference on it being affected might be in how we're
> > > running it? To check
> > > against just the changes being done in the specific patch, instead of
> > > the whole file, we
> > > trigger it by running `./scripts/checkpatch.pl --branch HEAD...HEAD^`.
> > > Could that be
> > > why it's triggering against existing files?
> >
> > I don't think that's a problem. It is just a different way ot getting
> > a list of git commit hashes to analyse - it'll still operate against
> > a patch diff IIUC.
> >
> > FWIW, I use   'checkpatch.pl master..' and/or 'git show| checkpatch.pl -'
> >
> > >
> > > Thanks,
> > > Nabih
> > >
> > > On Wed, Sep 17, 2025 at 2:24 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > >
> > > > On Tue, Sep 16, 2025 at 04:59:28PM +0000, Nabih Estefan wrote:
> > > > > When running the license check, if we are updating a license it is
> > > > > possible for the checkpatch script to test against old license lines
> > > > > instead of newer ones, since the removal lines appear before the
> > > > > addition lines in a .patch file.
> > > >
> > > > While we match the "SPDX-License-Identifier" text in any context,
> > > > the "file must have SDPX" validation is only performed against
> > > > files that are entirely new:
> > > >
> > > >   # Called at the end of processing a diff hunk for a file
> > > >   sub process_end_of_file {
> > > >         my $fileinfo = shift;
> > > >
> > > >         if ($fileinfo->{action} eq "new" &&
> > > >             !exists $fileinfo->{facts}->{sawspdx}) {
> > > >              ...raise error ....
> > > >
> > > > > Fix this by skipping over lines that start with "-" in the checkpatch
> > > > > script.
> > > >
> > > > A new file cannot have any "-" lines present, so there isn't any
> > > > bug that needs fixing AFAICT.  Can you show any patch or commit
> > > > where this would have made a difference to what checkpatch.pl
> > > > reports ?
> > > >
> > > > >
> > > > > Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> > > > > ---
> > > > >  scripts/checkpatch.pl | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > > index 833f20f555..c57a423f9f 100755
> > > > > --- a/scripts/checkpatch.pl
> > > > > +++ b/scripts/checkpatch.pl
> > > > > @@ -1813,7 +1813,8 @@ sub process {
> > > > >               }
> > > > >
> > > > >  # Check SPDX-License-Identifier references a permitted license
> > > > > -             if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
> > > > > +             if (($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) &&
> > > > > +                     $rawline !~ /^-/) {
> > > > >                       $fileinfo->{facts}->{sawspdx} = 1;
> > > > >                       &checkspdx($realfile, $1);
> > > > >               }
> > > > > --
> > > > > 2.51.0.384.g4c02a37b29-goog
> > > > >
> > > >
> > > > With 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 :|
> > > >
> > >
> >
> > With 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 :|
> >
> 

With 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 :|

Re: [PATCH] checkpatch: Ignore removed lines in license check

[Alex Bennée]

Nabih Estefan <nabihestefan@google.com> writes:

> When running the license check, if we are updating a license it is
> possible for the checkpatch script to test against old license lines
> instead of newer ones, since the removal lines appear before the
> addition lines in a .patch file.
>
> Fix this by skipping over lines that start with "-" in the checkpatch
> script.
>
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>

Queued to testing/next, thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro