Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <8436bc4191d92af283ea6aa7ca450e36da653e2e.camel@perches.com>
Subject: Re: [RFC PATCH] checkpatch: correctly detect lines of help text
From:   Joe Perches <joe@perches.com>
To:     Nicolai Fischer <nicolai.fischer@fau.de>,
        linux-kernel@vger.kernel.org, Andi Kleen <ak@linux.intel.com>
Cc:     apw@canonical.com, johannes.czekay@fau.de,
        linux-kernel@i4.cs.fau.de
Date:   Wed, 02 Dec 2020 10:54:24 -0800
In-Reply-To: <133ac827-c0d2-c5c7-39d8-7d2c17de7c76@fau.de>
References: <133ac827-c0d2-c5c7-39d8-7d2c17de7c76@fau.de>
Content-Type: text/plain; charset="ISO-8859-1"
User-Agent: Evolution 3.38.1-1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Wed, 2020-12-02 at 19:27 +0100, Nicolai Fischer wrote:
> 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

Perhaps it'd be better to create a new warning when the help text
block is not the last block of the config section.  Maybe warn when
a blank line or endif is not the separator to the next keyword.
Maybe warn when the next line after help is not indented 2 more
spaces than the help line.

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

This probably won't work, see below:

> 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.

Yes please.

> This could potentially be addressed in the warning message,
> though we are happy for any input on this.
[]
> diff --git 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(?:---)?$/) {

I believe all the '---help---' lines have been converted to just 'help'
so the '(?:---)?' bits here could be removed.

See:

commit 22a4ac026c15eba961883ed8466cb341e0447de1
Author: Masahiro Yamada <masahiroy@kernel.org>
Date:   Wed Jun 17 12:02:20 2020 +0900

    Revert "checkpatch: kconfig: prefer 'help' over '---help---'"
    
    This reverts commit 84af7a6194e493fae312a2b7fa5a3b51f76d9282.
    
    The conversion is done.
    
    Cc: Ulf Magnusson <ulfalizer@gmail.com>
    Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

> @@ -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;
> +					}

Indentation can vary in the help blocks.  For instance:

arch/Kconfig:   help
arch/Kconfig-     Functions will have the stack-protector canary logic added in>
arch/Kconfig-     of the following conditions:
arch/Kconfig-
arch/Kconfig-     - local variable's address used as part of the right hand sid>
arch/Kconfig-       assignment or function argument
arch/Kconfig-     - local variable is an array (or union containing an array),
arch/Kconfig-       regardless of array type or length
arch/Kconfig-     - uses register local variables
arch/Kconfig-

This doesn't allow blank lines for multi-paragraph help text either.

I think keyword parsing is necessary and some false positives are
inevitable as the parsing logic in a line-by-line analyzer will
always be incomplete.