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

Re-sending to get some comments on this.

On Thu, May 19, 2011 at 10:53 PM, anish <[email protected]> wrote:
From: anish kumar <[email protected]>

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

Quoted 3 lines

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

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

        Signed-off-by: anish kumar <[email protected]>
        ---
         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" .

Quoted 16 lines

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

Quoted 2 lines · Joe Perches

Quoted 2 lines

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([email protected]) that we can do this for
all signature types.

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

Signed-off-by anish kumar <[email protected]>

On Sun, May 22, 2011 at 7:46 PM, Joe Perches <[email protected]> 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.

Quoted 7 lines · anish singh

On Sun, May 22, 2011 at 7:46 PM, Joe Perches <[email protected]> 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.

Quoted 5 lines · Joe Perches

Quoted 3 lines · anish singh

On Sun, May 22, 2011 at 7:46 PM, Joe Perches <[email protected]> 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.

From: anish kumar <[email protected]> This patch generates warning when there is no space between the patch submitter and successive mail-id.

\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: [email protected].
So, $1 will contain ...'anish kumar <[email protected]>'

Quoted 7 lines · anish singh

\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: [email protected].
So, $1 will contain ...'anish kumar <[email protected]>'

Unless the email contains a quote or a period like:

Signed-off-by "J. Random Contributor" <[email protected]>

Quoted 8 lines · On Mon, 23 May 2011 09:36:38 -0700 Joe Perches wrote:

Quoted 7 lines · anish singh

\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: [email protected].
So, $1 will contain ...'anish kumar <[email protected]>'

Quoted 2 lines

Unless the email contains a quote or a period like:

Signed-off-by "J. Random Contributor" <[email protected]>

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

Quoted 14 lines · Joe Perches

Quoted 12 lines · Randy Dunlap

Quoted 10 lines · On Mon, 23 May 2011 09:36:38 -0700 Joe Perches wrote:

Quoted 7 lines · anish singh

\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: [email protected].
So, $1 will contain ...'anish kumar <[email protected]>'

Unless the email contains a quote or a period like:

Signed-off-by "J. Random Contributor" <[email protected]>

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

Quoted 2 lines

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

                                                        $herecurr);
                                        }

Qoute has been taken care too now.

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.

On Mon, May 23, 2011 at 10:48 PM, Joe Perches <[email protected]> wrote:

Quoted 4 lines · anish singh

Quoted 4 lines

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.

Quoted 4 lines

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.

   167     Signed-off-by: "Theodore Ts'o" <[email protected]>

Quoted 1 line · Joe Perches

   167     Signed-off-by: "Theodore Ts'o" <[email protected]>

personally, i dont think those quotes should be there

Quoted 4 lines · Mike Frysinger

Quoted 1 line · Joe Perches

   167     Signed-off-by: "Theodore Ts'o" <[email protected]>

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.

Quoted 10 lines · On Mon, May 23, 2011 at 21:24, <[email protected]> wrote:

Quoted 4 lines · Mike Frysinger

Quoted 1 line · Joe Perches

   167     Signed-off-by: "Theodore Ts'o" <[email protected]>

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.

From: anish kumar <[email protected]>

scripts/checkpatch.pl +9 -0

+		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/)) {

scripts/checkpatch.pl +4 -1

+							$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" .

Quoted 16 lines · anish

From: anish kumar <[email protected]>

scripts/checkpatch.pl +13 -0

+		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) {

scripts/checkpatch.pl sub process { · line 1387

Quoted 6 lines

scripts/checkpatch.pl +6 -0

+					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

scripts/checkpatch.pl sub process { · line 1396

Quoted 10 lines

scripts/checkpatch.pl +4 -1

+							$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" .

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

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

Quoted 2 lines · Steven Rostedt

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.

Quoted 6 lines · Joe Perches

Quoted 2 lines · Steven Rostedt

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.

From: anish kumar <[email protected]>

v4 Suggested by Joe Perches([email protected]) 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 <[email protected]>

On Fri, May 27, 2011 at 11:59 PM, Joe Perches <[email protected]> wrote:

8 bit chars, digits?
What is 8 bit char...could you describe it?

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

On Sat, May 28, 2011 at 12:38 AM, Joe Perches <[email protected]> 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.

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

I would like to know if anything else is missing?

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

> 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);

+ }

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.

and

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

+ }

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.

Quoted 2 lines · anish singh

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.

Quoted 2 lines

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.

On Sat, May 28, 2011 at 1:48 AM, Steven Rostedt <[email protected]> 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.

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