* [RFC-PATCH 0/2] send-email: new --quote-mail option @ 2016-05-23 19:30 Tom Russello 2016-05-23 19:30 ` [RFC-PATCH 1/2] send-email: new option to quote an email and reply to Tom Russello ` (3 more replies) 0 siblings, 4 replies; 96+ messages in thread From: Tom Russello @ 2016-05-23 19:30 UTC (permalink / raw) To: git; +Cc: samuel.groot, matthieu.moy, erwan.mathoniere, jordan.de-gea Hello, With the current send-email command, you can send a series of patches "in reply to" an email. This patch adds a new option to `git send-email`, `--quote-mail=<file>`, to quote an email in the cover letter in your series of patches. The "To", "Cc" and "Subject" fields will be filled appropriately and the message given quoted in the cover letter for the series of patches. In this first patch, the new option `--quote-mail=<file>` needs an email file and does not manage accents. There is still work in progress, including: 1. An option `--quote-mail-id=<message-id>` to download the message from any source, e.g. http://mid.gmane.org/<message-id>/raw. The server's address could be set in the repo's config file. 2. The proper documentation for `--quote-mail=<file>` and `--quote-mail-id=<message-id>` as soon as their definitive behavior is approved by the community. 3. The code to parse the email headers is currently duplicated several times, we should refactor it to help maintaining the code. 4. More tests on the features described above. git-send-email.perl | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- t/t9001-send-email.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 1 deletion(-) ^ permalink raw reply [flat|nested] 96+ messages in thread
* [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-23 19:30 [RFC-PATCH 0/2] send-email: new --quote-mail option Tom Russello @ 2016-05-23 19:30 ` Tom Russello 2016-05-23 19:55 ` Eric Wong 2016-05-23 20:00 ` Matthieu Moy 2016-05-23 19:30 ` [RFC-PATCH 2/2] t9001: adding --quote-mail option test Tom Russello ` (2 subsequent siblings) 3 siblings, 2 replies; 96+ messages in thread From: Tom Russello @ 2016-05-23 19:30 UTC (permalink / raw) To: git Cc: samuel.groot, matthieu.moy, erwan.mathoniere, jordan.de-gea, Tom Russello, Tom Russello From: Tom Russello <tom.russello@ensimag.grenoble-inp.fr> This new option takes an email message file, parses it, fills the "To", "Subject" and "Cc" fields appropriately and quote the message. This option involves the `--compose` mode to edit the cover letter quoting the given email. Signed-off-by: Tom Russello <tom.russello@grenoble-inp.org> Signed-off-by: Samuel Groot <samuel.groot@grenoble-inp.org> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr> --- diff --git a/git-send-email.perl b/git-send-email.perl index 6958785..784b8a6 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -55,6 +55,7 @@ git send-email --dump-aliases --[no-]bcc <str> * Email Bcc: --subject <str> * Email "Subject:" --in-reply-to <str> * Email "In-Reply-To:" + --quote-mail <str> * Email To, Cc and quote the message. --[no-]xmailer * Add "X-Mailer:" header (default). --[no-]annotate * Review each patch that will be sent in an editor. --compose * Open an editor for introduction. @@ -160,7 +161,7 @@ my $re_encoded_word = qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/; # Variables we fill in automatically, or via prompting: my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh, - $initial_reply_to,$initial_subject,@files, + $initial_reply_to,$quote_mail,$initial_subject,@files, $author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time); my $envelope_sender; @@ -304,6 +305,7 @@ $rc = GetOptions( "sender|from=s" => \$sender, "in-reply-to=s" => \$initial_reply_to, "subject=s" => \$initial_subject, + "quote-mail=s" => \$quote_mail, "to=s" => \@initial_to, "to-cmd=s" => \$to_cmd, "no-to" => \$no_to, @@ -638,6 +640,98 @@ if (@files) { print STDERR "\nNo patch files specified!\n\n"; usage(); } +my $message_quoted; +if ($quote_mail) { + $message_quoted = ""; + my $error = validate_patch($quote_mail); + $error and die "fatal: $quote_mail: $error\nwarning: no patches were sent\n"; + $compose = 1; + my @header = (); + open my $fh, "<", $quote_mail or die "can't open file $quote_mail"; + while(<$fh>) { + #for files containing crlf line endings + $_=~ s/\r//g; + last if /^\s*$/; + if (/^\s+\S/ and @header) { + chomp($header[$#header]); + s/^\s+/ /; + $header[$#header] .= $_; + } else { + push(@header, $_); + } + } + + foreach(@header) { + my $input_format; + my $initial_sender = $sender || $repoauthor || $repocommitter || ''; + if (/^From /) { + $input_format = 'mbox'; + next; + } + chomp; + if (!defined $input_format && /^[-A-Za-z]+:\s/) { + $input_format = 'mbox'; + } + + if (defined $input_format && $input_format eq 'mbox') { + if (/^Subject:\s+(.*)$/i) { + my $prefix_re = ""; + my $subject_re = $1; + if ($1=~/^[^Re:]/) { + $prefix_re = "Re: "; + } + $initial_subject = $prefix_re.$subject_re; + } + elsif (/^From:\s+(.*)$/i) { + push @initial_to, $1; + } + elsif (/^To:\s+(.*)$/i) { + foreach my $addr (parse_address_line($1)) { + if (!($addr eq $initial_sender)) { + push @initial_to, $addr; + } + } + } elsif (/^Cc:\s+(.*)$/i) { + foreach my $addr (parse_address_line($1)) { + my $qaddr = unquote_rfc2047($addr); + my $saddr = sanitize_address($qaddr); + if ($saddr eq $initial_sender) { + next if ($suppress_cc{'self'}); + } else { + next if ($suppress_cc{'cc'}); + } + push @initial_cc, $addr; + } + } elsif (/^Message-Id: (.*)/i) { + $initial_reply_to = $1; + } + } else { + # In the traditional + # "send lots of email" format, + # line 1 = cc + # line 2 = subject + # So let's support that, too. + $input_format = 'lots'; + if (@cc == 0 && !$suppress_cc{'cc'}) { + push @cc, $_; + } elsif (!defined $initial_subject) { + $initial_subject = $_; + } + } + } + + #Message body + while (<$fh>) { + #for files containing crlf line endings + $_=~ s/\r//g; + my $space=""; + if (/^[^>]/) { + $space = " "; + } + $message_quoted .= ">".$space.$_; + } + +} sub get_patch_subject { my $fn = shift; @@ -664,6 +758,7 @@ if ($compose) { my $tpl_sender = $sender || $repoauthor || $repocommitter || ''; my $tpl_subject = $initial_subject || ''; my $tpl_reply_to = $initial_reply_to || ''; + my $tpl_quote = $message_quoted || ''; print $c <<EOT; From $tpl_sender # This line is ignored. @@ -676,6 +771,8 @@ From: $tpl_sender Subject: $tpl_subject In-Reply-To: $tpl_reply_to +$tpl_quote + EOT for my $f (@files) { print $c get_patch_subject($f); -- 2.8.2 ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-23 19:30 ` [RFC-PATCH 1/2] send-email: new option to quote an email and reply to Tom Russello @ 2016-05-23 19:55 ` Eric Wong 2016-05-23 20:07 ` Matthieu Moy 2016-05-24 12:43 ` Samuel GROOT 2016-05-23 20:00 ` Matthieu Moy 1 sibling, 2 replies; 96+ messages in thread From: Eric Wong @ 2016-05-23 19:55 UTC (permalink / raw) To: Tom Russello Cc: git, samuel.groot, matthieu.moy, erwan.mathoniere, jordan.de-gea, Tom Russello Tom Russello <tom.russello@grenoble-inp.org> wrote: > This new option takes an email message file, parses it, fills the "To", > "Subject" and "Cc" fields appropriately and quote the message. > This option involves the `--compose` mode to edit the cover letter quoting the > given email. Cool! There should probably be some help text to encourage trimming down the quoted text to only relevant portions. > + #Message body > + while (<$fh>) { > + #for files containing crlf line endings > + $_=~ s/\r//g; > + my $space=""; > + if (/^[^>]/) { > + $space = " "; > + } > + $message_quoted .= ">".$space.$_; Is this really necessary to switch between "> " and ">" prefix? AFAIK, MUAs prefix unconditionally with "> ". At least that's how mutt does it... ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-23 19:55 ` Eric Wong @ 2016-05-23 20:07 ` Matthieu Moy 2016-05-23 22:10 ` Samuel GROOT 2016-05-24 12:43 ` Samuel GROOT 1 sibling, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-05-23 20:07 UTC (permalink / raw) To: Eric Wong Cc: Tom Russello, git, samuel.groot, erwan.mathoniere, jordan.de-gea, Tom Russello Eric Wong <e@80x24.org> writes: > Tom Russello <tom.russello@grenoble-inp.org> wrote: > >> + #Message body >> + while (<$fh>) { >> + #for files containing crlf line endings >> + $_=~ s/\r//g; >> + my $space=""; >> + if (/^[^>]/) { >> + $space = " "; >> + } >> + $message_quoted .= ">".$space.$_; > > Is this really necessary to switch between "> " and ">" prefix? > AFAIK, MUAs prefix unconditionally with "> ". I had the same question, but at least my mailer (Gnus) has the same special-case it seems. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-23 20:07 ` Matthieu Moy @ 2016-05-23 22:10 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-05-23 22:10 UTC (permalink / raw) To: Matthieu Moy, Eric Wong Cc: Tom Russello, git, erwan.mathoniere, jordan.de-gea, Tom Russello On 05/23/2016 10:07 PM, Matthieu Moy wrote: > Eric Wong <e@80x24.org> writes: > >> Tom Russello <tom.russello@grenoble-inp.org> wrote: >> >>> + #Message body >>> + while (<$fh>) { >>> + #for files containing crlf line endings >>> + $_=~ s/\r//g; >>> + my $space=""; >>> + if (/^[^>]/) { >>> + $space = " "; >>> + } >>> + $message_quoted .= ">".$space.$_; >> >> Is this really necessary to switch between "> " and ">" prefix? >> AFAIK, MUAs prefix unconditionally with "> ". > > I had the same question, but at least my mailer (Gnus) has the same > special-case it seems. Thunderbird behaves the same way, so we decided to mimic that behavior. It is specified neither in RFC 2822 [1] nor in RFC 5322 [2]. When we write an email, we write it with a maximum width of 72 columns. If we insert "> " with each reply, the 80-columns limit will be reached with only 4 replies. So IMHO we should trim the extra space to allow up to 7 replies before reaching the 80-columns limit. [1] https://www.ietf.org/rfc/rfc2822.txt [2] https://www.ietf.org/rfc/rfc5322.txt ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-23 19:55 ` Eric Wong 2016-05-23 20:07 ` Matthieu Moy @ 2016-05-24 12:43 ` Samuel GROOT 2016-05-24 12:49 ` Matthieu Moy 2016-05-24 21:23 ` Eric Wong 1 sibling, 2 replies; 96+ messages in thread From: Samuel GROOT @ 2016-05-24 12:43 UTC (permalink / raw) To: Eric Wong, Tom Russello Cc: git, matthieu.moy, erwan.mathoniere, Jordan DE GEA On 05/23/2016 09:55 PM, Eric Wong wrote: > Tom Russello <tom.russello@grenoble-inp.org> wrote: >> This new option takes an email message file, parses it, fills the "To", >> "Subject" and "Cc" fields appropriately and quote the message. >> This option involves the `--compose` mode to edit the cover letter quoting the >> given email. > > Cool! There should probably be some help text to encourage > trimming down the quoted text to only relevant portions. What kind of help text would you want to see? Maybe something like this: GIT: Quoted message body below. GIT: Feel free to trim down the quoted text GIT: to only relevant portions. As "GIT:" portions are ignored when parsed by `git send-email`. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-24 12:43 ` Samuel GROOT @ 2016-05-24 12:49 ` Matthieu Moy 2016-05-24 22:30 ` Aaron Schrab 2016-05-24 21:23 ` Eric Wong 1 sibling, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-05-24 12:49 UTC (permalink / raw) To: Samuel GROOT Cc: Eric Wong, Tom Russello, git, erwan.mathoniere, Jordan DE GEA Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > On 05/23/2016 09:55 PM, Eric Wong wrote: >> Tom Russello <tom.russello@grenoble-inp.org> wrote: >>> This new option takes an email message file, parses it, fills the "To", >>> "Subject" and "Cc" fields appropriately and quote the message. >>> This option involves the `--compose` mode to edit the cover letter quoting the >>> given email. >> >> Cool! There should probably be some help text to encourage >> trimming down the quoted text to only relevant portions. > > What kind of help text would you want to see? > > Maybe something like this: > > GIT: Quoted message body below. > GIT: Feel free to trim down the quoted text > GIT: to only relevant portions. > > As "GIT:" portions are ignored when parsed by `git send-email`. That's an option, but in the context of email, I think these instructions are not necessary. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-24 12:49 ` Matthieu Moy @ 2016-05-24 22:30 ` Aaron Schrab 2016-05-25 0:04 ` Tom Russello 0 siblings, 1 reply; 96+ messages in thread From: Aaron Schrab @ 2016-05-24 22:30 UTC (permalink / raw) To: Matthieu Moy Cc: Samuel GROOT, Eric Wong, Tom Russello, git, erwan.mathoniere, Jordan DE GEA At 14:49 +0200 24 May 2016, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote: >Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > >> What kind of help text would you want to see? >> >> Maybe something like this: >> >> GIT: Quoted message body below. >> GIT: Feel free to trim down the quoted text >> GIT: to only relevant portions. >> >> As "GIT:" portions are ignored when parsed by `git send-email`. > >That's an option, but in the context of email, I think these >instructions are not necessary. In an ideal world that would be true. But in the real world I think evidence of many messages to this mailing list containing full quotes suggests it might be helpful. I'd actually argue that the message be more forceful, making it a suggestion/request to trim rather than simply telling the user that it's allowed. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-24 22:30 ` Aaron Schrab @ 2016-05-25 0:04 ` Tom Russello 0 siblings, 0 replies; 96+ messages in thread From: Tom Russello @ 2016-05-25 0:04 UTC (permalink / raw) To: Eric Wong, git, erwan.mathoniere, Jordan DE GEA Cc: Matthieu Moy, Samuel GROOT On 05/25/16 00:30, Aaron Schrab wrote: > At 14:49 +0200 24 May 2016, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> > wrote: >> Samuel GROOT <samuel.groot@grenoble-inp.org> writes: >> >>> What kind of help text would you want to see? >>> >>> Maybe something like this: >>> >>> GIT: Quoted message body below. >>> GIT: Feel free to trim down the quoted text >>> GIT: to only relevant portions. >>> >>> As "GIT:" portions are ignored when parsed by `git send-email`. >> >> That's an option, but in the context of email, I think these >> instructions are not necessary. > > In an ideal world that would be true. But in the real world I think > evidence of many messages to this mailing list containing full quotes > suggests it might be helpful. I'd actually argue that the message be > more forceful, making it a suggestion/request to trim rather than simply > telling the user that it's allowed. Furthermore, it is a good way to avoid very long messages due to unnecessary parts quoted. Therefore, we thought about a request like "Please, trim down irrelevant sections in the quoted message to keep your email concise" ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-24 12:43 ` Samuel GROOT 2016-05-24 12:49 ` Matthieu Moy @ 2016-05-24 21:23 ` Eric Wong 1 sibling, 0 replies; 96+ messages in thread From: Eric Wong @ 2016-05-24 21:23 UTC (permalink / raw) To: Samuel GROOT Cc: Tom Russello, git, matthieu.moy, erwan.mathoniere, Jordan DE GEA Samuel GROOT <samuel.groot@grenoble-inp.org> wrote: > On 05/23/2016 09:55 PM, Eric Wong wrote: > >Cool! There should probably be some help text to encourage > >trimming down the quoted text to only relevant portions. > > What kind of help text would you want to see? > > Maybe something like this: > > GIT: Quoted message body below. > GIT: Feel free to trim down the quoted text > GIT: to only relevant portions. > > As "GIT:" portions are ignored when parsed by `git send-email`. Yes, given we have instructions for diffstat and table of contents; I think it'd be useful to discourage quoting irrelevant parts of the message (especially signatures and like). ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-23 19:30 ` [RFC-PATCH 1/2] send-email: new option to quote an email and reply to Tom Russello 2016-05-23 19:55 ` Eric Wong @ 2016-05-23 20:00 ` Matthieu Moy 2016-05-24 23:31 ` Samuel GROOT 1 sibling, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-05-23 20:00 UTC (permalink / raw) To: Tom Russello Cc: git, samuel.groot, erwan.mathoniere, jordan.de-gea, Tom Russello Tom Russello <tom.russello@grenoble-inp.org> writes: > This option involves the `--compose` mode to edit the cover letter quoting the s/involves/implies/ ? I don't think this is right: I often reply to an email with a single patch, for which it would clearly be overkill to have a cover-letter. Your --quote-mail does two things: 1) Populate the To and Cc field 2) Include the original message body with quotation prefix. When not using --compose, 1) clearly makes sense already, and there's no reason to prevent this use-case. When sending a single patch, 2) also makes sense as "below-tripple-dash comment", like This is the commit message for feature A. --- John Smith wrote: > You should implement feature A. Indeed, here's a patch. modified-file.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- When sending multiple patches without --compose, 2) may not make sense, but I think a sane behavior would be: * If --compose is given, cite the message there. * If --compose is not given, don't send a cover-letter but cite the body as comment in the first patch. As a first step, the second point can be changed to "if --compose is not given, don't cite the message, just populate the To: and Cc: fields". > --- > > diff --git a/git-send-email.perl b/git-send-email.perl No diffstat? > @@ -638,6 +640,98 @@ if (@files) { > print STDERR "\nNo patch files specified!\n\n"; > usage(); > } > +my $message_quoted; > +if ($quote_mail) { Style: The code you're adding doesn't look related to the code right before => separate them with a blank line. > + while(<$fh>) { Style: space before (. > + push(@header, $_); I think the code would be clearer if @header was a list of pairs (header-name, header-content). Then you'd need much less regex magic when going through it. > + #for files containing crlf line endings Sytle: space after #. > + foreach(@header) { Space before (. > + elsif (/^From:\s+(.*)$/i) { > + push @initial_to, $1; > + } > + elsif (/^To:\s+(.*)$/i) { > + foreach my $addr (parse_address_line($1)) { > + if (!($addr eq $initial_sender)) { > + push @initial_to, $addr; > + } > + } This adds the content of the To: field in the original email to the Cc: field in the new message, right? If so, this is a weird behavior: when following up to an email, one usually addresses to the person s/he's replying, keeping the others Cc-ed, hence the original From: becomes the To header, and the original To: and Cc: become Cc:. > + } elsif (/^Cc:\s+(.*)$/i) { Style: IIRC, there's no consensus on whether "elsif" should be on the same line as the closing }, but please follow the same convention inside a single if/elsif/ chain. > + #Message body Style: space after # (more below). And while you're there, the comment could be "Quote the message body" or so, to give a hint to the user about what's going on. > + while (<$fh>) { > + #for files containing crlf line endings > + $_=~ s/\r//g; > + my $space=""; Style: spaces around =. > @@ -676,6 +771,8 @@ From: $tpl_sender > Subject: $tpl_subject > In-Reply-To: $tpl_reply_to > > +$tpl_quote > + > EOT Doesn't this add two extra useless blank lines if $tpl_quote is empty? -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-23 20:00 ` Matthieu Moy @ 2016-05-24 23:31 ` Samuel GROOT 2016-05-25 6:29 ` Matthieu Moy 0 siblings, 1 reply; 96+ messages in thread From: Samuel GROOT @ 2016-05-24 23:31 UTC (permalink / raw) To: Matthieu Moy, Tom Russello Cc: git, erwan.mathoniere, jordan.de-gea, Tom Russello On 05/23/2016 10:00 PM, Matthieu Moy wrote: > I don't think this is right: I often reply to an email with a single > patch, for which it would clearly be overkill to have a cover-letter. Yes indeed, we are working on inserting the quoted message body in the patch's "below-triple-dash" section. > Your --quote-mail does two things: > > 1) Populate the To and Cc field > > 2) Include the original message body with quotation prefix. > > When not using --compose, 1) clearly makes sense already, and there's no > reason to prevent this use-case. When sending a single patch, 2) also > makes sense as "below-tripple-dash comment", like > > This is the commit message for feature A. > --- > John Smith wrote: > > You should implement feature A. > > Indeed, here's a patch. > > modified-file.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > > When sending multiple patches without --compose, 2) may not make sense, > but I think a sane behavior would be: > > * If --compose is given, cite the message there. > > * If --compose is not given, don't send a cover-letter but cite the body > as comment in the first patch. > > As a first step, the second point can be changed to "if --compose is not > given, don't cite the message, just populate the To: and Cc: fields". It seems a good behavior to me too. > * If --compose is not given, don't send a cover-letter but cite the body > as comment in the first patch. Then should the option imply `--annotate`, to let the user edit the patch containing the quoted email? >> + push(@header, $_); > > I think the code would be clearer if @header was a list of pairs > (header-name, header-content). Then you'd need much less regex magic > when going through it. The next series of patches may include (if the code is clean enough by then) a cleaner subroutine to parse the email, probably returning something like: return ( "from" => $from, "subject" => $subject, "date" => $date, "message_id" => $message_id, "to" => [@to], "cc" => [@cc], "xh" => [@xh], "config" => {%config} ); >> + elsif (/^From:\s+(.*)$/i) { >> + push @initial_to, $1; >> + } >> + elsif (/^To:\s+(.*)$/i) { >> + foreach my $addr (parse_address_line($1)) { >> + if (!($addr eq $initial_sender)) { >> + push @initial_to, $addr; >> + } >> + } > > This adds the content of the To: field in the original email to the Cc: > field in the new message, right? If so, this is a weird behavior: when > following up to an email, one usually addresses to the person s/he's > replying, keeping the others Cc-ed, hence the original From: becomes the > To header, and the original To: and Cc: become Cc:. We made the option behave like Thunderbird does, but indeed RFC 2822 [1] sees it the same you do, it will be fixed in next iteration. >> @@ -676,6 +771,8 @@ From: $tpl_sender >> Subject: $tpl_subject >> In-Reply-To: $tpl_reply_to >> >> +$tpl_quote >> + >> EOT > > Doesn't this add two extra useless blank lines if $tpl_quote is empty? Yes it does, it will be fixed in the next series of patches. Thank you for your time! [1] https://www.ietf.org/rfc/rfc2822.txt ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-24 23:31 ` Samuel GROOT @ 2016-05-25 6:29 ` Matthieu Moy 2016-05-25 15:40 ` Junio C Hamano 0 siblings, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-05-25 6:29 UTC (permalink / raw) To: Samuel GROOT Cc: Tom Russello, git, erwan.mathoniere, jordan.de-gea, Tom Russello Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > On 05/23/2016 10:00 PM, Matthieu Moy wrote: > >> Your --quote-mail does two things: >> >> 1) Populate the To and Cc field >> >> 2) Include the original message body with quotation prefix. >> [...] >> * If --compose is not given, don't send a cover-letter but cite the body >> as comment in the first patch. > > Then should the option imply `--annotate`, to let the user edit the > patch containing the quoted email? Actually, I'm not sure what the ideal behavior should be. Perhaps it's better to distinguish 1) and 2) above, and have two options --reply-to-email=<file> doing 1), and --quote doing 2), implying --compose and requiring --reply-to-email. That would be more flexible, but that would require two new options, and I also like to keep things simple. In any case, quoting the message without replying to it does not make sense (especially if you add instructions to trim it: the user would not even see it). So it its current form, I'd say --quote-email should imply --annotate. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-25 6:29 ` Matthieu Moy @ 2016-05-25 15:40 ` Junio C Hamano 2016-05-25 16:56 ` Matthieu Moy 0 siblings, 1 reply; 96+ messages in thread From: Junio C Hamano @ 2016-05-25 15:40 UTC (permalink / raw) To: Matthieu Moy Cc: Samuel GROOT, Tom Russello, git, erwan.mathoniere, jordan.de-gea, Tom Russello Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: > Actually, I'm not sure what the ideal behavior should be. Perhaps it's > better to distinguish 1) and 2) above, and have two options > --reply-to-email=<file> doing 1), and --quote doing 2), implying > --compose and requiring --reply-to-email. I tend to agree that sounds like a better way to structure these features. I wonder if we can safely repurpose existing --in-reply-to option? That is, if the value of --in-reply-to can be reliably determined as a filename that has the message (as opposed to a message-id), we read the "Message-Id:" from that file to figuire out what message-id to use, and figure out To/Cc: to use for the purpose of your (1) at the same time. In the future, you might even teach send-email, perhaps via a user configurable hook, a way to get to the message header and text given a message-id, and when it happens, the same logic can be used when --in-reply-to is given a message-id (i.e. you go from the id to the message and find the addresses you would To/Cc: your message). > In any case, quoting the message without replying to it does not make > sense (especially if you add instructions to trim it: the user would not > even see it). So it its current form, I'd say --quote-email should imply > --annotate. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-25 15:40 ` Junio C Hamano @ 2016-05-25 16:56 ` Matthieu Moy 2016-05-25 18:15 ` Junio C Hamano 0 siblings, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-05-25 16:56 UTC (permalink / raw) To: Junio C Hamano Cc: Samuel GROOT, Tom Russello, git, erwan.mathoniere, jordan.de-gea, Tom Russello Junio C Hamano <gitster@pobox.com> writes: > I wonder if we can safely repurpose existing --in-reply-to option? In theory, obviously no as there can be a file with this name _and_ it can be a valid message-id. In practice, it is clearly unlikely. The only use-case I can think of where both would be valid is if the user happens to have saved the message using the message-id as filename. But then, the ambiguity would not harm, as the message-id contained in the file would be the same as the filename. > That is, if the value of --in-reply-to can be reliably determined as > a filename that has the message (as opposed to a message-id), we > read the "Message-Id:" from that file to figuire out what message-id > to use, and figure out To/Cc: to use for the purpose of your (1) at > the same time. This should work, but sounds like too much of overloading of --in-reply-to IMHO: if given a message id, it would only add a reference to this message-id, but if given a file, it would also modify the To: and Cc: list. Not a strong objection, though. > In the future, you might even teach send-email, perhaps via a user > configurable hook, a way to get to the message header and text given a > message-id, and when it happens, the same logic can be used when > --in-reply-to is given a message-id (i.e. you go from the id to the > message and find the addresses you would To/Cc: your message). That is the plan indeed. Fetching from gmane for example should be rather easy in perl, and would be really convenient! -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-25 16:56 ` Matthieu Moy @ 2016-05-25 18:15 ` Junio C Hamano 2016-05-25 18:31 ` Matthieu Moy 0 siblings, 1 reply; 96+ messages in thread From: Junio C Hamano @ 2016-05-25 18:15 UTC (permalink / raw) To: Matthieu Moy Cc: Samuel GROOT, Tom Russello, git, erwan.mathoniere, jordan.de-gea, Tom Russello Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: > This should work, but sounds like too much of overloading of > --in-reply-to IMHO: if given a message id, it would only add a reference > to this message-id, but if given a file, it would also modify the To: > and Cc: list. > > Not a strong objection, though. Well, with your "that is the plan indeed", the option would behave the same whether given a message ID or a filename, no? But I do agree that those who have accustomed to the behaviour of --in-reply-to that does not mess with To/Cc:, such a behaviour change is not desirable. If we are adding a new --reply-to-email=<file|id>, it should behave as a superset of --in-reply-to (i.e. it should set In-Reply-to: using the message ID of the e-mail we are replying to), though. >> In the future, you might even teach send-email, perhaps via a user >> configurable hook, a way to get to the message header and text given a >> message-id, and when it happens, the same logic can be used when >> --in-reply-to is given a message-id (i.e. you go from the id to the >> message and find the addresses you would To/Cc: your message). > > That is the plan indeed. Fetching from gmane for example should be > rather easy in perl, and would be really convenient! ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-25 18:15 ` Junio C Hamano @ 2016-05-25 18:31 ` Matthieu Moy 2016-05-26 0:08 ` Samuel GROOT 0 siblings, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-05-25 18:31 UTC (permalink / raw) To: Junio C Hamano Cc: Samuel GROOT, Tom Russello, git, erwan.mathoniere, jordan.de-gea, Tom Russello Junio C Hamano <gitster@pobox.com> writes: > Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: > >> This should work, but sounds like too much of overloading of >> --in-reply-to IMHO: if given a message id, it would only add a reference >> to this message-id, but if given a file, it would also modify the To: >> and Cc: list. >> >> Not a strong objection, though. > > Well, with your "that is the plan indeed", the option would behave > the same whether given a message ID or a filename, no? The "fetch message from ID" feature should not be unconditional IMHO. So it would probably be stg like: git send-email --in-reply-to=<id> --fetch What's a bit counter-intuitive is that --fetch would not only trigger fetching the complete message, but also populate To/Cc. But thinking about it, it's not _that_ counter-intuitive, as fetching the message should be done for a reason, so the user can guess that the message is going to be used for something. So, a possible UI would be: git send-email --in-reply-to=<id> => just set In-Reply-To: field. git send-email --in-reply-to=<file> => set In-Reply-To, To and Cc. git send-email --in-reply-to=<file> --cite => in addition, add the body of the message quoted with '> '. git send-email --in-reply-to=<id> --fetch => fetch and do like <file> using the default configuration for fetch. This leaves room for: git send-email --in-reply-to=<id> --fetch=gmane => fetch from gmane (details on how to fetch would be in the config file) This UI wouldn't allow using a file to get only the message-id. But I'm not sure this is an interesting use-case. So, I guess you convinced me. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-25 18:31 ` Matthieu Moy @ 2016-05-26 0:08 ` Samuel GROOT 2016-05-27 9:06 ` Matthieu Moy 0 siblings, 1 reply; 96+ messages in thread From: Samuel GROOT @ 2016-05-26 0:08 UTC (permalink / raw) To: Matthieu Moy, Junio C Hamano Cc: Tom Russello, git, erwan.mathoniere, jordan.de-gea, Tom Russello On 05/25/2016 08:31 PM, Matthieu Moy wrote: > So, a possible UI would be: > > git send-email --in-reply-to=<id> => just set In-Reply-To: field. > > git send-email --in-reply-to=<file> => set In-Reply-To, To and Cc. > > git send-email --in-reply-to=<file> --cite => in addition, add the > body of the message quoted with '> '. > > git send-email --in-reply-to=<id> --fetch => fetch and do like <file> > using the default configuration for fetch. We designed a similar UI, except for the --fetch option: We wanted to try to fetch the email from a distant server (e.g. gmane) if that server address was set in the config file, and populate the To:/Cc: fields. If the file cannot be downloaded, or server address not set, just fill the Reply-to header. Either way, display what was done with the message-id given (unless --quiet is set, of course). > This leaves room for: > > git send-email --in-reply-to=<id> --fetch=gmane => fetch from gmane > (details on how to fetch would be in the config file) > > This UI wouldn't allow using a file to get only the message-id. But I'm > not sure this is an interesting use-case. IMHO when you reply to a thread with a patch, it seems counter-productive to reply without notifying (putting in To:/Cc:) the original author and people involved in the thread. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to 2016-05-26 0:08 ` Samuel GROOT @ 2016-05-27 9:06 ` Matthieu Moy 0 siblings, 0 replies; 96+ messages in thread From: Matthieu Moy @ 2016-05-27 9:06 UTC (permalink / raw) To: Samuel GROOT Cc: Junio C Hamano, Tom Russello, git, erwan.mathoniere, jordan.de-gea, Tom Russello Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > On 05/25/2016 08:31 PM, Matthieu Moy wrote: >> So, a possible UI would be: >> >> git send-email --in-reply-to=<id> => just set In-Reply-To: field. >> >> git send-email --in-reply-to=<file> => set In-Reply-To, To and Cc. >> >> git send-email --in-reply-to=<file> --cite => in addition, add the >> body of the message quoted with '> '. >> >> git send-email --in-reply-to=<id> --fetch => fetch and do like <file> >> using the default configuration for fetch. > > We designed a similar UI, except for the --fetch option: > > We wanted to try to fetch the email from a distant server (e.g. gmane) > if that server address was set in the config file, and populate the > To:/Cc: fields. > > If the file cannot be downloaded, or server address not set, just fill > the Reply-to header. I'm not sure this is right. I'd typically configure gmane in my user-wide config file, so I'd have it configured all the time, but I may not always want to fetch from it (e.g. replying to a message which is not on a mailing-list, or if gmane is down, or ...). Fetching by default would clearly work in most cases, but for the few remaning cases it may be painful for the user if the only way to disable fetching is to remove the configuration from the config file. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* [RFC-PATCH 2/2] t9001: adding --quote-mail option test 2016-05-23 19:30 [RFC-PATCH 0/2] send-email: new --quote-mail option Tom Russello 2016-05-23 19:30 ` [RFC-PATCH 1/2] send-email: new option to quote an email and reply to Tom Russello @ 2016-05-23 19:30 ` Tom Russello 2016-05-23 20:05 ` Matthieu Moy 2016-05-23 19:38 ` [RFC-PATCH 0/2] send-email: new --quote-mail option Matthieu Moy 2016-05-27 17:11 ` [RFC-PATCH v2 0/2] send-email: new --quote-email option Tom Russello 3 siblings, 1 reply; 96+ messages in thread From: Tom Russello @ 2016-05-23 19:30 UTC (permalink / raw) To: git Cc: samuel.groot, matthieu.moy, erwan.mathoniere, jordan.de-gea, Tom Russello, Tom Russello From: Tom Russello <tom.russello@ensimag.grenoble-inp.fr> Tests if the "To", "Cc" and "Subject" fields are adequately filled and if the message is correctly quoted. Signed-off-by: Tom Russello <tom.russello@grenoble-inp.org> Signed-off-by: Samuel Groot <samuel.groot@grenoble-inp.org> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr> --- diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index b3355d2..bda4018 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -1885,4 +1885,47 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' ' test_cmp expected-list actual-list ' +test_expect_success $PREREQ 'setup expect' ' + cat >email <<-\EOF + Message-Id: <author_123456@example.com> + From: author@example.com + To: to1@example.com + Cc: cc1@example.com + Date: Sat, 12 Jun 2010 15:53:58 +0200 + Subject: subject goes here + + Have you seen my previous email? + > Previous content + EOF +' + +test_expect_success $PREREQ 'From, To, Cc, Subject with --quote-mail are correct' ' + clean_fake_sendmail && + git send-email \ + --quote-mail=email \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -1 \ + 2>errors && + grep "From: Example <nobody@example.com>" msgtxt1 && + to_adr=$(awk "/^To: /,/^Cc: /" msgtxt1) && + echo "$to_adr" | grep author@example.com && + echo "$to_adr" | grep to1@example.com && + grep "Cc: cc1@example.com" msgtxt1 +' +test_expect_success $PREREQ 'the message given is quoted with --quote-mail' ' + grep "> Have you seen my previous email?" msgtxt1 && + grep ">> Previous content" msgtxt1 +' +test_expect_success $PREREQ 'Check if Re is written, only once with --quote-mail' ' + grep "Subject: Re: subject goes here" msgtxt1 && + git send-email \ + --quote-mail=msgtxt1 \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -1 \ + 2>errors && + grep "Subject: Re: subject goes here" msgtxt3 +' + test_done -- 2.8.2 ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 2/2] t9001: adding --quote-mail option test 2016-05-23 19:30 ` [RFC-PATCH 2/2] t9001: adding --quote-mail option test Tom Russello @ 2016-05-23 20:05 ` Matthieu Moy 0 siblings, 0 replies; 96+ messages in thread From: Matthieu Moy @ 2016-05-23 20:05 UTC (permalink / raw) To: Tom Russello Cc: git, samuel.groot, erwan.mathoniere, jordan.de-gea, Tom Russello > Subject: [RFC-PATCH 2/2] t9001: adding --quote-mail option test We write messages at imperative tone, hence s/adding/add/ Tom Russello <tom.russello@grenoble-inp.org> writes: > From: Tom Russello <tom.russello@ensimag.grenoble-inp.fr> Please use the same identity for email and commit to avoid this line. > --- > > > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh No diffstat again? Splitting a patch series as "code first; tests after" is not a good idea IMHO. When questionning the behavior of To: Vs Cc: in the previous patch, I would have appreciated having tests in the same message, to check that the tested behavior was indeed the one I was reading in the code. OTOH, having one patch to introduce "--quote-email populates To: and Cc: headers", and then another one for "--quote-email quotes the message body" would make the review much easier. Oh, BTW, this obviously lacks documentation (Documentation/git-send-email.txt). And that's one reason why the diffstat is useful: one can reply "this lacks tests and doc" before even reviewing the patch. Regards, -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 0/2] send-email: new --quote-mail option 2016-05-23 19:30 [RFC-PATCH 0/2] send-email: new --quote-mail option Tom Russello 2016-05-23 19:30 ` [RFC-PATCH 1/2] send-email: new option to quote an email and reply to Tom Russello 2016-05-23 19:30 ` [RFC-PATCH 2/2] t9001: adding --quote-mail option test Tom Russello @ 2016-05-23 19:38 ` Matthieu Moy 2016-05-23 19:56 ` Samuel GROOT 2016-05-27 17:11 ` [RFC-PATCH v2 0/2] send-email: new --quote-email option Tom Russello 3 siblings, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-05-23 19:38 UTC (permalink / raw) To: Tom Russello; +Cc: git, samuel.groot, erwan.mathoniere, jordan.de-gea Tom Russello <tom.russello@grenoble-inp.org> writes: > Hello, > > With the current send-email command, you can send a series of patches "in reply > to" an email. > This patch adds a new option to `git send-email`, `--quote-mail=<file>`, to I think the option name should be --quote-email. Even though "mail" usually means "email" for French people, there's still non-electronic mail for english-speaking ones. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH 0/2] send-email: new --quote-mail option 2016-05-23 19:38 ` [RFC-PATCH 0/2] send-email: new --quote-mail option Matthieu Moy @ 2016-05-23 19:56 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-05-23 19:56 UTC (permalink / raw) To: Matthieu Moy, Tom Russello; +Cc: git, erwan.mathoniere, jordan.de-gea On 05/23/2016 09:38 PM, Matthieu Moy wrote: > Tom Russello <tom.russello@grenoble-inp.org> writes: > >> Hello, >> >> With the current send-email command, you can send a series of patches "in reply >> to" an email. >> This patch adds a new option to `git send-email`, `--quote-mail=<file>`, to > > I think the option name should be --quote-email. Even though "mail" > usually means "email" for French people, there's still non-electronic > mail for english-speaking ones. That makes sense, all occurrences of "mail" will be changed into "email" for v2. Thanks for your feedback ! ^ permalink raw reply [flat|nested] 96+ messages in thread
* [RFC-PATCH v2 0/2] send-email: new --quote-email option 2016-05-23 19:30 [RFC-PATCH 0/2] send-email: new --quote-mail option Tom Russello ` (2 preceding siblings ...) 2016-05-23 19:38 ` [RFC-PATCH 0/2] send-email: new --quote-mail option Matthieu Moy @ 2016-05-27 17:11 ` Tom Russello 2016-05-27 17:11 ` [RFC-PATCH v2 1/2] send-email: quote-email populates the fields Tom Russello ` (2 more replies) 3 siblings, 3 replies; 96+ messages in thread From: Tom Russello @ 2016-05-27 17:11 UTC (permalink / raw) To: git Cc: jordan.de-gea, erwan.mathoniere, matthieu.moy, samuel.groot, e, aaron, gitster Hello, With the current send-email command, you can send a series of patches "in reply to" an email. This patch adds a new option to `git send-email`, `--quote-email=<file>` which does two things: - set fields appropriately (To, Cc, Subject, In-Reply-To) - quote the given message In this second patch, the new option `--quote-email=<file>` needs an email file and does not manage non ascii characters. There is still work in progress, including: 1. An option `--quote-email-id=<message-id>` to download the message from any source, e.g. http://mid.gmane.org/<message-id>/raw. The server's address could be set in the repo's config file. 2. There's also a discussion about whether this option should be integrated in the current `--in-reply-to` option or not * http://news.gmane.org/find-root.php?message_id=vpqh9dmfy5k.fsf@anie.imag.fr 3. The code to parse the email headers is currently duplicated several times, we are refactoring it to help maintaining the code. 4. Manage non ascii characters Documentation/git-send-email.txt | 8 +++++++ git-send-email.perl | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- t/t9001-send-email.sh | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 232 insertions(+), 3 deletions(-) ^ permalink raw reply [flat|nested] 96+ messages in thread
* [RFC-PATCH v2 1/2] send-email: quote-email populates the fields 2016-05-27 17:11 ` [RFC-PATCH v2 0/2] send-email: new --quote-email option Tom Russello @ 2016-05-27 17:11 ` Tom Russello 2016-05-28 14:35 ` Matthieu Moy 2016-05-27 17:11 ` [RFC-PATCH v2 2/2] send-email: quote-email quotes the message body Tom Russello 2016-06-07 14:01 ` [PATCH v3 0/6] send-email: cleaner tests and quote email Tom Russello 2 siblings, 1 reply; 96+ messages in thread From: Tom Russello @ 2016-05-27 17:11 UTC (permalink / raw) To: git Cc: jordan.de-gea, erwan.mathoniere, matthieu.moy, samuel.groot, e, aaron, gitster, Tom Russello Take an email message file, parse it and fill the "To", "Cc" and "In-Reply-To" fields appropriately. If `--compose` option is set, it will also fill the subject field with "Re: [<email_file>'s subject]". Signed-off-by: Tom Russello <tom.russello@grenoble-inp.org> Signed-off-by: Samuel Groot <samuel.groot@grenoble-inp.org> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr> --- As it is said in the cover letter, the file git-send-email.perl is being refactored therefore the parsing section with nested if's is ought to change. changes since v1: - option's name changed and is now --quote-email - original From: becomes To:, original To:'s become Cc: and original Cc:'s stay Cc: - coding style improved - documentation for the option - more tests Documentation/git-send-email.txt | 5 +++ git-send-email.perl | 87 +++++++++++++++++++++++++++++++++++++++- t/t9001-send-email.sh | 61 ++++++++++++++++++++++++++++ 3 files changed, 152 insertions(+), 1 deletion(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 771a7b5..2334d69 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -106,6 +106,11 @@ illustration below where `[PATCH v2 0/3]` is in reply to `[PATCH 0/2]`: Only necessary if --compose is also set. If --compose is not set, this will be prompted for. +--quote-email=<email_file>:: + Reply to the given email and automatically populate the "To:", "Cc:" and + "In-Reply-To:" fields. If `--compose` is set, this will also fill the + subject field with "Re: [<email_file>'s subject]". + --subject=<string>:: Specify the initial subject of the email thread. Only necessary if --compose is also set. If --compose diff --git a/git-send-email.perl b/git-send-email.perl index 6958785..9df3dee 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -55,6 +55,8 @@ git send-email --dump-aliases --[no-]bcc <str> * Email Bcc: --subject <str> * Email "Subject:" --in-reply-to <str> * Email "In-Reply-To:" + --quote-email <file> * Fill the fields "To:", "Cc:", "Subject:", + "In-Reply-To" appropriately. --[no-]xmailer * Add "X-Mailer:" header (default). --[no-]annotate * Review each patch that will be sent in an editor. --compose * Open an editor for introduction. @@ -160,7 +162,7 @@ my $re_encoded_word = qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/; # Variables we fill in automatically, or via prompting: my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh, - $initial_reply_to,$initial_subject,@files, + $initial_reply_to,$quote_email,$initial_subject,@files, $author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time); my $envelope_sender; @@ -304,6 +306,7 @@ $rc = GetOptions( "sender|from=s" => \$sender, "in-reply-to=s" => \$initial_reply_to, "subject=s" => \$initial_subject, + "quote-email=s" => \$quote_email, "to=s" => \@initial_to, "to-cmd=s" => \$to_cmd, "no-to" => \$no_to, @@ -639,6 +642,88 @@ if (@files) { usage(); } +if ($quote_email) { + my $error = validate_patch($quote_email); + $error and die "fatal: $quote_email: $error\nwarning: no patches were sent\n"; + + my @header = (); + + open my $fh, "<", $quote_email or die "can't open file $quote_email"; + + # Get the email header + while (<$fh>) { + # For files containing crlf line endings + s/\r//g; + last if /^\s*$/; + if (/^\s+\S/ and @header) { + chomp($header[$#header]); + s/^\s+/ /; + $header[$#header] .= $_; + } else { + push(@header, $_); + } + } + + # Parse the header + foreach (@header) { + my $input_format; + my $initial_sender = $sender || $repoauthor || $repocommitter || ''; + + if (/^From /) { + $input_format = 'mbox'; + next; + } + chomp; + if (!defined $input_format && /^[-A-Za-z]+:\s/) { + $input_format = 'mbox'; + } + + if (defined $input_format && $input_format eq 'mbox') { + if (/^Subject:\s+(.*)$/i) { + my $prefix_re = ""; + my $subject_re = $1; + if ($1 =~ /^[^Re:]/) { + $prefix_re = "Re: "; + } + $initial_subject = $prefix_re . $subject_re; + } elsif (/^From:\s+(.*)$/i) { + push @initial_to, $1; + } elsif (/^To:\s+(.*)$/i) { + foreach my $addr (parse_address_line($1)) { + if (!($addr eq $initial_sender)) { + push @initial_cc, $addr; + } + } + } elsif (/^Cc:\s+(.*)$/i) { + foreach my $addr (parse_address_line($1)) { + my $qaddr = unquote_rfc2047($addr); + my $saddr = sanitize_address($qaddr); + if ($saddr eq $initial_sender) { + next if ($suppress_cc{'self'}); + } else { + next if ($suppress_cc{'cc'}); + } + push @initial_cc, $addr; + } + } elsif (/^Message-Id: (.*)/i) { + $initial_reply_to = $1; + } + } else { + # In the traditional + # "send lots of email" format, + # line 1 = cc + # line 2 = subject + # So let's support that, too. + $input_format = 'lots'; + if (@cc == 0 && !$suppress_cc{'cc'}) { + push @cc, $_; + } elsif (!defined $initial_subject) { + $initial_subject = $_; + } + } + } +} + sub get_patch_subject { my $fn = shift; open (my $fh, '<', $fn); diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index b3355d2..389a54c 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -1885,4 +1885,65 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' ' test_cmp expected-list actual-list ' +test_expect_success $PREREQ 'setup expect' ' + cat >email <<-\EOF + Message-Id: <author_123456@example.com> + From: author@example.com + To: to1@example.com + Cc: cc1@example.com + Date: Sat, 12 Jun 2010 15:53:58 +0200 + Subject: subject goes here + + Have you seen my previous email? + > Previous content + EOF +' + +test_expect_success $PREREQ 'Fields with --quote-email are correct' ' + clean_fake_sendmail && + git send-email \ + --quote-email=email \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -1 \ + 2>errors && + grep "From: Example <nobody@example.com>" msgtxt1 && + grep "In-Reply-To: <author_123456@example.com>" msgtxt1 && + to_adr=$(awk "/^To: /,/^Cc: /" msgtxt1) && + cc_adr=$(awk "/^Cc: /,/^Date /" msgtxt1) && + echo "$to_adr" | grep author@example.com && + echo "$cc_adr" | grep to1@example.com && + echo "$cc_adr" | grep cc1@example.com +' + +test_expect_success $PREREQ 'Fields with --quote-email and --compose are correct' ' + clean_fake_sendmail && + git send-email \ + --quote-email=email \ + --compose \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -1 \ + 2>errors && + grep "From: Example <nobody@example.com>" msgtxt1 && + grep "In-Reply-To: <author_123456@example.com>" msgtxt1 && + grep "Subject: Re: subject goes here" msgtxt1 && + to_adr=$(awk "/^To: /,/^Cc: /" msgtxt1) && + cc_adr=$(awk "/^Cc: /,/^Date /" msgtxt1) && + echo "$to_adr" | grep author@example.com && + echo "$cc_adr" | grep to1@example.com && + echo "$cc_adr" | grep cc1@example.com +' + +test_expect_success $PREREQ 'Re: written only once with --quote-email and --compose ' ' + git send-email \ + --quote-email=msgtxt1 \ + --compose \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -1 \ + 2>errors && + grep "Subject: Re: subject goes here" msgtxt3 +' + test_done -- 2.8.2 ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH v2 1/2] send-email: quote-email populates the fields 2016-05-27 17:11 ` [RFC-PATCH v2 1/2] send-email: quote-email populates the fields Tom Russello @ 2016-05-28 14:35 ` Matthieu Moy 2016-05-29 23:38 ` Tom Russello 0 siblings, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-05-28 14:35 UTC (permalink / raw) To: Tom Russello Cc: git, jordan.de-gea, erwan.mathoniere, samuel.groot, e, aaron, gitster Tom Russello <tom.russello@grenoble-inp.org> writes: > --- a/Documentation/git-send-email.txt > +++ b/Documentation/git-send-email.txt > @@ -106,6 +106,11 @@ illustration below where `[PATCH v2 0/3]` is in reply to `[PATCH 0/2]`: > Only necessary if --compose is also set. If --compose > is not set, this will be prompted for. > > +--quote-email=<email_file>:: > + Reply to the given email and automatically populate the "To:", "Cc:" and > + "In-Reply-To:" fields. I think this is a bit too technical for a user documentation. To: and Cc: is OK, but people need not know about "In-Reply-To:" to understand this. See what the doc of --in-reply-to says. If you want to be technical, you'd need to mention the References: field too. Talking about Reference: field, something your patch could do is to add all references in <email_file> to the references of the new email (see what a mailer is doing when replying). This way, the recipient can still get threading if the last message being replied-to is missing. > +"Re: [<email_file>'s subject]". Perhaps `Re: ...` instead of double-quotes. > +if ($quote_email) { > + my $error = validate_patch($quote_email); > + $error and die "fatal: $quote_email: $error\nwarning: no patches were sent\n"; I know it's done this way elsewhere, but I don't like this "$error and die", I'd rather see a proper if here. > + if (defined $input_format && $input_format eq 'mbox') { To me, the input format refers to patch files, not the <email_file>. I'm not sure anyone still use the "lots of email" format, and you are not testing it. So, this is claiming that we have a feature without being sure we have it nor that anyone's ever going to use it. I'd just drop this "if" and the "else" branch, and just assume the email file is a normal email file. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH v2 1/2] send-email: quote-email populates the fields 2016-05-28 14:35 ` Matthieu Moy @ 2016-05-29 23:38 ` Tom Russello 0 siblings, 0 replies; 96+ messages in thread From: Tom Russello @ 2016-05-29 23:38 UTC (permalink / raw) To: Matthieu Moy Cc: git, jordan.de-gea, erwan.mathoniere, samuel.groot, e, aaron, gitster On 05/28/16 16:35, Matthieu Moy wrote: >> +--quote-email=<email_file>:: >> + Reply to the given email and automatically populate the "To:", "Cc:" and >> + "In-Reply-To:" fields. > > I think this is a bit too technical for a user documentation. To: and > Cc: is OK, but people need not know about "In-Reply-To:" to understand > this. See what the doc of --in-reply-to says. If you want to be > technical, you'd need to mention the References: field too. You have a point here. Maybe, we can explain that the `--quote-email` option behaves like a mailer when replying to someone without getting into details. > Talking about Reference: field, something your patch could do is to add > all references in <email_file> to the references of the new email (see > what a mailer is doing when replying). This way, the recipient can still > get threading if the last message being replied-to is missing. I didn't know about this field, it looks like it appends all the parent message ID's. >> +"Re: [<email_file>'s subject]". > > Perhaps `Re: ...` instead of double-quotes. Agreed. >> +if ($quote_email) { >> + my $error = validate_patch($quote_email); >> + $error and die "fatal: $quote_email: $error\nwarning: no patches were sent\n"; > > I know it's done this way elsewhere, but I don't like this "$error and > die", I'd rather see a proper if here. You're right, I'll change that in the next version. >> + if (defined $input_format && $input_format eq 'mbox') { > > To me, the input format refers to patch files, not the <email_file>. > > I'm not sure anyone still use the "lots of email" format, and you are > not testing it. So, this is claiming that we have a feature without > being sure we have it nor that anyone's ever going to use it. You summed up the situation well. > I'd just drop this "if" and the "else" branch, and just assume the email > file is a normal email file. I'll do that. Thank you for the review. ^ permalink raw reply [flat|nested] 96+ messages in thread
* [RFC-PATCH v2 2/2] send-email: quote-email quotes the message body 2016-05-27 17:11 ` [RFC-PATCH v2 0/2] send-email: new --quote-email option Tom Russello 2016-05-27 17:11 ` [RFC-PATCH v2 1/2] send-email: quote-email populates the fields Tom Russello @ 2016-05-27 17:11 ` Tom Russello 2016-05-28 15:01 ` Matthieu Moy 2016-06-07 14:01 ` [PATCH v3 0/6] send-email: cleaner tests and quote email Tom Russello 2 siblings, 1 reply; 96+ messages in thread From: Tom Russello @ 2016-05-27 17:11 UTC (permalink / raw) To: git Cc: jordan.de-gea, erwan.mathoniere, matthieu.moy, samuel.groot, e, aaron, gitster, Tom Russello Take an email message file, parse it and quote the message body. If `--compose` is set, then it will quote the message in the cover letter. Otherwise, imply `--annotate` option and put the quoted message in the first patch after the triple-dash. Signed-off-by: Tom Russello <tom.russello@grenoble-inp.org> Signed-off-by: Samuel Groot <samuel.groot@grenoble-inp.org> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr> --- Currently, `send-email` without `--compose` implies `--annotate`. The current behavior of `--annotate` is that changes made during edition are saved in patch files. Keeping that behavior when using `--quote-email` populates the patch file with the quoted message body, and the patch is saved no matter what. If the user closes his editor and then exits `send-email`, changes will be saved. Should we keep the current behavior for the user, keeping the changes (including the quoted message body) in the patch, or should we discard them? changes since v1: - default behaviour of the option: it is now --annotate as one may not want to send a cover letter for a single patch - "On [date], [original author] wrote:" is added before the quoted message - request to trim useless parts of the cited message when `--compose` is set - extra blank removed when the message quoted is empty Documentation/git-send-email.txt | 5 ++- git-send-email.perl | 79 +++++++++++++++++++++++++++++++++++++--- t/t9001-send-email.sh | 6 +++ 3 files changed, 84 insertions(+), 6 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 2334d69..68bbb6c 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -109,7 +109,10 @@ is not set, this will be prompted for. --quote-email=<email_file>:: Reply to the given email and automatically populate the "To:", "Cc:" and "In-Reply-To:" fields. If `--compose` is set, this will also fill the - subject field with "Re: [<email_file>'s subject]". + subject field with "Re: [<email_file>'s subject]" and quote the message body + of <email_file>. If `--compose` is not set, this will imply `--annotate` + option and will quote the message body of <email_file> after the triple-dash + in the first patch. --subject=<string>:: Specify the initial subject of the email thread. diff --git a/git-send-email.perl b/git-send-email.perl index 9df3dee..e73aa25 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -26,6 +26,7 @@ use Text::ParseWords; use Term::ANSIColor; use File::Temp qw/ tempdir tempfile /; use File::Spec::Functions qw(catfile); +use File::Copy; use Error qw(:try); use Git; @@ -55,8 +56,8 @@ git send-email --dump-aliases --[no-]bcc <str> * Email Bcc: --subject <str> * Email "Subject:" --in-reply-to <str> * Email "In-Reply-To:" - --quote-email <file> * Fill the fields "To:", "Cc:", "Subject:", - "In-Reply-To" appropriately. + --quote-email <file> * Fill To:, Cc:, Subject:, In-Reply-To: + appropriately and quote the message body. --[no-]xmailer * Add "X-Mailer:" header (default). --[no-]annotate * Review each patch that will be sent in an editor. --compose * Open an editor for introduction. @@ -642,11 +643,14 @@ if (@files) { usage(); } +my $message_quoted; if ($quote_email) { my $error = validate_patch($quote_email); $error and die "fatal: $quote_email: $error\nwarning: no patches were sent\n"; my @header = (); + my $date; + my $recipient; open my $fh, "<", $quote_email or die "can't open file $quote_email"; @@ -687,7 +691,8 @@ if ($quote_email) { } $initial_subject = $prefix_re . $subject_re; } elsif (/^From:\s+(.*)$/i) { - push @initial_to, $1; + $recipient = $1; + push @initial_to, $recipient; } elsif (/^To:\s+(.*)$/i) { foreach my $addr (parse_address_line($1)) { if (!($addr eq $initial_sender)) { @@ -707,6 +712,8 @@ if ($quote_email) { } } elsif (/^Message-Id: (.*)/i) { $initial_reply_to = $1; + } elsif (/^Date: (.*)/i) { + $date = $1; } } else { # In the traditional @@ -722,6 +729,23 @@ if ($quote_email) { } } } + + my $tpl_date = $date && "On $date, " || ''; + $message_quoted = $tpl_date . $recipient . " wrote:\n"; + + # Quote the message body + while (<$fh>) { + # Only for files containing crlf line endings + s/\r//g; + my $space = ""; + if (/^[^>]/) { + $space = " "; + } + $message_quoted .= ">" . $space . $_; + } + if (!$compose) { + $annotate = 1; + } } sub get_patch_subject { @@ -749,6 +773,9 @@ if ($compose) { my $tpl_sender = $sender || $repoauthor || $repocommitter || ''; my $tpl_subject = $initial_subject || ''; my $tpl_reply_to = $initial_reply_to || ''; + my $tpl_quote = $message_quoted && + "\nGIT: Please, trim down irrelevant sections in the quoted message\n". + "GIT: to keep your email concise\n" . $message_quoted || ''; print $c <<EOT; From $tpl_sender # This line is ignored. @@ -760,7 +787,7 @@ GIT: Clear the body content if you don't wish to send a summary. From: $tpl_sender Subject: $tpl_subject In-Reply-To: $tpl_reply_to - +$tpl_quote EOT for my $f (@files) { print $c get_patch_subject($f); @@ -825,9 +852,51 @@ EOT $compose = -1; } } elsif ($annotate) { - do_edit(@files); + if ($quote_email) { + my $quote_email_filename = ($repo ? + tempfile(".gitsendemail.msg.XXXXXX", + DIR => $repo->repo_path()) : + tempfile(".gitsendemail.msg.XXXXXX", + DIR => "."))[1]; + + do_insert_quoted_message($quote_email_filename, $files[0]); + + my $tmp = $files[0]; + $files[0] = $quote_email_filename; + + do_edit(@files); + + # Erase the original patch + move($quote_email_filename, $tmp); + $files[0] = $tmp; + } else { + do_edit(@files); + } } +sub do_insert_quoted_message { + my $tmp_file = shift; + my $original_file = shift; + + open my $c, "<", $original_file + or die "Failed to open $original_file : " . $!; + + open my $c2, ">", $tmp_file + or die "Failed to open $tmp_file : " . $!; + + # Insertion after the triple-dash + while (<$c>) { + print $c2 $_; + last if (/^---$/); + } + print $c2 $message_quoted; + while (<$c>) { + print $c2 $_; + } + + close $c; + close $c2; +} sub ask { my ($prompt, %arg) = @_; my $valid_re = $arg{valid_re}; diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 389a54c..5ab7533 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -1916,6 +1916,12 @@ test_expect_success $PREREQ 'Fields with --quote-email are correct' ' echo "$cc_adr" | grep cc1@example.com ' +test_expect_success $PREREQ 'correct quoted message with --quote-email' ' + grep "On Sat, 12 Jun 2010 15:53:58 +0200, author@example.com wrote:" msgtxt1 && + grep "> Have you seen my previous email?" msgtxt1 && + grep ">> Previous content" msgtxt1 +' + test_expect_success $PREREQ 'Fields with --quote-email and --compose are correct' ' clean_fake_sendmail && git send-email \ -- 2.8.2 ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH v2 2/2] send-email: quote-email quotes the message body 2016-05-27 17:11 ` [RFC-PATCH v2 2/2] send-email: quote-email quotes the message body Tom Russello @ 2016-05-28 15:01 ` Matthieu Moy 2016-05-29 11:41 ` Tom Russello 0 siblings, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-05-28 15:01 UTC (permalink / raw) To: Tom Russello Cc: git, jordan.de-gea, erwan.mathoniere, samuel.groot, e, aaron, gitster Tom Russello <tom.russello@grenoble-inp.org> writes: > Currently, `send-email` without `--compose` implies `--annotate`. I don't get it. Did you mean s/without/with/? Even if so, this is not exactly true: "git send-email --compose -1" will open the editor only for the cover-letter, while adding --annotate will also open it for the patch. > Keeping that behavior when using `--quote-email` populates the patch file with > the quoted message body, and the patch is saved no matter what. If the user > closes his editor and then exits `send-email`, changes will be saved. > > Should we keep the current behavior for the user, keeping the changes (including > the quoted message body) in the patch, or should we discard them? (Note: we discussed this off-list already, but I'll try to summarize my thoughts here) I don't have strong opinion on this, but I think there's a difference between launching the editor directly on the input patch files (resulting in _user_'s edit being done directly on them) and having the script modify it in-place (resulting in automatic changes done directly on the user's files). I usually use "git send-email" directly without using "git format-patch", so I'm not the best juge. But I can imagine a flow like 1) run "git send-email *.patch" 2) start editting 3) notice there's something wrong, give up for now (answer 'q' when git send-email prompts for confirmation, or kill it via Control-C in a terminal) 4) run "git send-email *.patch" again 5) be happy that changes done at 2) are still there. With --quote-email, it's different. The scenario above would result in 5') WTF, why is the email quoted twice? Unfortunately, I don't really have a solution for this. My first thought was that we should copy the files to a temporary location before starting the editor (that what I'm used to when using "git send-email" without "git format-patch"), but that would prevent 5) above. > @@ -109,7 +109,10 @@ is not set, this will be prompted for. > --quote-email=<email_file>:: > Reply to the given email and automatically populate the "To:", "Cc:" and > "In-Reply-To:" fields. If `--compose` is set, this will also fill the > - subject field with "Re: [<email_file>'s subject]". > + subject field with "Re: [<email_file>'s subject]" and quote the message body > + of <email_file>. I'd add "in the introductory message". > + while (<$fh>) { > + # Only for files containing crlf line endings > + s/\r//g; The comment doesn't really say what it does. What about "turn crlf line endings into lf-only"? > } elsif ($annotate) { > - do_edit(@files); > + if ($quote_email) { > + my $quote_email_filename = ($repo ? > + tempfile(".gitsendemail.msg.XXXXXX", > + DIR => $repo->repo_path()) : > + tempfile(".gitsendemail.msg.XXXXXX", > + DIR => "."))[1]; > + > + do_insert_quoted_message($quote_email_filename, $files[0]); > + > + my $tmp = $files[0]; > + $files[0] = $quote_email_filename; > + > + do_edit(@files); > + > + # Erase the original patch > + move($quote_email_filename, $tmp); > + $files[0] = $tmp; When writing comment, always try to ask the question "why?" more than "what?". This part is possibly controversial, think about a contributor finding this piece of code later without having followed the current conversation. He'd probably expect an explanation about why you need a temp file here and not elsewhere. > + open my $c, "<", $original_file > + or die "Failed to open $original_file : " . $!; > + > + open my $c2, ">", $tmp_file > + or die "Failed to open $tmp_file : " . $!; No space before :. > --- a/t/t9001-send-email.sh > +++ b/t/t9001-send-email.sh > @@ -1916,6 +1916,12 @@ test_expect_success $PREREQ 'Fields with --quote-email are correct' ' > echo "$cc_adr" | grep cc1@example.com > ' > > +test_expect_success $PREREQ 'correct quoted message with --quote-email' ' > + grep "On Sat, 12 Jun 2010 15:53:58 +0200, author@example.com wrote:" msgtxt1 && > + grep "> Have you seen my previous email?" msgtxt1 && > + grep ">> Previous content" msgtxt1 > +' When the spec says "if --compose ... then ...", "after the triple-dash", and "in the first patch", one would expect at least one test with --compose and one without, something to check that the insertion was done below the triple-dash, and one test with two patches, checking that the second patch is not altered by --quote-email. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [RFC-PATCH v2 2/2] send-email: quote-email quotes the message body 2016-05-28 15:01 ` Matthieu Moy @ 2016-05-29 11:41 ` Tom Russello 0 siblings, 0 replies; 96+ messages in thread From: Tom Russello @ 2016-05-29 11:41 UTC (permalink / raw) To: Matthieu Moy Cc: git, jordan.de-gea, erwan.mathoniere, samuel.groot, e, aaron, gitster On 05/28/16 17:01, Matthieu Moy wrote: >> Currently, `send-email` without `--compose` implies `--annotate`. > > I don't get it. Did you mean s/without/with/? Even if so, this is not > exactly true: "git send-email --compose -1" will open the editor only > for the cover-letter, while adding --annotate will also open it for the > patch. We meant that the default behavior of `--quote-email` (i.e. without --compose enabled) will open the editor with the given patches in argument and will quote the message body in the first one. > (Note: we discussed this off-list already, but I'll try to summarize my > thoughts here) > > I don't have strong opinion on this, but I think there's a difference > between launching the editor directly on the input patch files > (resulting in _user_'s edit being done directly on them) and having the > script modify it in-place (resulting in automatic changes done directly > on the user's files). > > I usually use "git send-email" directly without using "git > format-patch", so I'm not the best juge. But I can imagine a flow like > > 1) run "git send-email *.patch" > > 2) start editting > > 3) notice there's something wrong, give up for now (answer 'q' when git > send-email prompts for confirmation, or kill it via Control-C in a > terminal) > > 4) run "git send-email *.patch" again > > 5) be happy that changes done at 2) are still there. > > With --quote-email, it's different. The scenario above would result in > > 5') WTF, why is the email quoted twice? Actually the Control-C during the edition will cancel all the annotations written (including the cited email). > Unfortunately, I don't really have a solution for this. My first thought > was that we should copy the files to a temporary location before > starting the editor (that what I'm used to when using "git send-email" > without "git format-patch"), but that would prevent 5) above. It's already what we did: the first original patch is copied in a temporary file. However, if the edition went well (i.e. the editor closed by the user), the temporary file will erase the original one. >> @@ -109,7 +109,10 @@ is not set, this will be prompted for. >> --quote-email=<email_file>:: >> Reply to the given email and automatically populate the "To:", "Cc:" and >> "In-Reply-To:" fields. If `--compose` is set, this will also fill the >> - subject field with "Re: [<email_file>'s subject]". >> + subject field with "Re: [<email_file>'s subject]" and quote the message body >> + of <email_file>. > > I'd add "in the introductory message". Agreed. >> + while (<$fh>) { >> + # Only for files containing crlf line endings >> + s/\r//g; > > The comment doesn't really say what it does. > > What about "turn crlf line endings into lf-only"? Yes, I completely agree this suggestion. > When writing comment, always try to ask the question "why?" more than > "what?". This part is possibly controversial, think about a contributor > finding this piece of code later without having followed the current > conversation. He'd probably expect an explanation about why you need a > temp file here and not elsewhere. Thank you for the advice, I'll keep it in mind. >> + open my $c, "<", $original_file >> + or die "Failed to open $original_file : " . $!; >> + >> + open my $c2, ">", $tmp_file >> + or die "Failed to open $tmp_file : " . $!; > > No space before :. Sorry, I copied the previous error messages. > When the spec says "if --compose ... then ...", "after the triple-dash", > and "in the first patch", one would expect at least one test with > --compose and one without, something to check that the insertion was > done below the triple-dash, and one test with two patches, checking that > the second patch is not altered by --quote-email. Yes, indeed. I'll add these tests in the next version. Thank you for the review. ^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v3 0/6] send-email: cleaner tests and quote email 2016-05-27 17:11 ` [RFC-PATCH v2 0/2] send-email: new --quote-email option Tom Russello 2016-05-27 17:11 ` [RFC-PATCH v2 1/2] send-email: quote-email populates the fields Tom Russello 2016-05-27 17:11 ` [RFC-PATCH v2 2/2] send-email: quote-email quotes the message body Tom Russello @ 2016-06-07 14:01 ` Tom Russello 2016-06-07 14:01 ` [PATCH v3 1/6] t9001: non order-sensitive file comparison Tom Russello ` (4 more replies) 2 siblings, 5 replies; 96+ messages in thread From: Tom Russello @ 2016-06-07 14:01 UTC (permalink / raw) To: git Cc: erwan.mathoniere, samuel.groot, jordan.de-gea, matthieu.moy, e, aaron, gitster The purpose of this series of patches is to implement a new "quote-email" feature integrated in the current `--in-reply-to` option. * The first 2 patches make the tests less dependent to `git send-email`'s exact output. * Third patch makes `git send-email` a bit less verbose. * Fourth patch introduces our email parser subroutine. * Fifth patch makes the `--in-reply-to` open a email file (if it exists) and populates From:, To:, Cc:, In-reply-to and References: fields. * Sixth patch quotes the message body in the cover letter if `--compose` is set. Else, imply `--annotate` and insert quoted message body below triple-dash in the first patch. General changes since v2: - Modify tests to be less dependent on `git send-email`'s exact output. - New email parser subroutine. - `--quote-email` option is now merged with `--in-reply-to`. - Add `--cite` option to quote the message body. - `git send-email` is less verbose. Documentation/git-send-email.txt | 17 +++++-- git-send-email.perl | 178 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------ t/t9001-send-email.sh | 238 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------- 3 files changed, 356 insertions(+), 77 deletions(-) ^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v3 1/6] t9001: non order-sensitive file comparison 2016-06-07 14:01 ` [PATCH v3 0/6] send-email: cleaner tests and quote email Tom Russello @ 2016-06-07 14:01 ` Tom Russello 2016-06-08 1:07 ` Junio C Hamano 2016-06-07 14:01 ` [PATCH v3 2/6] t9001: check email address is in Cc: field Tom Russello ` (3 subsequent siblings) 4 siblings, 1 reply; 96+ messages in thread From: Tom Russello @ 2016-06-07 14:01 UTC (permalink / raw) To: git Cc: erwan.mathoniere, samuel.groot, jordan.de-gea, matthieu.moy, e, aaron, gitster, Tom RUSSELLO Tests might fail if lines compared in text files don't have the same order. Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> Signed-off-by: Tom RUSSELLO <tom.russello@grenoble-inp.org> Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> --- t/t9001-send-email.sh | 61 ++++++++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index b3355d2..4558e0f 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -54,6 +54,13 @@ test_no_confirm () { >no_confirm_okay } +# Check if two files have the same content, non-order sensitive +test_cmp_noorder () { + sort $1 >$1; + sort $2 >$2; + return $(test_cmp $1 $2) +} + # Exit immediately to prevent hang if a no-confirm test fails check_no_confirm () { if ! test -f no_confirm_okay @@ -97,7 +104,7 @@ test_expect_success $PREREQ 'setup expect' ' ' test_expect_success $PREREQ 'Verify commandline' ' - test_cmp expected commandline1 + test_cmp_noorder expected commandline1 ' test_expect_success $PREREQ 'Send patches with --envelope-sender' ' @@ -117,7 +124,7 @@ test_expect_success $PREREQ 'setup expect' ' ' test_expect_success $PREREQ 'Verify commandline' ' - test_cmp expected commandline1 + test_cmp_noorder expected commandline1 ' test_expect_success $PREREQ 'Send patches with --envelope-sender=auto' ' @@ -137,7 +144,7 @@ test_expect_success $PREREQ 'setup expect' ' ' test_expect_success $PREREQ 'Verify commandline' ' - test_cmp expected commandline1 + test_cmp_noorder expected commandline1 ' test_expect_success $PREREQ 'setup expect' " @@ -196,7 +203,7 @@ test_suppress_self () { >"expected-no-cc-$3" && (grep '^Cc:' msghdr1-$3 >"actual-no-cc-$3"; - test_cmp expected-no-cc-$3 actual-no-cc-$3) + test_cmp_noorder expected-no-cc-$3 actual-no-cc-$3) } test_suppress_self_unquoted () { @@ -269,7 +276,7 @@ test_expect_success $PREREQ 'Show all headers' ' -e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \ -e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \ >actual-show-all-headers && - test_cmp expected-show-all-headers actual-show-all-headers + test_cmp_noorder expected-show-all-headers actual-show-all-headers ' test_expect_success $PREREQ 'Prompting works' ' @@ -436,13 +443,13 @@ test_expect_success $PREREQ 'In-Reply-To without --chain-reply-to' ' 2>errors && # The first message is a reply to --in-reply-to sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual && - test_cmp expect actual && + test_cmp_noorder expect actual && # Second and subsequent messages are replies to the first one sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt1 >expect && sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual && - test_cmp expect actual && + test_cmp_noorder expect actual && sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual && - test_cmp expect actual + test_cmp_noorder expect actual ' test_expect_success $PREREQ 'In-Reply-To with --chain-reply-to' ' @@ -457,13 +464,13 @@ test_expect_success $PREREQ 'In-Reply-To with --chain-reply-to' ' $patches $patches $patches \ 2>errors && sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual && - test_cmp expect actual && + test_cmp_noorder expect actual && sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt1 >expect && sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual && - test_cmp expect actual && + test_cmp_noorder expect actual && sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt2 >expect && sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual && - test_cmp expect actual + test_cmp_noorder expect actual ' test_expect_success $PREREQ 'setup fake editor' ' @@ -537,7 +544,7 @@ test_suppression () { --smtp-server relay.example.com \ $patches | replace_variable_fields \ >actual-suppress-$1${2+"-$2"} && - test_cmp expected-suppress-$1${2+"-$2"} actual-suppress-$1${2+"-$2"} + test_cmp_noorder expected-suppress-$1${2+"-$2"} actual-suppress-$1${2+"-$2"} } test_expect_success $PREREQ 'sendemail.cc set' ' @@ -1213,7 +1220,7 @@ test_expect_success $PREREQ 'ASCII subject is not RFC2047 quoted' ' --8bit-encoding=UTF-8 \ email-using-8bit >stdout && grep "Subject" msgtxt1 >actual && - test_cmp expected actual + test_cmp_noorder expected actual ' test_expect_success $PREREQ 'setup expect' ' @@ -1234,7 +1241,7 @@ test_expect_success $PREREQ 'asks about and fixes 8bit encodings' ' grep email-using-8bit stdout && grep "Which 8bit encoding" stdout && egrep "Content|MIME" msgtxt1 >actual && - test_cmp actual content-type-decl + test_cmp_noorder actual content-type-decl ' test_expect_success $PREREQ 'sendemail.8bitEncoding works' ' @@ -1245,7 +1252,7 @@ test_expect_success $PREREQ 'sendemail.8bitEncoding works' ' --smtp-server="$(pwd)/fake.sendmail" \ email-using-8bit >stdout && egrep "Content|MIME" msgtxt1 >actual && - test_cmp actual content-type-decl + test_cmp_noorder actual content-type-decl ' test_expect_success $PREREQ '--8bit-encoding overrides sendemail.8bitEncoding' ' @@ -1257,7 +1264,7 @@ test_expect_success $PREREQ '--8bit-encoding overrides sendemail.8bitEncoding' ' --8bit-encoding=UTF-8 \ email-using-8bit >stdout && egrep "Content|MIME" msgtxt1 >actual && - test_cmp actual content-type-decl + test_cmp_noorder actual content-type-decl ' test_expect_success $PREREQ 'setup expect' ' @@ -1286,7 +1293,7 @@ test_expect_success $PREREQ '--8bit-encoding also treats subject' ' --8bit-encoding=UTF-8 \ email-using-8bit >stdout && grep "Subject" msgtxt1 >actual && - test_cmp expected actual + test_cmp_noorder expected actual ' test_expect_success $PREREQ 'setup expect' ' @@ -1335,7 +1342,7 @@ test_expect_success $PREREQ 'sendemail.transferencoding=8bit' ' 2>errors >out && sed '1,/^$/d' msgtxt1 >actual && sed '1,/^$/d' email-using-8bit >expected && - test_cmp expected actual + test_cmp_noorder expected actual ' test_expect_success $PREREQ 'setup expect' ' @@ -1352,7 +1359,7 @@ test_expect_success $PREREQ '8-bit and sendemail.transferencoding=quoted-printab email-using-8bit \ 2>errors >out && sed '1,/^$/d' msgtxt1 >actual && - test_cmp expected actual + test_cmp_noorder expected actual ' test_expect_success $PREREQ 'setup expect' ' @@ -1369,7 +1376,7 @@ test_expect_success $PREREQ '8-bit and sendemail.transferencoding=base64' ' email-using-8bit \ 2>errors >out && sed '1,/^$/d' msgtxt1 >actual && - test_cmp expected actual + test_cmp_noorder expected actual ' test_expect_success $PREREQ 'setup expect' ' @@ -1395,7 +1402,7 @@ test_expect_success $PREREQ 'convert from quoted-printable to base64' ' email-using-qp \ 2>errors >out && sed '1,/^$/d' msgtxt1 >actual && - test_cmp expected actual + test_cmp_noorder expected actual ' test_expect_success $PREREQ 'setup expect' " @@ -1425,7 +1432,7 @@ test_expect_success $PREREQ 'CRLF and sendemail.transferencoding=quoted-printabl email-using-crlf \ 2>errors >out && sed '1,/^$/d' msgtxt1 >actual && - test_cmp expected actual + test_cmp_noorder expected actual ' test_expect_success $PREREQ 'setup expect' ' @@ -1442,7 +1449,7 @@ test_expect_success $PREREQ 'CRLF and sendemail.transferencoding=base64' ' email-using-crlf \ 2>errors >out && sed '1,/^$/d' msgtxt1 >actual && - test_cmp expected actual + test_cmp_noorder expected actual ' @@ -1582,7 +1589,7 @@ test_dump_aliases () { "$(pwd)/.tmp-email-aliases" && git config sendemail.aliasfiletype "$filetype" && git send-email --dump-aliases 2>errors >actual && - test_cmp expect actual + test_cmp_noorder expect actual ' } @@ -1842,7 +1849,7 @@ test_expect_success $PREREQ 'use email list in --cc --to and --bcc' ' --bcc="bcc1@example.com, bcc2@example.com" \ 0001-add-master.patch | replace_variable_fields \ >actual-list && - test_cmp expected-list actual-list + test_cmp_noorder expected-list actual-list ' test_expect_success $PREREQ 'aliases work with email list' ' @@ -1858,7 +1865,7 @@ test_expect_success $PREREQ 'aliases work with email list' ' --bcc="bcc1@example.com, bcc2@example.com" \ 0001-add-master.patch | replace_variable_fields \ >actual-list && - test_cmp expected-list actual-list + test_cmp_noorder expected-list actual-list ' test_expect_success $PREREQ 'leading and trailing whitespaces are removed' ' @@ -1882,7 +1889,7 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' ' --bcc="bcc2@example.com" \ 0001-add-master.patch | replace_variable_fields \ >actual-list && - test_cmp expected-list actual-list + test_cmp_noorder expected-list actual-list ' test_done -- 2.8.3 ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison 2016-06-07 14:01 ` [PATCH v3 1/6] t9001: non order-sensitive file comparison Tom Russello @ 2016-06-08 1:07 ` Junio C Hamano 2016-06-08 8:23 ` Samuel GROOT 2016-06-09 5:51 ` Matthieu Moy 0 siblings, 2 replies; 96+ messages in thread From: Junio C Hamano @ 2016-06-08 1:07 UTC (permalink / raw) To: Tom Russello Cc: git, erwan.mathoniere, samuel.groot, jordan.de-gea, matthieu.moy, e, aaron Tom Russello <tom.russello@grenoble-inp.org> writes: > +# Check if two files have the same content, non-order sensitive > +test_cmp_noorder () { > + sort $1 >$1; Here is what I think happens: 0) the shell parses this command line; 1) the shell notices that the output has to go to $1; 2) the shell does open(2) of $1, 3) the shell spawns "sort" with a single argument, with its output connected to the file descriptor obtained in 2). Because "$1" becomes an empty file at 2), "sort" reads nothing and writes nothing. > + sort $2 >$2; > + return $(test_cmp $1 $2) What is this return doing? I would understand if it were just test_cmp $1 $2 Of course, all the places you use test_cmp_noorder are happy when this function returns 0/success, and because $1 and $2 at this point are both empty files and test_cmp will not say anything to its standard output, the return will just yield 0/success to the caller of the function, so it is likely that with this patch t9001 would have passed for you, but that is not necessarily a good thing X-<. > @@ -269,7 +276,7 @@ test_expect_success $PREREQ 'Show all headers' ' > -e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \ > -e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \ > >actual-show-all-headers && > - test_cmp expected-show-all-headers actual-show-all-headers > + test_cmp_noorder expected-show-all-headers actual-show-all-headers > ' It is dubious that it is a good idea to blindly sort two files and compare, especially because expected-show-all-headers is actually something like this: cat >expected-show-all-headers <<\EOF 0001-Second.patch (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' (mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> RCPT TO:<to@example.com> RCPT TO:<cc@example.com> ... To: to@example.com Cc: cc@example.com, A <author@example.com>, One <one@example.com>, two@example.com Subject: [PATCH 1/1] Second. Date: DATE-STRING Message-Id: MESSAGE-ID-STRING X-Mailer: X-MAILER-STRING In-Reply-To: <unique-message-id@example.com> References: <unique-message-id@example.com> Result: OK EOF We do want to see MAIL FROM: as the first thing we give to the server, followed by RCPT TO:, followed by the headers. I am having a hard time guessing what prompted you to sort the output, i.e. what problem you were trying to solve. It cannot be because addresses on a list (e.g. Cc:) could come out in an indeterministic order, because the address that a test expects to be the first (cc@example.com in the above example) may not appear as the first one, but in the textual output it _is_ shown differently from the remainder (i.e. even if you sort, from "Cc: cc@example.com," it is clear it was the first one output for Cc: and diferent from "A <author@example.com>". ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison 2016-06-08 1:07 ` Junio C Hamano @ 2016-06-08 8:23 ` Samuel GROOT 2016-06-08 16:09 ` Junio C Hamano 2016-06-09 5:51 ` Matthieu Moy 1 sibling, 1 reply; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 8:23 UTC (permalink / raw) To: Junio C Hamano, Tom Russello Cc: git, erwan.mathoniere, jordan.de-gea, matthieu.moy, e, aaron On 06/08/2016 03:07 AM, Junio C Hamano wrote: > I am having a hard time guessing what prompted you to sort the > output, i.e. what problem you were trying to solve. It cannot be > because addresses on a list (e.g. Cc:) could come out in an > indeterministic order, because the address that a test expects to be > the first (cc@example.com in the above example) may not appear as > the first one, but in the textual output it _is_ shown differently > from the remainder (i.e. even if you sort, from "Cc: > cc@example.com," it is clear it was the first one output for Cc: and > diferent from "A <author@example.com>". Actually we had issues when trying to refactor send-email's email parsing loop [1]. Email addresses in output file `commandeline1` in tests weren't sorted the same way as the reference file it was compared to. E.g.: !nobody@example.com! !author@example.com! !one@example.com! !two@example.com! I agree replacing test_cmp with test_cmp_noorder is pointless, I will fix it and re-roll. Thanks. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison 2016-06-08 8:23 ` Samuel GROOT @ 2016-06-08 16:09 ` Junio C Hamano 2016-06-08 16:46 ` Samuel GROOT 0 siblings, 1 reply; 96+ messages in thread From: Junio C Hamano @ 2016-06-08 16:09 UTC (permalink / raw) To: Samuel GROOT Cc: Tom Russello, git, erwan.mathoniere, jordan.de-gea, matthieu.moy, e, aaron Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > Actually we had issues when trying to refactor send-email's email > parsing loop [1]. Email addresses in output file `commandeline1` in > tests weren't sorted the same way as the reference file it was > compared to. E.g.: > > !nobody@example.com! > !author@example.com! > !one@example.com! > !two@example.com! And the reason why these addresses that are collected from the same input (i.e. command line, existing e-mail fields, footers, etc.) are shown in different order in your implementation is...? ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison 2016-06-08 16:09 ` Junio C Hamano @ 2016-06-08 16:46 ` Samuel GROOT 2016-06-09 6:01 ` Matthieu Moy 0 siblings, 1 reply; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 16:46 UTC (permalink / raw) To: Junio C Hamano Cc: Tom Russello, git, erwan.mathoniere, jordan.de-gea, matthieu.moy, e, aaron On 06/08/2016 06:09 PM, Junio C Hamano wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > >> Actually we had issues when trying to refactor send-email's email >> parsing loop [1]. Email addresses in output file `commandeline1` in >> tests weren't sorted the same way as the reference file it was >> compared to. E.g.: >> >> !nobody@example.com! >> !author@example.com! >> !one@example.com! >> !two@example.com! > > And the reason why these addresses that are collected from the same > input (i.e. command line, existing e-mail fields, footers, etc.) are > shown in different order in your implementation is...? It's not shown in different order in our implementation, it's just a leftover of my refactor attempt [1]. Maybe it's a bad idea to increase tests' complexity, but IMHO tests should be independent to the addresses' order. Plus, it would help refactor in the future, the data being processed differently: parsing and processing in different subroutines rather than doing everything in one gigantic loop. We can drop it if necessary, but it may be useful to make git-send-email.perl easier to maintain. [1] * http://article.gmane.org/gmane.comp.version-control.git/295753 ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison 2016-06-08 16:46 ` Samuel GROOT @ 2016-06-09 6:01 ` Matthieu Moy 2016-06-13 22:21 ` Samuel GROOT 0 siblings, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-06-09 6:01 UTC (permalink / raw) To: Samuel GROOT Cc: Junio C Hamano, Tom Russello, git, erwan.mathoniere, jordan.de-gea, e, aaron Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > On 06/08/2016 06:09 PM, Junio C Hamano wrote: >> Samuel GROOT <samuel.groot@grenoble-inp.org> writes: >> >>> Actually we had issues when trying to refactor send-email's email >>> parsing loop [1]. Email addresses in output file `commandeline1` in >>> tests weren't sorted the same way as the reference file it was >>> compared to. E.g.: >>> >>> !nobody@example.com! >>> !author@example.com! >>> !one@example.com! >>> !two@example.com! >> >> And the reason why these addresses that are collected from the same >> input (i.e. command line, existing e-mail fields, footers, etc.) are >> shown in different order in your implementation is...? > > It's not shown in different order in our implementation, it's just a > leftover of my refactor attempt [1]. I think the refactoring makes sense, but having this patch as PATCH 1/6 in a series about --in-reply-to confuses reviewers: they would expect this patch to be useful to the others in the series. If you have "reply to a message in a file" ready without the refactoring, and a mostly ready refactoring, then I think it makes sense to have two patch series, the first being only "reply to a message in a file". If the refactoring itself is not ready, you may send a separate series "tests clean up" and explain on the cover-letter that it's, well, only a test clean up. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison 2016-06-09 6:01 ` Matthieu Moy @ 2016-06-13 22:21 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-13 22:21 UTC (permalink / raw) To: Matthieu Moy Cc: Junio C Hamano, Tom Russello, git, erwan.mathoniere, jordan.de-gea, e, aaron On 06/09/2016 08:01 AM, Matthieu Moy wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > >> On 06/08/2016 06:09 PM, Junio C Hamano wrote: >>> Samuel GROOT <samuel.groot@grenoble-inp.org> writes: >>> >>>> Actually we had issues when trying to refactor send-email's email >>>> parsing loop [1]. Email addresses in output file `commandeline1` in >>>> tests weren't sorted the same way as the reference file it was >>>> compared to. E.g.: >>>> >>>> !nobody@example.com! >>>> !author@example.com! >>>> !one@example.com! >>>> !two@example.com! >>> >>> And the reason why these addresses that are collected from the same >>> input (i.e. command line, existing e-mail fields, footers, etc.) are >>> shown in different order in your implementation is...? >> >> It's not shown in different order in our implementation, it's just a >> leftover of my refactor attempt [1]. > > I think the refactoring makes sense, but having this patch as PATCH 1/6 > in a series about --in-reply-to confuses reviewers: they would expect > this patch to be useful to the others in the series. > > If you have "reply to a message in a file" ready without the > refactoring, and a mostly ready refactoring, then I think it makes sense > to have two patch series, the first being only "reply to a message in a > file". If the refactoring itself is not ready, you may send a separate > series "tests clean up" and explain on the cover-letter that it's, well, > only a test clean up. I think I will split the patch series into 3 smaller series: - "quote-email" feature - tests clean up - send-email code cleanup (including send-email's output) ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison 2016-06-08 1:07 ` Junio C Hamano 2016-06-08 8:23 ` Samuel GROOT @ 2016-06-09 5:51 ` Matthieu Moy 2016-06-09 8:15 ` Tom Russello 1 sibling, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-06-09 5:51 UTC (permalink / raw) To: Junio C Hamano Cc: Tom Russello, git, erwan.mathoniere, samuel.groot, jordan.de-gea, e, aaron Junio C Hamano <gitster@pobox.com> writes: > Tom Russello <tom.russello@grenoble-inp.org> writes: > >> +# Check if two files have the same content, non-order sensitive >> +test_cmp_noorder () { >> + sort $1 >$1; > > Here is what I think happens: > > 0) the shell parses this command line; > 1) the shell notices that the output has to go to $1; > 2) the shell does open(2) of $1, > 3) the shell spawns "sort" with a single argument, with its > output connected to the file descriptor obtained in 2). > > Because "$1" becomes an empty file at 2), "sort" reads nothing and > writes nothing. Tom: in case you're not convinced, try this: $ (echo b; echo a) >f $ sort f a b $ sort f >f $ cat f $ Also, useless ';' and missing double-quotes around "$1" to avoid bad surprises ifever test_cmp_noorder is called on file names with special characters. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison 2016-06-09 5:51 ` Matthieu Moy @ 2016-06-09 8:15 ` Tom Russello 0 siblings, 0 replies; 96+ messages in thread From: Tom Russello @ 2016-06-09 8:15 UTC (permalink / raw) To: Matthieu Moy, Junio C Hamano Cc: git, erwan.mathoniere, samuel.groot, jordan.de-gea, e, aaron On 06/09/16 07:51, Matthieu Moy wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> Tom Russello <tom.russello@grenoble-inp.org> writes: >> >>> +# Check if two files have the same content, non-order sensitive >>> +test_cmp_noorder () { >>> + sort $1 >$1; >> >> Here is what I think happens: >> >> 0) the shell parses this command line; >> 1) the shell notices that the output has to go to $1; >> 2) the shell does open(2) of $1, >> 3) the shell spawns "sort" with a single argument, with its >> output connected to the file descriptor obtained in 2). >> >> Because "$1" becomes an empty file at 2), "sort" reads nothing and >> writes nothing. > > Tom: in case you're not convinced, try this: > > $ (echo b; echo a) >f > $ sort f > a > b > $ sort f >f > $ cat f > $ > > Also, useless ';' and missing double-quotes around "$1" to avoid bad > surprises ifever test_cmp_noorder is called on file names with special > characters. I was totally convinced by Junio's explanation, it is partially fixed in v4 and will be entirely fixed in v5. Thanks. ^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v3 2/6] t9001: check email address is in Cc: field 2016-06-07 14:01 ` [PATCH v3 0/6] send-email: cleaner tests and quote email Tom Russello 2016-06-07 14:01 ` [PATCH v3 1/6] t9001: non order-sensitive file comparison Tom Russello @ 2016-06-07 14:01 ` Tom Russello 2016-06-09 5:55 ` Matthieu Moy 2016-06-07 14:01 ` [PATCH v3 3/6] t9001: shorten send-email's output Tom Russello ` (2 subsequent siblings) 4 siblings, 1 reply; 96+ messages in thread From: Tom Russello @ 2016-06-07 14:01 UTC (permalink / raw) To: git Cc: erwan.mathoniere, samuel.groot, jordan.de-gea, matthieu.moy, e, aaron, gitster, Tom Russello, Tom RUSSELLO Check if the given utf-8 email address is in the Cc: field. Signed-off-by: Tom RUSSELLO <tom.ressullo@grenoble-inp.org> Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> --- t/t9001-send-email.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 4558e0f..7fdc876 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -888,8 +888,8 @@ test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' ' --to=nobody@example.com \ --smtp-server="$(pwd)/fake.sendmail" \ outdir/*.patch && - grep "^ " msgtxt1 | - grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>" + cc_adr=$(awk "/^Cc: /{flag=1}/^Subject: /{flag=0} flag {print}" msgtxt1) && + echo "$cc_adr" | grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>" ' test_expect_success $PREREQ '--compose adds MIME for utf8 body' ' -- 2.8.3 ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v3 2/6] t9001: check email address is in Cc: field 2016-06-07 14:01 ` [PATCH v3 2/6] t9001: check email address is in Cc: field Tom Russello @ 2016-06-09 5:55 ` Matthieu Moy 2016-06-13 22:23 ` Samuel GROOT 0 siblings, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-06-09 5:55 UTC (permalink / raw) To: Tom Russello Cc: git, erwan.mathoniere, samuel.groot, jordan.de-gea, e, aaron, gitster, Tom RUSSELLO Tom Russello <tom.russello@grenoble-inp.org> writes: > Check if the given utf-8 email address is in the Cc: field. I wouldn't harm to explain what was the problem with existing code here. If I understand correctly, that would be: Existing code just checked that the address appeared in a line starting with a space, but not to which field the address belonged. But probably the real motivation for this was that you want to introduce code that changes the layout and breaks this "address appears on a line starting with space"? > --- a/t/t9001-send-email.sh > +++ b/t/t9001-send-email.sh > @@ -888,8 +888,8 @@ test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' ' > --to=nobody@example.com \ > --smtp-server="$(pwd)/fake.sendmail" \ > outdir/*.patch && > - grep "^ " msgtxt1 | > - grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>" > + cc_adr=$(awk "/^Cc: /{flag=1}/^Subject: /{flag=0} flag {print}" msgtxt1) && > + echo "$cc_adr" | grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>" > ' > > test_expect_success $PREREQ '--compose adds MIME for utf8 body' ' -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v3 2/6] t9001: check email address is in Cc: field 2016-06-09 5:55 ` Matthieu Moy @ 2016-06-13 22:23 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-13 22:23 UTC (permalink / raw) To: Matthieu Moy, Tom Russello Cc: git, erwan.mathoniere, jordan.de-gea, e, aaron, gitster, Tom RUSSELLO On 06/09/2016 07:55 AM, Matthieu Moy wrote: > Tom Russello <tom.russello@grenoble-inp.org> writes: > >> Check if the given utf-8 email address is in the Cc: field. > > I wouldn't harm to explain what was the problem with existing code here. > If I understand correctly, that would be: > > Existing code just checked that the address appeared in a line starting > with a space, but not to which field the address belonged. > > But probably the real motivation for this was that you want to introduce > code that changes the layout and breaks this "address appears on a line > starting with space"? Actually it was both, we wanted to make the tests less dependent to how data was displayed. ^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v3 3/6] t9001: shorten send-email's output 2016-06-07 14:01 ` [PATCH v3 0/6] send-email: cleaner tests and quote email Tom Russello 2016-06-07 14:01 ` [PATCH v3 1/6] t9001: non order-sensitive file comparison Tom Russello 2016-06-07 14:01 ` [PATCH v3 2/6] t9001: check email address is in Cc: field Tom Russello @ 2016-06-07 14:01 ` Tom Russello 2016-06-08 8:36 ` Eric Wong 2016-06-09 6:03 ` Matthieu Moy 2016-06-07 14:01 ` [PATCH v3 4/6] send-email: create email parser subroutine Tom Russello 2016-06-08 13:01 ` (unknown), Samuel GROOT 4 siblings, 2 replies; 96+ messages in thread From: Tom Russello @ 2016-06-07 14:01 UTC (permalink / raw) To: git Cc: erwan.mathoniere, samuel.groot, jordan.de-gea, matthieu.moy, e, aaron, gitster, Tom RUSSELLO Messages displayed by `send-email` should be shortened to avoid displaying unnecesseray informations. Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> Signed-off-by: Tom RUSSELLO <tom.russello@grenoble-inp.org> Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> --- git-send-email.perl | 22 +++++++++---------- t/t9001-send-email.sh | 58 +++++++++++++++++++++++++-------------------------- 2 files changed, 40 insertions(+), 40 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index 6958785..4822f41 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1478,14 +1478,14 @@ foreach my $t (@files) { $sauthor = sanitize_address($author); next if $suppress_cc{'author'}; next if $suppress_cc{'self'} and $sauthor eq $sender; - printf("(mbox) Adding cc: %s from line '%s'\n", - $1, $_) unless $quiet; + printf("Adding cc: %s from From: header\n", + $1) unless $quiet; push @cc, $1; } elsif (/^To:\s+(.*)$/i) { foreach my $addr (parse_address_line($1)) { - printf("(mbox) Adding to: %s from line '%s'\n", - $addr, $_) unless $quiet; + printf("Adding to: %s from To: header\n", + $addr) unless $quiet; push @to, $addr; } } @@ -1498,8 +1498,8 @@ foreach my $t (@files) { } else { next if ($suppress_cc{'cc'}); } - printf("(mbox) Adding cc: %s from line '%s'\n", - $addr, $_) unless $quiet; + printf("Adding cc: %s from Cc: header\n", + $addr) unless $quiet; push @cc, $addr; } } @@ -1532,8 +1532,8 @@ foreach my $t (@files) { # So let's support that, too. $input_format = 'lots'; if (@cc == 0 && !$suppress_cc{'cc'}) { - printf("(non-mbox) Adding cc: %s from line '%s'\n", - $_, $_) unless $quiet; + printf("Adding cc: %s from Cc: header\n", + $_) unless $quiet; push @cc, $_; } elsif (!defined $subject) { $subject = $_; @@ -1555,8 +1555,8 @@ foreach my $t (@files) { next if $suppress_cc{'bodycc'} and $what =~ /Cc/i; } push @cc, $c; - printf("(body) Adding cc: %s from line '%s'\n", - $c, $_) unless $quiet; + printf("Adding cc: %s from Signed-off-by trailer\n", + $c) unless $quiet; } } close $fh; @@ -1660,7 +1660,7 @@ sub recipients_cmd { $address = sanitize_address($address); next if ($address eq $sender and $suppress_cc{'self'}); push @addresses, $address; - printf("($prefix) Adding %s: %s from: '%s'\n", + printf("Adding %s: %s from: '%s'\n", $what, $address, $cmd) unless $quiet; } close $fh diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 7fdc876..9b1e56f 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -150,9 +150,9 @@ test_expect_success $PREREQ 'Verify commandline' ' test_expect_success $PREREQ 'setup expect' " cat >expected-show-all-headers <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -503,9 +503,9 @@ test_expect_success $PREREQ 'second message is patch' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-sob <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -555,9 +555,9 @@ test_expect_success $PREREQ 'sendemail.cc set' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-sob <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -587,10 +587,10 @@ test_expect_success $PREREQ 'sendemail.cc unset' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-cccmd <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' -(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header +Adding cc: C O Mitter <committer@example.com> from Signed-off-by: trailer Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -647,10 +647,10 @@ test_expect_success $PREREQ '--suppress-cc=all' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-body <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' -(cc-cmd) Adding cc: cc-cmd@example.com from: './cccmd' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header +Adding cc: cc-cmd@example.com from: './cccmd' Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -681,9 +681,9 @@ test_expect_success $PREREQ '--suppress-cc=body' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-body-cccmd <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -712,9 +712,9 @@ test_expect_success $PREREQ '--suppress-cc=body --suppress-cc=cccmd' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-sob <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -744,10 +744,10 @@ test_expect_success $PREREQ '--suppress-cc=sob' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-bodycc <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' -(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header +Adding cc: C O Mitter <committer@example.com> from Signed-off-by: trailer Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -778,8 +778,8 @@ test_expect_success $PREREQ '--suppress-cc=bodycc' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-cc <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>' +Adding cc: A <author@example.com> from From: header +Adding cc: C O Mitter <committer@example.com> from Signed-off-by: trailer Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> -- 2.8.3 ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v3 3/6] t9001: shorten send-email's output 2016-06-07 14:01 ` [PATCH v3 3/6] t9001: shorten send-email's output Tom Russello @ 2016-06-08 8:36 ` Eric Wong 2016-06-08 9:30 ` Samuel GROOT 2016-06-09 6:03 ` Matthieu Moy 1 sibling, 1 reply; 96+ messages in thread From: Eric Wong @ 2016-06-08 8:36 UTC (permalink / raw) To: Tom Russello Cc: git, erwan.mathoniere, samuel.groot, jordan.de-gea, matthieu.moy, aaron, gitster Tom Russello <tom.russello@grenoble-inp.org> wrote: > Messages displayed by `send-email` should be shortened to avoid displaying > unnecesseray informations. unnecessary information. In some of your other patches, the 'grep' can probably be better replaced by 'fgrep' for fixed strings. Otherwise, the '.' in the 'example.com' would match any character instead of the intended dot. Thanks. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v3 3/6] t9001: shorten send-email's output 2016-06-08 8:36 ` Eric Wong @ 2016-06-08 9:30 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 9:30 UTC (permalink / raw) To: Eric Wong, Tom Russello Cc: git, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, gitster On 06/08/2016 10:36 AM, Eric Wong wrote: > Tom Russello <tom.russello@grenoble-inp.org> wrote: >> Messages displayed by `send-email` should be shortened to avoid displaying >> unnecesseray informations. > > unnecessary information. > > In some of your other patches, the 'grep' can probably > be better replaced by 'fgrep' for fixed strings. > Otherwise, the '.' in the 'example.com' would match any > character instead of the intended dot. Thanks, will re-roll ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v3 3/6] t9001: shorten send-email's output 2016-06-07 14:01 ` [PATCH v3 3/6] t9001: shorten send-email's output Tom Russello 2016-06-08 8:36 ` Eric Wong @ 2016-06-09 6:03 ` Matthieu Moy 1 sibling, 0 replies; 96+ messages in thread From: Matthieu Moy @ 2016-06-09 6:03 UTC (permalink / raw) To: Tom Russello Cc: git, erwan.mathoniere, samuel.groot, jordan.de-gea, e, aaron, gitster Tom Russello <tom.russello@grenoble-inp.org> writes: > Messages displayed by `send-email` should be shortened to avoid displaying > unnecesseray informations. We usually use imperative tone in commit messages: Shorten messages displayed by `send-email` to avoid displaying unnecessary information. Actually, I'd rather have a bit more explanation about why this info is "unnecessary". -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v3 4/6] send-email: create email parser subroutine 2016-06-07 14:01 ` [PATCH v3 0/6] send-email: cleaner tests and quote email Tom Russello ` (2 preceding siblings ...) 2016-06-07 14:01 ` [PATCH v3 3/6] t9001: shorten send-email's output Tom Russello @ 2016-06-07 14:01 ` Tom Russello 2016-06-07 14:05 ` [PATCH v3 5/6] send-email: --in-reply-to=<file> populates the fields Tom Russello 2016-06-08 13:01 ` (unknown), Samuel GROOT 4 siblings, 1 reply; 96+ messages in thread From: Tom Russello @ 2016-06-07 14:01 UTC (permalink / raw) To: git Cc: erwan.mathoniere, samuel.groot, jordan.de-gea, matthieu.moy, e, aaron, gitster, Tom RUSSELLO We need a simple and generic way to parse an email file. Since it would be hard to include and maintain an external library, create an simple email parser subroutine to parse an email file. Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> Signed-off-by: Tom RUSSELLO <tom.russello@grenoble-inp.org> Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> --- We chose to create our own simple email parser and only use it for the "quote email" feature to pave the way for the refactorization of the patch parser [0] that may come after our current school project. [0] * http://thread.gmane.org/gmane.comp.version-control.git/295752 git-send-email.perl | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/git-send-email.perl b/git-send-email.perl index 4822f41..db114ae 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1750,3 +1750,31 @@ sub body_or_subject_has_nonascii { } return 0; } + +sub parse_email { + my %mail = (); + my $fh = shift; + my $last_header; + + # Unfold and parse multiline header fields + while (<$fh>) { + last if /^\s*$/; + s/\r\n|\n|\r//; + if (/^([^\s:]+):[\s]+(.*)$/) { + $last_header = lc($1); + @{$mail{$last_header}} = () + unless defined $mail{$last_header}; + push @{$mail{$last_header}}, $2; + } elsif (/^\s+\S/ and defined $last_header) { + s/^\s+/ /; + push @{$mail{$last_header}}, $_; + } else { + die("Mail format undefined !\n"); + } + } + + # Separate body from header + $mail{"body"} = [(<$fh>)]; + + return \%mail; +} -- 2.8.3 ^ permalink raw reply related [flat|nested] 96+ messages in thread
* [PATCH v3 5/6] send-email: --in-reply-to=<file> populates the fields 2016-06-07 14:01 ` [PATCH v3 4/6] send-email: create email parser subroutine Tom Russello @ 2016-06-07 14:05 ` Tom Russello 2016-06-07 14:05 ` [PATCH v3 6/6] send-email: add option --cite to quote the message body Tom Russello 0 siblings, 1 reply; 96+ messages in thread From: Tom Russello @ 2016-06-07 14:05 UTC (permalink / raw) To: git Cc: erwan.mathoniere, samuel.groot, jordan.de-gea, matthieu.moy, e, aaron, gitster, Tom Russello Take an email message file, parse it and fill the "From", "To", "Cc", "In-reply-to", "References" fields appropriately. If `--compose` option is set, it will also fill the subject field with `Re: [<email_file>'s subject]` in the introductory message. Signed-off-by: Tom RUSSELLO <tom.russello@grenoble-inp.org> Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> --- Check if the string given by argument with `--in-reply-to` leads to an existing plain text file. If not, consider it as a message-id. Changes sinces v2: - Fill the References: field to keep the thread even if some emails have been removed - Explicit error with a proper "if" when an error occured during email file opening - More precise comments - More tests Documentation/git-send-email.txt | 9 +++-- git-send-email.perl | 49 +++++++++++++++++++++++- t/t9001-send-email.sh | 83 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 5 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index edbba3a..21776f0 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -84,13 +84,16 @@ See the CONFIGURATION section for 'sendemail.multiEdit'. the value of GIT_AUTHOR_IDENT, or GIT_COMMITTER_IDENT if that is not set, as returned by "git var -l". ---in-reply-to=<identifier>:: +--in-reply-to=<Message-Id|email_file>:: Make the first mail (or all the mails with `--no-thread`) appear as a - reply to the given Message-Id, which avoids breaking threads to - provide a new patch series. + reply to the given Message-Id (given directly by argument or via the email + file), which avoids breaking threads to provide a new patch series. The second and subsequent emails will be sent as replies according to the `--[no]-chain-reply-to` setting. + +Furthermore, if the argument is an email file, parse it and populate header +fields appropriately for the reply. ++ So for example when `--thread` and `--no-chain-reply-to` are specified, the second and subsequent patches will be replies to the first one like in the illustration below where `[PATCH v2 0/3]` is in reply to `[PATCH 0/2]`: diff --git a/git-send-email.perl b/git-send-email.perl index db114ae..66aa2cd 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -55,6 +55,7 @@ git send-email --dump-aliases --[no-]bcc <str> * Email Bcc: --subject <str> * Email "Subject:" --in-reply-to <str> * Email "In-Reply-To:" + --in-reply-to <file> * Populate header fields appropriately. --[no-]xmailer * Add "X-Mailer:" header (default). --[no-]annotate * Review each patch that will be sent in an editor. --compose * Open an editor for introduction. @@ -160,7 +161,7 @@ my $re_encoded_word = qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/; # Variables we fill in automatically, or via prompting: my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh, - $initial_reply_to,$initial_subject,@files, + $initial_reply_to,$initial_references,$initial_subject,@files, $author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time); my $envelope_sender; @@ -639,6 +640,50 @@ if (@files) { usage(); } +if ($initial_reply_to && -f $initial_reply_to) { + my $error = validate_patch($initial_reply_to); + die "fatal: $initial_reply_to: $error\nwarning: no patches were sent\n" + if $error; + + open my $fh, "<", $initial_reply_to or die "can't open file $initial_reply_to"; + my $mail = parse_email($fh); + close $fh; + + my $initial_sender = $sender || $repoauthor || $repocommitter || ''; + + my $prefix_re = ""; + my $subject_re = $mail->{"subject"}[0]; + if ($subject_re =~ /^[^Re:]/) { + $prefix_re = "Re: "; + } + $initial_subject = $prefix_re . $subject_re; + + push @initial_to, $mail->{"from"}[0]; + + foreach my $to_addr (parse_address_line(join ",", @{$mail->{"to"}})) { + if (!($to_addr eq $initial_sender)) { + push @initial_cc, $to_addr; + } + } + + foreach my $cc_addr (parse_address_line(join ",", @{$mail->{"cc"}})) { + my $qaddr = unquote_rfc2047($cc_addr); + my $saddr = sanitize_address($qaddr); + if ($saddr eq $initial_sender) { + next if ($suppress_cc{'self'}); + } else { + next if ($suppress_cc{'cc'}); + } + push @initial_cc, $cc_addr; + } + + $initial_reply_to = $mail->{"message-id"}[0]; + if ($mail->{"references"}) { + $initial_references = join("", @{$mail->{"references"}}) . + " " . $initial_reply_to; + } +} + sub get_patch_subject { my $fn = shift; open (my $fh, '<', $fn); @@ -1426,7 +1471,7 @@ Message-Id: $message_id } $reply_to = $initial_reply_to; -$references = $initial_reply_to || ''; +$references = $initial_references || $initial_reply_to || ''; $subject = $initial_subject; $message_num = 0; diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 9b1e56f..2d67f6d 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -1892,4 +1892,87 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' ' test_cmp_noorder expected-list actual-list ' +test_expect_success $PREREQ 'setup expect' ' + cat >email <<-\EOF + Subject: subject goes here + From: author@example.com + To: to1@example.com + Cc: cc1@example.com, cc2@example.com, + cc3@example.com + Date: Sat, 12 Jun 2010 15:53:58 +0200 + Message-Id: <author_123456@example.com> + References: <firstauthor_654321@example.com> + <secondauthor_01546567@example.com> + <thirdauthor_1395838@example.com> + + Have you seen my previous email? + > Previous content + EOF +' + +test_expect_success $PREREQ 'Fields with --in-reply-to are correct' ' + clean_fake_sendmail && + git send-email \ + --in-reply-to=email \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -2 \ + 2>errors && + grep "From: Example <nobody@example.com>" msgtxt1 && + grep "In-Reply-To: <author_123456@example.com>" msgtxt1 && + to_adr=$(awk "/^To: /{flag=1}/^Cc: /{flag=0} flag {print}" msgtxt1) && + cc_adr=$(awk "/^Cc: /{flag=1}/^Subject: /{flag=0} flag {print}" msgtxt1) && + ref_adr=$(awk "/^References: /{flag=1}/^MIME-Version: /{flag=0} flag {print}" \ + msgtxt1) && + echo "$to_adr" | grep author@example.com && + echo "$cc_adr" | grep to1@example.com && + echo "$cc_adr" | grep cc1@example.com && + echo "$cc_adr" | grep cc2@example.com && + echo "$cc_adr" | grep cc3@example.com && + echo "$ref_adr" | grep "<firstauthor_654321@example.com>" && + echo "$ref_adr" | grep "<secondauthor_01546567@example.com>" && + echo "$ref_adr" | grep "<thirdauthor_1395838@example.com>" && + echo "$ref_adr" | grep "<author_123456@example.com>" && + echo "$ref_adr" | grep -v "References: <author_123456@example.com>" +' + +test_expect_success $PREREQ 'Fields with --in-reply-to and --compose are correct' ' + clean_fake_sendmail && + git send-email \ + --in-reply-to=email \ + --compose \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -1 \ + 2>errors && + grep "From: Example <nobody@example.com>" msgtxt1 && + grep "In-Reply-To: <author_123456@example.com>" msgtxt1 && + grep "Subject: Re: subject goes here" msgtxt1 && + to_adr=$(awk "/^To: /{flag=1}/^Cc: /{flag=0} flag {print}" msgtxt1) && + cc_adr=$(awk "/^Cc: /{flag=1}/^Subject: /{flag=0} flag {print}" msgtxt1) && + ref_adr=$(awk "/^References: /{flag=1}/^MIME-Version: /{flag=0} flag {print}" \ + msgtxt1) && + echo "$to_adr" | grep author@example.com && + echo "$cc_adr" | grep to1@example.com && + echo "$cc_adr" | grep cc1@example.com && + echo "$cc_adr" | grep cc2@example.com && + echo "$cc_adr" | grep cc3@example.com && + echo "$ref_adr" | grep "<firstauthor_654321@example.com>" && + echo "$ref_adr" | grep "<secondauthor_01546567@example.com>" && + echo "$ref_adr" | grep "<thirdauthor_1395838@example.com>" && + echo "$ref_adr" | grep "<author_123456@example.com>" && + echo "$ref_adr" | grep -v "References: <author_123456@example.com>" +' + +test_expect_success $PREREQ 'Re: written only once with --in-reply-to and --compose ' ' + git send-email \ + --in-reply-to=msgtxt1 \ + --compose \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -1 \ + 2>errors && + grep "Subject: Re: subject goes here" msgtxt3 +' + test_done -- 2.8.3 ^ permalink raw reply related [flat|nested] 96+ messages in thread
* [PATCH v3 6/6] send-email: add option --cite to quote the message body 2016-06-07 14:05 ` [PATCH v3 5/6] send-email: --in-reply-to=<file> populates the fields Tom Russello @ 2016-06-07 14:05 ` Tom Russello 0 siblings, 0 replies; 96+ messages in thread From: Tom Russello @ 2016-06-07 14:05 UTC (permalink / raw) To: git Cc: erwan.mathoniere, samuel.groot, jordan.de-gea, matthieu.moy, e, aaron, gitster, Tom Russello If used with `in-reply-to=<email_file>`, cite the message body of the given email file. Otherwise, do nothing. If `--compose` is set, quote the message body in the cover letter. Else, imply `--annotate` by default and quote the message body below the triple-dash section in the first patch only. Signed-off-by: Tom RUSSELLO <tom.russello@grenoble-inp.org> Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> --- Documentation/git-send-email.txt | 8 ++++ git-send-email.perl | 81 ++++++++++++++++++++++++++++++++++++++-- t/t9001-send-email.sh | 32 ++++++++++++++++ 3 files changed, 117 insertions(+), 4 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 21776f0..23cbd17 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -109,6 +109,14 @@ illustration below where `[PATCH v2 0/3]` is in reply to `[PATCH 0/2]`: Only necessary if --compose is also set. If --compose is not set, this will be prompted for. +--cite:: + When used with `--in-reply-to=<email_file>`, quote the message body of the + given email file. ++ +If `--compose` is also set, the message cited will be in the cover letter. If +`--compose` is not set, `--annotate` option is implied by default and the +message body will be cited in the "below-triple-dash" section. + --subject=<string>:: Specify the initial subject of the email thread. Only necessary if --compose is also set. If --compose diff --git a/git-send-email.perl b/git-send-email.perl index 66aa2cd..03483f5 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -26,6 +26,7 @@ use Text::ParseWords; use Term::ANSIColor; use File::Temp qw/ tempdir tempfile /; use File::Spec::Functions qw(catfile); +use File::Copy; use Error qw(:try); use Git; @@ -56,6 +57,8 @@ git send-email --dump-aliases --subject <str> * Email "Subject:" --in-reply-to <str> * Email "In-Reply-To:" --in-reply-to <file> * Populate header fields appropriately. + --cite * Quote the message body in the cover if + --compose is set, else in the first patch. --[no-]xmailer * Add "X-Mailer:" header (default). --[no-]annotate * Review each patch that will be sent in an editor. --compose * Open an editor for introduction. @@ -161,7 +164,7 @@ my $re_encoded_word = qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/; # Variables we fill in automatically, or via prompting: my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh, - $initial_reply_to,$initial_references,$initial_subject,@files, + $initial_reply_to,$initial_references,$cite,$initial_subject,@files, $author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time); my $envelope_sender; @@ -305,6 +308,7 @@ $rc = GetOptions( "sender|from=s" => \$sender, "in-reply-to=s" => \$initial_reply_to, "subject=s" => \$initial_subject, + "cite" => \$cite, "to=s" => \@initial_to, "to-cmd=s" => \$to_cmd, "no-to" => \$no_to, @@ -640,6 +644,7 @@ if (@files) { usage(); } +my $message_cited; if ($initial_reply_to && -f $initial_reply_to) { my $error = validate_patch($initial_reply_to); die "fatal: $initial_reply_to: $error\nwarning: no patches were sent\n" @@ -658,7 +663,8 @@ if ($initial_reply_to && -f $initial_reply_to) { } $initial_subject = $prefix_re . $subject_re; - push @initial_to, $mail->{"from"}[0]; + my $recipient = $mail->{"from"}[0]; + push @initial_to, $recipient; foreach my $to_addr (parse_address_line(join ",", @{$mail->{"to"}})) { if (!($to_addr eq $initial_sender)) { @@ -682,6 +688,25 @@ if ($initial_reply_to && -f $initial_reply_to) { $initial_references = join("", @{$mail->{"references"}}) . " " . $initial_reply_to; } + + if ($cite) { + my $date = $mail->{"date"}[0]; + my $tpl_date = $date && "On $date, " || ''; + $message_cited = $tpl_date . $recipient . " wrote:\n"; + + # Quote the message body + foreach (@{$mail->{"body"}}) { + my $space = ""; + if (/^[^>]/) { + $space = " "; + } + $message_cited .= ">" . $space . $_; + } + + if (!$compose) { + $annotate = 1; + } + } } sub get_patch_subject { @@ -709,6 +734,9 @@ if ($compose) { my $tpl_sender = $sender || $repoauthor || $repocommitter || ''; my $tpl_subject = $initial_subject || ''; my $tpl_reply_to = $initial_reply_to || ''; + my $tpl_quote = $message_cited && + "\nGIT: Please, trim down irrelevant sections in the cited message\n". + "GIT: to keep your email concise.\n" . $message_cited || ''; print $c <<EOT; From $tpl_sender # This line is ignored. @@ -720,7 +748,7 @@ GIT: Clear the body content if you don't wish to send a summary. From: $tpl_sender Subject: $tpl_subject In-Reply-To: $tpl_reply_to - +$tpl_quote EOT for my $f (@files) { print $c get_patch_subject($f); @@ -785,7 +813,52 @@ EOT $compose = -1; } } elsif ($annotate) { - do_edit(@files); + if ($message_cited) { + my $cite_email_filename = ($repo ? + tempfile(".gitsendemail.msg.XXXXXX", + DIR => $repo->repo_path()) : + tempfile(".gitsendemail.msg.XXXXXX", + DIR => "."))[1]; + + # Insertion in a temporary file to keep the original file clean + # in case of cancellation/error. + do_insert_cited_message($cite_email_filename, $files[0]); + + my $tmp = $files[0]; + $files[0] = $cite_email_filename; + + do_edit(@files); + + # Erase the original patch if the edition went well + move($cite_email_filename, $tmp); + $files[0] = $tmp; + } else { + do_edit(@files); + } +} + +sub do_insert_cited_message { + my $tmp_file = shift; + my $original_file = shift; + + open my $c, "<", $original_file + or die "Failed to open $original_file: " . $!; + + open my $c2, ">", $tmp_file + or die "Failed to open $tmp_file: " . $!; + + # Insertion after the triple-dash + while (<$c>) { + print $c2 $_; + last if (/^---$/); + } + print $c2 $message_cited; + while (<$c>) { + print $c2 $_; + } + + close $c; + close $c2; } sub ask { diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 2d67f6d..a107bde 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -1915,6 +1915,7 @@ test_expect_success $PREREQ 'Fields with --in-reply-to are correct' ' git send-email \ --in-reply-to=email \ --from="Example <nobody@example.com>" \ + --cite \ --smtp-server="$(pwd)/fake.sendmail" \ -2 \ 2>errors && @@ -1936,10 +1937,22 @@ test_expect_success $PREREQ 'Fields with --in-reply-to are correct' ' echo "$ref_adr" | grep -v "References: <author_123456@example.com>" ' +test_expect_success $PREREQ 'correct cited message with --in-reply-to' ' + msg_cited=$(grep -A 3 "^---$" msgtxt1) && + echo "$msg_cited" | grep "On Sat, 12 Jun 2010 15:53:58 +0200, author@example.com wrote:" && + echo "$msg_cited" | grep "> Have you seen my previous email?" && + echo "$msg_cited" | grep ">> Previous content" +' + +test_expect_success $PREREQ 'second patch body is not modified by --in-reply-to' ' + ! grep "Have you seen my previous email?" msgtxt2 +' + test_expect_success $PREREQ 'Fields with --in-reply-to and --compose are correct' ' clean_fake_sendmail && git send-email \ --in-reply-to=email \ + --cite \ --compose \ --from="Example <nobody@example.com>" \ --smtp-server="$(pwd)/fake.sendmail" \ @@ -1967,6 +1980,7 @@ test_expect_success $PREREQ 'Fields with --in-reply-to and --compose are correct test_expect_success $PREREQ 'Re: written only once with --in-reply-to and --compose ' ' git send-email \ --in-reply-to=msgtxt1 \ + --cite \ --compose \ --from="Example <nobody@example.com>" \ --smtp-server="$(pwd)/fake.sendmail" \ @@ -1975,4 +1989,22 @@ test_expect_success $PREREQ 'Re: written only once with --in-reply-to and --comp grep "Subject: Re: subject goes here" msgtxt3 ' +test_expect_success $PREREQ 'correct cited message with --in-reply-to and --compose' ' + grep "> On Sat, 12 Jun 2010 15:53:58 +0200, author@example.com wrote:" msgtxt3 && + grep ">> Have you seen my previous email?" msgtxt3 && + grep ">>> Previous content" msgtxt3 +' + +test_expect_success $PREREQ 'Message is not cited with only --in-reply-to' ' + clean_fake_sendmail && + git send-email \ + --in-reply-to=email \ + --compose \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -1 \ + 2>errors && + ! grep "Have you seen my previous email?" msgtxt1 +' + test_done -- 2.8.3 ^ permalink raw reply related [flat|nested] 96+ messages in thread
* (unknown), 2016-06-07 14:01 ` [PATCH v3 0/6] send-email: cleaner tests and quote email Tom Russello ` (3 preceding siblings ...) 2016-06-07 14:01 ` [PATCH v3 4/6] send-email: create email parser subroutine Tom Russello @ 2016-06-08 13:01 ` Samuel GROOT 2016-06-08 13:01 ` [PATCH v4 1/6] t9001: non order-sensitive file comparison Samuel GROOT ` (5 more replies) 4 siblings, 6 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 13:01 UTC (permalink / raw) To: git Cc: samuel.groot, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, gitster, aaron, e The purpose of this series of patches is to implement a new "quote-email" feature integrated in the current `--in-reply-to` option. * The first 2 patches make the tests less dependent to `git send-email`'s exact output. * Third patch makes `git send-email` a bit less verbose. * Fourth patch introduces our email parser subroutine. * Fifth patch makes the `--in-reply-to` open a email file (if it exists) and populates From:, To:, Cc:, In-reply-to and References: fields. * Sixth patch quotes the message body in the cover letter if `--compose` is set. Else, imply `--annotate` and insert quoted message body below triple-dash in the first patch. Changes since v3: - test_cmp_noorder shell function fixed (patch 1/6) - use fgrep instead of grep (patch 2/6) - typo fixed (patch 3/6) - email parser subroutine moved to Git.pm library (patch 4/6) - test if $mail->{"cc"} is defined (patch 5/6) [PATCH v4 1/6] t9001: non order-sensitive file comparison [PATCH v4 2/6] t9001: check email address is in Cc: field [PATCH v4 3/6] send-email: shorten send-email's output [PATCH v4 4/6] send-email: create email parser subroutine [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header [PATCH v4 6/6] send-email: add option --cite to quote the message Documentation/git-send-email.txt | 17 +++++- git-send-email.perl | 150 ++++++++++++++++++++++++++++++++++++++++++++----- perl/Git.pm | 34 ++++++++++++ t/t9001-send-email.sh | 190 +++++++++++++++++++++++++++++++++++++++++++++++++++------------ 4 files changed, 339 insertions(+), 52 deletions(-) ^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v4 1/6] t9001: non order-sensitive file comparison 2016-06-08 13:01 ` (unknown), Samuel GROOT @ 2016-06-08 13:01 ` Samuel GROOT 2016-06-08 14:22 ` Remi Galan Alfonso ` (2 more replies) 2016-06-08 13:01 ` [PATCH v4 2/6] t9001: check email address is in Cc: field Samuel GROOT ` (4 subsequent siblings) 5 siblings, 3 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 13:01 UTC (permalink / raw) To: git Cc: samuel.groot, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, gitster, aaron, e Tests might fail if lines compared in text files don't have the same order. Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> Signed-off-by: Tom RUSSELLO <tom.russello@grenoble-inp.org> Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> --- t/t9001-send-email.sh | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index b3355d2..56ad8ce 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -39,6 +39,13 @@ test_expect_success $PREREQ 'Extract patches' ' patches=$(git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1) ' +# Check if two files have the same content, non-order sensitive +test_cmp_noorder () { + sort "$1" >"$1_noorder" + sort "$2" >"$2_noorder" + test_cmp $1 $2 +} + # Test no confirm early to ensure remaining tests will not hang test_no_confirm () { rm -f no_confirm_okay @@ -97,7 +104,7 @@ test_expect_success $PREREQ 'setup expect' ' ' test_expect_success $PREREQ 'Verify commandline' ' - test_cmp expected commandline1 + test_cmp_noorder expected commandline1 ' test_expect_success $PREREQ 'Send patches with --envelope-sender' ' @@ -117,7 +124,7 @@ test_expect_success $PREREQ 'setup expect' ' ' test_expect_success $PREREQ 'Verify commandline' ' - test_cmp expected commandline1 + test_cmp_noorder expected commandline1 ' test_expect_success $PREREQ 'Send patches with --envelope-sender=auto' ' @@ -137,7 +144,7 @@ test_expect_success $PREREQ 'setup expect' ' ' test_expect_success $PREREQ 'Verify commandline' ' - test_cmp expected commandline1 + test_cmp_noorder expected commandline1 ' test_expect_success $PREREQ 'setup expect' " -- 2.8.2.537.gb153d2a ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison 2016-06-08 13:01 ` [PATCH v4 1/6] t9001: non order-sensitive file comparison Samuel GROOT @ 2016-06-08 14:22 ` Remi Galan Alfonso 2016-06-08 14:29 ` Samuel GROOT 2016-06-08 16:56 ` Junio C Hamano 2016-06-08 17:17 ` Junio C Hamano 2 siblings, 1 reply; 96+ messages in thread From: Remi Galan Alfonso @ 2016-06-08 14:22 UTC (permalink / raw) To: Samuel GROOT Cc: git, tom russello, erwan mathoniere, jordan de-gea, matthieu moy, gitster, aaron, e Hi Samuel, Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > +test_cmp_noorder () { > + sort "$1" >"$1_noorder" > + sort "$2" >"$2_noorder" > + test_cmp $1 $2 You meant `test_cmp "$1_noorder" "$2_noorder"`, I guess. Thanks, Rémi ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison 2016-06-08 14:22 ` Remi Galan Alfonso @ 2016-06-08 14:29 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 14:29 UTC (permalink / raw) To: Remi Galan Alfonso Cc: git, tom russello, erwan mathoniere, jordan de-gea, matthieu moy, gitster, aaron, e On 06/08/2016 04:22 PM, Remi Galan Alfonso wrote: >> +test_cmp_noorder () { >> + sort "$1" >"$1_noorder" >> + sort "$2" >"$2_noorder" >> + test_cmp $1 $2 > > You meant `test_cmp "$1_noorder" "$2_noorder"`, I guess. Yes, thanks for pointing it out! ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison 2016-06-08 13:01 ` [PATCH v4 1/6] t9001: non order-sensitive file comparison Samuel GROOT 2016-06-08 14:22 ` Remi Galan Alfonso @ 2016-06-08 16:56 ` Junio C Hamano 2016-06-08 19:21 ` Samuel GROOT 2016-06-08 17:17 ` Junio C Hamano 2 siblings, 1 reply; 96+ messages in thread From: Junio C Hamano @ 2016-06-08 16:56 UTC (permalink / raw) To: Samuel GROOT Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > @@ -117,7 +124,7 @@ test_expect_success $PREREQ 'setup expect' ' > ' > > test_expect_success $PREREQ 'Verify commandline' ' > - test_cmp expected commandline1 > + test_cmp_noorder expected commandline1 > ' > > test_expect_success $PREREQ 'Send patches with --envelope-sender=auto' ' I think this comment applies to all the other hunk in this patch (I didn't check very carefully though), but this is trying to see if the command line arguments that drives send-email are like this (one arg per line, enclosed in !! pairs for clarity): !patch@example.com! !-i! !nobody@example.com! !author@example.com! !one@example.com! !two@example.com! when these addresses are given from the command line: git send-email \ --envelope-sender="Patch Contributor <patch@example.com>" \ --suppress-cc=sob \ --from="Example <nobody@example.com>" \ --to=nobody@example.com \ --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors that creates something like $TRASH_DIRECTORY/fake.sendmail -f patch@example.com -i \ nobody@example.com author@example.com one@example.com two@example.com (all on a single line). The earliest address patch@example.com and later addresses have quite different meaning (the first one is meant to be the envelope sender address, and does not name a recipient). While I think it is a good idea to tell the test that the order of recipient addresses given to the sendmail command (i.e. nobody, author, one and two) does not matter by comparing sorted list of addresses, sorting the whole argument list and comparing is making the test _too_ loose. Don't you want to catch a potential bug that adds the envelope sender address to the list of recipients by mistake, for example? ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison 2016-06-08 16:56 ` Junio C Hamano @ 2016-06-08 19:21 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 19:21 UTC (permalink / raw) To: Junio C Hamano Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e On 06/08/2016 06:56 PM, Junio C Hamano wrote: > The earliest address patch@example.com and later addresses have > quite different meaning (the first one is meant to be the envelope > sender address, and does not name a recipient). While I think it is > a good idea to tell the test that the order of recipient addresses > given to the sendmail command (i.e. nobody, author, one and two) > does not matter by comparing sorted list of addresses, sorting the > whole argument list and comparing is making the test _too_ loose. > Don't you want to catch a potential bug that adds the envelope > sender address to the list of recipients by mistake, for example? That could be an idea to make a more precise test. But I rather focus on our `--quote-email` option, I will put this one aside for now. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison 2016-06-08 13:01 ` [PATCH v4 1/6] t9001: non order-sensitive file comparison Samuel GROOT 2016-06-08 14:22 ` Remi Galan Alfonso 2016-06-08 16:56 ` Junio C Hamano @ 2016-06-08 17:17 ` Junio C Hamano 2016-06-08 19:19 ` Samuel GROOT 2 siblings, 1 reply; 96+ messages in thread From: Junio C Hamano @ 2016-06-08 17:17 UTC (permalink / raw) To: Samuel GROOT Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > @@ -97,7 +104,7 @@ test_expect_success $PREREQ 'setup expect' ' > ' > > test_expect_success $PREREQ 'Verify commandline' ' > - test_cmp expected commandline1 > + test_cmp_noorder expected commandline1 > ' > > test_expect_success $PREREQ 'Send patches with --envelope-sender' ' By the way, don't you find it irritating to review this patch that has three hunks, all of which look like the above? You cannot easily tell which 3 among 27 instances of test_cmp are modified, because the hunks do not give useful context. This is not at all your fault, but because the existing tests are structured poorly. It separates one logical step into three pieces without a good reason. Here is an illustration of an organization that I think would be easier to read, and would result in a more readable patch when modification is made on top. The first two hunks collapse the overall "setup" steps that appear as three separate tests into a single "setup" test. The last hunk that begin at -83/+79 collapses a logically-single test that is split across three into one, and makes the order of things done in the test to (1) set an expectation, (2) execute the command and (3) compare the result with the expectation. I am not going to commit this myself, because I do not want to create conflicts with the change your topic is trying to do, and besides, almost all the remainder of the tests follow "one logical test split into three" pattern and need to be corrected before this "illustration" can become a real patch. I do not mind if you take it and complete it as a preliminary clean-up step in your series; or you can "keep it in mind, but ignore it for now", in which case this can be a "low hanging fruit" somebody else, hopefully somebody new to the development community, can use to dip their toes ;-) t/t9001-send-email.sh | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index b3355d2..858bdbe 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -6,14 +6,16 @@ test_description='git send-email' # May be altered later in the test PREREQ="PERL" -test_expect_success $PREREQ 'prepare reference tree' ' +clean_fake_sendmail () { + rm -f commandline* msgtxt* +} + +test_expect_success $PREREQ 'setup' ' echo "1A quick brown fox jumps over the" >file && echo "lazy dog" >>file && git add file && - GIT_AUTHOR_NAME="A" git commit -a -m "Initial." -' + GIT_AUTHOR_NAME="A" git commit -a -m "Initial." && -test_expect_success $PREREQ 'Setup helper tool' ' write_script fake.sendmail <<-\EOF && shift output=1 @@ -28,14 +30,8 @@ test_expect_success $PREREQ 'Setup helper tool' ' cat >"msgtxt$output" EOF git add fake.sendmail && - GIT_AUTHOR_NAME="A" git commit -a -m "Second." -' - -clean_fake_sendmail () { - rm -f commandline* msgtxt* -} + GIT_AUTHOR_NAME="A" git commit -a -m "Second." && -test_expect_success $PREREQ 'Extract patches' ' patches=$(git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1) ' @@ -83,20 +79,19 @@ test_expect_success $PREREQ 'No confirm with sendemail.confirm=never' ' check_no_confirm ' -test_expect_success $PREREQ 'Send patches' ' - git send-email --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors -' - -test_expect_success $PREREQ 'setup expect' ' - cat >expected <<-\EOF +test_expect_success $PREREQ 'with --suppress-cc=sob --from and --to' ' + cat >expected <<-\EOF && !nobody@example.com! !author@example.com! !one@example.com! !two@example.com! EOF -' -test_expect_success $PREREQ 'Verify commandline' ' + git send-email --suppress-cc=sob \ + --from="Example <nobody@example.com>" \ + --to=nobody@example.com \ + --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors && + test_cmp expected commandline1 ' ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison 2016-06-08 17:17 ` Junio C Hamano @ 2016-06-08 19:19 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 19:19 UTC (permalink / raw) To: Junio C Hamano Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e On 06/08/2016 07:17 PM, Junio C Hamano wrote: > Here is an illustration of an organization that I think would be > easier to read, and would result in a more readable patch when > modification is made on top. The first two hunks collapse the > overall "setup" steps that appear as three separate tests into a > single "setup" test. The last hunk that begin at -83/+79 collapses > a logically-single test that is split across three into one, and > makes the order of things done in the test to (1) set an > expectation, (2) execute the command and (3) compare the result with > the expectation. I totally agree. (1), (2) and (3) aren't even always in that order, some tests are very confusing. > I am not going to commit this myself, because I do not want to > create conflicts with the change your topic is trying to do, and > besides, almost all the remainder of the tests follow "one logical > test split into three" pattern and need to be corrected before this > "illustration" can become a real patch. > > I do not mind if you take it and complete it as a preliminary > clean-up step in your series; or you can "keep it in mind, but > ignore it for now", in which case this can be a "low hanging fruit" > somebody else, hopefully somebody new to the development community, > can use to dip their toes ;-) As said in my other reply, I will put this one aside for now, but t9001 definitely deserves its own cleanup patch series. ^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v4 2/6] t9001: check email address is in Cc: field 2016-06-08 13:01 ` (unknown), Samuel GROOT 2016-06-08 13:01 ` [PATCH v4 1/6] t9001: non order-sensitive file comparison Samuel GROOT @ 2016-06-08 13:01 ` Samuel GROOT 2016-06-08 17:34 ` Junio C Hamano 2016-06-08 13:01 ` [PATCH v4 3/6] send-email: shorten send-email's output Samuel GROOT ` (3 subsequent siblings) 5 siblings, 1 reply; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 13:01 UTC (permalink / raw) To: git Cc: samuel.groot, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, gitster, aaron, e Check if the given utf-8 email address is in the Cc: field. Signed-off-by: Tom RUSSELLO <tom.russello@grenoble-inp.org> Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> --- t/t9001-send-email.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 56ad8ce..943e6b7 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -888,8 +888,8 @@ test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' ' --to=nobody@example.com \ --smtp-server="$(pwd)/fake.sendmail" \ outdir/*.patch && - grep "^ " msgtxt1 | - grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>" + cc_adr=$(awk "/^Cc: /{flag=1}/^Subject: /{flag=0} flag {print}" msgtxt1) && + echo "$cc_adr" | fgrep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>" ' test_expect_success $PREREQ '--compose adds MIME for utf8 body' ' -- 2.8.2.537.gb153d2a ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v4 2/6] t9001: check email address is in Cc: field 2016-06-08 13:01 ` [PATCH v4 2/6] t9001: check email address is in Cc: field Samuel GROOT @ 2016-06-08 17:34 ` Junio C Hamano 2016-06-08 19:23 ` Samuel GROOT 0 siblings, 1 reply; 96+ messages in thread From: Junio C Hamano @ 2016-06-08 17:34 UTC (permalink / raw) To: Samuel GROOT Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > Check if the given utf-8 email address is in the Cc: field. > > Signed-off-by: Tom RUSSELLO <tom.russello@grenoble-inp.org> > Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> > Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> > --- > t/t9001-send-email.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh > index 56ad8ce..943e6b7 100755 > --- a/t/t9001-send-email.sh > +++ b/t/t9001-send-email.sh > @@ -888,8 +888,8 @@ test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' ' > --to=nobody@example.com \ > --smtp-server="$(pwd)/fake.sendmail" \ > outdir/*.patch && > - grep "^ " msgtxt1 | > - grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>" > + cc_adr=$(awk "/^Cc: /{flag=1}/^Subject: /{flag=0} flag {print}" msgtxt1) && > + echo "$cc_adr" | fgrep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>" > ' This still depends on that the output has Cc: before Subject: and there is no other header that can have an address on it. E.g. To: a@example.com Cc: b@example.com X-foo: <<whatever address you are looking for>> Subject: [PATCH] A sample patch would still say that the address is _on_ the CC: list. I do not usually do awk, but I think you should be able to avoid capturing output from it, echoing and then grepping, which is way too ugly. Perhaps you can start from something like below? #!/bin/sh awk ' BEGIN { in_cc = 0 } /^[Cc][Cc]: / { sub("^[Cc][Cc]: *", "") in_cc = 1 } /^[^ ]*:/ { in_cc = 0 } /^$/ { exit } in_cc { sub("^ *", "") sub(", *$", "") print } ' <<\EOF To: a@example.com Cc: b@example.com, c@example.com, d@example.com X-foo: e@example.com Subject: [PATCH] A sample patch Cc: foo@example.com EOF ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 2/6] t9001: check email address is in Cc: field 2016-06-08 17:34 ` Junio C Hamano @ 2016-06-08 19:23 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 19:23 UTC (permalink / raw) To: Junio C Hamano Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e On 06/08/2016 07:34 PM, Junio C Hamano wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: >> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh >> index 56ad8ce..943e6b7 100755 >> --- a/t/t9001-send-email.sh >> +++ b/t/t9001-send-email.sh >> @@ -888,8 +888,8 @@ test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' ' >> --to=nobody@example.com \ >> --smtp-server="$(pwd)/fake.sendmail" \ >> outdir/*.patch && >> - grep "^ " msgtxt1 | >> - grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>" >> + cc_adr=$(awk "/^Cc: /{flag=1}/^Subject: /{flag=0} flag {print}" msgtxt1) && >> + echo "$cc_adr" | fgrep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>" >> ' > > This still depends on that the output has Cc: before Subject: and > there is no other header that can have an address on it. E.g. > > To: a@example.com > Cc: b@example.com > X-foo: <<whatever address you are looking for>> > Subject: [PATCH] A sample patch > > would still say that the address is _on_ the CC: list. We thought of that but did not find the proper way to do it. > I do not usually do awk, but I think you should be able to avoid > capturing output from it, echoing and then grepping, which is way > too ugly. Perhaps you can start from something like below? > > #!/bin/sh > awk ' > BEGIN { in_cc = 0 } > /^[Cc][Cc]: / { > sub("^[Cc][Cc]: *", "") > in_cc = 1 > } > /^[^ ]*:/ { > in_cc = 0 > } > /^$/ { exit } > in_cc { > sub("^ *", "") > sub(", *$", "") > print > } > ' <<\EOF > To: a@example.com > Cc: b@example.com, > c@example.com, > d@example.com > X-foo: e@example.com > Subject: [PATCH] A sample patch > > Cc: foo@example.com > EOF Thanks, I will work on that :-) ^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v4 3/6] send-email: shorten send-email's output 2016-06-08 13:01 ` (unknown), Samuel GROOT 2016-06-08 13:01 ` [PATCH v4 1/6] t9001: non order-sensitive file comparison Samuel GROOT 2016-06-08 13:01 ` [PATCH v4 2/6] t9001: check email address is in Cc: field Samuel GROOT @ 2016-06-08 13:01 ` Samuel GROOT 2016-06-08 17:37 ` Junio C Hamano 2016-06-09 6:17 ` Matthieu Moy 2016-06-08 13:01 ` [PATCH v4 4/6] send-email: create email parser subroutine Samuel GROOT ` (2 subsequent siblings) 5 siblings, 2 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 13:01 UTC (permalink / raw) To: git Cc: samuel.groot, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, gitster, aaron, e Messages displayed by `send-email` should be shortened to avoid displaying unnecessary information. Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> Signed-off-by: Tom RUSSELLO <tom.russello@grenoble-inp.org> Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> --- git-send-email.perl | 22 +++++++++---------- t/t9001-send-email.sh | 58 +++++++++++++++++++++++++-------------------------- 2 files changed, 40 insertions(+), 40 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index 6958785..9b51062 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1478,14 +1478,14 @@ foreach my $t (@files) { $sauthor = sanitize_address($author); next if $suppress_cc{'author'}; next if $suppress_cc{'self'} and $sauthor eq $sender; - printf("(mbox) Adding cc: %s from line '%s'\n", - $1, $_) unless $quiet; + printf("Adding cc: %s from From: header\n", + $1) unless $quiet; push @cc, $1; } elsif (/^To:\s+(.*)$/i) { foreach my $addr (parse_address_line($1)) { - printf("(mbox) Adding to: %s from line '%s'\n", - $addr, $_) unless $quiet; + printf("Adding to: %s from To: header\n", + $addr) unless $quiet; push @to, $addr; } } @@ -1498,8 +1498,8 @@ foreach my $t (@files) { } else { next if ($suppress_cc{'cc'}); } - printf("(mbox) Adding cc: %s from line '%s'\n", - $addr, $_) unless $quiet; + printf("Adding cc: %s from Cc: header\n", + $addr) unless $quiet; push @cc, $addr; } } @@ -1532,8 +1532,8 @@ foreach my $t (@files) { # So let's support that, too. $input_format = 'lots'; if (@cc == 0 && !$suppress_cc{'cc'}) { - printf("(non-mbox) Adding cc: %s from line '%s'\n", - $_, $_) unless $quiet; + printf("Adding cc: %s from Cc: header\n", + $_) unless $quiet; push @cc, $_; } elsif (!defined $subject) { $subject = $_; @@ -1555,8 +1555,8 @@ foreach my $t (@files) { next if $suppress_cc{'bodycc'} and $what =~ /Cc/i; } push @cc, $c; - printf("(body) Adding cc: %s from line '%s'\n", - $c, $_) unless $quiet; + printf("Adding cc: %s from Signed-off-by: trailer\n", + $c) unless $quiet; } } close $fh; @@ -1660,7 +1660,7 @@ sub recipients_cmd { $address = sanitize_address($address); next if ($address eq $sender and $suppress_cc{'self'}); push @addresses, $address; - printf("($prefix) Adding %s: %s from: '%s'\n", + printf("Adding %s: %s from: '%s'\n", $what, $address, $cmd) unless $quiet; } close $fh diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 943e6b7..aca7d5c 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -150,9 +150,9 @@ test_expect_success $PREREQ 'Verify commandline' ' test_expect_success $PREREQ 'setup expect' " cat >expected-show-all-headers <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -503,9 +503,9 @@ test_expect_success $PREREQ 'second message is patch' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-sob <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -555,9 +555,9 @@ test_expect_success $PREREQ 'sendemail.cc set' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-sob <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -587,10 +587,10 @@ test_expect_success $PREREQ 'sendemail.cc unset' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-cccmd <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' -(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header +Adding cc: C O Mitter <committer@example.com> from Signed-off-by: trailer Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -647,10 +647,10 @@ test_expect_success $PREREQ '--suppress-cc=all' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-body <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' -(cc-cmd) Adding cc: cc-cmd@example.com from: './cccmd' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header +Adding cc: cc-cmd@example.com from: './cccmd' Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -681,9 +681,9 @@ test_expect_success $PREREQ '--suppress-cc=body' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-body-cccmd <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -712,9 +712,9 @@ test_expect_success $PREREQ '--suppress-cc=body --suppress-cc=cccmd' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-sob <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -744,10 +744,10 @@ test_expect_success $PREREQ '--suppress-cc=sob' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-bodycc <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' -(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>' +Adding cc: A <author@example.com> from From: header +Adding cc: One <one@example.com> from Cc: header +Adding cc: two@example.com from Cc: header +Adding cc: C O Mitter <committer@example.com> from Signed-off-by: trailer Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> @@ -778,8 +778,8 @@ test_expect_success $PREREQ '--suppress-cc=bodycc' ' test_expect_success $PREREQ 'setup expect' " cat >expected-suppress-cc <<\EOF 0001-Second.patch -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' -(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>' +Adding cc: A <author@example.com> from From: header +Adding cc: C O Mitter <committer@example.com> from Signed-off-by: trailer Dry-OK. Log says: Server: relay.example.com MAIL FROM:<from@example.com> -- 2.8.2.537.gb153d2a ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v4 3/6] send-email: shorten send-email's output 2016-06-08 13:01 ` [PATCH v4 3/6] send-email: shorten send-email's output Samuel GROOT @ 2016-06-08 17:37 ` Junio C Hamano 2016-06-08 19:18 ` Samuel GROOT 2016-06-09 6:17 ` Matthieu Moy 1 sibling, 1 reply; 96+ messages in thread From: Junio C Hamano @ 2016-06-08 17:37 UTC (permalink / raw) To: Samuel GROOT Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > + printf("Adding cc: %s from From: header\n", > + $1) unless $quiet; > + printf("Adding to: %s from To: header\n", > + $addr) unless $quiet; > + printf("Adding cc: %s from Cc: header\n", > + $addr) unless $quiet; > push @cc, $addr; > + printf("Adding cc: %s from Cc: header\n", > + $_) unless $quiet; These make the end result prettier by not repeating the same address twice, but is it just me who finds these inexplicable case differences irritating? Shouldn't these field references in the result mirror the field references in the origin of the information? ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 3/6] send-email: shorten send-email's output 2016-06-08 17:37 ` Junio C Hamano @ 2016-06-08 19:18 ` Samuel GROOT 2016-06-08 19:33 ` Junio C Hamano 0 siblings, 1 reply; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 19:18 UTC (permalink / raw) To: Junio C Hamano Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e On 06/08/2016 07:37 PM, Junio C Hamano wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: >> + printf("Adding cc: %s from From: header\n", >> + $1) unless $quiet; > >> + printf("Adding to: %s from To: header\n", >> + $addr) unless $quiet; > >> + printf("Adding cc: %s from Cc: header\n", >> + $addr) unless $quiet; >> push @cc, $addr; > >> + printf("Adding cc: %s from Cc: header\n", >> + $_) unless $quiet; > > These make the end result prettier by not repeating the same address > twice, but is it just me who finds these inexplicable case > differences irritating? Shouldn't these field references in the > result mirror the field references in the origin of the information? It makes sense only in the case below... >> + printf("Adding cc: %s from From: header\n", >> + $1) unless $quiet; ... because the sender should receive its own copy (at least to avoid breaking threaded view in his mailer) and be cc-ed. By the way, we should cc the sender when sending the cover letter too for the same reason. But in other cases, it seems pointless to display identical field reference twice. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 3/6] send-email: shorten send-email's output 2016-06-08 19:18 ` Samuel GROOT @ 2016-06-08 19:33 ` Junio C Hamano 2016-06-08 19:40 ` Samuel GROOT 0 siblings, 1 reply; 96+ messages in thread From: Junio C Hamano @ 2016-06-08 19:33 UTC (permalink / raw) To: Samuel GROOT Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > On 06/08/2016 07:37 PM, Junio C Hamano wrote: >> Samuel GROOT <samuel.groot@grenoble-inp.org> writes: >>> + printf("Adding cc: %s from From: header\n", >>> + $1) unless $quiet; >> >>> + printf("Adding to: %s from To: header\n", >>> + $addr) unless $quiet; >> >>> + printf("Adding cc: %s from Cc: header\n", >>> + $addr) unless $quiet; >>> push @cc, $addr; >> >>> + printf("Adding cc: %s from Cc: header\n", >>> + $_) unless $quiet; >> >> These make the end result prettier by not repeating the same address >> twice, but is it just me who finds these inexplicable case >> differences irritating? Shouldn't these field references in the >> result mirror the field references in the origin of the information? > > It makes sense only in the case below... > >>> + printf("Adding cc: %s from From: header\n", >>> + $1) unless $quiet; > > ... because the sender should receive its own copy (at least to avoid > breaking threaded view in his mailer) and be cc-ed. By the way, we > should cc the sender when sending the cover letter too for the same > reason. > > But in other cases, it seems pointless to display identical field > reference twice. My comment may have been a bit too oblique. What I meant was Adding cc: Samuel from From: header looked strange, and I thought it would be better written Adding Cc: Samuel from From: header Same for Adding to: Samuel from To: header being strange, and a better version of it would be Adding To: Samuel from To: header ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 3/6] send-email: shorten send-email's output 2016-06-08 19:33 ` Junio C Hamano @ 2016-06-08 19:40 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 19:40 UTC (permalink / raw) To: Junio C Hamano Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e On 06/08/2016 09:33 PM, Junio C Hamano wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > >> On 06/08/2016 07:37 PM, Junio C Hamano wrote: >>> Samuel GROOT <samuel.groot@grenoble-inp.org> writes: >>>> + printf("Adding cc: %s from From: header\n", >>>> + $1) unless $quiet; >>> >>>> + printf("Adding to: %s from To: header\n", >>>> + $addr) unless $quiet; >>> >>>> + printf("Adding cc: %s from Cc: header\n", >>>> + $addr) unless $quiet; >>>> push @cc, $addr; >>> >>>> + printf("Adding cc: %s from Cc: header\n", >>>> + $_) unless $quiet; >>> >>> These make the end result prettier by not repeating the same address >>> twice, but is it just me who finds these inexplicable case >>> differences irritating? Shouldn't these field references in the >>> result mirror the field references in the origin of the information? >> >> It makes sense only in the case below... >> >>>> + printf("Adding cc: %s from From: header\n", >>>> + $1) unless $quiet; >> >> ... because the sender should receive its own copy (at least to avoid >> breaking threaded view in his mailer) and be cc-ed. By the way, we >> should cc the sender when sending the cover letter too for the same >> reason. >> >> But in other cases, it seems pointless to display identical field >> reference twice. > > My comment may have been a bit too oblique. What I meant was > > Adding cc: Samuel from From: header > > looked strange, and I thought it would be better written > > Adding Cc: Samuel from From: header > > Same for > > Adding to: Samuel from To: header > > being strange, and a better version of it would be > > Adding To: Samuel from To: header Oh, I read your email a bit too fast, sorry. I kept the sentence as it was except for trimmed part, but it makes sense to have the same case. It will be fixed :-) ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 3/6] send-email: shorten send-email's output 2016-06-08 13:01 ` [PATCH v4 3/6] send-email: shorten send-email's output Samuel GROOT 2016-06-08 17:37 ` Junio C Hamano @ 2016-06-09 6:17 ` Matthieu Moy 2016-06-13 22:19 ` Samuel GROOT 1 sibling, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-06-09 6:17 UTC (permalink / raw) To: Samuel GROOT Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, gitster, aaron, e Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > @@ -647,10 +647,10 @@ test_expect_success $PREREQ '--suppress-cc=all' ' > test_expect_success $PREREQ 'setup expect' " > cat >expected-suppress-body <<\EOF > 0001-Second.patch > -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' > -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' > -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' > -(cc-cmd) Adding cc: cc-cmd@example.com from: './cccmd' > +Adding cc: A <author@example.com> from From: header > +Adding cc: One <one@example.com> from Cc: header > +Adding cc: two@example.com from Cc: header > +Adding cc: cc-cmd@example.com from: './cccmd' This hunk differs from the others a bit. I totally agree that removing the (mbox) prefix makes sense, but you're removing (cc-cmd) here, which did carry some information. I'd write it as Adding cc: cc-cmd@example.com from --cc-cmd: ./cccmd It might make sense to split this into two patches: one for (mbox) + headers and one for (cc-cmd) and (to-cmd). Spotting special-cases like the above inside a long patch is hard for reviewers. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 3/6] send-email: shorten send-email's output 2016-06-09 6:17 ` Matthieu Moy @ 2016-06-13 22:19 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-13 22:19 UTC (permalink / raw) To: Matthieu Moy Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, gitster, aaron, e On 06/09/2016 08:17 AM, Matthieu Moy wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > >> @@ -647,10 +647,10 @@ test_expect_success $PREREQ '--suppress-cc=all' ' >> test_expect_success $PREREQ 'setup expect' " >> cat >expected-suppress-body <<\EOF >> 0001-Second.patch >> -(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>' >> -(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com' >> -(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com' >> -(cc-cmd) Adding cc: cc-cmd@example.com from: './cccmd' >> +Adding cc: A <author@example.com> from From: header >> +Adding cc: One <one@example.com> from Cc: header >> +Adding cc: two@example.com from Cc: header >> +Adding cc: cc-cmd@example.com from: './cccmd' > > This hunk differs from the others a bit. I totally agree that removing > the (mbox) prefix makes sense, but you're removing (cc-cmd) here, which > did carry some information. > > I'd write it as > > Adding cc: cc-cmd@example.com from --cc-cmd: ./cccmd Indeed it's clearer, I will change that. > It might make sense to split this into two patches: one for (mbox) + > headers and one for (cc-cmd) and (to-cmd). Spotting special-cases like > the above inside a long patch is hard for reviewers. I will split in the future patch series dedicated to clean send-email. ^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 13:01 ` (unknown), Samuel GROOT ` (2 preceding siblings ...) 2016-06-08 13:01 ` [PATCH v4 3/6] send-email: shorten send-email's output Samuel GROOT @ 2016-06-08 13:01 ` Samuel GROOT 2016-06-08 17:58 ` Junio C Hamano 2016-06-08 20:38 ` Eric Wong 2016-06-08 13:07 ` [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields Samuel GROOT 2016-06-08 13:08 ` [PATCH v4 6/6] send-email: add option --cite to quote the message body Samuel GROOT 5 siblings, 2 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 13:01 UTC (permalink / raw) To: git Cc: samuel.groot, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, gitster, aaron, e We need a simple and generic way to parse an email file. Since it would be hard to include and maintain an external library, create an simple email parser subroutine to parse an email file. Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> Signed-off-by: Tom RUSSELLO <tom.russello@grenoble-inp.org> Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> --- We chose to create our own simple email parser and only use it for the "quote email" feature to pave the way for the refactorization of the patch parser [1] that may come after our current school project. [1] * http://thread.gmane.org/gmane.comp.version-control.git/295752 perl/Git.pm | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/perl/Git.pm b/perl/Git.pm index ce7e4e8..1af4805 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -865,6 +865,40 @@ sub ident_person { return "$ident[0] <$ident[1]>"; } +=item parse_email + +Return a hash of email fields extracted from a file handler. + +=cut + +sub parse_email { + my %mail = (); + my $fh = shift; + my $last_header; + + # Unfold and parse multiline header fields + while (<$fh>) { + last if /^\s*$/; + s/\r\n|\n|\r//; + if (/^([^\s:]+):[\s]+(.*)$/) { + $last_header = lc($1); + @{$mail{$last_header}} = () + unless defined $mail{$last_header}; + push @{$mail{$last_header}}, $2; + } elsif (/^\s+\S/ and defined $last_header) { + s/^\s+/ /; + push @{$mail{$last_header}}, $_; + } else { + die("Mail format undefined!\n"); + } + } + + # Separate body from header + $mail{"body"} = [(<$fh>)]; + + return \%mail; +} + =item parse_mailboxes Return an array of mailboxes extracted from a string. -- 2.8.2.537.gb153d2a ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 13:01 ` [PATCH v4 4/6] send-email: create email parser subroutine Samuel GROOT @ 2016-06-08 17:58 ` Junio C Hamano 2016-06-08 18:12 ` Eric Sunshine 2016-06-08 19:36 ` Samuel GROOT 2016-06-08 20:38 ` Eric Wong 1 sibling, 2 replies; 96+ messages in thread From: Junio C Hamano @ 2016-06-08 17:58 UTC (permalink / raw) To: Samuel GROOT Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > +sub parse_email { > + my %mail = (); > + my $fh = shift; > + my $last_header; > + # Unfold and parse multiline header fields > + while (<$fh>) { > + last if /^\s*$/; You stop at the end of fields before the message body starts. > + s/\r\n|\n|\r//; The pattern is not anchored at the right end of the string; intended? Is it worth worrying about a lone '\r'? > + if (/^([^\s:]+):[\s]+(.*)$/) { > + $last_header = lc($1); > + @{$mail{$last_header}} = () > + unless defined $mail{$last_header}; > + push @{$mail{$last_header}}, $2; > + } elsif (/^\s+\S/ and defined $last_header) { > + s/^\s+/ /; > + push @{$mail{$last_header}}, $_; Even though the comment said "unfold", you do not really do the unfolding here and the caller can (if it wants to) figure out where one logical header was folded in the original into multiple physical lines, because you are returning an arrayref. However, that means the caller still cannot tell what the original was if you are given: X-header: a b c X-header: d as you would return { 'X-header' => ["a b", "c", "d")] } In that sense, it may be better to do a real unfolding here, so that it would return { 'X-header' => ["a b c", "d"] } from here instead? I.e. instead of "push @{...}, $_", append $_ to the last element of that array? > + } else { > + die("Mail format undefined!\n"); What does that mean? It would probably help if you included the line that the code did not understand in the message. > + } > + } > + > + # Separate body from header > + $mail{"body"} = [(<$fh>)]; > + > + return \%mail; The name of the local thing is not observable from the caller, but because this is "parse-email-header" and returns "header fields" without reading the "mail", perhaps call it %header instead? > +} ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 17:58 ` Junio C Hamano @ 2016-06-08 18:12 ` Eric Sunshine 2016-06-08 18:32 ` Junio C Hamano 2016-06-08 19:30 ` Samuel GROOT 2016-06-08 19:36 ` Samuel GROOT 1 sibling, 2 replies; 96+ messages in thread From: Eric Sunshine @ 2016-06-08 18:12 UTC (permalink / raw) To: Junio C Hamano Cc: Samuel GROOT, Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron, Eric Wong On Wed, Jun 8, 2016 at 1:58 PM, Junio C Hamano <gitster@pobox.com> wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: >> +sub parse_email { >> + my %mail = (); >> + my $fh = shift; >> + my $last_header; > >> + # Unfold and parse multiline header fields >> + while (<$fh>) { >> + last if /^\s*$/; > > You stop at the end of fields before the message body starts. > >> + s/\r\n|\n|\r//; > > The pattern is not anchored at the right end of the string; > intended? Is it worth worrying about a lone '\r'? Thanks, I think you covered pretty much everything I was going to say. I'd just add that if the matching is going to be kept loose like this (rather than anchoring it), then s/[\r\n]+//g might be easier to read, but it's a minor point. >> + if (/^([^\s:]+):[\s]+(.*)$/) { >> + $last_header = lc($1); >> + @{$mail{$last_header}} = () >> + unless defined $mail{$last_header}; >> + push @{$mail{$last_header}}, $2; > >> + } elsif (/^\s+\S/ and defined $last_header) { >> + s/^\s+/ /; >> + push @{$mail{$last_header}}, $_; > > Even though the comment said "unfold", you do not really do the > unfolding here and the caller can (if it wants to) figure out where > one logical header was folded in the original into multiple physical > lines, because you are returning an arrayref. Also, the comment about folding lines should be moved down the part of the code which is actually (supposed to be) doing the folding rather than having the comment at the top of the loop. > However, that means the caller still cannot tell what the original > was if you are given: > > X-header: a b > c > X-header: d > > as you would return { 'X-header' => ["a b", "c", "d")] } > > In that sense, it may be better to do a real unfolding here, so that > it would return { 'X-header' => ["a b c", "d"] } from here instead? > > I.e. instead of "push @{...}, $_", append $_ to the last element of > that array? Right. >> + # Separate body from header >> + $mail{"body"} = [(<$fh>)]; >> + >> + return \%mail; > > The name of the local thing is not observable from the caller, but > because this is "parse-email-header" and returns "header fields" > without reading the "mail", perhaps call it %header instead? If there is (for some reason) a mail header named 'body', then this assignment of the body portion of the message will overwrite it. Perhaps this function should instead return multiple values: the header hash, and the message body. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 18:12 ` Eric Sunshine @ 2016-06-08 18:32 ` Junio C Hamano 2016-06-08 19:26 ` Samuel GROOT 2016-06-08 19:30 ` Samuel GROOT 1 sibling, 1 reply; 96+ messages in thread From: Junio C Hamano @ 2016-06-08 18:32 UTC (permalink / raw) To: Eric Sunshine Cc: Samuel GROOT, Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron, Eric Wong Eric Sunshine <sunshine@sunshineco.com> writes: >>> + # Separate body from header >>> + $mail{"body"} = [(<$fh>)]; >>> + >>> + return \%mail; >> >> The name of the local thing is not observable from the caller, but >> because this is "parse-email-header" and returns "header fields" >> without reading the "mail", perhaps call it %header instead? > > If there is (for some reason) a mail header named 'body', then this > assignment of the body portion of the message will overwrite it. > Perhaps this function should instead return multiple values: the > header hash, and the message body. Ah, I missed that it is attempting to return the body, too. Because the function takes an open filehandle, I think it is better to leave it to the callers. A caller that is only interested in headers can just close $fh after this helper returns without reading body that it is not interested in, and a caller that wants to read the body can do the slurping itself. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 18:32 ` Junio C Hamano @ 2016-06-08 19:26 ` Samuel GROOT 2016-06-08 19:31 ` Junio C Hamano 0 siblings, 1 reply; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 19:26 UTC (permalink / raw) To: Junio C Hamano, Eric Sunshine Cc: Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron, Eric Wong On 06/08/2016 08:32 PM, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: >>>> + # Separate body from header >>>> + $mail{"body"} = [(<$fh>)]; >>>> + >>>> + return \%mail; >>> >>> The name of the local thing is not observable from the caller, but >>> because this is "parse-email-header" and returns "header fields" >>> without reading the "mail", perhaps call it %header instead? >> >> If there is (for some reason) a mail header named 'body', then this >> assignment of the body portion of the message will overwrite it. >> Perhaps this function should instead return multiple values: the >> header hash, and the message body. > > Ah, I missed that it is attempting to return the body, too. > > Because the function takes an open filehandle, I think it is better > to leave it to the callers. A caller that is only interested in > headers can just close $fh after this helper returns without reading > body that it is not interested in, and a caller that wants to read > the body can do the slurping itself. I think it's the best way to do it indeed. Furthermore, we did trim CRs and LFs in header fields, but not in the message, making the subroutine inconsistent. Should we rename the subroutine to `parse_header` or leave it as it is? ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 19:26 ` Samuel GROOT @ 2016-06-08 19:31 ` Junio C Hamano 2016-06-08 19:42 ` Samuel GROOT 0 siblings, 1 reply; 96+ messages in thread From: Junio C Hamano @ 2016-06-08 19:31 UTC (permalink / raw) To: Samuel GROOT Cc: Eric Sunshine, Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron, Eric Wong Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > I think it's the best way to do it indeed. Furthermore, we did trim > CRs and LFs in header fields, but not in the message, making the > subroutine inconsistent. > > Should we rename the subroutine to `parse_header` or leave it as it is? If it lives inside git-send-email, then parse_header is sufficient as everybody would know it is about e-mail without being told. If it is in Git.pm, then parse_email_header would be more appropriate. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 19:31 ` Junio C Hamano @ 2016-06-08 19:42 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 19:42 UTC (permalink / raw) To: Junio C Hamano Cc: Eric Sunshine, Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron, Eric Wong On 06/08/2016 09:31 PM, Junio C Hamano wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > >> I think it's the best way to do it indeed. Furthermore, we did trim >> CRs and LFs in header fields, but not in the message, making the >> subroutine inconsistent. >> >> Should we rename the subroutine to `parse_header` or leave it as it is? > > If it lives inside git-send-email, then parse_header is sufficient > as everybody would know it is about e-mail without being told. If > it is in Git.pm, then parse_email_header would be more appropriate. It currently lives in Git.pm, following Eric Wong's advice to have a more packaged code. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 18:12 ` Eric Sunshine 2016-06-08 18:32 ` Junio C Hamano @ 2016-06-08 19:30 ` Samuel GROOT 2016-06-08 20:13 ` Eric Sunshine 1 sibling, 1 reply; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 19:30 UTC (permalink / raw) To: Eric Sunshine, Junio C Hamano Cc: Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron, Eric Wong On 06/08/2016 08:12 PM, Eric Sunshine wrote: > On Wed, Jun 8, 2016 at 1:58 PM, Junio C Hamano <gitster@pobox.com> wrote: >> Samuel GROOT <samuel.groot@grenoble-inp.org> writes: >>> +sub parse_email { >>> + my %mail = (); >>> + my $fh = shift; >>> + my $last_header; >> >>> + # Unfold and parse multiline header fields >>> + while (<$fh>) { >>> + last if /^\s*$/; >> >> You stop at the end of fields before the message body starts. >> >>> + s/\r\n|\n|\r//; >> >> The pattern is not anchored at the right end of the string; >> intended? Is it worth worrying about a lone '\r'? > > Thanks, I think you covered pretty much everything I was going to say. > I'd just add that if the matching is going to be kept loose like this > (rather than anchoring it), then s/[\r\n]+//g might be easier to read, > but it's a minor point. Indeed s/[\r\n]+//g is way better, it works even if there's a CR in the middle of the line. >>> + if (/^([^\s:]+):[\s]+(.*)$/) { >>> + $last_header = lc($1); >>> + @{$mail{$last_header}} = () >>> + unless defined $mail{$last_header}; >>> + push @{$mail{$last_header}}, $2; >> >>> + } elsif (/^\s+\S/ and defined $last_header) { >>> + s/^\s+/ /; >>> + push @{$mail{$last_header}}, $_; >> >> Even though the comment said "unfold", you do not really do the >> unfolding here and the caller can (if it wants to) figure out where >> one logical header was folded in the original into multiple physical >> lines, because you are returning an arrayref. > > Also, the comment about folding lines should be moved down the part of > the code which is actually (supposed to be) doing the folding rather > than having the comment at the top of the loop. Will do in next re-roll. >>> + # Separate body from header >>> + $mail{"body"} = [(<$fh>)]; >>> + >>> + return \%mail; >> >> The name of the local thing is not observable from the caller, but >> because this is "parse-email-header" and returns "header fields" >> without reading the "mail", perhaps call it %header instead? > > If there is (for some reason) a mail header named 'body', then this > assignment of the body portion of the message will overwrite it. > Perhaps this function should instead return multiple values: the > header hash, and the message body. I will drop the body part in re-roll. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 19:30 ` Samuel GROOT @ 2016-06-08 20:13 ` Eric Sunshine 2016-06-08 20:17 ` Junio C Hamano 0 siblings, 1 reply; 96+ messages in thread From: Eric Sunshine @ 2016-06-08 20:13 UTC (permalink / raw) To: Samuel GROOT Cc: Junio C Hamano, Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron, Eric Wong On Wed, Jun 8, 2016 at 3:30 PM, Samuel GROOT <samuel.groot@grenoble-inp.org> wrote: > On 06/08/2016 08:12 PM, Eric Sunshine wrote: >> On Wed, Jun 8, 2016 at 1:58 PM, Junio C Hamano <gitster@pobox.com> wrote: >>> Samuel GROOT <samuel.groot@grenoble-inp.org> writes: >>> The pattern is not anchored at the right end of the string; >>> intended? Is it worth worrying about a lone '\r'? >> >> Thanks, I think you covered pretty much everything I was going to say. >> I'd just add that if the matching is going to be kept loose like this >> (rather than anchoring it), then s/[\r\n]+//g might be easier to read, >> but it's a minor point. > > Indeed s/[\r\n]+//g is way better, it works even if there's a CR in the > middle of the line. An embedded CR probably shouldn't happen, but I'm not convinced that folding it out is a good idea. I would think that you'd want to preserve the header's value verbatim. If anything, I'd expect to see the regex tightened to: s/\r?\n$//; Alternately, consider using 'chop' or 'chomp'. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 20:13 ` Eric Sunshine @ 2016-06-08 20:17 ` Junio C Hamano 2016-06-08 23:54 ` Samuel GROOT 0 siblings, 1 reply; 96+ messages in thread From: Junio C Hamano @ 2016-06-08 20:17 UTC (permalink / raw) To: Eric Sunshine Cc: Samuel GROOT, Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron, Eric Wong Eric Sunshine <sunshine@sunshineco.com> writes: > An embedded CR probably shouldn't happen, but I'm not convinced that > folding it out is a good idea. I would think that you'd want to > preserve the header's value verbatim. If anything, I'd expect to see > the regex tightened to: > > s/\r?\n$//; Yes, that would be more sensible than silently removing \r in the middle which _is_ a sign of something funny going on. > Alternately, consider using 'chop' or 'chomp'. Even if you use chomp(), you'd still need to worry about possible \r at the end, no? ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 20:17 ` Junio C Hamano @ 2016-06-08 23:54 ` Samuel GROOT 2016-06-09 0:21 ` Eric Wong 2016-06-09 6:51 ` Eric Sunshine 0 siblings, 2 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 23:54 UTC (permalink / raw) To: Junio C Hamano, Eric Sunshine Cc: Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron, Eric Wong On 06/08/2016 10:17 PM, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> An embedded CR probably shouldn't happen, but I'm not convinced that >> folding it out is a good idea. I would think that you'd want to >> preserve the header's value verbatim. If anything, I'd expect to see >> the regex tightened to: >> >> s/\r?\n$//; > > Yes, that would be more sensible than silently removing \r in the > middle which _is_ a sign of something funny going on. >> Alternately, consider using 'chop' or 'chomp'. > > Even if you use chomp(), you'd still need to worry about possible \r > at the end, no? 'chomp' is what we used before, but with *.eml files (microsoft's file format, with CRLF), '\n' were removed but '\r' remained, that's why we used s/\r\n|\r|\n//. s/\r?\n$// looks fine. Email::Simple library uses qr/\x0a\x0d|\x0d\x0a|\x0a|\x0d/ [1]. Should we handle \n\r at end of line as well? [1] * http://cpansearch.perl.org/src/RJBS/Email-Simple-2.210/lib/Email/Simple.pm ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 23:54 ` Samuel GROOT @ 2016-06-09 0:21 ` Eric Wong 2016-06-13 22:18 ` Samuel GROOT 2016-06-09 6:51 ` Eric Sunshine 1 sibling, 1 reply; 96+ messages in thread From: Eric Wong @ 2016-06-09 0:21 UTC (permalink / raw) To: Samuel GROOT Cc: Junio C Hamano, Eric Sunshine, Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron Samuel GROOT <samuel.groot@grenoble-inp.org> wrote: > Email::Simple library uses qr/\x0a\x0d|\x0d\x0a|\x0a|\x0d/ [1]. > Should we handle \n\r at end of line as well? "\n\r" can never happen with local $/ = "\n" > [1] * http://cpansearch.perl.org/src/RJBS/Email-Simple-2.210/lib/Email/Simple.pm ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-09 0:21 ` Eric Wong @ 2016-06-13 22:18 ` Samuel GROOT 2016-06-13 22:47 ` Eric Wong 0 siblings, 1 reply; 96+ messages in thread From: Samuel GROOT @ 2016-06-13 22:18 UTC (permalink / raw) To: Eric Wong Cc: Junio C Hamano, Eric Sunshine, Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron On 06/09/2016 02:21 AM, Eric Wong wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> wrote: >> Email::Simple library uses qr/\x0a\x0d|\x0d\x0a|\x0a|\x0d/ [1]. >> Should we handle \n\r at end of line as well? > > "\n\r" can never happen with local $/ = "\n" If the email file contains "\n\r", setting $/ = "\n" will leave "\r" at the beginning of each line. We could trim them with: s/^\r//; s/\r?\n$//; But is it worth adding `s/^\r//;` to handle that extremely rare case? ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-13 22:18 ` Samuel GROOT @ 2016-06-13 22:47 ` Eric Wong 2016-06-14 22:18 ` Samuel GROOT 0 siblings, 1 reply; 96+ messages in thread From: Eric Wong @ 2016-06-13 22:47 UTC (permalink / raw) To: Samuel GROOT Cc: Junio C Hamano, Eric Sunshine, Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron Samuel GROOT <samuel.groot@grenoble-inp.org> wrote: > On 06/09/2016 02:21 AM, Eric Wong wrote: > >Samuel GROOT <samuel.groot@grenoble-inp.org> wrote: > >>Email::Simple library uses qr/\x0a\x0d|\x0d\x0a|\x0a|\x0d/ [1]. > >>Should we handle \n\r at end of line as well? > > > >"\n\r" can never happen with local $/ = "\n" > > If the email file contains "\n\r", setting $/ = "\n" will leave "\r" at > the beginning of each line. > > We could trim them with: > > s/^\r//; > s/\r?\n$//; > > But is it worth adding `s/^\r//;` to handle that extremely rare case? I doubt it. Having a "\r" in the wrong place is likely a bug in whatever program that generated the email. It should be exposed so whoever generated that email has a chance to fix it on their end rather than being quietly hidden. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-13 22:47 ` Eric Wong @ 2016-06-14 22:18 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-14 22:18 UTC (permalink / raw) To: Eric Wong Cc: Junio C Hamano, Eric Sunshine, Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron On 06/14/2016 12:47 AM, Eric Wong wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> wrote: >> On 06/09/2016 02:21 AM, Eric Wong wrote: >>> Samuel GROOT <samuel.groot@grenoble-inp.org> wrote: >>>> Email::Simple library uses qr/\x0a\x0d|\x0d\x0a|\x0a|\x0d/ [1]. >>>> Should we handle \n\r at end of line as well? >>> >>> "\n\r" can never happen with local $/ = "\n" >> >> If the email file contains "\n\r", setting $/ = "\n" will leave "\r" at >> the beginning of each line. >> >> We could trim them with: >> >> s/^\r//; >> s/\r?\n$//; >> >> But is it worth adding `s/^\r//;` to handle that extremely rare case? > > I doubt it. Having a "\r" in the wrong place is likely a bug in > whatever program that generated the email. It should be exposed > so whoever generated that email has a chance to fix it on their > end rather than being quietly hidden. s/\r?\n$// is fine then. Thanks. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 23:54 ` Samuel GROOT 2016-06-09 0:21 ` Eric Wong @ 2016-06-09 6:51 ` Eric Sunshine 2016-06-13 22:15 ` Samuel GROOT 1 sibling, 1 reply; 96+ messages in thread From: Eric Sunshine @ 2016-06-09 6:51 UTC (permalink / raw) To: Samuel GROOT Cc: Junio C Hamano, Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron, Eric Wong On Wed, Jun 8, 2016 at 7:54 PM, Samuel GROOT <samuel.groot@grenoble-inp.org> wrote: > On 06/08/2016 10:17 PM, Junio C Hamano wrote: >> Eric Sunshine <sunshine@sunshineco.com> writes: >>> An embedded CR probably shouldn't happen, but I'm not convinced that >>> folding it out is a good idea. I would think that you'd want to >>> preserve the header's value verbatim. If anything, I'd expect to see >>> the regex tightened to: >>> >>> s/\r?\n$//; >> >> Yes, that would be more sensible than silently removing \r in the >> middle which _is_ a sign of something funny going on. > > s/\r?\n$// looks fine. > > Email::Simple library uses qr/\x0a\x0d|\x0d\x0a|\x0a|\x0d/ [1]. > [1] * http://cpansearch.perl.org/src/RJBS/Email-Simple-2.210/lib/Email/Simple.pm You could certainly use s/\x0d?\x0a$// rather than s/\r?\n$// to be really robust, but I doubt it matters much today. The reason for using hex codes is that \r and \n will resolve to CR and LF in the local character encoding, and those may not be 0x0d and 0x0a, respectively. I believe the canonical example given in the Camel book was EBCIDIC in which \r is 0x0d, but \n is 0x25, not 0x0a as it is in ASCII. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-09 6:51 ` Eric Sunshine @ 2016-06-13 22:15 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-13 22:15 UTC (permalink / raw) To: Eric Sunshine Cc: Junio C Hamano, Git List, tom.russello, erwan.mathoniere, jordan.de-gea, Matthieu Moy, aaron, Eric Wong On 06/09/2016 08:51 AM, Eric Sunshine wrote: > On Wed, Jun 8, 2016 at 7:54 PM, Samuel GROOT > <samuel.groot@grenoble-inp.org> wrote: >> On 06/08/2016 10:17 PM, Junio C Hamano wrote: >>> Eric Sunshine <sunshine@sunshineco.com> writes: >>>> An embedded CR probably shouldn't happen, but I'm not convinced that >>>> folding it out is a good idea. I would think that you'd want to >>>> preserve the header's value verbatim. If anything, I'd expect to see >>>> the regex tightened to: >>>> >>>> s/\r?\n$//; >>> >>> Yes, that would be more sensible than silently removing \r in the >>> middle which _is_ a sign of something funny going on. >> >> s/\r?\n$// looks fine. >> >> Email::Simple library uses qr/\x0a\x0d|\x0d\x0a|\x0a|\x0d/ [1]. >> [1] * http://cpansearch.perl.org/src/RJBS/Email-Simple-2.210/lib/Email/Simple.pm > > You could certainly use s/\x0d?\x0a$// rather than s/\r?\n$// to be > really robust, but I doubt it matters much today. The reason for using > hex codes is that \r and \n will resolve to CR and LF in the local > character encoding, and those may not be 0x0d and 0x0a, respectively. > I believe the canonical example given in the Camel book was EBCIDIC in > which \r is 0x0d, but \n is 0x25, not 0x0a as it is in ASCII. My question was more about the `\n\r` case handled by Email::Simple, does it make sense to handle it? ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 17:58 ` Junio C Hamano 2016-06-08 18:12 ` Eric Sunshine @ 2016-06-08 19:36 ` Samuel GROOT 1 sibling, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 19:36 UTC (permalink / raw) To: Junio C Hamano Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e On 06/08/2016 07:58 PM, Junio C Hamano wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: >> +sub parse_email { >> + my %mail = (); >> + my $fh = shift; >> + my $last_header; > >> + # Unfold and parse multiline header fields >> + while (<$fh>) { >> + last if /^\s*$/; > > You stop at the end of fields before the message body starts. > >> + s/\r\n|\n|\r//; > > The pattern is not anchored at the right end of the string; > intended? Is it worth worrying about a lone '\r'? A lone '\r' shouldn't happen, but we are never too careful. It's fixed with what Eric suggested. >> + if (/^([^\s:]+):[\s]+(.*)$/) { >> + $last_header = lc($1); >> + @{$mail{$last_header}} = () >> + unless defined $mail{$last_header}; >> + push @{$mail{$last_header}}, $2; > >> + } elsif (/^\s+\S/ and defined $last_header) { >> + s/^\s+/ /; >> + push @{$mail{$last_header}}, $_; > > Even though the comment said "unfold", you do not really do the > unfolding here and the caller can (if it wants to) figure out where > one logical header was folded in the original into multiple physical > lines, because you are returning an arrayref. > > However, that means the caller still cannot tell what the original > was if you are given: > > X-header: a b > c > X-header: d > > as you would return { 'X-header' => ["a b", "c", "d")] } > > In that sense, it may be better to do a real unfolding here, so that > it would return { 'X-header' => ["a b c", "d"] } from here instead? > > I.e. instead of "push @{...}, $_", append $_ to the last element of > that array? I will do that. It makes more sense regarding Subject split into several lines, for example. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 4/6] send-email: create email parser subroutine 2016-06-08 13:01 ` [PATCH v4 4/6] send-email: create email parser subroutine Samuel GROOT 2016-06-08 17:58 ` Junio C Hamano @ 2016-06-08 20:38 ` Eric Wong 1 sibling, 0 replies; 96+ messages in thread From: Eric Wong @ 2016-06-08 20:38 UTC (permalink / raw) To: Samuel GROOT Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, gitster, aaron, Eric Sunshine Samuel GROOT <samuel.groot@grenoble-inp.org> wrote: > +++ b/perl/Git.pm > +sub parse_email { > + my %mail = (); > + my $fh = shift; > + my $last_header; > + > + # Unfold and parse multiline header fields When you libify, I suggest you localize $/ since $/ may be set to something other than "\n" by a caller and change the behavior of <$fh> and $fh->getline. local $/ = "\n"; > + while (<$fh>) { > + last if /^\s*$/; > + s/\r\n|\n|\r//; And, as Eric Sunshine stated: s/\r?\n$//; Explicitly localizing $/ means you wouldn't have to worry about multiple \n showing up in the line, either. And chomp/chop wouldn't work, here. Otherwise I like the move to Git.pm, thanks. ^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields 2016-06-08 13:01 ` (unknown), Samuel GROOT ` (3 preceding siblings ...) 2016-06-08 13:01 ` [PATCH v4 4/6] send-email: create email parser subroutine Samuel GROOT @ 2016-06-08 13:07 ` Samuel GROOT 2016-06-08 18:23 ` Junio C Hamano 2016-06-09 9:45 ` Matthieu Moy 2016-06-08 13:08 ` [PATCH v4 6/6] send-email: add option --cite to quote the message body Samuel GROOT 5 siblings, 2 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 13:07 UTC (permalink / raw) To: git Cc: samuel.groot, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, gitster, aaron, e Take an email message file, parse it and fill the "From", "To", "Cc", "In-reply-to", "References" fields appropriately. If `--compose` option is set, it will also fill the subject field with `Re: [<email_file>'s subject]` in the introductory message. Signed-off-by: Tom RUSSELLO <tom.russello@grenoble-inp.org> Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> --- Documentation/git-send-email.txt | 9 +++-- git-send-email.perl | 51 +++++++++++++++++++++++- t/t9001-send-email.sh | 83 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 5 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index edbba3a..21776f0 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -84,13 +84,16 @@ See the CONFIGURATION section for 'sendemail.multiEdit'. the value of GIT_AUTHOR_IDENT, or GIT_COMMITTER_IDENT if that is not set, as returned by "git var -l". ---in-reply-to=<identifier>:: +--in-reply-to=<Message-Id|email_file>:: Make the first mail (or all the mails with `--no-thread`) appear as a - reply to the given Message-Id, which avoids breaking threads to - provide a new patch series. + reply to the given Message-Id (given directly by argument or via the email + file), which avoids breaking threads to provide a new patch series. The second and subsequent emails will be sent as replies according to the `--[no]-chain-reply-to` setting. + +Furthermore, if the argument is an email file, parse it and populate header +fields appropriately for the reply. ++ So for example when `--thread` and `--no-chain-reply-to` are specified, the second and subsequent patches will be replies to the first one like in the illustration below where `[PATCH v2 0/3]` is in reply to `[PATCH 0/2]`: diff --git a/git-send-email.perl b/git-send-email.perl index 9b51062..b444ea6 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -55,6 +55,7 @@ git send-email --dump-aliases --[no-]bcc <str> * Email Bcc: --subject <str> * Email "Subject:" --in-reply-to <str> * Email "In-Reply-To:" + --in-reply-to <file> * Populate header fields appropriately. --[no-]xmailer * Add "X-Mailer:" header (default). --[no-]annotate * Review each patch that will be sent in an editor. --compose * Open an editor for introduction. @@ -160,7 +161,7 @@ my $re_encoded_word = qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/; # Variables we fill in automatically, or via prompting: my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh, - $initial_reply_to,$initial_subject,@files, + $initial_reply_to,$initial_references,$initial_subject,@files, $author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time); my $envelope_sender; @@ -639,6 +640,52 @@ if (@files) { usage(); } +if ($initial_reply_to && -f $initial_reply_to) { + my $error = validate_patch($initial_reply_to); + die "fatal: $initial_reply_to: $error\nwarning: no patches were sent\n" + if $error; + + open my $fh, "<", $initial_reply_to or die "can't open file $initial_reply_to"; + my $mail = Git::parse_email($fh); + close $fh; + + my $initial_sender = $sender || $repoauthor || $repocommitter || ''; + + my $prefix_re = ""; + my $subject_re = $mail->{"subject"}[0]; + if ($subject_re =~ /^[^Re:]/) { + $prefix_re = "Re: "; + } + $initial_subject = $prefix_re . $subject_re; + + push @initial_to, $mail->{"from"}[0]; + + foreach my $to_addr (parse_address_line(join ",", @{$mail->{"to"}})) { + if (!($to_addr eq $initial_sender)) { + push @initial_cc, $to_addr; + } + } + + if (defined $mail->{"cc"}) { + foreach my $cc_addr (parse_address_line(join ",", @{$mail->{"cc"}})) { + my $qaddr = unquote_rfc2047($cc_addr); + my $saddr = sanitize_address($qaddr); + if ($saddr eq $initial_sender) { + next if ($suppress_cc{'self'}); + } else { + next if ($suppress_cc{'cc'}); + } + push @initial_cc, $cc_addr; + } + } + + $initial_reply_to = $mail->{"message-id"}[0]; + if ($mail->{"references"}) { + $initial_references = join("", @{$mail->{"references"}}) . + " " . $initial_reply_to; + } +} + sub get_patch_subject { my $fn = shift; open (my $fh, '<', $fn); @@ -1426,7 +1473,7 @@ Message-Id: $message_id } $reply_to = $initial_reply_to; -$references = $initial_reply_to || ''; +$references = $initial_references || $initial_reply_to || ''; $subject = $initial_subject; $message_num = 0; diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index aca7d5c..7591342 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -1892,4 +1892,87 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' ' test_cmp expected-list actual-list ' +test_expect_success $PREREQ 'setup expect' ' + cat >email <<-\EOF + Subject: subject goes here + From: author@example.com + To: to1@example.com + Cc: cc1@example.com, cc2@example.com, + cc3@example.com + Date: Sat, 12 Jun 2010 15:53:58 +0200 + Message-Id: <author_123456@example.com> + References: <firstauthor_654321@example.com> + <secondauthor_01546567@example.com> + <thirdauthor_1395838@example.com> + + Have you seen my previous email? + > Previous content + EOF +' + +test_expect_success $PREREQ 'Fields with --in-reply-to are correct' ' + clean_fake_sendmail && + git send-email \ + --in-reply-to=email \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -2 \ + 2>errors && + grep "From: Example <nobody@example.com>" msgtxt1 && + grep "In-Reply-To: <author_123456@example.com>" msgtxt1 && + to_adr=$(awk "/^To: /{flag=1}/^Cc: /{flag=0} flag {print}" msgtxt1) && + cc_adr=$(awk "/^Cc: /{flag=1}/^Subject: /{flag=0} flag {print}" msgtxt1) && + ref_adr=$(awk "/^References: /{flag=1}/^MIME-Version: /{flag=0} flag {print}" \ + msgtxt1) && + echo "$to_adr" | grep author@example.com && + echo "$cc_adr" | grep to1@example.com && + echo "$cc_adr" | grep cc1@example.com && + echo "$cc_adr" | grep cc2@example.com && + echo "$cc_adr" | grep cc3@example.com && + echo "$ref_adr" | grep "<firstauthor_654321@example.com>" && + echo "$ref_adr" | grep "<secondauthor_01546567@example.com>" && + echo "$ref_adr" | grep "<thirdauthor_1395838@example.com>" && + echo "$ref_adr" | grep "<author_123456@example.com>" && + echo "$ref_adr" | grep -v "References: <author_123456@example.com>" +' + +test_expect_success $PREREQ 'Fields with --in-reply-to and --compose are correct' ' + clean_fake_sendmail && + git send-email \ + --in-reply-to=email \ + --compose \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -1 \ + 2>errors && + grep "From: Example <nobody@example.com>" msgtxt1 && + grep "In-Reply-To: <author_123456@example.com>" msgtxt1 && + grep "Subject: Re: subject goes here" msgtxt1 && + to_adr=$(awk "/^To: /{flag=1}/^Cc: /{flag=0} flag {print}" msgtxt1) && + cc_adr=$(awk "/^Cc: /{flag=1}/^Subject: /{flag=0} flag {print}" msgtxt1) && + ref_adr=$(awk "/^References: /{flag=1}/^MIME-Version: /{flag=0} flag {print}" \ + msgtxt1) && + echo "$to_adr" | grep author@example.com && + echo "$cc_adr" | grep to1@example.com && + echo "$cc_adr" | grep cc1@example.com && + echo "$cc_adr" | grep cc2@example.com && + echo "$cc_adr" | grep cc3@example.com && + echo "$ref_adr" | grep "<firstauthor_654321@example.com>" && + echo "$ref_adr" | grep "<secondauthor_01546567@example.com>" && + echo "$ref_adr" | grep "<thirdauthor_1395838@example.com>" && + echo "$ref_adr" | grep "<author_123456@example.com>" && + echo "$ref_adr" | grep -v "References: <author_123456@example.com>" +' + +test_expect_success $PREREQ 'Re: written only once with --in-reply-to and --compose ' ' + git send-email \ + --in-reply-to=msgtxt1 \ + --compose \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -1 \ + 2>errors && + grep "Subject: Re: subject goes here" msgtxt3 +' + test_done -- 2.8.2.537.gb153d2a ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields 2016-06-08 13:07 ` [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields Samuel GROOT @ 2016-06-08 18:23 ` Junio C Hamano 2016-06-14 22:26 ` Samuel GROOT 2016-06-09 9:45 ` Matthieu Moy 1 sibling, 1 reply; 96+ messages in thread From: Junio C Hamano @ 2016-06-08 18:23 UTC (permalink / raw) To: Samuel GROOT Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > +if ($initial_reply_to && -f $initial_reply_to) { > + my $error = validate_patch($initial_reply_to); This call is wrong, isn't it? You are not going to send out the message you are responding to (the message may not even be a patch), and you do not want to die with an error message that says "patch contains an overlong line". > + die "fatal: $initial_reply_to: $error\nwarning: no patches were sent\n" > + if $error; > + > + open my $fh, "<", $initial_reply_to or die "can't open file $initial_reply_to"; > + my $mail = Git::parse_email($fh); > + close $fh; my $header = Git::parse_email_header($fh); perhaps? > + my $initial_sender = $sender || $repoauthor || $repocommitter || ''; > + > + my $prefix_re = ""; > + my $subject_re = $mail->{"subject"}[0]; > + if ($subject_re =~ /^[^Re:]/) { > + $prefix_re = "Re: "; > + } > + $initial_subject = $prefix_re . $subject_re; I am not sure what the significance of the fact that the subject happens to begin with a letter other than 'R', 'e', or ':'. Did you mean to do something like this instead? my $subject = $mail->{"subject"}[0]; $subject =~ s/^(re:\s*)+//i; # strip "Re: Re: ..." $initial_subject = "Re: $subject"; instead? By the way, this is a good example why your "unfold" implementation in 4/6 is unwieldy for the caller. Imagine a rather long subject that is folded, i.e. To: Samuel Subject: Help! I am having a trouble running git-send-email correctly. Message-id: <...> ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields 2016-06-08 18:23 ` Junio C Hamano @ 2016-06-14 22:26 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-14 22:26 UTC (permalink / raw) To: Junio C Hamano Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, aaron, e On 06/08/2016 08:23 PM, Junio C Hamano wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > >> +if ($initial_reply_to && -f $initial_reply_to) { >> + my $error = validate_patch($initial_reply_to); > > This call is wrong, isn't it? > > You are not going to send out the message you are responding to (the > message may not even be a patch), and you do not want to die with an > error message that says "patch contains an overlong line". We used to handle email files like patch files (with Cc:, To:, From:, ... fields, a patch is almost an email, with some trailers). But that `validate_patch` subroutine call is indeed useless here, I will remove it. >> + die "fatal: $initial_reply_to: $error\nwarning: no patches were sent\n" >> + if $error; >> + >> + open my $fh, "<", $initial_reply_to or die "can't open file $initial_reply_to"; >> + my $mail = Git::parse_email($fh); >> + close $fh; > > my $header = Git::parse_email_header($fh); > > perhaps? Git::parse_email will be renamed into Git::parse_email_header in v5. >> + my $initial_sender = $sender || $repoauthor || $repocommitter || ''; >> + >> + my $prefix_re = ""; >> + my $subject_re = $mail->{"subject"}[0]; >> + if ($subject_re =~ /^[^Re:]/) { >> + $prefix_re = "Re: "; >> + } >> + $initial_subject = $prefix_re . $subject_re; > > I am not sure what the significance of the fact that the subject > happens to begin with a letter other than 'R', 'e', or ':'. > > Did you mean to do something like this instead? > > my $subject = $mail->{"subject"}[0]; > $subject =~ s/^(re:\s*)+//i; # strip "Re: Re: ..." > $initial_subject = "Re: $subject"; > > instead? That's way cleaner, thanks! > By the way, this is a good example why your "unfold" implementation > in 4/6 is unwieldy for the caller. Imagine a rather long subject > that is folded, i.e. > > To: Samuel > Subject: Help! I am having a trouble running git-send-email > correctly. > Message-id: <...> It's a good point. What you suggested in 4/6 reply will be used in v5. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields 2016-06-08 13:07 ` [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields Samuel GROOT 2016-06-08 18:23 ` Junio C Hamano @ 2016-06-09 9:45 ` Matthieu Moy 2016-06-14 22:35 ` Samuel GROOT 1 sibling, 1 reply; 96+ messages in thread From: Matthieu Moy @ 2016-06-09 9:45 UTC (permalink / raw) To: Samuel GROOT Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, gitster, aaron, e Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt > index edbba3a..21776f0 100644 > --- a/Documentation/git-send-email.txt > +++ b/Documentation/git-send-email.txt > @@ -84,13 +84,16 @@ See the CONFIGURATION section for 'sendemail.multiEdit'. > the value of GIT_AUTHOR_IDENT, or GIT_COMMITTER_IDENT if that is not > set, as returned by "git var -l". > > ---in-reply-to=<identifier>:: > +--in-reply-to=<Message-Id|email_file>:: > Make the first mail (or all the mails with `--no-thread`) appear as a > - reply to the given Message-Id, which avoids breaking threads to > - provide a new patch series. > + reply to the given Message-Id (given directly by argument or via the email > + file), which avoids breaking threads to provide a new patch series. > The second and subsequent emails will be sent as replies according to > the `--[no]-chain-reply-to` setting. > + > +Furthermore, if the argument is an email file, parse it and populate header > +fields appropriately for the reply. "populate header fields appropriately" would seem obscure to someone not having followed this converation. At least s/fields/To: and Cc: fields/. > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -55,6 +55,7 @@ git send-email --dump-aliases > --[no-]bcc <str> * Email Bcc: > --subject <str> * Email "Subject:" > --in-reply-to <str> * Email "In-Reply-To:" > + --in-reply-to <file> * Populate header fields appropriately. Likewise. To avoid an overly long line, I'd write just "Populate To/Cc/In-reply-to". Probably <file> should be <email_file>. > +if ($initial_reply_to && -f $initial_reply_to) { > + my $error = validate_patch($initial_reply_to); > + die "fatal: $initial_reply_to: $error\nwarning: no patches were sent\n" > + if $error; > + > + open my $fh, "<", $initial_reply_to or die "can't open file $initial_reply_to"; > + my $mail = Git::parse_email($fh); > + close $fh; > + > + my $initial_sender = $sender || $repoauthor || $repocommitter || ''; This is duplicated from the "if ($compose) { ... my $tpl_sender = ..." a bit later in the existing file. It would be better to get this "my $initial_sender = ..." out of your "if" and use $initial_sender directly later IMHO. Actually, $initial_sender does not seem to be a good variable name. It's not really "initial", right? > + my $prefix_re = ""; > + my $subject_re = $mail->{"subject"}[0]; > + if ($subject_re =~ /^[^Re:]/) { > + $prefix_re = "Re: "; > + } > + $initial_subject = $prefix_re . $subject_re; Why introduce $prefix_re. You can just my $subject = $mail->{"subject"}[0]; if (...) { $subject = "Re: " . $subject; } (preferably using sensible as '...' as noted by Junio ;-) ). In previous iterations of this series, you had issues with non-ascii characters in at least To: and Cc: fields (perhaps in the Subject field too?). Are they solved? I don't see any tests about it ... -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields 2016-06-09 9:45 ` Matthieu Moy @ 2016-06-14 22:35 ` Samuel GROOT 0 siblings, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-14 22:35 UTC (permalink / raw) To: Matthieu Moy Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, gitster, aaron, e On 06/09/2016 11:45 AM, Matthieu Moy wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > >> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt >> index edbba3a..21776f0 100644 >> --- a/Documentation/git-send-email.txt >> +++ b/Documentation/git-send-email.txt >> @@ -84,13 +84,16 @@ See the CONFIGURATION section for 'sendemail.multiEdit'. >> the value of GIT_AUTHOR_IDENT, or GIT_COMMITTER_IDENT if that is not >> set, as returned by "git var -l". >> >> ---in-reply-to=<identifier>:: >> +--in-reply-to=<Message-Id|email_file>:: >> Make the first mail (or all the mails with `--no-thread`) appear as a >> - reply to the given Message-Id, which avoids breaking threads to >> - provide a new patch series. >> + reply to the given Message-Id (given directly by argument or via the email >> + file), which avoids breaking threads to provide a new patch series. >> The second and subsequent emails will be sent as replies according to >> the `--[no]-chain-reply-to` setting. >> + >> +Furthermore, if the argument is an email file, parse it and populate header >> +fields appropriately for the reply. > > "populate header fields appropriately" would seem obscure to someone not > having followed this converation. At least s/fields/To: and Cc: fields/. We weren't sure how precise the documentation had to be, and tried to keep it concise. >> --- a/git-send-email.perl >> +++ b/git-send-email.perl >> @@ -55,6 +55,7 @@ git send-email --dump-aliases >> --[no-]bcc <str> * Email Bcc: >> --subject <str> * Email "Subject:" >> --in-reply-to <str> * Email "In-Reply-To:" >> + --in-reply-to <file> * Populate header fields appropriately. > > Likewise. To avoid an overly long line, I'd write just "Populate > To/Cc/In-reply-to". > > Probably <file> should be <email_file>. Thanks, will do in v5. >> +if ($initial_reply_to && -f $initial_reply_to) { >> + my $error = validate_patch($initial_reply_to); >> + die "fatal: $initial_reply_to: $error\nwarning: no patches were sent\n" >> + if $error; >> + >> + open my $fh, "<", $initial_reply_to or die "can't open file $initial_reply_to"; >> + my $mail = Git::parse_email($fh); >> + close $fh; >> + >> + my $initial_sender = $sender || $repoauthor || $repocommitter || ''; > > This is duplicated from the "if ($compose) { ... my $tpl_sender = ..." a > bit later in the existing file. It would be better to get this "my > $initial_sender = ..." out of your "if" and use $initial_sender directly > later IMHO. > > Actually, $initial_sender does not seem to be a good variable name. It's > not really "initial", right? $sender looks like a better name, I will work on that, thanks! >> + my $prefix_re = ""; >> + my $subject_re = $mail->{"subject"}[0]; >> + if ($subject_re =~ /^[^Re:]/) { >> + $prefix_re = "Re: "; >> + } >> + $initial_subject = $prefix_re . $subject_re; > > Why introduce $prefix_re. You can just > > my $subject = $mail->{"subject"}[0]; > if (...) { > $subject = "Re: " . $subject; > } > > (preferably using sensible as '...' as noted by Junio ;-) ). I will keep Junio's suggestion :-) > In previous iterations of this series, you had issues with non-ascii > characters in at least To: and Cc: fields (perhaps in the Subject field > too?). Are they solved? I don't see any tests about it ... Non-ascii characters are still an issue, I will work on that next week. ^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v4 6/6] send-email: add option --cite to quote the message body 2016-06-08 13:01 ` (unknown), Samuel GROOT ` (4 preceding siblings ...) 2016-06-08 13:07 ` [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields Samuel GROOT @ 2016-06-08 13:08 ` Samuel GROOT 2016-06-09 11:49 ` Matthieu Moy 5 siblings, 1 reply; 96+ messages in thread From: Samuel GROOT @ 2016-06-08 13:08 UTC (permalink / raw) To: git Cc: samuel.groot, tom.russello, erwan.mathoniere, jordan.de-gea, matthieu.moy, gitster, aaron, e If used with `in-reply-to=<email_file>`, cite the message body of the given email file. Otherwise, do nothing. If `--compose` is also set, quote the message body in the cover letter. Else, imply `--annotate` by default and quote the message body below the triple-dash section in the first patch only. Signed-off-by: Tom RUSSELLO <tom.russello@grenoble-inp.org> Signed-off-by: Samuel GROOT <samuel.groot@grenoble-inp.org> Signed-off-by: Matthieu MOY <matthieu.moy@grenoble-inp.fr> --- Documentation/git-send-email.txt | 8 ++++ git-send-email.perl | 81 ++++++++++++++++++++++++++++++++++++++-- t/t9001-send-email.sh | 32 ++++++++++++++++ 3 files changed, 117 insertions(+), 4 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 21776f0..23cbd17 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -109,6 +109,14 @@ illustration below where `[PATCH v2 0/3]` is in reply to `[PATCH 0/2]`: Only necessary if --compose is also set. If --compose is not set, this will be prompted for. +--cite:: + When used with `--in-reply-to=<email_file>`, quote the message body of the + given email file. ++ +If `--compose` is also set, the message cited will be in the cover letter. If +`--compose` is not set, `--annotate` option is implied by default and the +message body will be cited in the "below-triple-dash" section. + --subject=<string>:: Specify the initial subject of the email thread. Only necessary if --compose is also set. If --compose diff --git a/git-send-email.perl b/git-send-email.perl index b444ea6..6877ea7 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -26,6 +26,7 @@ use Text::ParseWords; use Term::ANSIColor; use File::Temp qw/ tempdir tempfile /; use File::Spec::Functions qw(catfile); +use File::Copy; use Error qw(:try); use Git; @@ -56,6 +57,8 @@ git send-email --dump-aliases --subject <str> * Email "Subject:" --in-reply-to <str> * Email "In-Reply-To:" --in-reply-to <file> * Populate header fields appropriately. + --cite * Quote the message body in the cover if + --compose is set, else in the first patch. --[no-]xmailer * Add "X-Mailer:" header (default). --[no-]annotate * Review each patch that will be sent in an editor. --compose * Open an editor for introduction. @@ -161,7 +164,7 @@ my $re_encoded_word = qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/; # Variables we fill in automatically, or via prompting: my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh, - $initial_reply_to,$initial_references,$initial_subject,@files, + $initial_reply_to,$initial_references,$cite,$initial_subject,@files, $author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time); my $envelope_sender; @@ -305,6 +308,7 @@ $rc = GetOptions( "sender|from=s" => \$sender, "in-reply-to=s" => \$initial_reply_to, "subject=s" => \$initial_subject, + "cite" => \$cite, "to=s" => \@initial_to, "to-cmd=s" => \$to_cmd, "no-to" => \$no_to, @@ -640,6 +644,7 @@ if (@files) { usage(); } +my $message_cited; if ($initial_reply_to && -f $initial_reply_to) { my $error = validate_patch($initial_reply_to); die "fatal: $initial_reply_to: $error\nwarning: no patches were sent\n" @@ -658,7 +663,8 @@ if ($initial_reply_to && -f $initial_reply_to) { } $initial_subject = $prefix_re . $subject_re; - push @initial_to, $mail->{"from"}[0]; + my $recipient = $mail->{"from"}[0]; + push @initial_to, $recipient; foreach my $to_addr (parse_address_line(join ",", @{$mail->{"to"}})) { if (!($to_addr eq $initial_sender)) { @@ -684,6 +690,25 @@ if ($initial_reply_to && -f $initial_reply_to) { $initial_references = join("", @{$mail->{"references"}}) . " " . $initial_reply_to; } + + if ($cite) { + my $date = $mail->{"date"}[0]; + my $tpl_date = $date && "On $date, " || ''; + $message_cited = $tpl_date . $recipient . " wrote:\n"; + + # Quote the message body + foreach (@{$mail->{"body"}}) { + my $space = ""; + if (/^[^>]/) { + $space = " "; + } + $message_cited .= ">" . $space . $_; + } + + if (!$compose) { + $annotate = 1; + } + } } sub get_patch_subject { @@ -711,6 +736,9 @@ if ($compose) { my $tpl_sender = $sender || $repoauthor || $repocommitter || ''; my $tpl_subject = $initial_subject || ''; my $tpl_reply_to = $initial_reply_to || ''; + my $tpl_quote = $message_cited && + "\nGIT: Please, trim down irrelevant sections in the cited message\n". + "GIT: to keep your email concise.\n" . $message_cited || ''; print $c <<EOT; From $tpl_sender # This line is ignored. @@ -722,7 +750,7 @@ GIT: Clear the body content if you don't wish to send a summary. From: $tpl_sender Subject: $tpl_subject In-Reply-To: $tpl_reply_to - +$tpl_quote EOT for my $f (@files) { print $c get_patch_subject($f); @@ -787,7 +815,52 @@ EOT $compose = -1; } } elsif ($annotate) { - do_edit(@files); + if ($message_cited) { + my $cite_email_filename = ($repo ? + tempfile(".gitsendemail.msg.XXXXXX", + DIR => $repo->repo_path()) : + tempfile(".gitsendemail.msg.XXXXXX", + DIR => "."))[1]; + + # Insertion in a temporary file to keep the original file clean + # in case of cancellation/error. + do_insert_cited_message($cite_email_filename, $files[0]); + + my $tmp = $files[0]; + $files[0] = $cite_email_filename; + + do_edit(@files); + + # Erase the original patch if the edition went well + move($cite_email_filename, $tmp); + $files[0] = $tmp; + } else { + do_edit(@files); + } +} + +sub do_insert_cited_message { + my $tmp_file = shift; + my $original_file = shift; + + open my $c, "<", $original_file + or die "Failed to open $original_file: " . $!; + + open my $c2, ">", $tmp_file + or die "Failed to open $tmp_file: " . $!; + + # Insertion after the triple-dash + while (<$c>) { + print $c2 $_; + last if (/^---$/); + } + print $c2 $message_cited; + while (<$c>) { + print $c2 $_; + } + + close $c; + close $c2; } sub ask { diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 7591342..29e28f2 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -1915,6 +1915,7 @@ test_expect_success $PREREQ 'Fields with --in-reply-to are correct' ' git send-email \ --in-reply-to=email \ --from="Example <nobody@example.com>" \ + --cite \ --smtp-server="$(pwd)/fake.sendmail" \ -2 \ 2>errors && @@ -1936,10 +1937,22 @@ test_expect_success $PREREQ 'Fields with --in-reply-to are correct' ' echo "$ref_adr" | grep -v "References: <author_123456@example.com>" ' +test_expect_success $PREREQ 'correct cited message with --in-reply-to' ' + msg_cited=$(grep -A 3 "^---$" msgtxt1) && + echo "$msg_cited" | grep "On Sat, 12 Jun 2010 15:53:58 +0200, author@example.com wrote:" && + echo "$msg_cited" | grep "> Have you seen my previous email?" && + echo "$msg_cited" | grep ">> Previous content" +' + +test_expect_success $PREREQ 'second patch body is not modified by --in-reply-to' ' + ! grep "Have you seen my previous email?" msgtxt2 +' + test_expect_success $PREREQ 'Fields with --in-reply-to and --compose are correct' ' clean_fake_sendmail && git send-email \ --in-reply-to=email \ + --cite \ --compose \ --from="Example <nobody@example.com>" \ --smtp-server="$(pwd)/fake.sendmail" \ @@ -1967,6 +1980,7 @@ test_expect_success $PREREQ 'Fields with --in-reply-to and --compose are correct test_expect_success $PREREQ 'Re: written only once with --in-reply-to and --compose ' ' git send-email \ --in-reply-to=msgtxt1 \ + --cite \ --compose \ --from="Example <nobody@example.com>" \ --smtp-server="$(pwd)/fake.sendmail" \ @@ -1975,4 +1989,22 @@ test_expect_success $PREREQ 'Re: written only once with --in-reply-to and --comp grep "Subject: Re: subject goes here" msgtxt3 ' +test_expect_success $PREREQ 'correct cited message with --in-reply-to and --compose' ' + grep "> On Sat, 12 Jun 2010 15:53:58 +0200, author@example.com wrote:" msgtxt3 && + grep ">> Have you seen my previous email?" msgtxt3 && + grep ">>> Previous content" msgtxt3 +' + +test_expect_success $PREREQ 'Message is not cited with only --in-reply-to' ' + clean_fake_sendmail && + git send-email \ + --in-reply-to=email \ + --compose \ + --from="Example <nobody@example.com>" \ + --smtp-server="$(pwd)/fake.sendmail" \ + -1 \ + 2>errors && + ! grep "Have you seen my previous email?" msgtxt1 +' + test_done -- 2.8.2.537.gb153d2a ^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body 2016-06-08 13:08 ` [PATCH v4 6/6] send-email: add option --cite to quote the message body Samuel GROOT @ 2016-06-09 11:49 ` Matthieu Moy 2016-06-14 22:53 ` Samuel GROOT 2016-06-15 22:21 ` Tom Russello 0 siblings, 2 replies; 96+ messages in thread From: Matthieu Moy @ 2016-06-09 11:49 UTC (permalink / raw) To: Samuel GROOT Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, gitster, aaron, e Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > If used with `in-reply-to=<email_file>`, cite the message body of the given > email file. Otherwise, do nothing. It should at least warn when --in-reply-to=<email_file> is not given (either no --in-reply-to or --in-reply-to=<id>). I don't see any use-case where a user would want --cite on the command-line and not want --in-reply-to=<email_file>. OTOH, it seems a plausible user-error, and the user would appreciate a message saying what's going on. > @@ -56,6 +57,8 @@ git send-email --dump-aliases > --subject <str> * Email "Subject:" > --in-reply-to <str> * Email "In-Reply-To:" > --in-reply-to <file> * Populate header fields appropriately. > + --cite * Quote the message body in the cover if > + --compose is set, else in the first patch. > --[no-]xmailer * Add "X-Mailer:" header (default). > --[no-]annotate * Review each patch that will be sent in an editor. > --compose * Open an editor for introduction. Just wondering: would it make sense to activate --cite by default when --in-reply-to=file is used, and to allow --no-cite to disable this? This is something we can easily do now without breaking backward compatibility (--in-reply-to=file doesn't exist yet), but would be more painful to do later. > @@ -640,6 +644,7 @@ if (@files) { > usage(); > } > > +my $message_cited; Nit: I read "$message_cited" as "Boolean saying whether the message was cited". $cited_message would be clearer to me (but this is to be taken with a grain of salt as I'm not a native speaker), since the variable holds the content of the cited message. > +sub do_insert_cited_message { > + my $tmp_file = shift; > + my $original_file = shift; > + > + open my $c, "<", $original_file > + or die "Failed to open $original_file: " . $!; > + > + open my $c2, ">", $tmp_file > + or die "Failed to open $tmp_file: " . $!; > + > + # Insertion after the triple-dash > + while (<$c>) { > + print $c2 $_; > + last if (/^---$/); > + } > + print $c2 $message_cited; I would add a newline here to get a blank line between the message cited and the diffstat. I think non-ascii characters would deserve particular attention here too. For example, if the patch contain only ascii and the cited part contains UTF-8, does the generated patch have a proper Content-type: header? I can imagine worse, like a patch containing latin1 character and a cited message with another 8-bit encoding. > +test_expect_success $PREREQ 'correct cited message with --in-reply-to and --compose' ' > + grep "> On Sat, 12 Jun 2010 15:53:58 +0200, author@example.com wrote:" msgtxt3 && I would prefer to have the full address including the real name here (A <author@example.com>) in this example. Actually, after a quick look at the code, I don't understand where the name has gone (what's shown here is extracted from the From: header). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body 2016-06-09 11:49 ` Matthieu Moy @ 2016-06-14 22:53 ` Samuel GROOT 2016-06-15 22:21 ` Tom Russello 1 sibling, 0 replies; 96+ messages in thread From: Samuel GROOT @ 2016-06-14 22:53 UTC (permalink / raw) To: Matthieu Moy Cc: git, tom.russello, erwan.mathoniere, jordan.de-gea, gitster, aaron, e On 06/09/2016 01:49 PM, Matthieu Moy wrote: > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > >> If used with `in-reply-to=<email_file>`, cite the message body of the given >> email file. Otherwise, do nothing. > > It should at least warn when --in-reply-to=<email_file> is not given > (either no --in-reply-to or --in-reply-to=<id>). I don't see any > use-case where a user would want --cite on the command-line and not want > --in-reply-to=<email_file>. OTOH, it seems a plausible user-error, and > the user would appreciate a message saying what's going on. We weren't sure how to warn the user. If --in-reply-to is not an mail file, should we check it with a regex to make sure it's a message-id? >> @@ -56,6 +57,8 @@ git send-email --dump-aliases >> --subject <str> * Email "Subject:" >> --in-reply-to <str> * Email "In-Reply-To:" >> --in-reply-to <file> * Populate header fields appropriately. >> + --cite * Quote the message body in the cover if >> + --compose is set, else in the first patch. >> --[no-]xmailer * Add "X-Mailer:" header (default). >> --[no-]annotate * Review each patch that will be sent in an editor. >> --compose * Open an editor for introduction. > > Just wondering: would it make sense to activate --cite by default when > --in-reply-to=file is used, and to allow --no-cite to disable this? > > This is something we can easily do now without breaking backward > compatibility (--in-reply-to=file doesn't exist yet), but would be more > painful to do later. It's an interesting question. IMHO we should have `--cite` by default and allow `--no-cite` to disable quoting the message body, because it's easier to remove extra unwanted lines than copying lines from another file and adding "> " before each line. >> @@ -640,6 +644,7 @@ if (@files) { >> usage(); >> } >> >> +my $message_cited; > > Nit: I read "$message_cited" as "Boolean saying whether the message was > cited". $cited_message would be clearer to me (but this is to be taken > with a grain of salt as I'm not a native speaker), since the variable > holds the content of the cited message. > >> +sub do_insert_cited_message { >> + my $tmp_file = shift; >> + my $original_file = shift; >> + >> + open my $c, "<", $original_file >> + or die "Failed to open $original_file: " . $!; >> + >> + open my $c2, ">", $tmp_file >> + or die "Failed to open $tmp_file: " . $!; >> + >> + # Insertion after the triple-dash >> + while (<$c>) { >> + print $c2 $_; >> + last if (/^---$/); >> + } >> + print $c2 $message_cited; > > I would add a newline here to get a blank line between the message cited > and the diffstat. A newline here makes it easier to distinguish the different sections in the email file indeed. > I think non-ascii characters would deserve particular attention here > too. For example, if the patch contain only ascii and the cited part > contains UTF-8, does the generated patch have a proper Content-type: > header? Non-ascii characters are still an issue, I'll work on that next week. > I can imagine worse, like a patch containing latin1 character and a > cited message with another 8-bit encoding. > >> +test_expect_success $PREREQ 'correct cited message with --in-reply-to and --compose' ' >> + grep "> On Sat, 12 Jun 2010 15:53:58 +0200, author@example.com wrote:" msgtxt3 && > > I would prefer to have the full address including the real name here (A > <author@example.com>) in this example. Actually, after a quick look at > the code, I don't understand where the name has gone (what's shown here > is extracted from the From: header). Yep, after a quick look at the code involved, I don't understand either, I will investigate this week. ^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body 2016-06-09 11:49 ` Matthieu Moy 2016-06-14 22:53 ` Samuel GROOT @ 2016-06-15 22:21 ` Tom Russello 1 sibling, 0 replies; 96+ messages in thread From: Tom Russello @ 2016-06-15 22:21 UTC (permalink / raw) To: Matthieu Moy, Samuel GROOT Cc: git, erwan.mathoniere, jordan.de-gea, gitster, aaron, e Le 09/06/2016 à 13:49, Matthieu Moy a écrit : > Samuel GROOT <samuel.groot@grenoble-inp.org> writes: > >> If used with `in-reply-to=<email_file>`, cite the message body of the given >> email file. Otherwise, do nothing. > > It should at least warn when --in-reply-to=<email_file> is not given > (either no --in-reply-to or --in-reply-to=<id>). I don't see any > use-case where a user would want --cite on the command-line and not want > --in-reply-to=<email_file>. OTOH, it seems a plausible user-error, and > the user would appreciate a message saying what's going on. You're right. We'll warn the user with the next version. >> @@ -56,6 +57,8 @@ git send-email --dump-aliases >> --subject <str> * Email "Subject:" >> --in-reply-to <str> * Email "In-Reply-To:" >> --in-reply-to <file> * Populate header fields appropriately. >> + --cite * Quote the message body in the cover if >> + --compose is set, else in the first patch. >> --[no-]xmailer * Add "X-Mailer:" header (default). >> --[no-]annotate * Review each patch that will be sent in an editor. >> --compose * Open an editor for introduction. > > Just wondering: would it make sense to activate --cite by default when > --in-reply-to=file is used, and to allow --no-cite to disable this? > This is something we can easily do now without breaking backward > compatibility (--in-reply-to=file doesn't exist yet), but would be more > painful to do later. Indeed, it can be more intuitive especially for a user who is used to cite messages. >> @@ -640,6 +644,7 @@ if (@files) { >> usage(); >> } >> >> +my $message_cited; > > Nit: I read "$message_cited" as "Boolean saying whether the message was > cited". $cited_message would be clearer to me (but this is to be taken > with a grain of salt as I'm not a native speaker), since the variable > holds the content of the cited message. Sorry, French habits.. :-) >> +sub do_insert_cited_message { >> + my $tmp_file = shift; >> + my $original_file = shift; >> + >> + open my $c, "<", $original_file >> + or die "Failed to open $original_file: " . $!; >> + >> + open my $c2, ">", $tmp_file >> + or die "Failed to open $tmp_file: " . $!; >> + >> + # Insertion after the triple-dash >> + while (<$c>) { >> + print $c2 $_; >> + last if (/^---$/); >> + } >> + print $c2 $message_cited; > > I would add a newline here to get a blank line between the message cited > and the diffstat. Agreed. > I think non-ascii characters would deserve particular attention here > too. For example, if the patch contain only ascii and the cited part > contains UTF-8, does the generated patch have a proper Content-type: > header? > > I can imagine worse, like a patch containing latin1 character and a > cited message with another 8-bit encoding. I tried to manage them with the built-in Base64 module but there is still work in progress. >> +test_expect_success $PREREQ 'correct cited message with --in-reply-to and --compose' ' >> + grep "> On Sat, 12 Jun 2010 15:53:58 +0200, author@example.com wrote:" msgtxt3 && > > I would prefer to have the full address including the real name here (A > <author@example.com>) in this example. Actually, after a quick look at > the code, I don't understand where the name has gone (what's shown here > is extracted from the From: header). Agreed, I'll figure out where the problem is. ^ permalink raw reply [flat|nested] 96+ messages in thread
end of thread, other threads:[~2016-06-15 22:21 UTC | newest] Thread overview: 96+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-23 19:30 [RFC-PATCH 0/2] send-email: new --quote-mail option Tom Russello 2016-05-23 19:30 ` [RFC-PATCH 1/2] send-email: new option to quote an email and reply to Tom Russello 2016-05-23 19:55 ` Eric Wong 2016-05-23 20:07 ` Matthieu Moy 2016-05-23 22:10 ` Samuel GROOT 2016-05-24 12:43 ` Samuel GROOT 2016-05-24 12:49 ` Matthieu Moy 2016-05-24 22:30 ` Aaron Schrab 2016-05-25 0:04 ` Tom Russello 2016-05-24 21:23 ` Eric Wong 2016-05-23 20:00 ` Matthieu Moy 2016-05-24 23:31 ` Samuel GROOT 2016-05-25 6:29 ` Matthieu Moy 2016-05-25 15:40 ` Junio C Hamano 2016-05-25 16:56 ` Matthieu Moy 2016-05-25 18:15 ` Junio C Hamano 2016-05-25 18:31 ` Matthieu Moy 2016-05-26 0:08 ` Samuel GROOT 2016-05-27 9:06 ` Matthieu Moy 2016-05-23 19:30 ` [RFC-PATCH 2/2] t9001: adding --quote-mail option test Tom Russello 2016-05-23 20:05 ` Matthieu Moy 2016-05-23 19:38 ` [RFC-PATCH 0/2] send-email: new --quote-mail option Matthieu Moy 2016-05-23 19:56 ` Samuel GROOT 2016-05-27 17:11 ` [RFC-PATCH v2 0/2] send-email: new --quote-email option Tom Russello 2016-05-27 17:11 ` [RFC-PATCH v2 1/2] send-email: quote-email populates the fields Tom Russello 2016-05-28 14:35 ` Matthieu Moy 2016-05-29 23:38 ` Tom Russello 2016-05-27 17:11 ` [RFC-PATCH v2 2/2] send-email: quote-email quotes the message body Tom Russello 2016-05-28 15:01 ` Matthieu Moy 2016-05-29 11:41 ` Tom Russello 2016-06-07 14:01 ` [PATCH v3 0/6] send-email: cleaner tests and quote email Tom Russello 2016-06-07 14:01 ` [PATCH v3 1/6] t9001: non order-sensitive file comparison Tom Russello 2016-06-08 1:07 ` Junio C Hamano 2016-06-08 8:23 ` Samuel GROOT 2016-06-08 16:09 ` Junio C Hamano 2016-06-08 16:46 ` Samuel GROOT 2016-06-09 6:01 ` Matthieu Moy 2016-06-13 22:21 ` Samuel GROOT 2016-06-09 5:51 ` Matthieu Moy 2016-06-09 8:15 ` Tom Russello 2016-06-07 14:01 ` [PATCH v3 2/6] t9001: check email address is in Cc: field Tom Russello 2016-06-09 5:55 ` Matthieu Moy 2016-06-13 22:23 ` Samuel GROOT 2016-06-07 14:01 ` [PATCH v3 3/6] t9001: shorten send-email's output Tom Russello 2016-06-08 8:36 ` Eric Wong 2016-06-08 9:30 ` Samuel GROOT 2016-06-09 6:03 ` Matthieu Moy 2016-06-07 14:01 ` [PATCH v3 4/6] send-email: create email parser subroutine Tom Russello 2016-06-07 14:05 ` [PATCH v3 5/6] send-email: --in-reply-to=<file> populates the fields Tom Russello 2016-06-07 14:05 ` [PATCH v3 6/6] send-email: add option --cite to quote the message body Tom Russello 2016-06-08 13:01 ` (unknown), Samuel GROOT 2016-06-08 13:01 ` [PATCH v4 1/6] t9001: non order-sensitive file comparison Samuel GROOT 2016-06-08 14:22 ` Remi Galan Alfonso 2016-06-08 14:29 ` Samuel GROOT 2016-06-08 16:56 ` Junio C Hamano 2016-06-08 19:21 ` Samuel GROOT 2016-06-08 17:17 ` Junio C Hamano 2016-06-08 19:19 ` Samuel GROOT 2016-06-08 13:01 ` [PATCH v4 2/6] t9001: check email address is in Cc: field Samuel GROOT 2016-06-08 17:34 ` Junio C Hamano 2016-06-08 19:23 ` Samuel GROOT 2016-06-08 13:01 ` [PATCH v4 3/6] send-email: shorten send-email's output Samuel GROOT 2016-06-08 17:37 ` Junio C Hamano 2016-06-08 19:18 ` Samuel GROOT 2016-06-08 19:33 ` Junio C Hamano 2016-06-08 19:40 ` Samuel GROOT 2016-06-09 6:17 ` Matthieu Moy 2016-06-13 22:19 ` Samuel GROOT 2016-06-08 13:01 ` [PATCH v4 4/6] send-email: create email parser subroutine Samuel GROOT 2016-06-08 17:58 ` Junio C Hamano 2016-06-08 18:12 ` Eric Sunshine 2016-06-08 18:32 ` Junio C Hamano 2016-06-08 19:26 ` Samuel GROOT 2016-06-08 19:31 ` Junio C Hamano 2016-06-08 19:42 ` Samuel GROOT 2016-06-08 19:30 ` Samuel GROOT 2016-06-08 20:13 ` Eric Sunshine 2016-06-08 20:17 ` Junio C Hamano 2016-06-08 23:54 ` Samuel GROOT 2016-06-09 0:21 ` Eric Wong 2016-06-13 22:18 ` Samuel GROOT 2016-06-13 22:47 ` Eric Wong 2016-06-14 22:18 ` Samuel GROOT 2016-06-09 6:51 ` Eric Sunshine 2016-06-13 22:15 ` Samuel GROOT 2016-06-08 19:36 ` Samuel GROOT 2016-06-08 20:38 ` Eric Wong 2016-06-08 13:07 ` [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields Samuel GROOT 2016-06-08 18:23 ` Junio C Hamano 2016-06-14 22:26 ` Samuel GROOT 2016-06-09 9:45 ` Matthieu Moy 2016-06-14 22:35 ` Samuel GROOT 2016-06-08 13:08 ` [PATCH v4 6/6] send-email: add option --cite to quote the message body Samuel GROOT 2016-06-09 11:49 ` Matthieu Moy 2016-06-14 22:53 ` Samuel GROOT 2016-06-15 22:21 ` Tom Russello
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).