From: anish kumar <[email protected]>
This patch generates warning when there is no space between the
patch submitter name and successive mail-id.
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" .
+ if ($line =~ /^\s*signed-off-by:(.*)/i) {
+ if($1 !~ /^(\s[a-zA-Z].*)/i) {
+ WARN("Space required after Signed-off-by:\n" .
$herecurr);
}
+ if($1 !~ /[\sa-zA-Z]+\s<.*>/i) {
+ WARN("Space required between Full Name & Mail-id:\n" .
+ $herecurr);
+ }
+ }
}
# Check for wrappage within a valid hunk of the file
--
1.7.0.4
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 <[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.
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" .
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...)
On Sat, May 21, 2011 at 9:33 PM, Joe Perches <[email protected]> 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 <[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.
>>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 <[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" .
>> 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 <[email protected]>
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.
Signed-off-by: anish kumar <[email protected]>
---
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
On Sun, 2011-05-22 at 01:08 +0530, anish wrote:
> On Sat, May 21, 2011 at 9:33 PM, Joe Perches <[email protected]> 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([email protected]) that we can do this for
> all signature types.
Typos and spacing issues in commit message.
> Signed-off-by: anish kumar <[email protected]>
> 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
From: anish kumar <[email protected]>
This patch generates warning when there is no space between
the patch submitter and successive mail-id.
Modification:Suggested by Joe Perches([email protected]) 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 <[email protected]>
---
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
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 <[email protected]>
[]
> 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.
On Mon, 2011-05-23 at 08:26 +0530, anish singh wrote:
> 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.
From: anish kumar <[email protected]>
This patch generates warning when there is no space between
the patch submitter and successive mail-id.
V2 Modification:Suggested by Joe Perches([email protected]) 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 <[email protected]>
---
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
On Mon, 2011-05-23 at 20:51 +0530, anish wrote:
> From: anish kumar <[email protected]>
> 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 (
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 <[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.
>
Of course, we usually do "[...]" or "[..]". Without the dots, it just
looks like some kind of uninitialized array.
-- Steve
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 <[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.
Email isn't C and I'm a trendsetter.
(Either that or I'm alone again in right field...)
cheers, Joe
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: [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]>
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: [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.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
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: [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.
And still should be accepted.
On Mon, 23 May 2011 22:36:12 +0530 anish singh wrote:
> On Mon, May 23, 2011 at 10:12 PM, Joe Perches <[email protected]> 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: [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.
> >
> > 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 ***
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.
On Mon, 2011-05-23 at 23:07 +0530, anish singh wrote:
> On Mon, May 23, 2011 at 10:48 PM, Joe Perches <[email protected]> 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 <[email protected]>", 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 <[email protected]>
3137 Signed-off-by: Greg Kroah-Hartman <[email protected]>
2232 Signed-off-by: John W. Linville <[email protected]>
386 Signed-off-by: Wey-Yi Guy <[email protected]>
292 Signed-off-by: Gustavo F. Padovan <[email protected]>
257 Signed-off-by: Uwe Kleine-König <[email protected]>
223 Signed-off-by: Rafael J. Wysocki <[email protected]>
193 Signed-off-by: J. Bruce Fields <[email protected]>
167 Signed-off-by: "Theodore Ts'o" <[email protected]>
166 Signed-off-by: H. Peter Anvin <[email protected]>
159 Signed-off-by: Paul E. McKenney <[email protected]>
126 Signed-off-by: Jean-François Moine <[email protected]>
121 Signed-off-by: Luis R. Rodriguez <[email protected]>
107 Signed-off-by: Michał Mirosław <[email protected]>
106 Acked-by: David S. Miller <[email protected]>
103 Signed-off-by: Rafał Miłecki <[email protected]>
76 Signed-off-by: Aneesh Kumar K.V <[email protected]>
69 Signed-off-by: Lars-Peter Clausen <[email protected]>
66 Signed-off-by: Stephen M. Cameron <[email protected]>
63 Signed-off-by: Justin P. Mattock <[email protected]>
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...)
On Mon, May 23, 2011 at 14:09, Joe Perches wrote:
> 167 Signed-off-by: "Theodore Ts'o" <[email protected]>
personally, i dont think those quotes should be there
-mike
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" <[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.
On Mon, May 23, 2011 at 21:24, <[email protected]> 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" <[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.
-mike
On Mon, 23 May 2011 21:31:29 -0400 Mike Frysinger wrote:
> On Mon, May 23, 2011 at 21:24, <[email protected]> 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" <[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.
They certainly facilitate copy-and-paste at least.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
From: anish kumar <[email protected]>
This patch generates warning when there is no space between
the patch submitter and successive mail-id.
V2 Modification:Suggested by Joe Perches([email protected]) 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([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]>
---
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
On Fri, 2011-05-27 at 23:31 +0530, anish wrote:
> From: anish kumar <[email protected]>
>
> + 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" .
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 <[email protected]>
> >
>
> > + 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 ***
On Fri, 2011-05-27 at 23:31 +0530, anish wrote:
> 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.
NAK. No it's not.
> Signed-off-by: anish kumar <[email protected]>
[]
> + 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.
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
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
On Sat, 2011-05-28 at 00:28 +0530, Manish Kumar Singh wrote:
> 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?
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.
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
On Sat, 2011-05-28 at 00:47 +0530, anish singh wrote:
> 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.
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
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
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: <[email protected]>
Because that will not match the above, because the $1 will be
"<[email protected]>". With no space.
-- Steve
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...)
}
}
On Fri, 27 May 2011 23:31:51 +0530
anish <[email protected]> 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.
On Sat, 2011-05-28 at 07:08 +0530, anish singh wrote:
> 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.
Good. And please use text only not html emails
when cc'ing any list at vger.kernel.org.