lkml.org 
[lkml]   [2020]   [Aug]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
From
Subjectin_commit_log: is the wrong token being matched for the MAINTAINERS need updating message?
Date
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@suse.de>
Date: Mon, 10 Aug 2020 15:36:47 +0200
Subject: [PATCH] XXX test patch moving file

Signed-off-by: Claudio Fontana <cfontana@suse.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

\
 
 \ /
  Last update: 2020-08-10 18:47    [W:0.030 / U:0.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site