[PATCH v2 1/2] send-email: Rename variable for clarity

[PATCH v2 1/2] send-email: Rename variable for clarity

[Christian Ludwig]

The SMTP protocol has both, the 'Reply-To' and the 'In-Reply-To' header
fields. We only use the latter. To avoid confusion, rename the variable
for it.

Signed-off-by: Christian Ludwig <chrissicool@gmail.com>
---
 git-send-email.perl | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index edcc6d346..0c07f48d5 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -166,13 +166,13 @@ 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_in_reply_to,$initial_subject,@files,
 	$author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
 
 my $envelope_sender;
 
 # Example reply to:
-#$initial_reply_to = ''; #<20050203173208.GA23964@foobar.com>';
+#$initial_in_reply_to = ''; #<20050203173208.GA23964@foobar.com>';
 
 my $repo = eval { Git->repository() };
 my @repo = $repo ? ($repo) : ();
@@ -314,7 +314,7 @@ die __("--dump-aliases incompatible with other options\n")
     if !$help and $dump_aliases and @ARGV;
 $rc = GetOptions(
 		    "sender|from=s" => \$sender,
-                    "in-reply-to=s" => \$initial_reply_to,
+                    "in-reply-to=s" => \$initial_in_reply_to,
 		    "subject=s" => \$initial_subject,
 		    "to=s" => \@initial_to,
 		    "to-cmd=s" => \$to_cmd,
@@ -676,7 +676,7 @@ if ($compose) {
 
 	my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
 	my $tpl_subject = $initial_subject || '';
-	my $tpl_reply_to = $initial_reply_to || '';
+	my $tpl_in_reply_to = $initial_in_reply_to || '';
 
 	print $c <<EOT1, Git::prefix_lines("GIT: ", __ <<EOT2), <<EOT3;
 From $tpl_sender # This line is ignored.
@@ -689,7 +689,7 @@ Clear the body content if you don't wish to send a summary.
 EOT2
 From: $tpl_sender
 Subject: $tpl_subject
-In-Reply-To: $tpl_reply_to
+In-Reply-To: $tpl_in_reply_to
 
 EOT3
 	for my $f (@files) {
@@ -736,7 +736,7 @@ EOT3
 				quote_subject($subject, $compose_encoding) .
 				"\n";
 		} elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
-			$initial_reply_to = $1;
+			$initial_in_reply_to = $1;
 			next;
 		} elsif (/^From:\s*(.+)\s*$/i) {
 			$sender = $1;
@@ -872,16 +872,16 @@ sub expand_one_alias {
 @initial_cc = process_address_list(@initial_cc);
 @bcclist = process_address_list(@bcclist);
 
-if ($thread && !defined $initial_reply_to && $prompting) {
-	$initial_reply_to = ask(
+if ($thread && !defined $initial_in_reply_to && $prompting) {
+	$initial_in_reply_to = ask(
 		__("Message-ID to be used as In-Reply-To for the first email (if any)? "),
 		default => "",
 		valid_re => qr/\@.*\./, confirm_only => 1);
 }
-if (defined $initial_reply_to) {
-	$initial_reply_to =~ s/^\s*<?//;
-	$initial_reply_to =~ s/>?\s*$//;
-	$initial_reply_to = "<$initial_reply_to>" if $initial_reply_to ne '';
+if (defined $initial_in_reply_to) {
+	$initial_in_reply_to =~ s/^\s*<?//;
+	$initial_in_reply_to =~ s/>?\s*$//;
+	$initial_in_reply_to = "<$initial_in_reply_to>" if $initial_in_reply_to ne '';
 }
 
 if (!defined $smtp_server) {
@@ -901,7 +901,7 @@ if ($compose && $compose > 0) {
 }
 
 # Variables we set as part of the loop over files
-our ($message_id, %mail, $subject, $reply_to, $references, $message,
+our ($message_id, %mail, $subject, $in_reply_to, $references, $message,
 	$needs_confirm, $message_num, $ask_default);
 
 sub extract_valid_address {
@@ -1310,9 +1310,9 @@ Message-Id: $message_id
 	if ($use_xmailer) {
 		$header .= "X-Mailer: git-send-email $gitversion\n";
 	}
-	if ($reply_to) {
+	if ($in_reply_to) {
 
-		$header .= "In-Reply-To: $reply_to\n";
+		$header .= "In-Reply-To: $in_reply_to\n";
 		$header .= "References: $references\n";
 	}
 	if (@xh) {
@@ -1489,8 +1489,8 @@ EOF
 	return 1;
 }
 
-$reply_to = $initial_reply_to;
-$references = $initial_reply_to || '';
+$in_reply_to = $initial_in_reply_to;
+$references = $initial_in_reply_to || '';
 $subject = $initial_subject;
 $message_num = 0;
 
@@ -1700,9 +1700,9 @@ foreach my $t (@files) {
 
 	# set up for the next message
 	if ($thread && $message_was_sent &&
-		($chain_reply_to || !defined $reply_to || length($reply_to) == 0 ||
+		($chain_reply_to || !defined $in_reply_to || length($in_reply_to) == 0 ||
 		$message_num == 1)) {
-		$reply_to = $message_id;
+		$in_reply_to = $message_id;
 		if (length $references > 0) {
 			$references .= "\n $message_id";
 		} else {
-- 
2.15.1

[PATCH v2 2/2] send-email: Support separate Reply-To address

[Christian Ludwig]

In some projects contributions from groups are only accepted from a
common group email address. But every individual may want to recieve
replies to her own personal address. That's what we have 'Reply-To'
headers for in SMTP.

Introduce an optional '--reply-to' command line option. Unfortunately
the $reply_to variable name was already taken for the 'In-Reply-To'
header field. To reduce code churn, use $reply_address as variable
name instead.

Signed-off-by: Christian Ludwig <chrissicool@gmail.com>
---
 Documentation/git-send-email.txt       |  5 +++++
 contrib/completion/git-completion.bash |  2 +-
 git-send-email.perl                    | 18 +++++++++++++++++-
 t/t9001-send-email.sh                  |  2 ++
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 8060ea35c..71ef97ba9 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -84,6 +84,11 @@ 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".
 
+--reply-to=<address>::
+	Specify the address where replies from recipients should go to.
+	Use this if replies to messages should go to another address than what
+	is specified with the --from parameter.
+
 --in-reply-to=<identifier>::
 	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
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3683c772c..2a0dc4eef 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2081,7 +2081,7 @@ _git_send_email ()
 			--compose --confirm= --dry-run --envelope-sender
 			--from --identity
 			--in-reply-to --no-chain-reply-to --no-signed-off-by-cc
-			--no-suppress-from --no-thread --quiet
+			--no-suppress-from --no-thread --quiet --reply-to
 			--signed-off-by-cc --smtp-pass --smtp-server
 			--smtp-server-port --smtp-encryption= --smtp-user
 			--subject --suppress-cc= --suppress-from --thread --to
diff --git a/git-send-email.perl b/git-send-email.perl
index 0c07f48d5..9bf758307 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -56,6 +56,7 @@ git send-email --dump-aliases
     --[no-]cc               <str>  * Email Cc:
     --[no-]bcc              <str>  * Email Bcc:
     --subject               <str>  * Email "Subject:"
+    --reply-to              <str>  * Email "Reply-To:"
     --in-reply-to           <str>  * Email "In-Reply-To:"
     --[no-]xmailer                 * Add "X-Mailer:" header (default).
     --[no-]annotate                * Review each patch that will be sent in an editor.
@@ -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_in_reply_to,$initial_subject,@files,
+	$initial_in_reply_to,$reply_to,$initial_subject,@files,
 	$author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
 
 my $envelope_sender;
@@ -315,6 +316,7 @@ die __("--dump-aliases incompatible with other options\n")
 $rc = GetOptions(
 		    "sender|from=s" => \$sender,
                     "in-reply-to=s" => \$initial_in_reply_to,
+		    "reply-to=s" => \$reply_to,
 		    "subject=s" => \$initial_subject,
 		    "to=s" => \@initial_to,
 		    "to-cmd=s" => \$to_cmd,
@@ -677,6 +679,7 @@ if ($compose) {
 	my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
 	my $tpl_subject = $initial_subject || '';
 	my $tpl_in_reply_to = $initial_in_reply_to || '';
+	my $tpl_reply_to = $reply_to || '';
 
 	print $c <<EOT1, Git::prefix_lines("GIT: ", __ <<EOT2), <<EOT3;
 From $tpl_sender # This line is ignored.
@@ -688,6 +691,7 @@ for the patch you are writing.
 Clear the body content if you don't wish to send a summary.
 EOT2
 From: $tpl_sender
+Reply-To: $tpl_reply_to
 Subject: $tpl_subject
 In-Reply-To: $tpl_in_reply_to
 
@@ -738,6 +742,9 @@ EOT3
 		} elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
 			$initial_in_reply_to = $1;
 			next;
+		} elsif (/^Reply-To:\s*(.+)\s*$/i) {
+			$reply_to = $1;
+			next;
 		} elsif (/^From:\s*(.+)\s*$/i) {
 			$sender = $1;
 			next;
@@ -884,6 +891,12 @@ if (defined $initial_in_reply_to) {
 	$initial_in_reply_to = "<$initial_in_reply_to>" if $initial_in_reply_to ne '';
 }
 
+if (defined $reply_to) {
+	$reply_to =~ s/^\s+|\s+$//g;
+	($reply_to) = expand_aliases($reply_to);
+	$reply_to = sanitize_address($reply_to);
+}
+
 if (!defined $smtp_server) {
 	my @sendmail_paths = qw( /usr/sbin/sendmail /usr/lib/sendmail );
 	push @sendmail_paths, map {"$_/sendmail"} split /:/, $ENV{PATH};
@@ -1315,6 +1328,9 @@ Message-Id: $message_id
 		$header .= "In-Reply-To: $in_reply_to\n";
 		$header .= "References: $references\n";
 	}
+	if ($reply_to) {
+		$header .= "Reply-To: $reply_to\n";
+	}
 	if (@xh) {
 		$header .= join("\n", @xh) . "\n";
 	}
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 81869d891..c62318a78 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -205,6 +205,7 @@ Message-Id: MESSAGE-ID-STRING
 X-Mailer: X-MAILER-STRING
 In-Reply-To: <unique-message-id@example.com>
 References: <unique-message-id@example.com>
+Reply-To: Reply <reply@example.com>
 
 Result: OK
 EOF
@@ -297,6 +298,7 @@ test_expect_success $PREREQ 'Show all headers' '
 		--dry-run \
 		--suppress-cc=sob \
 		--from="Example <from@example.com>" \
+		--reply-to="Reply <reply@example.com>" \
 		--to=to@example.com \
 		--cc=cc@example.com \
 		--bcc=bcc@example.com \
-- 
2.15.1

Re: [PATCH v2 1/2] send-email: Rename variable for clarity

[Junio C Hamano]

Christian Ludwig <chrissicool@googlemail.com> writes:

> The SMTP protocol has both, the 'Reply-To' and the 'In-Reply-To' header
> fields. We only use the latter. To avoid confusion, rename the variable
> for it.
>
> Signed-off-by: Christian Ludwig <chrissicool@gmail.com>
> ---

Makes sense.

>  git-send-email.perl | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index edcc6d346..0c07f48d5 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -166,13 +166,13 @@ 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_in_reply_to,$initial_subject,@files,
>  	$author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
>  
>  my $envelope_sender;
>  
>  # Example reply to:
> -#$initial_reply_to = ''; #<20050203173208.GA23964@foobar.com>';
> +#$initial_in_reply_to = ''; #<20050203173208.GA23964@foobar.com>';
>  
>  my $repo = eval { Git->repository() };
>  my @repo = $repo ? ($repo) : ();
> @@ -314,7 +314,7 @@ die __("--dump-aliases incompatible with other options\n")
>      if !$help and $dump_aliases and @ARGV;
>  $rc = GetOptions(
>  		    "sender|from=s" => \$sender,
> -                    "in-reply-to=s" => \$initial_reply_to,
> +                    "in-reply-to=s" => \$initial_in_reply_to,
>  		    "subject=s" => \$initial_subject,
>  		    "to=s" => \@initial_to,
>  		    "to-cmd=s" => \$to_cmd,
> @@ -676,7 +676,7 @@ if ($compose) {
>  
>  	my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
>  	my $tpl_subject = $initial_subject || '';
> -	my $tpl_reply_to = $initial_reply_to || '';
> +	my $tpl_in_reply_to = $initial_in_reply_to || '';
>  
>  	print $c <<EOT1, Git::prefix_lines("GIT: ", __ <<EOT2), <<EOT3;
>  From $tpl_sender # This line is ignored.
> @@ -689,7 +689,7 @@ Clear the body content if you don't wish to send a summary.
>  EOT2
>  From: $tpl_sender
>  Subject: $tpl_subject
> -In-Reply-To: $tpl_reply_to
> +In-Reply-To: $tpl_in_reply_to
>  
>  EOT3
>  	for my $f (@files) {
> @@ -736,7 +736,7 @@ EOT3
>  				quote_subject($subject, $compose_encoding) .
>  				"\n";
>  		} elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
> -			$initial_reply_to = $1;
> +			$initial_in_reply_to = $1;
>  			next;
>  		} elsif (/^From:\s*(.+)\s*$/i) {
>  			$sender = $1;
> @@ -872,16 +872,16 @@ sub expand_one_alias {
>  @initial_cc = process_address_list(@initial_cc);
>  @bcclist = process_address_list(@bcclist);
>  
> -if ($thread && !defined $initial_reply_to && $prompting) {
> -	$initial_reply_to = ask(
> +if ($thread && !defined $initial_in_reply_to && $prompting) {
> +	$initial_in_reply_to = ask(
>  		__("Message-ID to be used as In-Reply-To for the first email (if any)? "),
>  		default => "",
>  		valid_re => qr/\@.*\./, confirm_only => 1);
>  }
> -if (defined $initial_reply_to) {
> -	$initial_reply_to =~ s/^\s*<?//;
> -	$initial_reply_to =~ s/>?\s*$//;
> -	$initial_reply_to = "<$initial_reply_to>" if $initial_reply_to ne '';
> +if (defined $initial_in_reply_to) {
> +	$initial_in_reply_to =~ s/^\s*<?//;
> +	$initial_in_reply_to =~ s/>?\s*$//;
> +	$initial_in_reply_to = "<$initial_in_reply_to>" if $initial_in_reply_to ne '';
>  }
>  
>  if (!defined $smtp_server) {
> @@ -901,7 +901,7 @@ if ($compose && $compose > 0) {
>  }
>  
>  # Variables we set as part of the loop over files
> -our ($message_id, %mail, $subject, $reply_to, $references, $message,
> +our ($message_id, %mail, $subject, $in_reply_to, $references, $message,
>  	$needs_confirm, $message_num, $ask_default);
>  
>  sub extract_valid_address {
> @@ -1310,9 +1310,9 @@ Message-Id: $message_id
>  	if ($use_xmailer) {
>  		$header .= "X-Mailer: git-send-email $gitversion\n";
>  	}
> -	if ($reply_to) {
> +	if ($in_reply_to) {
>  
> -		$header .= "In-Reply-To: $reply_to\n";
> +		$header .= "In-Reply-To: $in_reply_to\n";
>  		$header .= "References: $references\n";
>  	}
>  	if (@xh) {
> @@ -1489,8 +1489,8 @@ EOF
>  	return 1;
>  }
>  
> -$reply_to = $initial_reply_to;
> -$references = $initial_reply_to || '';
> +$in_reply_to = $initial_in_reply_to;
> +$references = $initial_in_reply_to || '';
>  $subject = $initial_subject;
>  $message_num = 0;
>  
> @@ -1700,9 +1700,9 @@ foreach my $t (@files) {
>  
>  	# set up for the next message
>  	if ($thread && $message_was_sent &&
> -		($chain_reply_to || !defined $reply_to || length($reply_to) == 0 ||
> +		($chain_reply_to || !defined $in_reply_to || length($in_reply_to) == 0 ||
>  		$message_num == 1)) {
> -		$reply_to = $message_id;
> +		$in_reply_to = $message_id;
>  		if (length $references > 0) {
>  			$references .= "\n $message_id";
>  		} else {

Re: [PATCH v2 2/2] send-email: Support separate Reply-To address

[Junio C Hamano]

Christian Ludwig <chrissicool@googlemail.com> writes:

> In some projects contributions from groups are only accepted from a
> common group email address. But every individual may want to recieve
> replies to her own personal address. That's what we have 'Reply-To'
> headers for in SMTP.
>
> Introduce an optional '--reply-to' command line option. Unfortunately
> the $reply_to variable name was already taken for the 'In-Reply-To'
> header field. To reduce code churn, use $reply_address as variable
> name instead.
>
> Signed-off-by: Christian Ludwig <chrissicool@gmail.com>
> ---
>  Documentation/git-send-email.txt       |  5 +++++
>  contrib/completion/git-completion.bash |  2 +-
>  git-send-email.perl                    | 18 +++++++++++++++++-
>  t/t9001-send-email.sh                  |  2 ++
>  4 files changed, 25 insertions(+), 2 deletions(-)

Thanks.

While merging this with other topics in flight on 'pu', there were
minor merge conflicts, especially with np/send-email-header-parsing
that ends at b6049542 ("send-email: extract email-parsing code into
a subroutine", 2017-12-15) that attempts to refactor the code that
reads the header lines.  As there is *no* real change that benefits
by the refactoring accompanying the topic, it was a bit hard for me
to say if it is needless code churn or if it is a good refactoring.

I wonder if this change can be a good demonstration to measure the
goodness of it.  IOW, how would these two patches look if rebased on
the result of merging b6049542 to today's 'master'?  Would it make
these two patches cleaner and easier to grok?

Re: [PATCH v2 2/2] send-email: Support separate Reply-To address

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 17 2018, Christian Ludwig jotted:

> +if (defined $reply_to) {
> +	$reply_to =~ s/^\s+|\s+$//g;
> +	($reply_to) = expand_aliases($reply_to);
> +	$reply_to = sanitize_address($reply_to);
> +}

Just a note to other reviewers: I found it odd to call a function with
one argument and then enforce list context, but I see expand_aliases()
expects to be called that way, and this is copied from a previous
pattern earlier in the file.

(As an aside, that code looks a bit odd, but then I see 302e04ea4d
("send-email: detect cycles in alias expansion", 2009-07-23) )

Looks good to me.

Re: [PATCH v2 2/2] send-email: Support separate Reply-To address

[Martin Ågren]

On 17 January 2018 at 19:08, Christian Ludwig
<chrissicool@googlemail.com> wrote:
> In some projects contributions from groups are only accepted from a
> common group email address. But every individual may want to recieve
> replies to her own personal address. That's what we have 'Reply-To'
> headers for in SMTP.

s/recieve/receive/

> Introduce an optional '--reply-to' command line option. Unfortunately
> the $reply_to variable name was already taken for the 'In-Reply-To'
> header field. To reduce code churn, use $reply_address as variable
> name instead.

"To reduce ..." no longer describes the patch, since v2 actually
performs that refactoring in patch 1/2.  "Unfortunately ..." is
correct, but seems less relevant now. Except:

I suppose that a non-git.git patch which uses $reply_to could start
misbehaving now that this series changes the meaning of that variable.
Just thinking out loud, it could make some sense to take patch 1/2 for
sanity, but to then *not* re-use $reply_to for a new purpose, but to
actually take your v1-patch as 2/2. Or, this potential problem can
perhaps be ignored (except in the commit message?)..

> Signed-off-by: Christian Ludwig <chrissicool@gmail.com>

"From:" and "Signed-off-by:" are different. Not sure if that's ok.

Martin