Re: PATCH] patch to generate warning when signed-of line in patch in not proper

Re: PATCH] patch to generate warning when signed-of line in patch in not proper

[anish]

On Sat, May 21, 2011 at 9:33 PM, Joe Perches <joe@perches.com> wrote:

On Sat, 2011-05-21 at 20:14 +0530, anish singh wrote:
> Re-sending to get some comments on this.
>
> On Thu, May 19, 2011 at 10:53 PM, anish <anish198519851985@gmail.com>
> wrote:
>         From: anish kumar <anish198519851985@gmail.com>
>
>         This patch generates warning when there is no space between
>         the
>         patch submitter name and successive mail-id.


>>If you do this, why not do it for all signature types?

>>our $Valid_Signatures "(?:Signed-off-by:|Reviewed-by:|Acked-by:)"

Definitely.Please check below modified patch.

>         Signed-off-by: anish kumar <anish198519851985@gmail.com>
>         ---
>          scripts/checkpatch.pl |   10 ++++++++--
>          1 files changed, 8 insertions(+), 2 deletions(-)
>
>         diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>         index d867081..437c6d4 100755
>         --- a/scripts/checkpatch.pl
>         +++ b/scripts/checkpatch.pl
>         @@ -1373,10 +1373,16 @@ sub process {
>                                        WARN("Signed-off-by: is the
>         preferred form\n" .
>                                                $herecurr);
>                                }
>         -                       if ($line =~ /^\s*signed-off-by:\S/i)
>         {
>         -                               WARN("space required after
>         Signed-off-by:\n" .


>>       if ($line =~ /^(\s*)($ValidSignatures)(\s*)(.*)$/i) {
>>               my $space_before = $1;
>>               my $sign_off = $2;
>>               my $space_after = $3;
>>               my $email = $4;
>>               if (defined $space_before && $space_before ne "") {
>>                       warning (no space before...)
>>               }
>>               if ($sign_off !~ /$Valid_Signature/) {
>>                       warning (signature case...)
>>               }
>>               if (!defined $space_after || $space_after ne " ") {
>>                       warning (need only one space after colon...)
>>               }
>>               if (!validate_email($4)) {
>>                       warning (bad email...)

Please check the below patch.

--------------------------------------------------------------------------

From: anish kumar <anish198519851985@gmail.com>

This patch generates warning when there is no space betweenthe  patch
submitter name and successive mail-id.As suggested by
Joe Perches(joe@perches.com) that we can do this for
all signature types.

Signed-off-by: anish kumar <anish198519851985@gmail.com>
---
 scripts/checkpatch.pl |   38 ++++++++++++++++++++++++--------------
 1 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d867081..6d67a17 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -29,6 +29,8 @@ my $summary_file = 0;
 my $root;
 my %debug;
 my $help = 0;
+my $sign;
+my @signs = ("Reviewed-by:","Acked-by:","Signed-off-by:","Tested-by:");
 
 sub help {
 	my ($exitcode) = @_;
@@ -1365,20 +1367,27 @@ sub process {
 			}
 		}
 
-#check the patch for a signoff:
-		if ($line =~ /^\s*signed-off-by:/i) {
-			# This is a signoff, if ugly, so do not double report.
-			$signoff++;
-			if (!($line =~ /^\s*Signed-off-by:/)) {
-				WARN("Signed-off-by: is the preferred form\n" .
-					$herecurr);
-			}
-			if ($line =~ /^\s*signed-off-by:\S/i) {
-				WARN("space required after Signed-off-by:\n" .
-					$herecurr);
-			}
-		}
-
+#check the patch for a signoff/Reviewed/Acked/Tested:
+        foreach $sign (@signs) {
+                 if ($line =~ /^\s*$sign/i) {
+                        # This is a signoff, if ugly, so do not double report.
+                        $signoff++;
+                   if (!($line =~ /^\s*$sign/)) {
+                                WARN("$sign is the preferred form\n" .
+                                        $herecurr);
+                        }
+                     if ($line =~ /^\s*$sign(.*)/i) {
+                          if($1 !~ /^\s*(\s[a-zA-Z]*.*)/i) {
+                                WARN("Space required after $sign\n" .
+                                        $herecurr);
+                                  }
+                              if($1 !~ /[\sa-zA-Z]+\s<.*>/i) {
+                                   WARN("Space required b/w Full Name & Mail-id:\n" .
+                                        $herecurr);
+                                }
+                        }
+        }
+}
 # Check for wrappage within a valid hunk of the file
 		if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) {
 			ERROR("patch seems to be corrupt (line wrapped?)\n" .
@@ -2964,3 +2973,4 @@ sub process {
 
 	return $clean;
 }
+
-- 
1.7.0.4

Re: PATCH] patch to generate warning when signed-of line in patch in not proper

[Joe Perches]

On Sun, 2011-05-22 at 01:08 +0530, anish wrote:
> On Sat, May 21, 2011 at 9:33 PM, Joe Perches <joe@perches.com> wrote:
> >>If you do this, why not do it for all signature types?
> >>our $Valid_Signatures "(?:Signed-off-by:|Reviewed-by:|Acked-by:)"
> Definitely.Please check below modified patch.
[]
> This patch generates warning when there is no space betweenthe  patch
> submitter name and successive mail-id.As suggested by
> Joe Perches(joe@perches.com) that we can do this for
> all signature types.

Typos and spacing issues in commit message.

> Signed-off-by: anish kumar <anish198519851985@gmail.com>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> +my $sign;
> +my @signs = ("Reviewed-by:","Acked-by:","Signed-off-by:","Tested-by:");
[]
> +#check the patch for a signoff/Reviewed/Acked/Tested:
> +        foreach $sign (@signs) {

It's inefficient to loop for this.
I think what I suggested is neater,
but what you're doing is OK.

Your email subject line should be more like:

[PATCH] checkpatch: More signature format verification

And when you revise patches, use some version number
in the [PATCH] block like:

[PATCH v2] checkpatch: etc...

cheers, Joe

[patch v2] checkpatch: Signature format verification

[anish]

From: anish kumar <anish198519851985@gmail.com>

This patch generates warning when there is no space between
the patch submitter and successive mail-id.

Modification:Suggested by Joe Perches(joe@perches.com) that
we can add this check for all signature types so added that
change and added logic to remove the inefficent looping so
that it can come out as soon as signature type is matched.

Signed-off-by: anish kumar <anish198519851985@gmail.com>
---
 scripts/checkpatch.pl |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d867081..0622f41 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -29,6 +29,8 @@ my $summary_file = 0;
 my $root;
 my %debug;
 my $help = 0;
+my ($sign,$loop_brk);
+my @signs = ("Reviewed-by:","Acked-by:","Signed-off-by:","Tested-by:");
 
 sub help {
 	my ($exitcode) = @_;
@@ -1365,20 +1367,30 @@ sub process {
 			}
 		}
 
-#check the patch for a signoff:
-		if ($line =~ /^\s*signed-off-by:/i) {
-			# This is a signoff, if ugly, so do not double report.
-			$signoff++;
-			if (!($line =~ /^\s*Signed-off-by:/)) {
-				WARN("Signed-off-by: is the preferred form\n" .
+#check the patch for a signoff/Reviewed/Acked/Tested:
+foreach $sign (@signs) {
+	$loop_brk=0;
+	if ($line =~ /^\s*$sign/i) {
+		# This is a signoff, if ugly, so do not double report.
+		$signoff++;
+		$loop_brk++;
+		if (!($line =~ /^\s*$sign/)) {
+			WARN("$sign is the preferred form\n" .
+				$herecurr);
+		}
+		if ($line =~ /^\s*$sign(.*)/i) {
+			if($1 !~ /^\s*(\s[a-zA-Z]*.*)/i) {
+				WARN("Space required after $sign\n" .
 					$herecurr);
 			}
-			if ($line =~ /^\s*signed-off-by:\S/i) {
-				WARN("space required after Signed-off-by:\n" .
+			if($1 !~ /[\sa-zA-Z]+\s<.*>/i) {
+				WARN("Space required b/w Full Name & Mail-id:\n" .
 					$herecurr);
 			}
 		}
-
+	}
+last if ($loop_brk == 1);
+}
 # Check for wrappage within a valid hunk of the file
 		if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) {
 			ERROR("patch seems to be corrupt (line wrapped?)\n" .
-- 
1.7.0.4

Re: [patch v2] checkpatch: Signature format verification

[Joe Perches]

On Sun, 2011-05-22 at 15:48 +0530, anish wrote:
> This patch generates warning when there is no space between
> the patch submitter and successive mail-id.
[]
> Signed-off-by: anish kumar <anish198519851985@gmail.com>
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d867081..0622f41 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -29,6 +29,8 @@ my $summary_file = 0;
>  my $root;
>  my %debug;
>  my $help = 0;
> +my ($sign,$loop_brk);

These 2 variables don't need to be at this scope.

> @@ -1365,20 +1367,30 @@ sub process {
[]
> +#check the patch for a signoff/Reviewed/Acked/Tested:
> +foreach $sign (@signs) {

Please use the appropriate indentation or add a new function.

Re: [patch v2] checkpatch: Signature format verification

[Joe Perches]

On Mon, 2011-05-23 at 08:26 +0530, anish singh wrote:
> On Sun, May 22, 2011 at 7:46 PM, Joe Perches <joe@perches.com> wrote:
>         On Sun, 2011-05-22 at 15:48 +0530, anish wrote:
>         > This patch generates warning when there is no space between
>         > the patch submitter and successive mail-id.
>         []
> What does this mean?I checked in kernelnewbies archives and asked
> on IRC channel but couldn't get the meaning.Kindly let me know.

It means I didn't quote all of your original message.

[patch v3] checkpatch: Signature format verification

[anish]

From: anish kumar <anish198519851985@gmail.com>

This patch generates warning when there is no space between
the patch submitter and successive mail-id.

V2 Modification:Suggested by Joe Perches(joe@perches.com) that
we can add this check for all signature types so added that
change and added logic to remove the inefficent looping so
that it can come out as soon as signature type is matched.

V3 Modification:Moved the variable from global to local scope.

Signed-off-by: anish kumar <anish198519851985@gmail.com>
---
 scripts/checkpatch.pl |   36 ++++++++++++++++++++++++------------
 1 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d867081..fbc0e62 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1365,20 +1365,32 @@ sub process {
 			}
 		}
 
-#check the patch for a signoff:
-		if ($line =~ /^\s*signed-off-by:/i) {
-			# This is a signoff, if ugly, so do not double report.
-			$signoff++;
-			if (!($line =~ /^\s*Signed-off-by:/)) {
-				WARN("Signed-off-by: is the preferred form\n" .
-					$herecurr);
-			}
-			if ($line =~ /^\s*signed-off-by:\S/i) {
-				WARN("space required after Signed-off-by:\n" .
-					$herecurr);
+#check the patch for a signoff/Reviewed/Acked/Tested:
+		my ($sign,$loop_brk);
+		my @signs = ("Reviewed-by:","Acked-by:","Signed-off-by:","Tested-by:");
+		foreach $sign (@signs) {
+			$loop_brk=0;
+			if ($line =~ /^\s*$sign/i) {
+				# This is a signoff, if ugly, so do not double report.
+				$signoff++;
+				$loop_brk++;
+				if (!($line =~ /^\s*$sign/)) {
+					WARN("$sign is the preferred form\n" .
+						$herecurr);
+				}
+				if ($line =~ /^\s*$sign(.*)/i) {
+					if($1 !~ /^\s*(\s[a-zA-Z]*.*)/i) {
+						WARN("Space required after $sign\n" .
+							$herecurr);
+					}
+					if($1 !~ /[\sa-zA-Z]+\s<.*>/i) {
+						WARN("Space required b/w Full Name & Mail-id:\n" .
+							$herecurr);
+					}
+				}
 			}
+			last if ($loop_brk == 1);
 		}
-
 # Check for wrappage within a valid hunk of the file
 		if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) {
 			ERROR("patch seems to be corrupt (line wrapped?)\n" .
-- 
1.7.0.4

Re: [patch v3] checkpatch: Signature format verification

[Joe Perches]

On Mon, 2011-05-23 at 20:51 +0530, anish wrote:
> From: anish kumar <anish198519851985@gmail.com>
> This patch generates warning when there is no space between
> the patch submitter and successive mail-id.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> +				if (!($line =~ /^\s*$sign/)) {

				if  ($line !~ /etc...)

> +					WARN("$sign is the preferred form\n" .
> +						$herecurr);
> +				}
> +				if ($line =~ /^\s*$sign(.*)/i) {
> +					if($1 !~ /^\s*(\s[a-zA-Z]*.*)/i) {

Email addresses may start with a quote (") or a digit.

Space between if and (

> +						WARN("Space required after $sign\n" .
> +							$herecurr);
> +					}
> +					if($1 !~ /[\sa-zA-Z]+\s<.*>/i) {

Space between if and (

Re: [patch v2] checkpatch: Signature format verification

[Steven Rostedt]

On Sun, 2011-05-22 at 21:14 -0700, Joe Perches wrote:
> On Mon, 2011-05-23 at 08:26 +0530, anish singh wrote:
> > On Sun, May 22, 2011 at 7:46 PM, Joe Perches <joe@perches.com> wrote:
> >         On Sun, 2011-05-22 at 15:48 +0530, anish wrote:
> >         > This patch generates warning when there is no space between
> >         > the patch submitter and successive mail-id.
> >         []
> > What does this mean?I checked in kernelnewbies archives and asked
> > on IRC channel but couldn't get the meaning.Kindly let me know.
> 
> It means I didn't quote all of your original message.
> 

Of course, we usually do "[...]" or "[..]". Without the dots, it just
looks like some kind of uninitialized array.

-- Steve

Re: [patch v2] checkpatch: Signature format verification

[Joe Perches]

On Mon, 2011-05-23 at 12:22 -0400, Steven Rostedt wrote:
> On Sun, 2011-05-22 at 21:14 -0700, Joe Perches wrote:
> > On Mon, 2011-05-23 at 08:26 +0530, anish singh wrote:
> > > On Sun, May 22, 2011 at 7:46 PM, Joe Perches <joe@perches.com> wrote:
> > >         []
> > > What does this mean?
> > It means I didn't quote all of your original message.
> Of course, we usually do "[...]" or "[..]". Without the dots, it just
> looks like some kind of uninitialized array.

Email isn't C and I'm a trendsetter.
(Either that or I'm alone again in right field...)

cheers, Joe

Re: [patch v3] checkpatch: Signature format verification

[Joe Perches]

On Mon, 2011-05-23 at 22:03 +0530, anish singh wrote:
> \s[a-zA-Z]* => this is a regular expression for name & space b/w
> Last/First name
>                     Ex: anish kumar
> .* => It is used to grep whatever follows name...in our context
> mail-id
>         Ex: abc@bbc.com.
> So, $1 will contain ...'anish kumar <anish@gmail.com>'

Unless the email contains a quote or a period like:

Signed-off-by: "J. Random Contributor" <jrc@domain.tld>

Re: [patch v3] checkpatch: Signature format verification

[Randy Dunlap]

On Mon, 23 May 2011 09:36:38 -0700 Joe Perches wrote:

> On Mon, 2011-05-23 at 22:03 +0530, anish singh wrote:
> > \s[a-zA-Z]* => this is a regular expression for name & space b/w
> > Last/First name
> >                     Ex: anish kumar
> > .* => It is used to grep whatever follows name...in our context
> > mail-id
> >         Ex: abc@bbc.com.
> > So, $1 will contain ...'anish kumar <anish@gmail.com>'
> 
> Unless the email contains a quote or a period like:
> 
> Signed-off-by: "J. Random Contributor" <jrc@domain.tld>

and quotes are not required (i.e., are optional) AFAIK.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [patch v3] checkpatch: Signature format verification

[Joe Perches]

On Mon, 2011-05-23 at 09:39 -0700, Randy Dunlap wrote:
> On Mon, 23 May 2011 09:36:38 -0700 Joe Perches wrote:
> > On Mon, 2011-05-23 at 22:03 +0530, anish singh wrote:
> > > \s[a-zA-Z]* => this is a regular expression for name & space b/w
> > > Last/First name
> > >                     Ex: anish kumar
> > > .* => It is used to grep whatever follows name...in our context
> > > mail-id
> > >         Ex: abc@bbc.com.
> > > So, $1 will contain ...'anish kumar <anish@gmail.com>'
> > Unless the email contains a quote or a period like:
> > Signed-off-by: "J. Random Contributor" <jrc@domain.tld>
> and quotes are not required (i.e., are optional) AFAIK.

And still should be accepted.

Re: [patch v3] checkpatch: Signature format verification

[Randy Dunlap]

On Mon, 23 May 2011 22:36:12 +0530 anish singh wrote:

> On Mon, May 23, 2011 at 10:12 PM, Joe Perches <joe@perches.com> wrote:
> 
> > On Mon, 2011-05-23 at 09:39 -0700, Randy Dunlap wrote:
> > > On Mon, 23 May 2011 09:36:38 -0700 Joe Perches wrote:
> > > > On Mon, 2011-05-23 at 22:03 +0530, anish singh wrote:
> > > > > \s[a-zA-Z]* => this is a regular expression for name & space b/w
> > > > > Last/First name
> > > > >                     Ex: anish kumar
> > > > > .* => It is used to grep whatever follows name...in our context
> > > > > mail-id
> > > > >         Ex: abc@bbc.com.
> > > > > So, $1 will contain ...'anish kumar <anish@gmail.com>'
> > > > Unless the email contains a quote or a period like:
> > > > Signed-off-by: "J. Random Contributor" <jrc@domain.tld>
> > > and quotes are not required (i.e., are optional) AFAIK.
> >
> > And still should be accepted.
> >
> if ($1 !~ /^\s+([a-zA-Z\s\"]*.*)/i) {
>             WARN("Space required after $sign\n" .
>             $herecurr);
>   }
>  if ($1 !~ /[\sa-zA-Z\"]*\s<.*>/i) {
>  WARN("Space required b/w Full Name & Mail-id:\n" .

"b/w" is not a good abbreviation (for "between" ???).

>                                                         $herecurr);
>                                         }
> Qoute has been taken care too now.


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [patch v3] checkpatch: Signature format verification

[Joe Perches]

On Mon, 2011-05-23 at 22:36 +0530, anish singh wrote:
>  if ($1 !~ /[\sa-zA-Z\"]*\s<.*>/i) {
>  WARN("Space required b/w Full Name & Mail-id:\n" .
>                                                         $herecurr);
>                                         }
> Quote has been taken care too now.

If you're going to try to validate email addresses,
please do it reasonably correctly or not at all.

At a minimum you need to add digits, single quote,
period, and comma.

Look at git-send-email.perl or scripts/get_maintainer.pl
for email address validation samples.

Re: [patch v3] checkpatch: Signature format verification

[Joe Perches]

On Mon, 2011-05-23 at 23:07 +0530, anish singh wrote:
> On Mon, May 23, 2011 at 10:48 PM, Joe Perches <joe@perches.com> wrote:
> On Mon, 2011-05-23 at 22:36 +0530, anish singh wrote:
> > >  if ($1 !~ /[\sa-zA-Z\"]*\s<.*>/i) {
> > >  WARN("Space required b/w Full Name & Mail-id:\n" .
> > >       $herecurr);
> > > Quote has been taken care too now.
> > If you're going to try to validate email addresses,
> > please do it reasonably correctly or not at all.
> > At a minimum you need to add digits, single quote,
> > period, and comma.
> I think there is a confusion.Let me state it once again.
> This patch is used to check space between "signs" & name/ name &
> mail-id.

I'm not confused.

You are only checking names with this form (without quotes)
"First Last <fl@domain.tld>", now quoted as well.

Names can have many forms.

8 bit chars, commas, quotes, apostrophes, all sorts of things.

for instance:

$ git log v2.6.36.. | grep -P "by:.*" | \
	grep -vP "by:[\sa-zA-Z\"]*\s<.*>" | \
	sort | uniq -c | sort -rn | head -20
   3421     Signed-off-by: David S. Miller <davem@davemloft.net>
   3137     Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
   2232     Signed-off-by: John W. Linville <linville@tuxdriver.com>
    386     Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
    292     Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
    257     Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
    223     Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
    193     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
    167     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
    166     Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
    159     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    126     Signed-off-by: Jean-François Moine <moinejf@free.fr>
    121     Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
    107     Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
    106     Acked-by: David S. Miller <davem@davemloft.net>
    103     Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
     76     Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
     69     Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
     66     Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
     63     Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

I still think what I suggested after your first patch
attempt is simple and appropriate.

our $Valid_Signatures "(?:Signed-off-by:|Reviewed-by:|Acked-by:)"
...
        if ($line =~ /^(\s*)($ValidSignatures)(\s*)(.*)$/i) {
                my $space_before = $1;
                my $sign_off = $2;
                my $space_after = $3;
                my $email = $4;
                if (defined $space_before && $space_before ne "") {
                        warning (no space before...)
                }
                if ($sign_off !~ /$Valid_Signature/) {
                        warning (signature case...)
                }
                if (!defined $space_after || $space_after ne " ") {
                        warning (need only one space after colon...)
                }
                if (!validate_email($4)) {
                        warning (bad email...)

Re: [patch v3] checkpatch: Signature format verification

[Mike Frysinger]

On Mon, May 23, 2011 at 14:09, Joe Perches wrote:
>    167     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

personally, i dont think those quotes should be there
-mike

Re: [patch v3] checkpatch: Signature format verification

[Valdis.Kletnieks]

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

On Mon, 23 May 2011 18:51:42 EDT, Mike Frysinger said:
> On Mon, May 23, 2011 at 14:09, Joe Perches wrote:
> >    167     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> 
> personally, i dont think those quotes should be there

Although not strictly required by the ABNF of RFC822 and its successors, it's
probably a wise thing to apply the double quotes in this case anyhow, due to
the fairly high likelyhood that some software will choke on the ' character in
Ted's last name if unquoted.


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

Re: [patch v3] checkpatch: Signature format verification

[Mike Frysinger]

On Mon, May 23, 2011 at 21:24,  <Valdis.Kletnieks@vt.edu> wrote:
> On Mon, 23 May 2011 18:51:42 EDT, Mike Frysinger said:
>> On Mon, May 23, 2011 at 14:09, Joe Perches wrote:
>> >    167     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>>
>> personally, i dont think those quotes should be there
>
> Although not strictly required by the ABNF of RFC822 and its successors, it's
> probably a wise thing to apply the double quotes in this case anyhow, due to
> the fairly high likelyhood that some software will choke on the ' character in
> Ted's last name if unquoted.

then people will notice their software blows and they'll fix it

i dont have a problem with "over" quoting with the From/To/CC headers
in say `git format-patch` or `git send-email`, but i dont think they
make sense in s-o-b/related tags.
-mike

Re: [patch v3] checkpatch: Signature format verification

[Randy Dunlap]

On Mon, 23 May 2011 21:31:29 -0400 Mike Frysinger wrote:

> On Mon, May 23, 2011 at 21:24,  <Valdis.Kletnieks@vt.edu> wrote:
> > On Mon, 23 May 2011 18:51:42 EDT, Mike Frysinger said:
> >> On Mon, May 23, 2011 at 14:09, Joe Perches wrote:
> >> >    167     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> >>
> >> personally, i dont think those quotes should be there
> >
> > Although not strictly required by the ABNF of RFC822 and its successors, it's
> > probably a wise thing to apply the double quotes in this case anyhow, due to
> > the fairly high likelyhood that some software will choke on the ' character in
> > Ted's last name if unquoted.
> 
> then people will notice their software blows and they'll fix it
> 
> i dont have a problem with "over" quoting with the From/To/CC headers
> in say `git format-patch` or `git send-email`, but i dont think they
> make sense in s-o-b/related tags.

They certainly facilitate copy-and-paste at least.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

[patch v4] checkpatch: Signature format verification

[anish]

From: anish kumar <anish198519851985@gmail.com>

This patch generates warning when there is no space between
the patch submitter and successive mail-id.

V2 Modification:Suggested by Joe Perches(joe@perches.com) that
we can add this check for all signature types so added that
change and added logic to remove the inefficent looping so
that it can come out as soon as signature type is matched.

V3 Modification:Moved the variable from global to local scope.

v4 Suggested by Joe Perches(joe@perches.com) that names can
have many forms.8 bit chars,commas,quotes,apostrphes,all sort
of things.This is now taken care of.

Signed-off-by: anish kumar <anish198519851985@gmail.com>
---
 scripts/checkpatch.pl |   36 ++++++++++++++++++++++++------------
 1 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d867081..bf724c9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1365,20 +1365,32 @@ sub process {
 			}
 		}
 
-#check the patch for a signoff:
-		if ($line =~ /^\s*signed-off-by:/i) {
-			# This is a signoff, if ugly, so do not double report.
-			$signoff++;
-			if (!($line =~ /^\s*Signed-off-by:/)) {
-				WARN("Signed-off-by: is the preferred form\n" .
-					$herecurr);
-			}
-			if ($line =~ /^\s*signed-off-by:\S/i) {
-				WARN("space required after Signed-off-by:\n" .
-					$herecurr);
+#check the patch for a signoff/Reviewed/Acked/Tested:
+		my ($sign,$loop_brk);
+		my @signs = ("Reviewed-by:","Acked-by:","Signed-off-by:","Tested-by:");
+		foreach $sign (@signs) {
+			$loop_brk=0;
+			if ($line =~ /^\s*$sign/i) {
+				# This is a signoff, if ugly, so do not double report.
+				$signoff++;
+				$loop_brk++;
+				if (!($line =~ /^\s*$sign/)) {
+					WARN("$sign is the preferred form\n" .
+						$herecurr);
+				}
+				if ($line =~ /^\s*$sign(.*)/i) {
+					if ($1 !~ /^\s+([a-zA-Z\s\"\.\-\'\,]*.*)/i) {
+						WARN("Space required after $sign\n" .
+							$herecurr);
+					}
+					if ($1 !~ /([\sa-zA-Z\"\.\-\'\,]*)\s<.*>/i) {
+						WARN("Space required b/w Full Name & Mail-id:\n" .
+							$herecurr);
+					}
+				}
 			}
+			last if ($loop_brk == 1);
 		}
-
 # Check for wrappage within a valid hunk of the file
 		if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) {
 			ERROR("patch seems to be corrupt (line wrapped?)\n" .
-- 
1.7.0.4

Re: [patch v4] checkpatch: Signature format verification

[Steven Rostedt]

On Fri, 2011-05-27 at 23:31 +0530, anish wrote:
> From: anish kumar <anish198519851985@gmail.com>
> 

> +		my ($sign,$loop_brk);
> +		my @signs = ("Reviewed-by:","Acked-by:","Signed-off-by:","Tested-by:");
> +		foreach $sign (@signs) {
> +			$loop_brk=0;
> +			if ($line =~ /^\s*$sign/i) {
> +				# This is a signoff, if ugly, so do not double report.
> +				$signoff++;
> +				$loop_brk++;
> +				if (!($line =~ /^\s*$sign/)) {
> +					WARN("$sign is the preferred form\n" .
> +						$herecurr);
> +				}
> +				if ($line =~ /^\s*$sign(.*)/i) {
> +					if ($1 !~ /^\s+([a-zA-Z\s\"\.\-\'\,]*.*)/i) {
> +						WARN("Space required after $sign\n" .
> +							$herecurr);
> +					}
> +					if ($1 !~ /([\sa-zA-Z\"\.\-\'\,]*)\s<.*>/i) {
> +						WARN("Space required b/w Full Name & Mail-id:\n" .

What's "b/w" black and white?

Also do we check for <>. As the above will trigger for missing <> and
give a warning about spaces.

I once had my quilt mail send crap to LKML because one of the Cc's in a
patch I pulled was missing the ending ">".

-- Steve

> +							$herecurr);
> +					}
> +				}
>  			}
> +			last if ($loop_brk == 1);
>  		}
> -
>  # Check for wrappage within a valid hunk of the file
>  		if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) {
>  			ERROR("patch seems to be corrupt (line wrapped?)\n" .

Re: [patch v4] checkpatch: Signature format verification

[Randy Dunlap]

On Fri, 27 May 2011 14:16:57 -0400 Steven Rostedt wrote:

> On Fri, 2011-05-27 at 23:31 +0530, anish wrote:
> > From: anish kumar <anish198519851985@gmail.com>
> > 
> 
> > +		my ($sign,$loop_brk);
> > +		my @signs = ("Reviewed-by:","Acked-by:","Signed-off-by:","Tested-by:");
> > +		foreach $sign (@signs) {
> > +			$loop_brk=0;
> > +			if ($line =~ /^\s*$sign/i) {
> > +				# This is a signoff, if ugly, so do not double report.
> > +				$signoff++;
> > +				$loop_brk++;
> > +				if (!($line =~ /^\s*$sign/)) {
> > +					WARN("$sign is the preferred form\n" .
> > +						$herecurr);
> > +				}
> > +				if ($line =~ /^\s*$sign(.*)/i) {
> > +					if ($1 !~ /^\s+([a-zA-Z\s\"\.\-\'\,]*.*)/i) {
> > +						WARN("Space required after $sign\n" .
> > +							$herecurr);
> > +					}
> > +					if ($1 !~ /([\sa-zA-Z\"\.\-\'\,]*)\s<.*>/i) {
> > +						WARN("Space required b/w Full Name & Mail-id:\n" .
> 
> What's "b/w" black and white?

I _think_ that it's "between", but I already requested that it
be changed.  It's not good.  :(


> Also do we check for <>. As the above will trigger for missing <> and
> give a warning about spaces.
> 
> I once had my quilt mail send crap to LKML because one of the Cc's in a
> patch I pulled was missing the ending ">".
> 
> -- Steve
> 
> > +							$herecurr);
> > +					}
> > +				}
> >  			}
> > +			last if ($loop_brk == 1);
> >  		}
> > -
> >  # Check for wrappage within a valid hunk of the file
> >  		if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) {
> >  			ERROR("patch seems to be corrupt (line wrapped?)\n" .


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [patch v4] checkpatch: Signature format verification

[Joe Perches]

On Fri, 2011-05-27 at 23:31 +0530, anish wrote:
> From: anish kumar <anish198519851985@gmail.com>
[]
> v4 Suggested by Joe Perches(joe@perches.com) that names can
> have many forms.8 bit chars,commas,quotes,apostrphes,all sort
> of things.This is now taken care of.

NAK.  No it's not.

> Signed-off-by: anish kumar <anish198519851985@gmail.com>
[]
> +				if ($line =~ /^\s*$sign(.*)/i) {
> +					if ($1 !~ /^\s+([a-zA-Z\s\"\.\-\'\,]*.*)/i) {
> +						WARN("Space required after $sign\n" .
> +							$herecurr);

8 bit chars, digits?

> +					}
> +					if ($1 !~ /([\sa-zA-Z\"\.\-\'\,]*)\s<.*>/i) {
> +						WARN("Space required b/w Full Name & Mail-id:\n" .
> +							$herecurr);

For the 3rd time, please use this form:

        if ($line =~ /^(\s*)($ValidSignatures)(\s*)(.*)$/i) {

and if you really want email format validation,
use a separate function.

Re: [patch v4] checkpatch: Signature format verification

[Joe Perches]

On Fri, 2011-05-27 at 14:16 -0400, Steven Rostedt wrote:
> I once had my quilt mail send crap to LKML because one of the Cc's in a
> patch I pulled was missing the ending ">".

I'm pretty sure I've done that using git-send-email
via bad copy/paste.

Actually, that one's been done a few times.

$ git log --pretty=oneline -E -i --grep "-by:.*<[^>]+$" | wc -l
129

But that's out of ~3.5 million non-merge commits.

cheers, Joe

Re: [patch v4] checkpatch: Signature format verification

[Joe Perches]

On Fri, 2011-05-27 at 11:45 -0700, Joe Perches wrote:
> But that's out of ~3.5 million non-merge commits.

Oops. Off by an order magnitude, stupid fingers.

$ git log --pretty=oneline --no-merges  | wc -l
232816

Re: [patch v4] checkpatch: Signature format verification

[Joe Perches]

On Sat, 2011-05-28 at 00:28 +0530, Manish Kumar Singh wrote:
> On Fri, May 27, 2011 at 11:59 PM, Joe Perches <joe@perches.com> wrote:
[]
>         8 bit chars, digits?
> What is 8 bit char...could you describe it?

An ascii char with the high order bit set.
Any char from 128 to 255.

[]

>         For the 3rd time, please use this form:
>                if ($line =~ /^(\s*)($ValidSignatures)(\s*)(.*)$/i) {
>         and if you really want email format validation,
>         use a separate function.

[]

> Problem with your way of doing is if anyone misses space between
> "sign" & name,
> the regular expression (/^(\s*)($ValidSignatures)(\s*)(.*)$/i )
> doesn't match

That's incorrect.  Try it.

Re: [patch v4] checkpatch: Signature format verification

[Steven Rostedt]

On Fri, 2011-05-27 at 11:45 -0700, Joe Perches wrote:
> On Fri, 2011-05-27 at 14:16 -0400, Steven Rostedt wrote:
> > I once had my quilt mail send crap to LKML because one of the Cc's in a
> > patch I pulled was missing the ending ">".
> 
> I'm pretty sure I've done that using git-send-email
> via bad copy/paste.
> 
> Actually, that one's been done a few times.
> 
> $ git log --pretty=oneline -E -i --grep "-by:.*<[^>]+$" | wc -l
> 129
> 
> But that's out of 232816 non-merge commits.

Even so, it would have been nice, because I ended up sending a crap load
of emails to LKML without any content. I still use quilt as I like to
verify what I send. But it took a dump on the missing ">" and caused all
patches to be sent out as crap.

-- Steve

Re: [patch v4] checkpatch: Signature format verification

[Joe Perches]

On Sat, 2011-05-28 at 00:47 +0530, anish singh wrote:
> On Sat, May 28, 2011 at 12:38 AM, Joe Perches <joe@perches.com> wrote:
>         On Sat, 2011-05-28 at 00:28 +0530, Manish Kumar Singh wrote:
>         >         For the 3rd time, please use this form:
>         >                if ($line =~ /^(\s*)($ValidSignatures)(\s*)(.*)$/i) {
>         >         and if you really want email format validation,
>         >         use a separate function.
>         []
>         > Problem with your way of doing is if anyone misses space
>         between
>         > "sign" & name,
>         > the regular expression
>         (/^(\s*)($ValidSignatures)(\s*)(.*)$/i )
>         > doesn't match
>         That's incorrect.  Try it.

Anish, do you have a response to this?

> I have replaced b/w with "between".
> And as Manish rightly exlained this patch is not meant for 
> email validation.

Yet it does incorrectly anyway to nominally skip
name and find <.*>.  That's the problem.

Look again at the patterns:

+                               if ($line =~ /^\s*$sign(.*)/i) {
+                                       if ($1 !~ /^\s+([a-zA-Z\s\"\.\-\'\,]*.*)/i) {
+                                               WARN("Space required after $sign\n" .
+                                                       $herecurr);
+                                       }

and

+                                       if ($1 !~ /([\sa-zA-Z\"\.\-\'\,]*)\s<.*>/i) {
+                                               WARN("Space required b/w Full Name & Mail-id:\n" .
+                                                       $herecurr);
+                                       }

> I would like to know if anything else is missing?

Correctness.

Keep at it, it'll eventually be good to go.

cheers, Joe

Re: [patch v4] checkpatch: Signature format verification

[Joe Perches]

On Fri, 2011-05-27 at 15:22 -0400, Steven Rostedt wrote:
> On Fri, 2011-05-27 at 11:45 -0700, Joe Perches wrote:
> > On Fri, 2011-05-27 at 14:16 -0400, Steven Rostedt wrote:
> > > I once had my quilt mail send crap to LKML because one of the Cc's in a
> > > patch I pulled was missing the ending ">".
> > $ git log --pretty=oneline -E -i --grep "-by:.*<[^>]+$" | wc -l
> > 129
> > But that's out of 232816 non-merge commits.
> Even so, it would have been nice, because I ended up sending a crap load
> of emails to LKML without any content. I still use quilt as I like to
> verify what I send. But it took a dump on the missing ">" and caused all
> patches to be sent out as crap.

No doubt.

This concept should also verify the "To: " and "cc: " tag lines.

cheers, Joe

Re: [patch v4] checkpatch: Signature format verification

[Steven Rostedt]

On Sat, 2011-05-28 at 01:24 +0530, anish singh wrote:
> 

>         
>         Anish, do you have a response to this?
> Your approach is correct but i am trying to do in my own way.

Why?? Correct is more important than individual preferences.

>         
>         > I have replaced b/w with "between".
>         > And as Manish rightly exlained this patch is not meant for
>         > email validation.
>         
>         
>         Yet it does incorrectly anyway to nominally skip
>         name and find <.*>.  That's the problem.
>         
>         Look again at the patterns:
>         
>         +                               if ($line =~ /^\s*$sign(.*)/i)
>         {
>         +                                       if ($1 !~ /^\s
>         +([a-zA-Z\s\"\.\-\'\,]*.*)/i) {

Note, why the []? Because you basically have:

	/^\s+([blahblah]*.*)/

which is the same as:

	/^\s+(.*)/

>         +                                               WARN("Space
>         required after $sign\n" .
>         +
>         $herecurr);
>         
>         +                                       }
>  
> here i am trying to check space between sign & name.
> In first if loop i am checking if sign is present in line or not.
> If yes, enter into second if loop & check if $1 starts with space 
> or not(which takes care of checking space between sign & name).
> In this if loop again i am catching name & rest of the things in $1.
> $1 will be used for checking space between name & mail-id.

BTW, people hate perl for this very reason. This subtle changing of $1
is very hard to follow if you are not a perl expert. I use perl all the
time (recordmcount.pl and ktest.pl), but if you look at my code, you
will see that I purposely avoid these subtle characteristics of perl,
because that's the (very rightfully so) reason people complain that perl
is a write once and never maintain language.

I understood from the beginning what you were doing, but I had to think
about it more than I should have.

>         
>         and
>         
>         +                                       if ($1 !~ /([\sa-zA-Z
>         \"\.\-\'\,]*)\s<.*>/i) {
>         +                                               WARN("Space
>         required b/w Full Name & Mail-id:\n" .
>         +
>         $herecurr);
>         
>         +                                       }


You really need to get a new MUA.
>         
>         
>  
> In this If loop i am using $1 (returned by earlier pattern)  to check
> space 
> between mail and <mail-id>.
> If we print $1 after last if loop, we can get name appropriately.
> So, i am not skipping name here to match <>.
>  
> I will change <.*> to <.*?> (non-greedy match) for matching very first
> enclosing bracket.

Again, this doesn't explain the space warning when I have just his:

 Signed-off-by: <me@foo.bar>

Because that will not match the above, because the $1 will be
"<me@foo.bar>". With no space.


-- Steve

Re: [patch v4] checkpatch: Signature format verification

[Joe Perches]

On Fri, 2011-05-27 at 16:18 -0400, Steven Rostedt wrote:
> On Sat, 2011-05-28 at 01:24 +0530, anish singh wrote:
> >         Anish, do you have a response to this?
> > Your approach is correct but i am trying to do in my own way.
> Why?? Correct is more important than individual preferences.

Thanks Steven, I was about to write something similar.

[]
> > In this if loop again i am catching name & rest of the things in $1.
> > $1 will be used for checking space between name & mail-id.
> BTW, people hate perl for this very reason. This subtle changing of $1
> is very hard to follow if you are not a perl expert. I use perl all the
> time (recordmcount.pl and ktest.pl), but if you look at my code, you
> will see that I purposely avoid these subtle characteristics of perl,
> because that's the (very rightfully so) reason people complain that perl
> is a write once and never maintain language.

I'm not a perl geek, nor do I want to be, but
that's exactly why I suggested after Anish's
first attempt something I think reasonably clear
and straightforward which avoids any reuse of $n.

Anish, please use this style.

	if ($line =~ /^(\s*)($ValidSignatures)(\s*)(.*)$/i) {
		my $space_before = $1;
		my $sign_off = $2;
		my $space_after = $3;
		my $email = $4;
		if (defined $space_before && $space_before ne "") {
			warning (no space before...)
		}
		if ($sign_off !~ /$Valid_Signature/) {
			warning (signature case...)
		}
		if (!defined $space_after || $space_after ne " ") {
			warning (need only one space after colon...)
		}
		if (!validate_email($email)) {
			warning (bad email...)
		}
	}

Re: [patch v4] checkpatch: Signature format verification

[Andrew Morton]

On Fri, 27 May 2011 23:31:51 +0530
anish <anish198519851985@gmail.com> wrote:

> This patch generates warning when there is no space between
> the patch submitter and successive mail-id.

I don't even know what this means :( Please modify the changelog to
include a complete example of the offending input and of the generated
warning.

Re: [patch v4] checkpatch: Signature format verification

[Joe Perches]

On Sat, 2011-05-28 at 07:08 +0530, anish singh wrote:
> On Sat, May 28, 2011 at 1:48 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>         On Sat, 2011-05-28 at 01:24 +0530, anish singh wrote:
>         >         Anish, do you have a response to this?
>         > Your approach is correct but i am trying to do in my own
>         way.
>         Why?? Correct is more important than individual preferences.
> Absolutely.Will use Joe suggested style of coding.

Good.  And please use text only not html emails
when cc'ing any list at vger.kernel.org.