Messages in this thread | | | Date | Mon, 16 Jul 2018 11:20:14 -0700 | From | pheragu@codeauro ... | Subject | Re: [PATCH] checkpatch: Require commit text and warn on long commit text lines |
| |
On 2018-07-13 17:08, Joe Perches wrote: > On Fri, 2018-07-13 at 16:28 -0700, pheragu@codeaurora.org wrote: >> On 2018-07-13 14:46, Joe Perches wrote: >> > On Fri, 2018-07-13 at 14:40 -0700, Prakruthi Deepak Heragu wrote: >> > > Commit text is almost always necessary to explain why a change is >> > > needed. >> > >> > This bit seems sensible, but perhaps it should just count the >> > number of lines after the end of email headers and before any >> > Signed-off-by:/Signature line >> > >> >> While committing the changes, one can just write the subject and not >> write >> the commit text at all. So, if we just count the lines between email >> headers >> and signed-off, we still do count lines which form the subject, but >> the >> commit text is still absent. Also, subject can be longer than one >> line. >> So, >> just counting lines doesn't really guarantee the presence of commit >> text. > > Not true. > Look at $in_header_lines and $in_commit_log. > >> > > Also, warn on commit text lines longer than 75 characters. The commit >> > > text >> > > are indented and may wrap on a terminal if they are longer than 75 >> > > characters. >> > >> > This is already exists via >> > >> > # Check for line lengths > 75 in commit log, warn once >> > if ($in_commit_log && !$commit_log_long_line && >> > length($line) > 75 && >> > >> >> True, but this patch points out every line of the commit text that is >> exceeding the limit. > > Which is bad because things like dump_stack() are added in > commit logs and those are already allowed to be > 75 chars. > > Anyway, something like this probably works. Please test. > --- > scripts/checkpatch.pl | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index b5c875d7132b..8b5f3dae31c9 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -2240,6 +2240,7 @@ sub process { > my $in_header_lines = $file ? 0 : 1; > my $in_commit_log = 0; #Scanning lines before patch > my $has_commit_log = 0; #Encountered lines before patch > + my $commit_log_lines = 0; #Number of commit log lines > my $commit_log_possible_stack_dump = 0; > my $commit_log_long_line = 0; > my $commit_log_has_diff = 0; > @@ -2497,6 +2498,18 @@ sub process { > > $cnt_lines++ if ($realcnt != 0); > > +# Verify the existence of a commit log if appropriate > +# 2 is used because a $signature is counted in $commit_log_lines > + if ($in_commit_log) { > + if ($line !~ /^\s*$/) { > + $commit_log_lines++; #could be a $signature > + } > + } else if ($has_commit_log && $commit_log_lines < 2) { > + WARN("COMMIT_MESSAGE", > + "Missing commit description - Add an appropriate one\n"); > + $commit_log_lines = 2; #warn only once > + } > + > # Check if the commit log has what seems like a diff which can confuse > patch > if ($in_commit_log && !$commit_log_has_diff && > (($line =~ m@^\s+diff\b.*a/[\w/]+@ && I checked all the cases that I mentioned before. The change you suggested works for every case. Would you take care of merging this fix?
| |