[PATCH 0/2] New send-email option --quote-email

[PATCH 0/2] New send-email option --quote-email

[Payre Nathan]

Those patches implements a new --quote-email=<file> option.

Typical use case: the user receives a bug report by email and replies with a patch.
Before this patch, to make a proper reply, the user had to perform
several steps manually using "git send-email":

* Add --in-reply-to=<message_id> to the command-line for proper
  threading.

* Include the original recipients of the message using --to and --cc.

* Copy and prefix the original message with '> ' in the "below triple
  dash" part of the patch.

This patch allows send-email to do most of the job for the user, who can
now save the email to a file and use:

  git send-email --quote-email=<file>

"To" and "Cc" will be added automaticaly and the email quoted.
It's possible to edit the email before sending with --compose.

Based-on-patch-by: Tom Russello <tom.russello@grenoble-inp.org>
Signed-off: Nathan Payre <nathan.payre@etu.univ-lyon1.fr>
Signed-off: Matthieu Moy <matthieu.moy@univ-lyon1.fr>

Tom Russello (2):
  quote-email populates the fields
  send-email: quote-email quotes the message body

 Documentation/git-send-email.txt |   5 ++
 git-send-email.perl              | 146 +++++++++++++++++++++++++++++++++++++--
 t/t9001-send-email.sh            | 134 ++++++++++++++++++++++++-----------
 3 files changed, 240 insertions(+), 45 deletions(-)

-- 
2.14.2

[PATCH 1/2] quote-email populates the fields

[Payre Nathan]

From: Tom Russello <tom.russello@grenoble-inp.org>

---
 Documentation/git-send-email.txt |   3 +
 git-send-email.perl              |  70 ++++++++++++++++++++++-
 t/t9001-send-email.sh            | 117 +++++++++++++++++++++++++--------------
 3 files changed, 147 insertions(+), 43 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index bac9014ac..710b5ff32 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -106,6 +106,9 @@ 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>::
+	Fill appropriately header fields for the reply to the given email.
+
 --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 2208dcc21..665c47d15 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -57,6 +57,7 @@ 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> * 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.
@@ -166,7 +167,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,$quote_email,$initial_subject,@files,
 	$author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
 
 my $envelope_sender;
@@ -316,6 +317,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,
@@ -652,6 +654,70 @@ if (@files) {
 	usage();
 }
 
+if ($quote_email) {
+	my $error = validate_patch($quote_email);
+	die "fatal: $quote_email: $error\nwarning: no patches were sent\n"
+		if $error;
+
+	my @header = ();
+
+	open my $fh, "<", $quote_email or die "can't open file $quote_email";
+
+	# Get the email header
+	while (<$fh>) {
+		# Turn crlf line endings into lf-only
+		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 $initial_sender = $sender || $repoauthor || $repocommitter || '';
+
+		chomp;
+
+		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;
+		} elsif (/^References:\s+(.*)/i) {
+			$initial_references = $1;
+		}
+	}
+	$initial_references = $initial_references . $initial_reply_to;
+}
+
 sub get_patch_subject {
 	my $fn = shift;
 	open (my $fh, '<', $fn);
@@ -1488,7 +1554,7 @@ EOF
 }
 
 $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 f30980895..ce12a1164 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1917,52 +1917,87 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '
 	test_cmp expected-list actual-list
 '
 
-test_expect_success $PREREQ 'invoke hook' '
-	mkdir -p .git/hooks &&
-
-	write_script .git/hooks/sendemail-validate <<-\EOF &&
-	# test that we have the correct environment variable, pwd, and
-	# argument
-	case "$GIT_DIR" in
-	*.git)
-		true
-		;;
-	*)
-		false
-		;;
-	esac &&
-	test -f 0001-add-master.patch &&
-	grep "add master" "$1"
-	EOF
-
-	mkdir subdir &&
-	(
-		# Test that it works even if we are not at the root of the
-		# working tree
-		cd subdir &&
-		git send-email \
-			--from="Example <nobody@example.com>" \
-			--to=nobody@example.com \
-			--smtp-server="$(pwd)/../fake.sendmail" \
-			../0001-add-master.patch &&
+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>
 
-		# Verify error message when a patch is rejected by the hook
-		sed -e "s/add master/x/" ../0001-add-master.patch >../another.patch &&
-		git send-email \
-			--from="Example <nobody@example.com>" \
-			--to=nobody@example.com \
-			--smtp-server="$(pwd)/../fake.sendmail" \
-			../another.patch 2>err
-		test_i18ngrep "rejected by sendemail-validate hook" err
-	)
+	Have you seen my previous email?
+	> Previous content
+	EOF
 '
 
