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

[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(-)

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

[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

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

[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

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

[Matthieu Moy]

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/

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

[Eric Wong]

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

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

[Samuel GROOT]

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 !

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

[Matthieu Moy]

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/

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

[Matthieu Moy]

> 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/

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

[Matthieu Moy]

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/

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

[Samuel GROOT]

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

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

[Samuel GROOT]

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

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

[Matthieu Moy]

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/

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

[Eric Wong]

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

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

[Aaron Schrab]

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.

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

[Samuel GROOT]

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

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

[Tom Russello]

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"

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

[Matthieu Moy]

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/

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

[Junio C Hamano]

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.

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

[Matthieu Moy]

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/

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

[Junio C Hamano]

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!

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

[Matthieu Moy]

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/

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

[Samuel GROOT]

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.

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

[Matthieu Moy]

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/

[RFC-PATCH v2 0/2] send-email: new --quote-email 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-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(-)

[RFC-PATCH v2 1/2] send-email: quote-email populates the fields

[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

[RFC-PATCH v2 2/2] send-email: quote-email quotes the message body

[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

Re: [RFC-PATCH v2 1/2] send-email: quote-email populates the fields

[Matthieu Moy]

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/

Re: [RFC-PATCH v2 2/2] send-email: quote-email quotes the message body

[Matthieu Moy]

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/

Re: [RFC-PATCH v2 2/2] send-email: quote-email quotes the message body

[Tom Russello]

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.

Re: [RFC-PATCH v2 1/2] send-email: quote-email populates the fields

[Tom Russello]

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.

[PATCH v3 0/6] send-email: cleaner tests and quote email

[Tom Russello]

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

[PATCH v3 1/6] t9001: non order-sensitive file comparison

[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

[PATCH v3 2/6] t9001: check email address is in Cc: field

[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

[PATCH v3 3/6] t9001: shorten send-email's output

[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

[PATCH v3 4/6] send-email: create email parser subroutine

[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

[PATCH v3 5/6] send-email: --in-reply-to=<file> populates the fields

[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

[PATCH v3 6/6] send-email: add option --cite to quote the message body

[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

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Junio C Hamano]

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

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Samuel GROOT]

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.

Re: [PATCH v3 3/6] t9001: shorten send-email's output

[Eric Wong]

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.

Re: [PATCH v3 3/6] t9001: shorten send-email's output

[Samuel GROOT]

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

(unknown),

[Samuel GROOT]

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

[PATCH v4 1/6] t9001: non order-sensitive file comparison

[Samuel GROOT]

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

[PATCH v4 2/6] t9001: check email address is in Cc: field

[Samuel GROOT]

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

[PATCH v4 3/6] send-email: shorten send-email's output

[Samuel GROOT]

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

[PATCH v4 4/6] send-email: create email parser subroutine

[Samuel GROOT]

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

[PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields

[Samuel GROOT]

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

[PATCH v4 6/6] send-email: add option --cite to quote the message body

[Samuel GROOT]

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

Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison

[Remi Galan Alfonso]

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

Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison

[Samuel GROOT]

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!

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Junio C Hamano]

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

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Samuel GROOT]

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

Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison

[Junio C Hamano]

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?

Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison

[Junio C Hamano]

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
 '
 

Re: [PATCH v4 2/6] t9001: check email address is in Cc: field

[Junio C Hamano]

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

Re: [PATCH v4 3/6] send-email: shorten send-email's output

[Junio C Hamano]

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?

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Junio C Hamano]

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?

> +}

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Eric Sunshine]

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.

Re: [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields

[Junio C Hamano]

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

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Junio C Hamano]

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.

Re: [PATCH v4 3/6] send-email: shorten send-email's output

[Samuel GROOT]

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.

Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison

[Samuel GROOT]

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.

Re: [PATCH v4 1/6] t9001: non order-sensitive file comparison

[Samuel GROOT]

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.

Re: [PATCH v4 2/6] t9001: check email address is in Cc: field

[Samuel GROOT]

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

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Samuel GROOT]

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?

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Samuel GROOT]

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.

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Junio C Hamano]

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.

Re: [PATCH v4 3/6] send-email: shorten send-email's output

[Junio C Hamano]

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

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Samuel GROOT]

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.

Re: [PATCH v4 3/6] send-email: shorten send-email's output

[Samuel GROOT]

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

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Samuel GROOT]

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.

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Eric Sunshine]

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

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Junio C Hamano]

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?

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Eric Wong]

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.

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Samuel GROOT]

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

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Eric Wong]

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

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Matthieu Moy]

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/

Re: [PATCH v3 2/6] t9001: check email address is in Cc: field

[Matthieu Moy]

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/

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Matthieu Moy]

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/

Re: [PATCH v3 3/6] t9001: shorten send-email's output

[Matthieu Moy]

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/

Re: [PATCH v4 3/6] send-email: shorten send-email's output

[Matthieu Moy]

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/

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Eric Sunshine]

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.

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Tom Russello]

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.

Re: [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields

[Matthieu Moy]

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/

Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body

[Matthieu Moy]

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/

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Samuel GROOT]

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?

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Samuel GROOT]

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?

Re: [PATCH v4 3/6] send-email: shorten send-email's output

[Samuel GROOT]

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.

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Samuel GROOT]

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)

Re: [PATCH v3 2/6] t9001: check email address is in Cc: field

[Samuel GROOT]

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.

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Eric Wong]

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.

Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Samuel GROOT]

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.

Re: [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields

[Samuel GROOT]

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.

Re: [PATCH v4 5/6] send-email: --in-reply-to=<file> populate header fields

[Samuel GROOT]

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.

Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body

[Samuel GROOT]

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.

Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body

[Tom Russello]

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.