[RFC-PATCH 0/2] send-email: new --quote-mail option

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[RFC-PATCH 0/2] send-email: new --quote-mail option

Tom Russello
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(-)

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Tom Russello
From: Tom Russello <[hidden email]>

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 <[hidden email]>
Signed-off-by: Samuel Groot <[hidden email]>
Signed-off-by: Matthieu Moy <[hidden email]>

---

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[RFC-PATCH 2/2] t9001: adding --quote-mail option test

Tom Russello
In reply to this post by Tom Russello
From: Tom Russello <[hidden email]>

Tests if the "To", "Cc" and "Subject" fields are adequately filled and if the
message is correctly quoted.

Signed-off-by: Tom Russello <[hidden email]>
Signed-off-by: Samuel Groot <[hidden email]>
Signed-off-by: Matthieu Moy <[hidden email]>

---


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: <[hidden email]>
+ From: [hidden email]
+ To: [hidden email]
+ Cc: [hidden email]
+ 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 <[hidden email]>" \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ -1 \
+ 2>errors &&
+ grep "From: Example <[hidden email]>" msgtxt1 &&
+ to_adr=$(awk "/^To: /,/^Cc: /" msgtxt1) &&
+ echo "$to_adr" | grep [hidden email] &&
+ echo "$to_adr" | grep [hidden email] &&
+ grep "Cc: [hidden email]" 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 <[hidden email]>" \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ -1 \
+ 2>errors &&
+ grep "Subject: Re: subject goes here" msgtxt3
+'
+
 test_done
--
2.8.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 0/2] send-email: new --quote-mail option

Matthieu Moy-2
In reply to this post by Tom Russello
Tom Russello <[hidden email]> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Eric Wong-2
In reply to this post by Tom Russello
Tom Russello <[hidden email]> 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...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 0/2] send-email: new --quote-mail option

Samuel GROOT-2
In reply to this post by Matthieu Moy-2


On 05/23/2016 09:38 PM, Matthieu Moy wrote:

> Tom Russello <[hidden email]> 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 !
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Matthieu Moy-2
In reply to this post by Tom Russello
Tom Russello <[hidden email]> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 2/2] t9001: adding --quote-mail option test

Matthieu Moy-2
In reply to this post by 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 <[hidden email]> writes:

> From: Tom Russello <[hidden email]>

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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Matthieu Moy-2
In reply to this post by Eric Wong-2
Eric Wong <[hidden email]> writes:

> Tom Russello <[hidden email]> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Samuel GROOT-2


On 05/23/2016 10:07 PM, Matthieu Moy wrote:

> Eric Wong <[hidden email]> writes:
>
>> Tom Russello <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Samuel GROOT-2
In reply to this post by Eric Wong-2


On 05/23/2016 09:55 PM, Eric Wong wrote:
> Tom Russello <[hidden email]> 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`.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Matthieu Moy-2
Samuel GROOT <[hidden email]> writes:

> On 05/23/2016 09:55 PM, Eric Wong wrote:
>> Tom Russello <[hidden email]> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Eric Wong-2
In reply to this post by Samuel GROOT-2
Samuel GROOT <[hidden email]> 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).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Aaron Schrab
In reply to this post by Matthieu Moy-2
At 14:49 +0200 24 May 2016, Matthieu Moy <[hidden email]> wrote:

>Samuel GROOT <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Samuel GROOT-2
In reply to this post by Matthieu Moy-2
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Tom Russello
In reply to this post by Aaron Schrab
On 05/25/16 00:30, Aaron Schrab wrote:

> At 14:49 +0200 24 May 2016, Matthieu Moy <[hidden email]>
> wrote:
>> Samuel GROOT <[hidden email]> 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"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Matthieu Moy-2
In reply to this post by Samuel GROOT-2
Samuel GROOT <[hidden email]> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Junio C Hamano
Matthieu Moy <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Matthieu Moy-2
Junio C Hamano <[hidden email]> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

Junio C Hamano
Matthieu Moy <[hidden email]> 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!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
12