[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for COMMIT_LOG_LONG_LINE

Lukas Bulwahn lukas.bulwahn at gmail.com
Mon Nov 2 07:25:00 UTC 2020



On Sat, 31 Oct 2020, Aditya Srivastava wrote:

> Currently, if a line exceeds 75 characters in a commit message,
> checkpatch.pl gives a warning corresponding to the same.
> 
> E.g., running checkpatch on commit a1af2afbd244 ("drm/nouveau/volt:Fix
> for some cards having 0 maximum voltage") reports this warning:
> 
> WARNING: Possible unwrapped commit description (prefer a maximum 75
> chars per line)
> 
> Some, mostly Fermi, vbioses appear to have zero max voltage. That causes Nouveau to not parse voltage entries, thus users not being able to set higher clocks.
> 
> Fix this warning by truncating the line after 75 chars, and push the
> remaining content to the next line
> 
> Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>

This is wrong on multiple levels:

1. We must first ensure that the COMMIT_LOG_LONG_LINE are actually 
largely true positives, i.e., it really makes sense to break those lines. 
In my evaluations, I found many cases where it is clear that lines should 
not be broken because it is tool output etc.

2. You cannot just break at character 75. You would need to identify word 
boundaries and break there.

3. The proper fix is to identify the whole paragraph and then word-wrap 
the whole paragraph with a limit of 75. Just adding some content in a new 
line will make the commit message even more unreadable than before. So, 
that is not a fix.


So, I suggest to come up with a good strategy to really identify 
COMMIT_LOG_LONG_LINE that sensibly can be word-wrapped, i.e.,
distinguish between free-floating commit description and tool output,
and then implement that word wrap properly.

Or to prioritize another issue and possiblity for fix rules.

You can see that there are potentially some good options for fixes around 
the tags and sign-offs Dwaipayan has investigated.

I am pretty sure this patch is this state will not be accepted.

Lukas

> ---
>  scripts/checkpatch.pl | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 5329927fc1c1..a0107ed257d1 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2881,8 +2881,20 @@ sub process {
>  		      $line =~ /^\s*(?:Fixes:|Link:)/i ||
>  					# A Fixes: or Link: line
>  		      $commit_log_possible_stack_dump)) {
> -			WARN("COMMIT_LOG_LONG_LINE",
> -			     "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr);
> +			if (WARN("COMMIT_LOG_LONG_LINE",
> +			     	 "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr) &&
> +			    $fix) {
> +				fix_delete_line($fixlinenr, $rawline);
> +				my $curr_line = $rawline;
> +				my $fixedline;
> +				while (length($curr_line) > 0) {
> +					# trim current line for any unnecessary spaces
> +					$curr_line = trim($curr_line);
> +					$fixedline = substr($curr_line, 0, 75);
> +					fix_insert_line($fixlinenr+1, $fixedline);
> +					$curr_line =~ s/$fixedline//;
> +				}
> +			}
>  			$commit_log_long_line = 1;
>  		}
>  
> -- 
> 2.17.1
> 
> 


More information about the Linux-kernel-mentees mailing list