Date:   Mon, 10 Aug 2020 18:45:52 +0200
From:   Claudio Fontana <cfontana@...e.de>
To:     Andy Whitcroft <apw@...onical.com>, Joe Perches <joe@...ches.com>
Cc:     linux-kernel@...r.kernel.org
Subject: in_commit_log: is the wrong token being matched for the MAINTAINERS
 need updating message?

Hi,

while working on a downstream version of the checkpatch.pl script used for qemu,

I experienced a "bug" that I traced back to the kernel upstream version of checkpatch.pl, and hence the question:

what is the exact meaning of the variable in_commit_log, is it supposed to be including the git diff stats or not?

For example, lets take this sample useless patch as input:

-------- cut --------
>From 1873ba53039d8824e37452dc39af79b4b0327085 Mon Sep 17 00:00:00 2001
From: Claudio Fontana <cfontana@...e.de>
Date: Mon, 10 Aug 2020 15:36:47 +0200
Subject: [PATCH] XXX test patch moving file

Signed-off-by: Claudio Fontana <cfontana@...e.de>
---
 sound/Makefile              | 2 +-
 sound/{ => ac97}/ac97_bus.c | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename sound/{ => ac97}/ac97_bus.c (100%)

diff --git a/sound/Makefile b/sound/Makefile
index 797ecdcd35e2..d7a08163ea1a 100644
--- a/sound/Makefile
+++ b/sound/Makefile
@@ -9,7 +9,7 @@ obj-$(CONFIG_SND) += core/ i2c/ drivers/ isa/ pci/ ppc/ arm/ sh/ synth/ usb/ \
 obj-$(CONFIG_SND_AOA) += aoa/
 
 # This one must be compilable even if sound is configured out
-obj-$(CONFIG_AC97_BUS) += ac97_bus.o
+obj-$(CONFIG_AC97_BUS) += ac97/ac97_bus.o
 obj-$(CONFIG_AC97_BUS_NEW) += ac97/
 
 ifeq ($(CONFIG_SND),y)
diff --git a/sound/ac97_bus.c b/sound/ac97/ac97_bus.c
similarity index 100%
rename from sound/ac97_bus.c
rename to sound/ac97/ac97_bus.c
-- 
2.16.4

-------- cut --------

and using this simple patch to highlight the issue:

-------- cut --------
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 599b8c4933a7..e87d5d00a9cd 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2876,16 +2876,17 @@ sub process {
                        }
                }
 
+               print $line . "\n";
 # Check for added, moved or deleted files
                if (!$reported_maintainer_file && !$in_commit_log &&
                    ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ ||
                     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
                     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
                      (defined($1) || defined($2))))) {
+                       print("1: $1 2: $2\n");
                        $is_patch = 1;
                        $reported_maintainer_file = 1;
-                       WARN("FILE_PATH_CHANGES",
-                            "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
+                       print("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
                }
 
 # Check for adding new DT bindings not in schema format
-------- cut --------

We can see that in the sample patch input we are matching the line

 sound/{ => ac97}/ac97_bus.c | 0

with the regex ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/

Is this actually wanted?

I would expect that in this case we want instead to match the lines:

rename from sound/ac97_bus.c
rename to sound/ac97/ac97_bus.c

It does not hurt in the kernel, but it does hurt in my downstream patch, where I try to make use of $realfile ,
which is not set if I match too early in the patch. 

Would it make sense to move this chunk earlier:


# Check if it's the start of a commit log                                                                                                   
# (not a header line and we haven't seen the patch filename)                                                                                
                if ($in_header_lines && $realfile =~ /^$/ &&
                    !($rawline =~ /^\s+(?:\S|$)/ ||
                      $rawline =~ /^(?:commit\b|from\b|[\w-]+:)/i)) {
                        $in_header_lines = 0;
                        $in_commit_log = 1;
                        $has_commit_log = 1;
                }

so that the check: 

if (!$reported_maintainer_file && !$in_commit_log

fails for the testcase shown (in_commit_log ends up as true), and the match will happen later, where realfile is set and available for use?

Thanks!

Claudio


-- 
Claudio Fontana
Engineering Manager Virtualization, SUSE Labs Core

SUSE Software Solutions Italy Srl

Powered by blists - more mailing lists