[PATCH] checkpatch: Require author Signed-off-by

[PATCH] checkpatch: Require author Signed-off-by

[Geert Uytterhoeven]

Print an error if none of the Signed-off-by lines cover the patch
author.

Non-ASCII quoted printable encoding in From: headers and (lack of)
double quotes are handled.
Split From: headers are not fully handled: only the first part is
compared.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Tested using a set of ca. 4000 real world commits.

Most common offenders are people using:
  - different email addresses for author and Sob,
  - different firstname/lastname order, or other different name
    spelling,
  - suse.de vs. suse.com.
---
 scripts/checkpatch.pl | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 8d0bad190c25e2e0..53f3dcf9171447f4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -13,6 +13,7 @@ use POSIX;
 use File::Basename;
 use Cwd 'abs_path';
 use Term::ANSIColor qw(:constants);
+use MIME::Words 'decode_mimewords';
 
 my $P = $0;
 my $D = dirname(abs_path($P));
@@ -2236,6 +2237,8 @@ sub process {
 
 	our $clean = 1;
 	my $signoff = 0;
+	my $author = '';
+	my $authorsignoff = 0;
 	my $is_patch = 0;
 	my $in_header_lines = $file ? 0 : 1;
 	my $in_commit_log = 0;		#Scanning lines before patch
@@ -2518,10 +2521,23 @@ sub process {
 			}
 		}
 
+# Check the patch for a From:
+		if ($line =~ /^\s*From: (.*)/i) {
+			$author = decode_mimewords($1);
+			$author =~ s/"//g;
+		}
+
 # Check the patch for a signoff:
 		if ($line =~ /^\s*signed-off-by:/i) {
 			$signoff++;
 			$in_commit_log = 0;
+			if ($author ne '') {
+				my $l = $line;
+				$l =~ s/"//g;
+				if ($l =~ /^\s*signed-off-by: \Q$author\E/i) {
+				    $authorsignoff = 1;
+				}
+			}
 		}
 
 # Check if MAINTAINERS is being updated.  If so, there's probably no need to
@@ -6487,9 +6503,14 @@ sub process {
 		ERROR("NOT_UNIFIED_DIFF",
 		      "Does not appear to be a unified-diff format patch\n");
 	}
-	if ($is_patch && $has_commit_log && $chk_signoff && $signoff == 0) {
-		ERROR("MISSING_SIGN_OFF",
-		      "Missing Signed-off-by: line(s)\n");
+	if ($is_patch && $has_commit_log && $chk_signoff) {
+		if ($signoff == 0) {
+			ERROR("MISSING_SIGN_OFF",
+			      "Missing Signed-off-by: line(s)\n");
+		} elsif (!$authorsignoff) {
+			ERROR("NO_AUTHOR_SIGN_OFF",
+			      "Missing Signed-off-by: line by patch author\n");
+		}
 	}
 
 	print report_dump();
-- 
2.17.1

Re: [PATCH] checkpatch: Require author Signed-off-by

[Joe Perches]

On Tue, 2018-07-10 at 14:10 +0200, Geert Uytterhoeven wrote:
> Print an error if none of the Signed-off-by lines cover the patch
> author.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -13,6 +13,7 @@ use POSIX;
>  use File::Basename;
>  use Cwd 'abs_path';
>  use Term::ANSIColor qw(:constants);
> +use MIME::Words 'decode_mimewords';

Is this a typically installed perl module?
If so, what perl version installed it by default?

Re: [PATCH] checkpatch: Require author Signed-off-by

[Geert Uytterhoeven]

Hi Joe,

On Tue, Jul 10, 2018 at 4:34 PM Joe Perches <joe@perches.com> wrote:
> On Tue, 2018-07-10 at 14:10 +0200, Geert Uytterhoeven wrote:
> > Print an error if none of the Signed-off-by lines cover the patch
> > author.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -13,6 +13,7 @@ use POSIX;
> >  use File::Basename;
> >  use Cwd 'abs_path';
> >  use Term::ANSIColor qw(:constants);
> > +use MIME::Words 'decode_mimewords';
>
> Is this a typically installed perl module?
> If so, what perl version installed it by default?

I don't know. At least my Ubuntu 16.04LTS and 18.04LTS systems had it
installed.
It does not seem to be part of the core modules, cfr.
https://perldoc.perl.org/index-modules-M.html

I've tried using MIME::QuotedPrint instead, but then we need to parse
=?UTF-8?q? ourselves, as decode_qp() only transforms the actual
quoted printable bits in side the encoded From: address.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] checkpatch: Require author Signed-off-by

[Joe Perches]

On Tue, 2018-07-10 at 17:07 +0200, Geert Uytterhoeven wrote:
> Hi Joe,

Hi Geert

> On Tue, Jul 10, 2018 at 4:34 PM Joe Perches <joe@perches.com> wrote:
> > On Tue, 2018-07-10 at 14:10 +0200, Geert Uytterhoeven wrote:
> > > Print an error if none of the Signed-off-by lines cover the patch
> > > author.
> > 
> > []
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > 
> > []
> > > @@ -13,6 +13,7 @@ use POSIX;
> > >  use File::Basename;
> > >  use Cwd 'abs_path';
> > >  use Term::ANSIColor qw(:constants);
> > > +use MIME::Words 'decode_mimewords';
> > 
> > Is this a typically installed perl module?
> > If so, what perl version installed it by default?
> 
> I don't know. At least my Ubuntu 16.04LTS and 18.04LTS systems had it
> installed.
> It does not seem to be part of the core modules, cfr.
> https://perldoc.perl.org/index-modules-M.html
> 
> I've tried using MIME::QuotedPrint instead, but then we need to parse
> =?UTF-8?q? ourselves, as decode_qp() only transforms the actual
> quoted printable bits in side the encoded From: address.