-test_expect_success $PREREQ 'test that send-email works outside a repo' '
-	nongit git send-email \
+test_expect_success $PREREQ 'Fields with --quote-email are correct' '
+	clean_fake_sendmail &&
+	git send-email \
+		--quote-email=email \
 		--from="Example <nobody@example.com>" \
-		--to=nobody@example.com \
 		--smtp-server="$(pwd)/fake.sendmail" \
-		"$(pwd)/0001-add-master.patch"
+		-1 \
+		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 --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: /{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 --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.14.2

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

[Payre Nathan]

From: Tom Russello <tom.russello@grenoble-inp.org>

---
 Documentation/git-send-email.txt |  4 +-
 git-send-email.perl              | 80 ++++++++++++++++++++++++++++++++++++++--
 t/t9001-send-email.sh            | 19 +++++++++-
 3 files changed, 97 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 710b5ff32..329af66af 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -107,7 +107,9 @@ Only necessary if --compose is also set.  If --compose
 is not set, this will be prompted for.
 
 --quote-email=<email_file>::
-	Fill appropriately header fields for the reply to the given email.
+	Fill appropriately header fields for the reply to the given email and quote
+	the message body in the cover letter if `--compose` is set or otherwise
+	after the triple-dash in the first patch given.
 
 --subject=<string>::
 	Specify the initial subject of the email thread.
diff --git a/git-send-email.perl b/git-send-email.perl
index 665c47d15..6f6995c9d 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(catdir catfile);
+use File::Copy;
 use Error qw(:try);
 use Cwd qw(abs_path cwd);
 use Git;
@@ -57,7 +58,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> * Populate header fields appropriately.
+    --quote-email           <file> * Populate header fields 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.
@@ -654,12 +656,15 @@ if (@files) {
 	usage();
 }
 
+my $message_quoted;
 if ($quote_email) {
 	my $error = validate_patch($quote_email);
 	die "fatal: $quote_email: $error\nwarning: no patches were sent\n"
 		if $error;
 
 	my @header = ();
+	my $date;
+	my $recipient;
 
 	open my $fh, "<", $quote_email or die "can't open file $quote_email";
 
@@ -691,7 +696,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)) {
@@ -713,9 +719,28 @@ if ($quote_email) {
 			$initial_reply_to = $1;
 		} elsif (/^References:\s+(.*)/i) {
 			$initial_references = $1;
+		} elsif (/^Date: (.*)/i) {
+			$date = $1;
 		}
 	}
 	$initial_references = $initial_references . $initial_reply_to;
