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

Samuel GROOT [off-list ref] writes:

"populate header fields appropriately" would seem obscure to someone not
having followed this converation. At least s/fields/To: and Cc: fields/.

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/

On 06/09/2016 11:45 AM, Matthieu Moy wrote:

Samuel GROOT [off-list ref] writes:

quoted

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.

quoted

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

quoted

+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!

quoted

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