On Tue, Jul 14, 2020 at 1:16 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
wrote:

>
>
> On Mon, 13 Jul 2020, Mrinal Pandey wrote:
>
> > In all the scripts, the SPDX license should be on the second line,
> > the first line being the "sh-bang", but checkpatch issues a warning
> > "Misplaced SPDX-License-Identifier tag - use line 1 instead" for the
> > scripts that have SPDX license in the second line.
> >
> > However, this warning is not issued when checkpatch is run on a file
> using
> > `-f` option. The case for files has been handled gracefully by changing
> > `$checklicenseline` to `2` but a corresponding check when running
> checkpatch
> > on a commit hash is missing.
> >
> > I noticed this false positive while running checkpatch on the set of
> > commits from v5.7 to v5.8-rc1 of the kernel on the commits which modified
> > a script file.
> >
> > This check is missing in checkpatch since commit a8da38a9cf0e
> > ("checkpatch: add test for SPDX-License-Identifier on wrong line #")
> > when the corresponding rule was first commited.
> >
> > Fix this by setting `$checklicenseline` to `2` when the diff content that
> > is being checked originates from a script, thus, informing checkpatch
> that
> > the SPDX license should be on the second line.
> >
> > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> > ---
> >  scripts/checkpatch.pl | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 4c820607540b..bbffd0c4449d 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3218,6 +3218,9 @@ sub process {
> >               next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
> >
> >  # check for using SPDX-License-Identifier on the wrong line number
> > +             if ($realfile =~ /^scripts/) {
> > +                    $checklicenseline = 2;
> > +             }
>
> I think this is somehow wrong here. The check for checklicenseline = 2
> looks very different above.
>
> Why does -f work and using a patch file not work?
>

Sir,

I am going to explain my observation based on file
`scripts/atomic/gen-atomic-fallback.sh` and
commit hash `37f8173dd849`.

If we are checking against the file, `checklicenseline` is set to 1 and
when `realline` is 1 the above
`if` block is triggered, then we check if this line is of the form `#!/`
using the regular expression
`^[ \+]\s*\#\!\s*\/`. If this is the case we set `checklicenseline` to `2`
informing checkpatch that it should
expect license on the second line and this works all fine for a file.
The `if` block below my proposed changes evaluates to false in this case
and thus it emits no false warning.

However, If we are checking a diff content, the above `if` block is not
triggered at all. This is
because `realline` stores the actual line number of the line we are
checking currently out of diff content.
This value is 2 because SPDX identifier is indeed at the second line in the
file but `checklicenseline` is still `1`.
`realline` will never become equal to 1 again and thus the above `if`
condition will never be true in this case.
Even if the above `if` block is triggered it would not update
`checklicenseline` to 2 as the regular expression
is not satisfied since we don't have sh-bang in diff content and just the
SPDX tag.
If we don't do this, the `if` block below evaluates to true when `realline`
is 2 and `checklicensline` is `1` leading
to the emission of a false warning.

So, what I did was to check if the diff content we are checking actually
comes from a script, if yes, we can set
`checklicenseline` to `2` to avoid this confusion.

Please let me know if this is reasonable.

Thank you.


>
> Lukas
>
>
> >               if ($realline != $checklicenseline &&
> >                   $rawline =~ /\bSPDX-License-Identifier:/ &&
> >                   substr($line, @-, @+ - @-) eq "$;" x (@+ - @-)) {
> > --
> > 2.25.1
> >
> >
>