+
+	my $tpl_date = $date && "On $date, " || '';
+	$message_quoted = $tpl_date . $recipient . " wrote:\n";
+
+	# Quote the message body
+	while (<$fh>) {
+		# Turn crlf line endings into lf-only
+		s/\r//g;
+		my $space = "";
+		if (/^[^>]/) {
+			$space = " ";
+		}
+		$message_quoted .= ">" . $space . $_;
+	}
+	if (!$compose) {
+		$annotate = 1;
+	}
 }
 
 sub get_patch_subject {
@@ -743,6 +768,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 <<EOT1, Git::prefix_lines("GIT: ", __ <<EOT2), <<EOT3;
 From $tpl_sender # This line is ignored.
@@ -756,7 +784,7 @@ EOT2
 From: $tpl_sender
 Subject: $tpl_subject
 In-Reply-To: $tpl_reply_to
-
+$tpl_quote
 EOT3
 	for my $f (@files) {
 		print $c get_patch_subject($f);
@@ -821,9 +849,53 @@ EOT3
 		$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];
+
+		# Insertion in a temporary file to keep the original file clean
+		# in case of cancellation/error.
+		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 if the edition went well
+		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 ce12a1164..7c29c829d 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1941,7 +1941,7 @@ test_expect_success $PREREQ 'Fields with --quote-email are correct' '
 		--quote-email=email \
 		--from="Example <nobody@example.com>" \
 		--smtp-server="$(pwd)/fake.sendmail" \
-		-1 \
+		-2 \
 		2>errors &&
 	grep "From: Example <nobody@example.com>" msgtxt1 &&
 	grep "In-Reply-To: <author_123456@example.com>" msgtxt1 &&
@@ -1961,6 +1961,17 @@ test_expect_success $PREREQ 'Fields with --quote-email are correct' '
 	echo "$ref_adr" | grep -v "References: <author_123456@example.com>"
 '
 
+test_expect_success $PREREQ 'correct quoted message with --quote-email' '
+	msg_quoted=$(grep -A 3 "^---$" msgtxt1) &&
+	echo "$msg_quoted" | grep "On Sat, 12 Jun 2010 15:53:58 +0200, author@example.com wrote:" &&
+	echo "$msg_quoted" | grep "> Have you seen my previous email?" &&
+	echo "$msg_quoted" | grep ">> Previous content"
+'
+
+test_expect_success $PREREQ 'second patch body is not modified by --quote-email' '
+	! grep "Have you seen my previous email?" msgtxt2
+'
+
 test_expect_success $PREREQ 'Fields with --quote-email and --compose are correct' '
 	clean_fake_sendmail &&
 	git send-email \
@@ -2000,4 +2011,10 @@ test_expect_success $PREREQ 'Re: written only once with --quote-email and --comp
 	grep "Subject: Re: subject goes here" msgtxt3
 '
 
+test_expect_success $PREREQ 'correct quoted message with --quote-email 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_done
-- 
2.14.2

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

[Junio C Hamano]

Payre Nathan <second.payre@gmail.com> writes:

> From: Tom Russello <tom.russello@grenoble-inp.org>
>
> ---

Missing something here???

>  Documentation/git-send-email.txt |   3 +
>  git-send-email.perl              |  70 ++++++++++++++++++++++-
>  t/t9001-send-email.sh            | 117 +++++++++++++++++++++++++--------------
>  3 files changed, 147 insertions(+), 43 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index bac9014ac..710b5ff32 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -106,6 +106,9 @@ 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>::
> +	Fill appropriately header fields for the reply to the given email.
> +

The cover letter said:

    This patch allows send-email to do most of the job for the user, who can
    now save the email to a file and use:

      git send-email --quote-email=<file>

    "To" and "Cc" will be added automaticaly and the email quoted.
    It's possible to edit the email before sending with --compose.

and I somehow expected to see the body of the e-mail this option is
"quoting" to be also inserted in the text.  After all, that is what
"quote" means.

But the description above (and the code below, judging from the way
the reading from $fh that was opened form $quote_email stops at the
first blank line, aka end of header) says what is happening is quite
different.  The contents of the file is used to extract what the
user would have given to --cc/--to/--in-reply-to from the command
line by looking at it, if this option were not available.

I personally prefer the "pick up the header information so that the
user do not have to formulate the command line options" behaviour
that does *NOT* quote the body of the message into the outgoing
message.  So:

 * Do not call this option "quote" anything; you are not quoting,
   just using some info from the given file.  

   I wonder if we can simply reuse "--in-reply-to" option for this
   purpose.  If it is a message id and not a file on the filesystem,
   we behave just as before.  Otherwise we try to open it as a file
   and grab the "Message-ID:" header from it and use it.

 * The description "Fill *appropriately* header fileds" is useless,
   as what looks "appropriate" to you is not clear/known to
   readers.  Instead, say what header is filled with what
   information (e.g. "find Message-Id: and place its value on
   In-Reply-To: header").

   For that matter, "To and CC will be added automatically" in the
   coer letter is still vague; are you reading To/CC in the given
   file and placing their values on some (unnamed) header of the
   outgoing message?  Or are you reading some (unnamed) header in
   the given file and placing their values on To/CC header of the
   outging message?

> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2208dcc21..665c47d15 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -57,6 +57,7 @@ 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> * Populate header fields appropriately.

Likewise.  If what's "appropriate" is clear to the readers, the word
in this description adds no value because everybody would know how
fields are populated.  Otherwise, it does not add any value because
everybody would have no clue how fields are populated.

> @@ -652,6 +654,70 @@ if (@files) {
>  	usage();
>  }
>  
> +if ($quote_email) {
> +	my $error = validate_patch($quote_email);
> +	die "fatal: $quote_email: $error\nwarning: no patches were sent\n"
> +		if $error;

validate_patch() calls sendemail-validate hook that is expecting to
be fed a patch email you are going to send out that might have
errors so that it can catch it and save you from embarrassment.  The
file you are feeding it is *NOT* what you are going to send out, but
is what you are responding to with your patch.  Even if it had an
embarassing error as a patch, that is not something you care about
(and it is something you received, so catching this late won't save
the sender from embarrassment anyway).

> +
> +	my @header = ();
> +
> +	open my $fh, "<", $quote_email or die "can't open file $quote_email";
> +
> +	# Get the email header
> +	while (<$fh>) {
> +		# Turn crlf line endings into lf-only
> +		s/\r//g;
> +		last if /^\s*$/;
> +		if (/^\s+\S/ and @header) {

I wonder how significant this requirement to have at least one "\S"
on the line is.  I know you copied&pasted this from the main sending
loop, so this is not a new issue and not something we may want to
fix in this patch.

> +			chomp($header[$#header]);
> +			s/^\s+/ /;
> +			$header[$#header] .= $_;
> +		} else {
> +			push(@header, $_);
> +		}
> +	}

You do not use $fh after this point.  Do not force readers to
realize that fact by scanning to the end of the function--instead,
close it here.

> +	# Parse the header
> +	foreach (@header) {
> +		my $initial_sender = $sender || $repoauthor || $repocommitter || '';
> +
> +		chomp;
> +
> +		if (/^Subject:\s+(.*)$/i) {
> +			my $prefix_re = "";
> +			my $subject_re = $1;

What does "_re" mean in the variable name $subject_re?

> +			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)) {

This if() condition makes a policy decision; shouldn't it honor the
setting of "--[no-]suppress-from", "--suppress-cc" and friends?

> +					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;
> +		} elsif (/^References:\s+(.*)/i) {
> +			$initial_references = $1;
> +		}
> +	}
> +	$initial_references = $initial_references . $initial_reply_to;

I cannot see how this can produce correct result by simply
concatenating them with nothing in between.  Shouldn't you make sure
there is a SP in between, at least?

By the way, if you are adding a new variable $initial_references,
make sure it is initialized to either an empty string or an undef
(and if you choose to do the latter, the right hand side of this
assignment cannot blindly reference $initial_references that could
still be undef); the way the existing code handles $initial_reply_to
may serve as an example.

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

[Junio C Hamano]

Payre Nathan <second.payre@gmail.com> writes:

> From: Tom Russello <tom.russello@grenoble-inp.org>
>
> ---
>  Documentation/git-send-email.txt |  4 +-
>  git-send-email.perl              | 80 ++++++++++++++++++++++++++++++++++++++--
>  t/t9001-send-email.sh            | 19 +++++++++-
>  3 files changed, 97 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 710b5ff32..329af66af 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -107,7 +107,9 @@ Only necessary if --compose is also set.  If --compose
>  is not set, this will be prompted for.
>  
>  --quote-email=<email_file>::
> -	Fill appropriately header fields for the reply to the given email.
> +	Fill appropriately header fields for the reply to the given email and quote
> +	the message body in the cover letter if `--compose` is set or otherwise
> +	after the triple-dash in the first patch given.

Hmmm.  I have a strong suspicion that people want an option to
trigger the feature from just 1/2 but not 2/2 some of the time.
Sure, removing the unwanted lines in the compose editor may be easy,
but it feels wasteful use of user's time to include the lines of
text from the original only to have them removed.

Also, if you are not offering these two as separate features, there
isn't much point splitting them into two patches.

> @@ -743,6 +768,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 <<EOT1, Git::prefix_lines("GIT: ", __ <<EOT2), <<EOT3;
>  From $tpl_sender # This line is ignored.
> @@ -756,7 +784,7 @@ EOT2
>  From: $tpl_sender
>  Subject: $tpl_subject
>  In-Reply-To: $tpl_reply_to
> -
> +$tpl_quote
>  EOT3

OK, by emitting it into $compose_filename as part of the front
matter, you get the "do we have to do mime?" etc. for free, which
sort-of makes sense.

> @@ -821,9 +849,53 @@ EOT3
>  		$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];
> +
> +		# Insertion in a temporary file to keep the original file clean
> +		# in case of cancellation/error.
> +		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 if the edition went well
> +		move($quote_email_filename, $tmp);
> +		$files[0] = $tmp;
> +	} else {
> +		do_edit(@files);
> +	}
>  }

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Payre Nathan <second.payre@gmail.com> writes:
>
>> From: Tom Russello <tom.russello@grenoble-inp.org>
>>
>> ---
>
> Missing something here???

To clarify for Nathan, Thimothee and Danial: the cover-letter is an
introduction send before the patch series. It can be needed to explain
the overall approach followed by the series. But in general, it does not
end up in the Git history, i.e. after the review is finished, the
cover-letter is forgotten.

OTOH, the commit messages for each patch is what ends up in the Git
history. This is what people will find later when running e.g. "git
blame", "git bisect" or so. Clearly the future user examining history
expects more than "quote-email populates the fields" (which was a good
reminder during development, but is actually a terrible subject line for
a final version).

A quick advice: if in doubt, prefer writing explanations in commit
message rather than the cover letter. If still in doubt, write the
explanations twice: once quickly in the cover letter and once more
detailed in the commit message.

>  * Do not call this option "quote" anything; you are not quoting,
>    just using some info from the given file.  
>
>    I wonder if we can simply reuse "--in-reply-to" option for this
>    purpose.  If it is a message id and not a file on the filesystem,
>    we behave just as before.  Otherwise we try to open it as a file
>    and grab the "Message-ID:" header from it and use it.

There's a possible ambiguity since user may in theory want to run
"--in-reply-to=msgid" with a file named msgid and still want the old
behavior. But: a real message-id is typically something rather cryptic
and it is safe to assume that users won't have a file named exactly like
an actual message id containing something which isn't the message in
question.

The main drawback I see in re-using "--in-reply-to" is that typos are
hard to miss. For example, running

  git send-email --in-reply-to=msgi

when the user actually wanted msgid would trigger a different behavior
instead of raising an error (no such file or directory). I think it's
acceptable: in the current form of send-email, we're already not
user-friendly to users when they write a typo in the --in-reply-to
argument (and we can't really detect typos anyway).

>> +if ($quote_email) {
>> +	my $error = validate_patch($quote_email);
>> +	die "fatal: $quote_email: $error\nwarning: no patches were sent\n"
>> +		if $error;
>
> validate_patch() calls sendemail-validate hook that is expecting to
> be fed a patch email you are going to send out that might have
> errors so that it can catch it and save you from embarrassment.  The
> file you are feeding it is *NOT* what you are going to send out, but
> is what you are responding to with your patch.  Even if it had an
> embarassing error as a patch, that is not something you care about
> (and it is something you received, so catching this late won't save
> the sender from embarrassment anyway).

I think the intention was to detect cases when $quote_email is not a
patch at all (and give a proper error message instead of trying to
continue with probably absurd behavior).

But I agree that there's no point in being too strict here, and if that
was the intension then it should be documented with a comment.

-- 
Matthieu Moy
https://matthieu-moy.fr/

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Hmmm.  I have a strong suspicion that people want an option to
> trigger the feature from just 1/2 but not 2/2 some of the time.
> Sure, removing the unwanted lines in the compose editor may be easy,
> but it feels wasteful use of user's time to include the lines of
> text from the original only to have them removed.

So, that could be

  git send-email --in-reply-to=message-id  # message-id is not a file
  => existing behavior

  git send-email --in-reply-to=file
  => populate To:, Cc:, In-Reply-To: and References:

  git send-email --in-reply-to=file --quote
  => in addition to the above, include the quoted message in the body

(perhaps --quote should be --cite, I'm not sure which one looks best for
a native speaker)

This also leaves room for

  git send-email --in-reply-to=message-id --fetch [--quote]
  => download the message body from e.g. public-inbox and do the same as
     for --in-reply-to=file

(which doesn't have to be implemented now, but would be a nice-to-have
in the future)

-- 
Matthieu Moy
https://matthieu-moy.fr/

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

[Stefan Beller]

On Wed, Nov 1, 2017 at 4:04 AM, Matthieu Moy <Matthieu.Moy@univ-lyon1.fr> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Payre Nathan <second.payre@gmail.com> writes:
>>
>>> From: Tom Russello <tom.russello@grenoble-inp.org>
>>>
>>> ---
>>
>> Missing something here???
>
> To clarify for Nathan, Thimothee and Danial: the cover-letter is an
> introduction send before the patch series. It can be needed to explain
> the overall approach followed by the series. But in general, it does not
> end up in the Git history, i.e. after the review is finished, the
> cover-letter is forgotten.
>
> OTOH, the commit messages for each patch is what ends up in the Git
> history. This is what people will find later when running e.g. "git
> blame", "git bisect" or so. Clearly the future user examining history
> expects more than "quote-email populates the fields" (which was a good
> reminder during development, but is actually a terrible subject line for
> a final version).
>
> A quick advice: if in doubt, prefer writing explanations in commit
> message rather than the cover letter. If still in doubt, write the
> explanations twice: once quickly in the cover letter and once more
> detailed in the commit message.

Oh, and I thought the sign offs.

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

[Nathan PAYRE]

I Will send the modification in the next patch, I prefer to refractor
a part of the code before.

>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 2208dcc21..665c47d15 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -57,6 +57,7 @@ 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> * Populate header fields appropriately.

> Likewise.  If what's "appropriate" is clear to the readers, the word
> in this description adds no value because everybody would know how
> fields are populated.  Otherwise, it does not add any value because
> everybody would have no clue how fields are populated.

Remove "approprietly" done.


>> @@ -652,6 +654,70 @@ if (@files) {
>>       usage();
>>  }
>>
>> +if ($quote_email) {
>> +     my $error = validate_patch($quote_email);
>> +     die "fatal: $quote_email: $error\nwarning: no patches were sent\n"
>> +             if $error;

> validate_patch() calls sendemail-validate hook that is expecting to
> be fed a patch email you are going to send out that might have
> errors so that it can catch it and save you from embarrassment.  The
> file you are feeding it is *NOT* what you are going to send out, but
> is what you are responding to with your patch.  Even if it had an
> embarassing error as a patch, that is not something you care about
> (and it is something you received, so catching this late won't save
>  the sender from embarrassment anyway).

I will remove lines which use validate_patch().


>> +                     chomp($header[$#header]);
>> +                     s/^\s+/ /;
>> +                     $header[$#header] .= $_;
>> +             } else {
>> +                     push(@header, $_);
>> +             }
>> +     }

> You do not use $fh after this point.  Do not force readers to
> realize that fact by scanning to the end of the function--instead,
> close it here.

In fact $fh is reuse at the end of the if($quote_email) {} but if you
don't see it maybe it's because it's anormal to reuse it after a
long block of code, that's why I think to create a subroutine
for the following code which is similar to the part of if($compose).

foreach (@header) {
   my $initial_sender = $sender || $repoauthor || $repocommitter || '';

   chomp;

   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) {
      $recipient = $1;
      push @initial_to, $recipient;
   } 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;
   } elsif (/^References:\s+(.*)/i) {
      $initial_references = $1;
   } elsif (/^Date: (.*)/i) {
   $date = $1;
   }
}


I close $fh after the second call then.


>> +     # Parse the header
>> +     foreach (@header) {
>> +             my $initial_sender = $sender || $repoauthor || $repocommitter || '';
>> +
>> +             chomp;
>> +
>> +             if (/^Subject:\s+(.*)$/i) {
>> +                     my $prefix_re = "";
>> +                     my $subject_re = $1;

> What does "_re" mean in the variable name $subject_re?

"_re" mean regular expression but maybe it's clumsy because
it contain the result of a regular expression. What do you think
about rename it into "$prefix" and "$subject" ?


2017-11-01 3:44 GMT+01:00 Junio C Hamano <gitster@pobox.com>:
> Payre Nathan <second.payre@gmail.com> writes:
>
>> From: Tom Russello <tom.russello@grenoble-inp.org>
>>
>> ---
>
> Missing something here???
>
>>  Documentation/git-send-email.txt |   3 +
>>  git-send-email.perl              |  70 ++++++++++++++++++++++-
>>  t/t9001-send-email.sh            | 117 +++++++++++++++++++++++++--------------
>>  3 files changed, 147 insertions(+), 43 deletions(-)
>>
>> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
>> index bac9014ac..710b5ff32 100644
>> --- a/Documentation/git-send-email.txt
>> +++ b/Documentation/git-send-email.txt
>> @@ -106,6 +106,9 @@ 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>::
>> +     Fill appropriately header fields for the reply to the given email.
>> +
>
> The cover letter said:
>
>     This patch allows send-email to do most of the job for the user, who can
>     now save the email to a file and use:
>
>       git send-email --quote-email=<file>
>
>     "To" and "Cc" will be added automaticaly and the email quoted.
>     It's possible to edit the email before sending with --compose.
>
> and I somehow expected to see the body of the e-mail this option is
> "quoting" to be also inserted in the text.  After all, that is what
> "quote" means.
>
> But the description above (and the code below, judging from the way
> the reading from $fh that was opened form $quote_email stops at the
> first blank line, aka end of header) says what is happening is quite
> different.  The contents of the file is used to extract what the
> user would have given to --cc/--to/--in-reply-to from the command
> line by looking at it, if this option were not available.
>
> I personally prefer the "pick up the header information so that the
> user do not have to formulate the command line options" behaviour
> that does *NOT* quote the body of the message into the outgoing
> message.  So:
>
>  * Do not call this option "quote" anything; you are not quoting,
>    just using some info from the given file.
>
>    I wonder if we can simply reuse "--in-reply-to" option for this
>    purpose.  If it is a message id and not a file on the filesystem,
>    we behave just as before.  Otherwise we try to open it as a file
>    and grab the "Message-ID:" header from it and use it.
>
>  * The description "Fill *appropriately* header fileds" is useless,
>    as what looks "appropriate" to you is not clear/known to
>    readers.  Instead, say what header is filled with what
>    information (e.g. "find Message-Id: and place its value on
>    In-Reply-To: header").
>
>    For that matter, "To and CC will be added automatically" in the
>    coer letter is still vague; are you reading To/CC in the given
>    file and placing their values on some (unnamed) header of the
>    outgoing message?  Or are you reading some (unnamed) header in
>    the given file and placing their values on To/CC header of the
>    outging message?
>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 2208dcc21..665c47d15 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -57,6 +57,7 @@ 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> * Populate header fields appropriately.
>
> Likewise.  If what's "appropriate" is clear to the readers, the word
> in this description adds no value because everybody would know how
> fields are populated.  Otherwise, it does not add any value because
> everybody would have no clue how fields are populated.
>
>> @@ -652,6 +654,70 @@ if (@files) {
>>       usage();
>>  }
>>
>> +if ($quote_email) {
>> +     my $error = validate_patch($quote_email);
>> +     die "fatal: $quote_email: $error\nwarning: no patches were sent\n"
>> +             if $error;
>
> validate_patch() calls sendemail-validate hook that is expecting to
> be fed a patch email you are going to send out that might have
> errors so that it can catch it and save you from embarrassment.  The
> file you are feeding it is *NOT* what you are going to send out, but
> is what you are responding to with your patch.  Even if it had an
> embarassing error as a patch, that is not something you care about
> (and it is something you received, so catching this late won't save
> the sender from embarrassment anyway).
>
>> +
>> +     my @header = ();
>> +
>> +     open my $fh, "<", $quote_email or die "can't open file $quote_email";
>> +
>> +     # Get the email header
>> +     while (<$fh>) {
>> +             # Turn crlf line endings into lf-only
>> +             s/\r//g;
>> +             last if /^\s*$/;
>> +             if (/^\s+\S/ and @header) {
>
> I wonder how significant this requirement to have at least one "\S"
> on the line is.  I know you copied&pasted this from the main sending
> loop, so this is not a new issue and not something we may want to
> fix in this patch.
>
>> +                     chomp($header[$#header]);
>> +                     s/^\s+/ /;
>> +                     $header[$#header] .= $_;
>> +             } else {
>> +                     push(@header, $_);
>> +             }
>> +     }
>
> You do not use $fh after this point.  Do not force readers to
> realize that fact by scanning to the end of the function--instead,
> close it here.
>
>> +     # Parse the header
>> +     foreach (@header) {
>> +             my $initial_sender = $sender || $repoauthor || $repocommitter || '';
>> +
>> +             chomp;
>> +
>> +             if (/^Subject:\s+(.*)$/i) {
>> +                     my $prefix_re = "";
>> +                     my $subject_re = $1;
>
> What does "_re" mean in the variable name $subject_re?
>
>> +                     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)) {
>
> This if() condition makes a policy decision; shouldn't it honor the
> setting of "--[no-]suppress-from", "--suppress-cc" and friends?
>
>> +                                     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;
>> +             } elsif (/^References:\s+(.*)/i) {
>> +                     $initial_references = $1;
>> +             }
>> +     }
>> +     $initial_references = $initial_references . $initial_reply_to;
>
> I cannot see how this can produce correct result by simply
> concatenating them with nothing in between.  Shouldn't you make sure
> there is a SP in between, at least?
>
> By the way, if you are adding a new variable $initial_references,
> make sure it is initialized to either an empty string or an undef
> (and if you choose to do the latter, the right hand side of this
> assignment cannot blindly reference $initial_references that could
> still be undef); the way the existing code handles $initial_reply_to
> may serve as an example.