threads / patch / 47625

v2, 2 partssend-email: Rename variable for clarity

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

## tl;dr

6 messages between Jan 17, 2018 and Jan 18, 2018. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Christian Ludwig· Jan 17, 2018, 18:08 UTC · lore

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(-)
Show changes to git-send-email.perl +19 −19
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
Christian Ludwig· Jan 17, 2018, 18:08 UTC · re: Christian Ludwig · lore

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

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(-)
Show changes to 4 files +25 −2

Documentation/git-send-email.txt, contrib/completion/git-completion.bash, git-send-email.perl, t/t9001-send-email.sh

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
Junio C Hamano· Jan 17, 2018, 21:02 UTC · re: Christian Ludwig · lore

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

Christian Ludwig <chrissicool@googlemail.com> writes:
Show 17 quoted lines
> 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?

Ævar Arnfjörð Bjarmason· Jan 17, 2018, 22:06 UTC · re: Christian Ludwig · lore

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

On Wed, Jan 17 2018, Christian Ludwig jotted:
Show 5 quoted lines
> +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.
Martin Ågren· Jan 18, 2018, 11:00 UTC · re: Christian Ludwig · lore

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

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
Junio C Hamano· Jan 17, 2018, 19:05 UTC · re: Christian Ludwig · lore

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

Christian Ludwig <chrissicool@googlemail.com> writes:
Show 6 quoted lines
> 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.
Show 126 quoted lines
>  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 {

← back to recent threads