| Message ID | 20220928163417.1839682-1-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/1] scripts: check commit message length to fit 75 characters | expand |
"Denis V. Lunev" <den@openvz.org> writes: > There are a lot of commits descriptions which are rendered in the > 'git log' with line wrap. Apparently, this is looking awkward. Let us > add check into checkpatch.pl for that. > > I am not very good Perl developer, but there is an implementation in > Linux kernel's checkpatch.pl. Linux kernel people have faced a lot of > obstacles here thus direct port from them looks beneficial. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Alexey Kardashevskiy <aik@ozlabs.ru> > CC: "Philippe Mathieu-Daudé" <f4bug@amsat.org> > CC: "Marc-André Lureau" <marcandre.lureau@redhat.com> > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: Eric Blake <eblake@redhat.com> > CC: Markus Armbruster <armbru@redhat.com> > --- > Changes from v1: > - fixed formatting to match one in the checkpatch.pl file. That was not > obvious :( > > scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index d900d18048..fe1ff6c97d 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -36,6 +36,18 @@ my $root; > my %debug; > my $help = 0; > > +our $signature_tags = qr{(?xi: > + Signed-off-by:| > + Co-developed-by:| > + Acked-by:| > + Tested-by:| > + Reviewed-by:| > + Reported-by:| > + Suggested-by:| > + To:| > + Cc: > +)}; > + > sub help { > my ($exitcode) = @_; > > @@ -1303,6 +1315,7 @@ sub process { > > my $in_header_lines = $file ? 0 : 1; > my $in_commit_log = 0; #Scanning lines before patch > + my $commit_log_long_line = 0; > my $reported_maintainer_file = 0; > my $non_utf8_charset = 0; > > @@ -1585,6 +1598,19 @@ sub process { > WARN("8-bit UTF-8 used in possible commit log\n" . $herecurr); > } > > + if ($in_commit_log && !$commit_log_long_line && length($line) > 75 && > + !($line =~ /^\s*[a-zA-Z0-9_\/\.]+\s+\|\s+\d+/ || > + # file delta changes > + $line =~ /^\s*(?:[\w\.\-\+]*\/)++[\w\.\-\+]+:/ || > + # filename then : > + $line =~ /^\s*(?:Fixes:|Link:|$signature_tags)/i > + # A Fixes: or Link: line or signature tag line > + )) { > + WARN("Possible unwrapped commit description (prefer a maximum " . > + "75 chars per line)\n" . $herecurr); > + $commit_log_long_line = 1; > + } > + > # ignore non-hunk lines and lines being removed > next if (!$hunk_line || $line =~ /^-/); For comparison, Linux's version: # Check for line lengths > 75 in commit log, warn once if ($in_commit_log && !$commit_log_long_line && length($line) > 75 && !($line =~ /^\s*[a-zA-Z0-9_\/\.]+\s+\|\s+\d+/ || # file delta changes $line =~ /^\s*(?:[\w\.\-\+]*\/)++[\w\.\-\+]+:/ || # filename then : $line =~ /^\s*(?:Fixes:|Link:|$signature_tags)/i || # A Fixes: or Link: line or signature tag line $commit_log_possible_stack_dump)) { WARN("COMMIT_LOG_LONG_LINE", "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr); $commit_log_long_line = 1; } Differences: * Initial comment lost. Let's add it back. * One fewer line break, and spaces instead of tabs. I think it's best to stick to the original there. * We don't have $commit_log_possible_stack_dump. Should we? Initial Linux version was 2a076f40d8c9 checkpatch, SubmittingPatches: suggest line wrapping commit messages at 75 columns Updates since: 369c8dd390ba checkpatch: improve tests for fixes:, long lines and stack dumps in commit log 27b379af6102 checkpatch: avoid COMMIT_LOG_LONG_LINE warning for signature tags 36f8b348a94c checkpatch: relax regexp for COMMIT_LOG_LONG_LINE The first of these also messes with $commit_log_possible_stack_dump. Added before in bf4daf12a9fb checkpatch: avoid some commit message long line warnings and updated later in 634cffcc9478 checkpatch: don't interpret stack dumps as commit IDs
On 9/29/22 13:48, Markus Armbruster wrote: > "Denis V. Lunev" <den@openvz.org> writes: > >> There are a lot of commits descriptions which are rendered in the >> 'git log' with line wrap. Apparently, this is looking awkward. Let us >> add check into checkpatch.pl for that. >> >> I am not very good Perl developer, but there is an implementation in >> Linux kernel's checkpatch.pl. Linux kernel people have faced a lot of >> obstacles here thus direct port from them looks beneficial. >> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Alexey Kardashevskiy <aik@ozlabs.ru> >> CC: "Philippe Mathieu-Daudé" <f4bug@amsat.org> >> CC: "Marc-André Lureau" <marcandre.lureau@redhat.com> >> CC: Paolo Bonzini <pbonzini@redhat.com> >> CC: Eric Blake <eblake@redhat.com> >> CC: Markus Armbruster <armbru@redhat.com> >> --- >> Changes from v1: >> - fixed formatting to match one in the checkpatch.pl file. That was not >> obvious :( >> >> scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index d900d18048..fe1ff6c97d 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -36,6 +36,18 @@ my $root; >> my %debug; >> my $help = 0; >> >> +our $signature_tags = qr{(?xi: >> + Signed-off-by:| >> + Co-developed-by:| >> + Acked-by:| >> + Tested-by:| >> + Reviewed-by:| >> + Reported-by:| >> + Suggested-by:| >> + To:| >> + Cc: >> +)}; >> + >> sub help { >> my ($exitcode) = @_; >> >> @@ -1303,6 +1315,7 @@ sub process { >> >> my $in_header_lines = $file ? 0 : 1; >> my $in_commit_log = 0; #Scanning lines before patch >> + my $commit_log_long_line = 0; >> my $reported_maintainer_file = 0; >> my $non_utf8_charset = 0; >> >> @@ -1585,6 +1598,19 @@ sub process { >> WARN("8-bit UTF-8 used in possible commit log\n" . $herecurr); >> } >> >> + if ($in_commit_log && !$commit_log_long_line && length($line) > 75 && >> + !($line =~ /^\s*[a-zA-Z0-9_\/\.]+\s+\|\s+\d+/ || >> + # file delta changes >> + $line =~ /^\s*(?:[\w\.\-\+]*\/)++[\w\.\-\+]+:/ || >> + # filename then : >> + $line =~ /^\s*(?:Fixes:|Link:|$signature_tags)/i >> + # A Fixes: or Link: line or signature tag line >> + )) { >> + WARN("Possible unwrapped commit description (prefer a maximum " . >> + "75 chars per line)\n" . $herecurr); >> + $commit_log_long_line = 1; >> + } >> + >> # ignore non-hunk lines and lines being removed >> next if (!$hunk_line || $line =~ /^-/); > For comparison, Linux's version: > > # Check for line lengths > 75 in commit log, warn once > if ($in_commit_log && !$commit_log_long_line && > length($line) > 75 && > !($line =~ /^\s*[a-zA-Z0-9_\/\.]+\s+\|\s+\d+/ || > # file delta changes > $line =~ /^\s*(?:[\w\.\-\+]*\/)++[\w\.\-\+]+:/ || > # filename then : > $line =~ /^\s*(?:Fixes:|Link:|$signature_tags)/i || > # A Fixes: or Link: line or signature tag line > $commit_log_possible_stack_dump)) { > WARN("COMMIT_LOG_LONG_LINE", > "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr); > $commit_log_long_line = 1; > } > > Differences: > > * Initial comment lost. Let's add it back. right. Good catch > * One fewer line break, and spaces instead of tabs. I think it's best > to stick to the original there. right. Some places are correctly filled with tabs, though there are some lines with spaces. This is a bit boring as QEMU style demands spaces, but ok. Will do. > * We don't have $commit_log_possible_stack_dump. Should we? made specific check. The real question here is the following: should we allow commits like 205ccfd7a5e hw/display/ati_2d: Fix buffer overflow in ati_2d_blt (CVE-2021-3638) to be passed without this script? If yes, we should invent something similar but a bit different as userspace callstacks are really different. > Initial Linux version was > > 2a076f40d8c9 checkpatch, SubmittingPatches: suggest line wrapping > commit messages at 75 columns > > Updates since: > > 369c8dd390ba checkpatch: improve tests for fixes:, long lines and > stack dumps in commit log > > 27b379af6102 checkpatch: avoid COMMIT_LOG_LONG_LINE warning for > signature tags > > 36f8b348a94c checkpatch: relax regexp for COMMIT_LOG_LONG_LINE > > The first of these also messes with $commit_log_possible_stack_dump. > Added before in > > bf4daf12a9fb checkpatch: avoid some commit message long line warnings > > and updated later in > > 634cffcc9478 checkpatch: don't interpret stack dumps as commit IDs > wow! That is amazing thing :) I was a bit lazy doing this porting. I will definitely update the commit message in the next submission with this info :) Den
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index d900d18048..fe1ff6c97d 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -36,6 +36,18 @@ my $root; my %debug; my $help = 0; +our $signature_tags = qr{(?xi: + Signed-off-by:| + Co-developed-by:| + Acked-by:| + Tested-by:| + Reviewed-by:| + Reported-by:| + Suggested-by:| + To:| + Cc: +)}; + sub help { my ($exitcode) = @_; @@ -1303,6 +1315,7 @@ sub process { my $in_header_lines = $file ? 0 : 1; my $in_commit_log = 0; #Scanning lines before patch + my $commit_log_long_line = 0; my $reported_maintainer_file = 0; my $non_utf8_charset = 0; @@ -1585,6 +1598,19 @@ sub process { WARN("8-bit UTF-8 used in possible commit log\n" . $herecurr); } + if ($in_commit_log && !$commit_log_long_line && length($line) > 75 && + !($line =~ /^\s*[a-zA-Z0-9_\/\.]+\s+\|\s+\d+/ || + # file delta changes + $line =~ /^\s*(?:[\w\.\-\+]*\/)++[\w\.\-\+]+:/ || + # filename then : + $line =~ /^\s*(?:Fixes:|Link:|$signature_tags)/i + # A Fixes: or Link: line or signature tag line + )) { + WARN("Possible unwrapped commit description (prefer a maximum " . + "75 chars per line)\n" . $herecurr); + $commit_log_long_line = 1; + } + # ignore non-hunk lines and lines being removed next if (!$hunk_line || $line =~ /^-/);
There are a lot of commits descriptions which are rendered in the 'git log' with line wrap. Apparently, this is looking awkward. Let us add check into checkpatch.pl for that. I am not very good Perl developer, but there is an implementation in Linux kernel's checkpatch.pl. Linux kernel people have faced a lot of obstacles here thus direct port from them looks beneficial. Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Alexey Kardashevskiy <aik@ozlabs.ru> CC: "Philippe Mathieu-Daudé" <f4bug@amsat.org> CC: "Marc-André Lureau" <marcandre.lureau@redhat.com> CC: Paolo Bonzini <pbonzini@redhat.com> CC: Eric Blake <eblake@redhat.com> CC: Markus Armbruster <armbru@redhat.com> --- Changes from v1: - fixed formatting to match one in the checkpatch.pl file. That was not obvious :( scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)