Re: [PATCH] git-send-email: Add ability to cc: any "trailers" from commit message
On Wed, 2016-08-31 at 10:54 -0700, Junio C Hamano wrote: > Joe Percheswrites: > > > > Many commits have various forms of trailers similar to > > "Acked-by: Name " and "Reported-by: Name " > > > > Add the ability to cc these trailers when using git send-email. > I thought you were asking what we call these " followed by > " at the end of the log message, and "footers or trailers" > was the answer. > > I do not have a strong objection against limiting to "-by:" lines; > for one thing, it would automatically avoid having to worry about > "Bug-ID:" and other trailers that won't have e-mail address at all. > > But if you are _only_ picking up "-by:" lines, then calling this > option "trailers" is way too wide and confusing. I do not think > there is any specific name for "-by:" lines, though. Perhaps you > would need to invent some name that has "-by" as a substring. > > "any-by"? or just "by"? I dunno. Thinking about this a little, "bylines" seems much better. > >@@ -1545,7 +1545,7 @@ foreach my $t (@files) { > > # Now parse the message body > > while(<$fh>) { > > $message .= $_; > > - if (/^(Signed-off-by|Cc): (.*)$/i) { > > + if (/^(Signed-off-by|Cc|[^\s]+[_-]by): (.*)$/i) { > Micronits: > > (1) do you really want to grab a run of any non-blanks? Don't > you want to exclude at least a colon? It could use [\w_-]+ > (2) allowing an underscore looks a bit unusual. It's for typos. A relatively high percentage of these things in at least the kernel were malformed when I started this 5 years ago. I don't have an objection to requiring the proper form using only dashes though. Maybe that'd help reduce the typo frequency anyway. > I am aware of the fact that people sometimes write only a name with > no e-mail address when giving credit to a third-party and we want to > avoid upsetting the underlying MTA by feeding it a non-address. > > Looking at existing helper subs like extract_valid_address and > sanitize_address that all addresses we pass to the MTA go through, > it appears to me that we try to support an addr-spec with only > local-part without @domain, so this new check might turn out to be > too strict from that point of view, but on the other hand I suspect > it won't be a huge issue because the addresses in the footers are > for public consumption and it may not make much sense to have a > local-only address there. I dunno. > > > > > push @cc, $c; > > printf("(body) Adding cc: %s from line '%s'\n", me either but I think it doesn't hurt because as you suggest, these are supposed to be public. Thanks for the review. cheers, Joe
Re: [PATCH] git-send-email: Add ability to cc: any "trailers" from commit message
On Wed, 2016-08-31 at 10:54 -0700, Junio C Hamano wrote: > Joe Perches writes: > > > > Many commits have various forms of trailers similar to > > "Acked-by: Name " and "Reported-by: Name " > > > > Add the ability to cc these trailers when using git send-email. > I thought you were asking what we call these " followed by > " at the end of the log message, and "footers or trailers" > was the answer. > > I do not have a strong objection against limiting to "-by:" lines; > for one thing, it would automatically avoid having to worry about > "Bug-ID:" and other trailers that won't have e-mail address at all. > > But if you are _only_ picking up "-by:" lines, then calling this > option "trailers" is way too wide and confusing. I do not think > there is any specific name for "-by:" lines, though. Perhaps you > would need to invent some name that has "-by" as a substring. > > "any-by"? or just "by"? I dunno. Thinking about this a little, "bylines" seems much better. > >@@ -1545,7 +1545,7 @@ foreach my $t (@files) { > > # Now parse the message body > > while(<$fh>) { > > $message .= $_; > > - if (/^(Signed-off-by|Cc): (.*)$/i) { > > + if (/^(Signed-off-by|Cc|[^\s]+[_-]by): (.*)$/i) { > Micronits: > > (1) do you really want to grab a run of any non-blanks? Don't > you want to exclude at least a colon? It could use [\w_-]+ > (2) allowing an underscore looks a bit unusual. It's for typos. A relatively high percentage of these things in at least the kernel were malformed when I started this 5 years ago. I don't have an objection to requiring the proper form using only dashes though. Maybe that'd help reduce the typo frequency anyway. > I am aware of the fact that people sometimes write only a name with > no e-mail address when giving credit to a third-party and we want to > avoid upsetting the underlying MTA by feeding it a non-address. > > Looking at existing helper subs like extract_valid_address and > sanitize_address that all addresses we pass to the MTA go through, > it appears to me that we try to support an addr-spec with only > local-part without @domain, so this new check might turn out to be > too strict from that point of view, but on the other hand I suspect > it won't be a huge issue because the addresses in the footers are > for public consumption and it may not make much sense to have a > local-only address there. I dunno. > > > > > push @cc, $c; > > printf("(body) Adding cc: %s from line '%s'\n", me either but I think it doesn't hurt because as you suggest, these are supposed to be public. Thanks for the review. cheers, Joe
Re: [PATCH] git-send-email: Add ability to cc: any "trailers" from commit message
Joe Percheswrites: > On Wed, 2016-08-31 at 10:54 -0700, Junio C Hamano wrote: >> Joe Perches writes: >> >> > >> > Many commits have various forms of trailers similar to >> > "Acked-by: Name " and "Reported-by: Name " >> > >> > Add the ability to cc these trailers when using git send-email. >> I thought you were asking what we call these " followed by >> " at the end of the log message, and "footers or trailers" >> was the answer. >> >> I do not have a strong objection against limiting to "-by:" lines; >> for one thing, it would automatically avoid having to worry about >> "Bug-ID:" and other trailers that won't have e-mail address at all. >> >> But if you are _only_ picking up "-by:" lines, then calling this >> option "trailers" is way too wide and confusing. I do not think >> there is any specific name for "-by:" lines, though. Perhaps you >> would need to invent some name that has "-by" as a substring. >> >> "any-by"? or just "by"? I dunno. > > Signatures? Helped-by: and Reported-by: are often written by the author of the patch and not by those who helped or reported, so calling them signatures imply the author is forging them, too ;-) I dunno. Any name that hints that this applies only to the trailers that ends with "-by" is fine but "Signatures-by" does not sound very grammatical.
Re: [PATCH] git-send-email: Add ability to cc: any "trailers" from commit message
Joe Perches writes: > On Wed, 2016-08-31 at 10:54 -0700, Junio C Hamano wrote: >> Joe Perches writes: >> >> > >> > Many commits have various forms of trailers similar to >> > "Acked-by: Name " and "Reported-by: Name " >> > >> > Add the ability to cc these trailers when using git send-email. >> I thought you were asking what we call these " followed by >> " at the end of the log message, and "footers or trailers" >> was the answer. >> >> I do not have a strong objection against limiting to "-by:" lines; >> for one thing, it would automatically avoid having to worry about >> "Bug-ID:" and other trailers that won't have e-mail address at all. >> >> But if you are _only_ picking up "-by:" lines, then calling this >> option "trailers" is way too wide and confusing. I do not think >> there is any specific name for "-by:" lines, though. Perhaps you >> would need to invent some name that has "-by" as a substring. >> >> "any-by"? or just "by"? I dunno. > > Signatures? Helped-by: and Reported-by: are often written by the author of the patch and not by those who helped or reported, so calling them signatures imply the author is forging them, too ;-) I dunno. Any name that hints that this applies only to the trailers that ends with "-by" is fine but "Signatures-by" does not sound very grammatical.
Re: [PATCH] git-send-email: Add ability to cc: any "trailers" from commit message
On Wed, 2016-08-31 at 10:54 -0700, Junio C Hamano wrote: > Joe Percheswrites: > > > > > Many commits have various forms of trailers similar to > > "Acked-by: Name " and "Reported-by: Name " > > > > Add the ability to cc these trailers when using git send-email. > I thought you were asking what we call these " followed by > " at the end of the log message, and "footers or trailers" > was the answer. > > I do not have a strong objection against limiting to "-by:" lines; > for one thing, it would automatically avoid having to worry about > "Bug-ID:" and other trailers that won't have e-mail address at all. > > But if you are _only_ picking up "-by:" lines, then calling this > option "trailers" is way too wide and confusing. I do not think > there is any specific name for "-by:" lines, though. Perhaps you > would need to invent some name that has "-by" as a substring. > > "any-by"? or just "by"? I dunno. Signatures?
Re: [PATCH] git-send-email: Add ability to cc: any "trailers" from commit message
On Wed, 2016-08-31 at 10:54 -0700, Junio C Hamano wrote: > Joe Perches writes: > > > > > Many commits have various forms of trailers similar to > > "Acked-by: Name " and "Reported-by: Name " > > > > Add the ability to cc these trailers when using git send-email. > I thought you were asking what we call these " followed by > " at the end of the log message, and "footers or trailers" > was the answer. > > I do not have a strong objection against limiting to "-by:" lines; > for one thing, it would automatically avoid having to worry about > "Bug-ID:" and other trailers that won't have e-mail address at all. > > But if you are _only_ picking up "-by:" lines, then calling this > option "trailers" is way too wide and confusing. I do not think > there is any specific name for "-by:" lines, though. Perhaps you > would need to invent some name that has "-by" as a substring. > > "any-by"? or just "by"? I dunno. Signatures?
Re: [PATCH] git-send-email: Add ability to cc: any "trailers" from commit message
Joe Percheswrites: > Many commits have various forms of trailers similar to > "Acked-by: Name " and "Reported-by: Name " > > Add the ability to cc these trailers when using git send-email. I thought you were asking what we call these " followed by " at the end of the log message, and "footers or trailers" was the answer. I do not have a strong objection against limiting to "-by:" lines; for one thing, it would automatically avoid having to worry about "Bug-ID:" and other trailers that won't have e-mail address at all. But if you are _only_ picking up "-by:" lines, then calling this option "trailers" is way too wide and confusing. I do not think there is any specific name for "-by:" lines, though. Perhaps you would need to invent some name that has "-by" as a substring. "any-by"? or just "by"? I dunno. > if ($suppress_cc{'all'}) { > - foreach my $entry (qw (cccmd cc author self sob body bodycc)) { > + foreach my $entry (qw (cccmd cc author self sob body bodycc trailers)) { > $suppress_cc{$entry} = 1; > } OK. > @@ -448,7 +448,7 @@ $suppress_cc{'self'} = $suppress_from if defined > $suppress_from; > $suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc; > > if ($suppress_cc{'body'}) { > - foreach my $entry (qw (sob bodycc)) { > + foreach my $entry (qw (sob bodycc trailers)) { > $suppress_cc{$entry} = 1; > } > delete $suppress_cc{'body'}; OK. > @@ -1545,7 +1545,7 @@ foreach my $t (@files) { > # Now parse the message body > while(<$fh>) { > $message .= $_; > - if (/^(Signed-off-by|Cc): (.*)$/i) { > + if (/^(Signed-off-by|Cc|[^\s]+[_-]by): (.*)$/i) { Micronits: (1) do you really want to grab a run of any non-blanks? Don't you want to exclude at least a colon? (2) allowing an underscore looks a bit unusual. > @@ -1555,6 +1555,12 @@ foreach my $t (@files) { > } else { > next if $suppress_cc{'sob'} and $what =~ > /Signed-off-by/i; > next if $suppress_cc{'bodycc'} and $what =~ > /Cc/i; > + next if $suppress_cc{'trailers'} and $what !~ > /Signed-off-by/i && $what =~ /by$/i; > + } It is a bit unfortunate that S-o-b is a subset of any-by that forces you to do this. > + if ($c !~ /.+@.+/) { > + printf("(body) Ignoring %s from line '%s'\n", > +$what, $_) unless $quiet; > + next; > } This check is new and applies to sob/cc, too. I am aware of the fact that people sometimes write only a name with no e-mail address when giving credit to a third-party and we want to avoid upsetting the underlying MTA by feeding it a non-address. Looking at existing helper subs like extract_valid_address and sanitize_address that all addresses we pass to the MTA go through, it appears to me that we try to support an addr-spec with only local-part without @domain, so this new check might turn out to be too strict from that point of view, but on the other hand I suspect it won't be a huge issue because the addresses in the footers are for public consumption and it may not make much sense to have a local-only address there. I dunno. > push @cc, $c; > printf("(body) Adding cc: %s from line '%s'\n",
Re: [PATCH] git-send-email: Add ability to cc: any "trailers" from commit message
Joe Perches writes: > Many commits have various forms of trailers similar to > "Acked-by: Name " and "Reported-by: Name " > > Add the ability to cc these trailers when using git send-email. I thought you were asking what we call these " followed by " at the end of the log message, and "footers or trailers" was the answer. I do not have a strong objection against limiting to "-by:" lines; for one thing, it would automatically avoid having to worry about "Bug-ID:" and other trailers that won't have e-mail address at all. But if you are _only_ picking up "-by:" lines, then calling this option "trailers" is way too wide and confusing. I do not think there is any specific name for "-by:" lines, though. Perhaps you would need to invent some name that has "-by" as a substring. "any-by"? or just "by"? I dunno. > if ($suppress_cc{'all'}) { > - foreach my $entry (qw (cccmd cc author self sob body bodycc)) { > + foreach my $entry (qw (cccmd cc author self sob body bodycc trailers)) { > $suppress_cc{$entry} = 1; > } OK. > @@ -448,7 +448,7 @@ $suppress_cc{'self'} = $suppress_from if defined > $suppress_from; > $suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc; > > if ($suppress_cc{'body'}) { > - foreach my $entry (qw (sob bodycc)) { > + foreach my $entry (qw (sob bodycc trailers)) { > $suppress_cc{$entry} = 1; > } > delete $suppress_cc{'body'}; OK. > @@ -1545,7 +1545,7 @@ foreach my $t (@files) { > # Now parse the message body > while(<$fh>) { > $message .= $_; > - if (/^(Signed-off-by|Cc): (.*)$/i) { > + if (/^(Signed-off-by|Cc|[^\s]+[_-]by): (.*)$/i) { Micronits: (1) do you really want to grab a run of any non-blanks? Don't you want to exclude at least a colon? (2) allowing an underscore looks a bit unusual. > @@ -1555,6 +1555,12 @@ foreach my $t (@files) { > } else { > next if $suppress_cc{'sob'} and $what =~ > /Signed-off-by/i; > next if $suppress_cc{'bodycc'} and $what =~ > /Cc/i; > + next if $suppress_cc{'trailers'} and $what !~ > /Signed-off-by/i && $what =~ /by$/i; > + } It is a bit unfortunate that S-o-b is a subset of any-by that forces you to do this. > + if ($c !~ /.+@.+/) { > + printf("(body) Ignoring %s from line '%s'\n", > +$what, $_) unless $quiet; > + next; > } This check is new and applies to sob/cc, too. I am aware of the fact that people sometimes write only a name with no e-mail address when giving credit to a third-party and we want to avoid upsetting the underlying MTA by feeding it a non-address. Looking at existing helper subs like extract_valid_address and sanitize_address that all addresses we pass to the MTA go through, it appears to me that we try to support an addr-spec with only local-part without @domain, so this new check might turn out to be too strict from that point of view, but on the other hand I suspect it won't be a huge issue because the addresses in the footers are for public consumption and it may not make much sense to have a local-only address there. I dunno. > push @cc, $c; > printf("(body) Adding cc: %s from line '%s'\n",
[PATCH] git-send-email: Add ability to cc: any "trailers" from commit message
Many commits have various forms of trailers similar to "Acked-by: Name " and "Reported-by: Name " Add the ability to cc these trailers when using git send-email. This can be suppressed with --suppress-cc=trailers. Signed-off-by: Joe Perches--- Documentation/git-send-email.txt | 10 ++ git-send-email.perl | 16 +++- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 642d0ef..999c842 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -278,9 +278,10 @@ Automating the value of `sendemail.identity`. --[no-]signed-off-by-cc:: - If this is set, add emails found in Signed-off-by: or Cc: lines to the - cc list. Default is the value of `sendemail.signedoffbycc` configuration - value; if that is unspecified, default to --signed-off-by-cc. + If this is set, add emails found in Signed-off-by: or Cc: or any other + trailer -by: lines to the cc list. Default is the value of + `sendemail.signedoffbycc` configuration value; if that is unspecified, + default to --signed-off-by-cc. --[no-]cc-cover:: If this is set, emails found in Cc: headers in the first patch of @@ -307,8 +308,9 @@ Automating patch body (commit message) except for self (use 'self' for that). - 'sob' will avoid including anyone mentioned in Signed-off-by lines except for self (use 'self' for that). +- 'trailers' will avoid including anyone mentioned in any "-by:" lines. - 'cccmd' will avoid running the --cc-cmd. -- 'body' is equivalent to 'sob' + 'bodycc' +- 'body' is equivalent to 'sob' + 'bodycc' + 'trailers' - 'all' will suppress all auto cc values. -- + diff --git a/git-send-email.perl b/git-send-email.perl index da81be4..255465a 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -84,7 +84,7 @@ git send-email --dump-aliases --identity* Use the sendemail. options. --to-cmd * Email To: via ` \$patch_path` --cc-cmd * Email Cc: via ` \$patch_path` ---suppress-cc * author, self, sob, cc, cccmd, body, bodycc, all. +--suppress-cc * author, self, sob, cc, cccmd, body, bodycc, trailers, all. --[no-]cc-cover* Email Cc: addresses in the cover letter. --[no-]to-cover* Email To: addresses in the cover letter. --[no-]signed-off-by-cc* Send to Signed-off-by: addresses. Default on. @@ -431,13 +431,13 @@ my(%suppress_cc); if (@suppress_cc) { foreach my $entry (@suppress_cc) { die "Unknown --suppress-cc field: '$entry'\n" - unless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/; + unless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc|trailers)$/; $suppress_cc{$entry} = 1; } } if ($suppress_cc{'all'}) { - foreach my $entry (qw (cccmd cc author self sob body bodycc)) { + foreach my $entry (qw (cccmd cc author self sob body bodycc trailers)) { $suppress_cc{$entry} = 1; } delete $suppress_cc{'all'}; @@ -448,7 +448,7 @@ $suppress_cc{'self'} = $suppress_from if defined $suppress_from; $suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc; if ($suppress_cc{'body'}) { - foreach my $entry (qw (sob bodycc)) { + foreach my $entry (qw (sob bodycc trailers)) { $suppress_cc{$entry} = 1; } delete $suppress_cc{'body'}; @@ -1545,7 +1545,7 @@ foreach my $t (@files) { # Now parse the message body while(<$fh>) { $message .= $_; - if (/^(Signed-off-by|Cc): (.*)$/i) { + if (/^(Signed-off-by|Cc|[^\s]+[_-]by): (.*)$/i) { chomp; my ($what, $c) = ($1, $2); chomp $c; @@ -1555,6 +1555,12 @@ foreach my $t (@files) { } else { next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i; next if $suppress_cc{'bodycc'} and $what =~ /Cc/i; + next if $suppress_cc{'trailers'} and $what !~ /Signed-off-by/i && $what =~ /by$/i; + } + if ($c !~ /.+@.+/) { + printf("(body) Ignoring %s from line '%s'\n", + $what, $_) unless $quiet; + next; } push @cc, $c; printf("(body) Adding cc: %s from line '%s'\n", -- 2.10.0.rc2.1.gb2aa91d
[PATCH] git-send-email: Add ability to cc: any "trailers" from commit message
Many commits have various forms of trailers similar to "Acked-by: Name " and "Reported-by: Name " Add the ability to cc these trailers when using git send-email. This can be suppressed with --suppress-cc=trailers. Signed-off-by: Joe Perches --- Documentation/git-send-email.txt | 10 ++ git-send-email.perl | 16 +++- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 642d0ef..999c842 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -278,9 +278,10 @@ Automating the value of `sendemail.identity`. --[no-]signed-off-by-cc:: - If this is set, add emails found in Signed-off-by: or Cc: lines to the - cc list. Default is the value of `sendemail.signedoffbycc` configuration - value; if that is unspecified, default to --signed-off-by-cc. + If this is set, add emails found in Signed-off-by: or Cc: or any other + trailer -by: lines to the cc list. Default is the value of + `sendemail.signedoffbycc` configuration value; if that is unspecified, + default to --signed-off-by-cc. --[no-]cc-cover:: If this is set, emails found in Cc: headers in the first patch of @@ -307,8 +308,9 @@ Automating patch body (commit message) except for self (use 'self' for that). - 'sob' will avoid including anyone mentioned in Signed-off-by lines except for self (use 'self' for that). +- 'trailers' will avoid including anyone mentioned in any "-by:" lines. - 'cccmd' will avoid running the --cc-cmd. -- 'body' is equivalent to 'sob' + 'bodycc' +- 'body' is equivalent to 'sob' + 'bodycc' + 'trailers' - 'all' will suppress all auto cc values. -- + diff --git a/git-send-email.perl b/git-send-email.perl index da81be4..255465a 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -84,7 +84,7 @@ git send-email --dump-aliases --identity* Use the sendemail. options. --to-cmd * Email To: via ` \$patch_path` --cc-cmd * Email Cc: via ` \$patch_path` ---suppress-cc * author, self, sob, cc, cccmd, body, bodycc, all. +--suppress-cc * author, self, sob, cc, cccmd, body, bodycc, trailers, all. --[no-]cc-cover* Email Cc: addresses in the cover letter. --[no-]to-cover* Email To: addresses in the cover letter. --[no-]signed-off-by-cc* Send to Signed-off-by: addresses. Default on. @@ -431,13 +431,13 @@ my(%suppress_cc); if (@suppress_cc) { foreach my $entry (@suppress_cc) { die "Unknown --suppress-cc field: '$entry'\n" - unless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/; + unless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc|trailers)$/; $suppress_cc{$entry} = 1; } } if ($suppress_cc{'all'}) { - foreach my $entry (qw (cccmd cc author self sob body bodycc)) { + foreach my $entry (qw (cccmd cc author self sob body bodycc trailers)) { $suppress_cc{$entry} = 1; } delete $suppress_cc{'all'}; @@ -448,7 +448,7 @@ $suppress_cc{'self'} = $suppress_from if defined $suppress_from; $suppress_cc{'sob'} = !$signed_off_by_cc if defined $signed_off_by_cc; if ($suppress_cc{'body'}) { - foreach my $entry (qw (sob bodycc)) { + foreach my $entry (qw (sob bodycc trailers)) { $suppress_cc{$entry} = 1; } delete $suppress_cc{'body'}; @@ -1545,7 +1545,7 @@ foreach my $t (@files) { # Now parse the message body while(<$fh>) { $message .= $_; - if (/^(Signed-off-by|Cc): (.*)$/i) { + if (/^(Signed-off-by|Cc|[^\s]+[_-]by): (.*)$/i) { chomp; my ($what, $c) = ($1, $2); chomp $c; @@ -1555,6 +1555,12 @@ foreach my $t (@files) { } else { next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i; next if $suppress_cc{'bodycc'} and $what =~ /Cc/i; + next if $suppress_cc{'trailers'} and $what !~ /Signed-off-by/i && $what =~ /by$/i; + } + if ($c !~ /.+@.+/) { + printf("(body) Ignoring %s from line '%s'\n", + $what, $_) unless $quiet; + next; } push @cc, $c; printf("(body) Adding cc: %s from line '%s'\n", -- 2.10.0.rc2.1.gb2aa91d