I'd looked too and couldn't find it from 5.10 forward,
so I'd prefer not requiring that module.

Also, the From: line is also an email header and I don't
believe it should be used author information in the
same way as a patch From: line.

So likely the

+# Check the patch for a From:
+               if ($line =~ /^\s*From: (.*)/i) {

should be "if ($in_commit_log && $line =~ /^\s*From: (.*)/) {

with some regex or function to remove the MIME encoded word.

Re: [PATCH] checkpatch: Require author Signed-off-by

[Geert Uytterhoeven]

Hi Joe,

On Tue, Jul 10, 2018 at 5:28 PM Joe Perches <joe@perches.com> wrote:
> On Tue, 2018-07-10 at 17:07 +0200, Geert Uytterhoeven wrote:
> > On Tue, Jul 10, 2018 at 4:34 PM Joe Perches <joe@perches.com> wrote:
> > > On Tue, 2018-07-10 at 14:10 +0200, Geert Uytterhoeven wrote:
> > > > Print an error if none of the Signed-off-by lines cover the patch
> > > > author.
> > >
> > > []
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > >
> > > []
> > > > @@ -13,6 +13,7 @@ use POSIX;
> > > >  use File::Basename;
> > > >  use Cwd 'abs_path';
> > > >  use Term::ANSIColor qw(:constants);
> > > > +use MIME::Words 'decode_mimewords';
> > >
> > > Is this a typically installed perl module?
> > > If so, what perl version installed it by default?
> >
> > I don't know. At least my Ubuntu 16.04LTS and 18.04LTS systems had it
> > installed.
> > It does not seem to be part of the core modules, cfr.
> > https://perldoc.perl.org/index-modules-M.html
> >
> > I've tried using MIME::QuotedPrint instead, but then we need to parse
> > =?UTF-8?q? ourselves, as decode_qp() only transforms the actual
> > quoted printable bits in side the encoded From: address.
>
> I'd looked too and couldn't find it from 5.10 forward,
> so I'd prefer not requiring that module.

"that module" = MIME::Words?
So I can use MIME::QuotedPrint?

> Also, the From: line is also an email header and I don't
> believe it should be used author information in the
> same way as a patch From: line.
>
> So likely the
>
> +# Check the patch for a From:
> +               if ($line =~ /^\s*From: (.*)/i) {
>
> should be "if ($in_commit_log && $line =~ /^\s*From: (.*)/) {

That doesn't work. The "From:" line in the email body is only added by
git-send-email when it detects the "From:" line in the patch differs from
user.email in git-config.
It is not present in patches generated by git-format-patch.

So to work for both the sending ("From:" in email header only) and
receiving ("From:" in email header, to be overridden by "From:" in email
body) side, the $in_commit_log check must not be present.

> with some regex or function to remove the MIME encoded word.

I guess I can do that.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] checkpatch: Require author Signed-off-by

[Joe Perches]

On Tue, 2018-07-10 at 17:50 +0200, Geert Uytterhoeven wrote:
> So I can use MIME::QuotedPrint?

How about 

use Encode;
[]
	if (decode("MIME-Header", $line) =~ /^from:\s*(.*)/i) {
etc...

Re: [PATCH] checkpatch: Require author Signed-off-by

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 358 bytes --]

Hi!

> Print an error if none of the Signed-off-by lines cover the patch
> author.

Is it good idea? Signed-off-by system was _not_ meant to prevent
submission of third party patches.

								Pavel
								 
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] checkpatch: Require author Signed-off-by

[Geert Uytterhoeven]

Hi Pavel,

On Tue, Jul 10, 2018 at 10:47 PM Pavel Machek <pavel@ucw.cz> wrote:
> > Print an error if none of the Signed-off-by lines cover the patch
> > author.
>
> Is it good idea? Signed-off-by system was _not_ meant to prevent
> submission of third party patches.

My main motivation is catching accidentally forgotten Sobs.
It's still up to you to submit or accept a patch that triggers
checkpatch errors.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] checkpatch: Require author Signed-off-by

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

On Tue 2018-07-10 22:50:14, Geert Uytterhoeven wrote:
> Hi Pavel,
> 
> On Tue, Jul 10, 2018 at 10:47 PM Pavel Machek <pavel@ucw.cz> wrote:
> > > Print an error if none of the Signed-off-by lines cover the patch
> > > author.
> >
> > Is it good idea? Signed-off-by system was _not_ meant to prevent
> > submission of third party patches.
> 
> My main motivation is catching accidentally forgotten Sobs.
> It's still up to you to submit or accept a patch that triggers
> checkpatch errors.

Should it be at warning level, then, or explicitely saying this is
okay if you are not patch author?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] checkpatch: Require author Signed-off-by

[Joe Perches]

On Wed, 2018-07-11 at 00:08 +0200, Pavel Machek wrote:
> On Tue 2018-07-10 22:50:14, Geert Uytterhoeven wrote:
> > Hi Pavel,
> > 
> > On Tue, Jul 10, 2018 at 10:47 PM Pavel Machek <pavel@ucw.cz> wrote:
> > > > Print an error if none of the Signed-off-by lines cover the patch
> > > > author.
> > > 
> > > Is it good idea? Signed-off-by system was _not_ meant to prevent
> > > submission of third party patches.
> > 
> > My main motivation is catching accidentally forgotten Sobs.
> > It's still up to you to submit or accept a patch that triggers
> > checkpatch errors.
> 
> Should it be at warning level, then, or explicitely saying this is
> okay if you are not patch author?

Yes to both.