[RFC PATCH] checkpatch: correctly detect lines of help text

* [RFC PATCH] checkpatch: correctly detect lines of help text
@ 2020-12-02 18:27 Nicolai Fischer
  2020-12-02 18:54 ` Joe Perches
  0 siblings, 1 reply; 8+ messages in thread
From: Nicolai Fischer @ 2020-12-02 18:27 UTC (permalink / raw)
  To: linux-kernel; +Cc: apw, joe, johannes.czekay, linux-kernel

Currently, checkpatch uses keywords to determine the end
of a Kconfig help message which leads to false positives:

1) if a line of the help text starts with any of the keywords, e.g. if:

+config FOO
+	help
+	  help text
+	  if condition
+	  previous line causes warning
+	  last line.

2) if the help attribute is not specified last, checkpatch counts
other attributes like depends on towards the line count:

+config FOO
+	help
+	bool "no help message, but passes checkpatch"
+	default n
+	depends on SYSFS
+	depends on MULTIUSER

This patch fixes this behavior by using the indentation to determine
the end of the help message.

Signed-off-by: Nicolai Fischer <nicolai.fischer@fau.de>
Co-developed-by: Johannes Czekay <johannes.czekay@fau.de>
Signed-off-by: Johannes Czekay <johannes.czekay@fau.de>
---


The code responsible for counting the lines of the help message
seems overly complicated and we could rewrite it entirely
in order to be more clear and compact if requested.


Additionally this if block is only meant to run
when adding a new config option, because, as the comment indicates,
otherwise the diff context might not include the whole help message.
However if one renames an option, the regex matches as well and
might trigger another false positive.
This could potentially be addressed in the warning message,
though we are happy for any input on this.


 scripts/checkpatch.pl | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7dc094445d83..671b369a39d4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3234,6 +3234,7 @@ sub process {
 			my $f;
 			my $is_start = 0;
 			my $is_end = 0;
+			my $help_indent;
 			for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) {
 				$f = $lines[$ln - 1];
 				$cnt-- if ($lines[$ln - 1] !~ /^-/);
@@ -3245,7 +3246,12 @@ sub process {
 				if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
 					$is_start = 1;
 				} elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) {
-					$length = -1;
+					$length = 0;
+					if (defined $lines[$ln]) {
+						$lines[$ln] =~ /^\+(\s*)\S+/;
+						$help_indent = $1;
+					}
+					next;
 				}
 
 				$f =~ s/^.//;
@@ -3253,14 +3259,13 @@ sub process {
 				$f =~ s/^\s+//;
 				next if ($f =~ /^$/);
 
-				# This only checks context lines in the patch
-				# and so hopefully shouldn't trigger false
-				# positives, even though some of these are
-				# common words in help texts
-				if ($f =~ /^\s*(?:config|menuconfig|choice|endchoice|
-						  if|endif|menu|endmenu|source)\b/x) {
-					$is_end = 1;
-					last;
+				# Help text ends if a line has a smaller indentation
+				# than the first line of the message
+				if (defined $help_indent) {
+					if ($lines[$ln - 1] !~ /^\+$help_indent\S+/) {
+						$is_end = 1;
+						last;
+					}
 				}
 				$length++;
 			}
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread