2018-07-13 21:41:39

by Prakruthi Deepak Heragu

[permalink] [raw]
Subject: [PATCH] checkpatch: Require commit text and warn on long commit text lines

Commit text is almost always necessary to explain why a change is needed.
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.

Signed-off-by: David Keitel <[email protected]>
Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
---
scripts/checkpatch.pl | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 91 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a9c0550..336a8e5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -14,6 +14,13 @@ use File::Basename;
use Cwd 'abs_path';
use Term::ANSIColor qw(:constants);

+use constant BEFORE_SHORTTEXT => 0;
+use constant IN_SHORTTEXT_BLANKLINE => 1;
+use constant IN_SHORTTEXT => 2;
+use constant AFTER_SHORTTEXT => 3;
+use constant CHECK_NEXT_SHORTTEXT => 4;
+use constant SHORTTEXT_LIMIT => 75;
+
my $P = $0;
my $D = dirname(abs_path($P));

@@ -2227,6 +2234,8 @@ sub process {
my $prevrawline="";
my $stashline="";
my $stashrawline="";
+ my $subjectline="";
+ my $sublinenr="";

my $length;
my $indent;
@@ -2282,6 +2291,9 @@ sub process {
my $setup_docs = 0;

my $camelcase_file_seeded = 0;
+ my $shorttext = BEFORE_SHORTTEXT;
+ my $shorttext_exspc = 0;
+ my $commit_text_present = 0;

my $checklicenseline = 1;

@@ -2487,13 +2499,91 @@ sub process {
$checklicenseline = 1;
next;
}
-
$here .= "FILE: $realfile:$realline:" if ($realcnt != 0);

my $hereline = "$here\n$rawline\n";
my $herecurr = "$here\n$rawline\n";
my $hereprev = "$here\n$prevrawline\n$rawline\n";

+ if ($shorttext != AFTER_SHORTTEXT) {
+ if ($shorttext == IN_SHORTTEXT_BLANKLINE && $line=~/\S/) {
+ # the subject line was just processed,
+ # a blank line must be next
+ $shorttext = IN_SHORTTEXT;
+ # this non-blank line may or may not be commit text -
+ # a warning has been generated so assume it is commit
+ # text and move on
+ $commit_text_present = 1;
+ # fall through and treat this line as IN_SHORTTEXT
+ }
+ if ($shorttext == IN_SHORTTEXT) {
+ if ($line=~/^---/ || $line=~/^diff.*/) {
+ if ($commit_text_present == 0) {
+ WARN("NO_COMMIT_TEXT",
+ "please add commit text explaining " .
+ "*why* the change is needed\n" .
+ $herecurr);
+ }
+ $shorttext = AFTER_SHORTTEXT;
+ } elsif (length($line) > (SHORTTEXT_LIMIT +
+ $shorttext_exspc)
+ && $line !~ /^:([0-7]{6}\s){2}
+ ([[:xdigit:]]+\.*
+ \s){2}\w+\s\w+/xms) {
+ WARN("LONG_COMMIT_TEXT",
+ "commit text line over " .
+ SHORTTEXT_LIMIT .
+ " characters\n" . $herecurr);
+ $commit_text_present = 1;
+ } elsif ($line=~/^\s*[\x21-\x39\x3b-\x7e]+:/) {
+ # this is a tag, there must be commit
+ # text by now
+ if ($commit_text_present == 0) {
+ WARN("NO_COMMIT_TEXT",
+ "please add commit text explaining " .
+ "*why* the change is needed\n" .
+ $herecurr);
+ # prevent duplicate warnings
+ $commit_text_present = 1;
+ }
+ } elsif ($line=~/\S/) {
+ $commit_text_present = 1;
+ }
+ } elsif ($shorttext == IN_SHORTTEXT_BLANKLINE) {
+ # case of non-blank line in this state handled above
+ $shorttext = IN_SHORTTEXT;
+ } elsif ($shorttext == CHECK_NEXT_SHORTTEXT) {
+# The Subject line doesn't have to be the last header in the patch.
+# Avoid moving to the IN_SHORTTEXT state until clear of all headers.
+# Per RFC5322, continuation lines must be folded, so any left-justified
+# text which looks like a header is definitely a header.
+ if ($line!~/^[\x21-\x39\x3b-\x7e]+:/) {
+ # Every rolled over summary-line leaves
+ # a space at the beginning
+ if ($line !~ /^\s/) {
+ $shorttext = IN_SHORTTEXT;
+ if (length($line) != 0) {
+ $commit_text_present = 1;
+ }
+ }
+ }
+ # The next two cases are BEFORE_SHORTTEXT.
+ } elsif ($line=~/^Subject: \[[^\]]*\] (.*)/) {
+ # This is the subject line. Go to
+ # CHECK_NEXT_SHORTTEXT to wait for the commit
+ # text to show up.
+ $shorttext = CHECK_NEXT_SHORTTEXT;
+ $subjectline = $line;
+ $sublinenr = "#$linenr & ";
+ } elsif ($line=~/^ (.*)/) {
+ # Indented format, this must be the summary
+ # line (i.e. git show). There will be no more
+ # headers so we are now in the shorttext.
+ $shorttext = IN_SHORTTEXT_BLANKLINE;
+ $shorttext_exspc = 4;
+ }
+ }
+
$cnt_lines++ if ($realcnt != 0);

# Check if the commit log has what seems like a diff which can confuse patch
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project



2018-07-13 21:47:43

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] checkpatch: Require commit text and warn on long commit text lines

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

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

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -14,6 +14,13 @@ use File::Basename;
> use Cwd 'abs_path';
> use Term::ANSIColor qw(:constants);
>
> +use constant BEFORE_SHORTTEXT => 0;
> +use constant IN_SHORTTEXT_BLANKLINE => 1;
> +use constant IN_SHORTTEXT => 2;
> +use constant AFTER_SHORTTEXT => 3;
> +use constant CHECK_NEXT_SHORTTEXT => 4;
> +use constant SHORTTEXT_LIMIT => 75;

probably overly complicated


2018-07-13 23:29:03

by Prakruthi Deepak Heragu

[permalink] [raw]
Subject: Re: [PATCH] checkpatch: Require commit text and warn on long commit text lines

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.

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

>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -14,6 +14,13 @@ use File::Basename;
>> use Cwd 'abs_path';
>> use Term::ANSIColor qw(:constants);
>>
>> +use constant BEFORE_SHORTTEXT => 0;
>> +use constant IN_SHORTTEXT_BLANKLINE => 1;
>> +use constant IN_SHORTTEXT => 2;
>> +use constant AFTER_SHORTTEXT => 3;
>> +use constant CHECK_NEXT_SHORTTEXT => 4;
>> +use constant SHORTTEXT_LIMIT => 75;
>
> probably overly complicated

2018-07-14 00:11:19

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] checkpatch: Require commit text and warn on long commit text lines

On Fri, 2018-07-13 at 16:28 -0700, [email protected] 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/]+@ &&

2018-07-16 18:22:07

by Prakruthi Deepak Heragu

[permalink] [raw]
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, [email protected] 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?

2018-07-25 18:51:08

by Prakruthi Deepak Heragu

[permalink] [raw]
Subject: Re: [PATCH] checkpatch: Require commit text and warn on long commit text lines

On 2018-07-16 11:20, [email protected] wrote:
> On 2018-07-13 17:08, Joe Perches wrote:
>> On Fri, 2018-07-13 at 16:28 -0700, [email protected] 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?
Any updates on this patch? Would you take care of merging this fix?

2018-07-26 02:24:31

by Joe Perches

[permalink] [raw]
Subject: [PATCH] checkpatch: Warn when a patch doesn't have a description

Potential patches should have a commit description.
Emit a warning when there isn't one.

Suggested-by: Prakruthi Deepak Heragu <[email protected]>
Signed-off-by: Joe Perches <[email protected]>
---
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/]+@ &&

2018-07-26 10:09:09

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] checkpatch: Warn when a patch doesn't have a description

On Wed, Jul 25, 2018 at 07:22:47PM -0700, Joe Perches wrote:
> Potential patches should have a commit description.
> Emit a warning when there isn't one.
>
> Suggested-by: Prakruthi Deepak Heragu <[email protected]>
> Signed-off-by: Joe Perches <[email protected]>

Reviewed-by: Greg Kroah-Hartman <[email protected]>

2018-07-26 22:10:12

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] checkpatch: Warn when a patch doesn't have a description

On Wed, 25 Jul 2018 19:22:47 -0700 Joe Perches <[email protected]> wrote:

> Potential patches should have a commit description.
> Emit a warning when there isn't one.
>
> ...
>
> --- 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/]+@ &&

This explodes all over the place.

Variable "$clean" is not imported at scripts/checkpatch.pl line 6565.
Variable "$clean" is not imported at scripts/checkpatch.pl line 6590.
Variable "$cnt_error" is not imported at scripts/checkpatch.pl line 6592.
Variable "$cnt_warn" is not imported at scripts/checkpatch.pl line 6592.
Variable "$cnt_chk" is not imported at scripts/checkpatch.pl line 6593.
Variable "$cnt_lines" is not imported at scripts/checkpatch.pl line 6594.
Variable "$clean" is not imported at scripts/checkpatch.pl line 6599.
Variable "$clean" is not imported at scripts/checkpatch.pl line 6618.
Variable "$clean" is not imported at scripts/checkpatch.pl line 6659.
Variable "$clean" is not imported at scripts/checkpatch.pl line 6665.
syntax error at scripts/checkpatch.pl line 2520, near "else if"
Global symbol "$herecurr" requires explicit package name (did you forget to declare "my $herecurr"?) at scripts/checkpatch.pl line 2533.
Global symbol "$herecurr" requires explicit package name (did you forget to declare "my $herecurr"?) at scripts/checkpatch.pl line 2584.
Global symbol "$herecurr" requires explicit package name (did you forget to declare "my $herecurr"?) at scripts/checkpatch.pl line 2588.
etc

I did this:

--- a/scripts/checkpatch.pl~checkpatch-warn-when-a-patch-doesnt-have-a-description-fix
+++ a/scripts/checkpatch.pl
@@ -2517,7 +2517,7 @@ sub process {
if ($line !~ /^\s*$/) {
$commit_log_lines++; #could be a $signature
}
- } else if ($has_commit_log && $commit_log_lines < 2) {
+ } elsif ($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

But I worry that you didn't send out the version which you tested, so
please check.


2018-07-27 01:27:58

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] checkpatch: Warn when a patch doesn't have a description

On Thu, 2018-07-26 at 15:08 -0700, Andrew Morton wrote:
> On Wed, 25 Jul 2018 19:22:47 -0700 Joe Perches <[email protected]> wrote:
>
> > Potential patches should have a commit description.
> > Emit a warning when there isn't one.
[]
> I did this:
>
> --- a/scripts/checkpatch.pl~checkpatch-warn-when-a-patch-doesnt-have-a-description-fix
> +++ a/scripts/checkpatch.pl
> @@ -2517,7 +2517,7 @@ sub process {
> if ($line !~ /^\s*$/) {
> $commit_log_lines++; #could be a $signature
> }
> - } else if ($has_commit_log && $commit_log_lines < 2) {
> + } elsif ($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
>
> But I worry that you didn't send out the version which you tested, so
> please check.

You're right, I inserted the wrong one.
Anyway, what you did is correct and that was the only change.
Thanks.

$ diff -urN cp_commit_log_lines.diff cp_commit_message.diff
--- cp_commit_log_lines.diff 2018-07-13 17:06:29.115337477 -0700
+++ cp_commit_message.diff 2018-07-15 05:46:29.878805336 -0700
@@ -2,7 +2,7 @@
1 file changed, 13 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
-index b5c875d7132b..8b5f3dae31c9 100755
+index b5c875d7132b..4ccf84079ea4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2240,6 +2240,7 @@ sub process {
@@ -23,7 +23,7 @@
+ if ($line !~ /^\s*$/) {
+ $commit_log_lines++; #could be a $signature
+ }
-+ } else if ($has_commit_log && $commit_log_lines < 2) {
++ } elsif ($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