git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
May 9, 2019, 11:48 UTC
Message-ID
<20190509114830.29647-4-avarab@gmail.com>
In-Reply-To
<20190508105607.178244-1-gitster@pobox.com>

Change the git-send-email command-line argument parsing and config reading code to parse those two in the right order. I.e. first we set our hardcoded defaults, then we read our config, and finally we read the command-line, with later sets overriding earlier sets.

This fixes a bug introduced in e67a228cd8 ("send-email: automatically determine transfer-encoding", 2018-07-08). That change broke the broke the reading of sendmail.transferencoding because it wasn't careful to update our fragile code dealing with doing this in the previous "defaults -> getopt -> config" order..

But as we can see from the history for this file doing it this way was never what we actually wanted, it just something we grew organically as of 5483c71d7a ("git-send-email: make options easier to configure.", 2007-06-27) and have been dealing with the fallout since, e.g. in 463b0ea22b ("send-email: Fix %config_path_settings handling", 2011-10-14).

As can be seen in this change the only place where we actually want to do something clever is with the to/cc/bcc variables, where setting them on the command-line (or using --no-{to,cc,bcc}) should clear out values we grab from the config.

All the rest are things where the command-line should simply override the config values, and by reading the config first the config code doesn't need all this "let's not set it was on the command-line" special-casing, as [1] shows we'd otherwise need to care about the difference between whether something was a default or present in config to fix the bug in e67a228cd8.

1. https://public-inbox.org/git/20190508105607.178244-2-gitster@pobox.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl   | 92 +++++++++++++++++++++++--------------------
 t/t9001-send-email.sh | 13 +++++-
 2 files changed, 62 insertions(+), 43 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 48ed18a85c..fab255249f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -177,11 +177,15 @@ sub format_2822_time {
 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,@initial_bcc,$no_bcc,@xh,
+my (@to,@cc,,@xh,$envelope_sender,
 	$initial_in_reply_to,$reply_to,$initial_subject,@files,
-	$author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
-
-my $envelope_sender;
+	$author,$sender,$smtp_authpass,$annotate,$compose,$time);
+# Things we either get from config, *or* are overridden on the
+# command-line.
+my ($no_cc, $no_to, $no_bcc);
+my (@config_to, @getopt_to);
+my (@config_cc, @getopt_cc);
+my (@config_bcc, @getopt_bcc);
 
 # Example reply to:
 #$initial_in_reply_to = ''; #<20050203173208.GA23964@foobar.com>';
@@ -228,33 +232,37 @@ sub do_edit {
 }
 
 # Variables with corresponding config settings
-my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
+my ($suppress_from, $signed_off_by_cc);
 my ($cover_cc, $cover_to);
 my ($to_cmd, $cc_cmd);
 my ($smtp_server, $smtp_server_port, @smtp_server_options);
 my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);
 my ($batch_size, $relogin_delay);
 my ($identity, $aliasfiletype, @alias_files, $smtp_domain, $smtp_auth);
-my ($validate, $confirm);
+my ($confirm);
 my (@suppress_cc);
 my ($auto_8bit_encoding);
 my ($compose_encoding);
-my $target_xfer_encoding = 'auto';
-
+# Variables with corresponding config settings & hardcoded defaults
 my ($debug_net_smtp) = 0;		# Net::SMTP, see send_message()
+my $thread = 1;
+my $chain_reply_to = 0;
+my $use_xmailer = 1;
+my $validate = 1;
+my $target_xfer_encoding = 'auto';
 
 my %config_bool_settings = (
-    "thread" => [\$thread, 1],
-    "chainreplyto" => [\$chain_reply_to, 0],
-    "suppressfrom" => [\$suppress_from, undef],
-    "signedoffbycc" => [\$signed_off_by_cc, undef],
-    "cccover" => [\$cover_cc, undef],
-    "tocover" => [\$cover_to, undef],
-    "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
-    "validate" => [\$validate, 1],
-    "multiedit" => [\$multiedit, undef],
-    "annotate" => [\$annotate, undef],
-    "xmailer" => [\$use_xmailer, 1]
+    "thread" => \$thread,
+    "chainreplyto" => \$chain_reply_to,
+    "suppressfrom" => \$suppress_from,
+    "signedoffbycc" => \$signed_off_by_cc,
+    "cccover" => \$cover_cc,
+    "tocover" => \$cover_to,
+    "signedoffcc" => \$signed_off_by_cc,
+    "validate" => \$validate,
+    "multiedit" => \$multiedit,
+    "annotate" => \$annotate,
+    "xmailer" => \$use_xmailer,
 );
 
 my %config_settings = (
@@ -267,12 +275,12 @@ sub do_edit {
     "smtpauth" => \$smtp_auth,
     "smtpbatchsize" => \$batch_size,
     "smtprelogindelay" => \$relogin_delay,
-    "to" => \@initial_to,
+    "to" => \@config_to,
     "tocmd" => \$to_cmd,
-    "cc" => \@initial_cc,
+    "cc" => \@config_cc,
     "cccmd" => \$cc_cmd,
     "aliasfiletype" => \$aliasfiletype,
-    "bcc" => \@initial_bcc,
+    "bcc" => \@config_bcc,
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
     "confirm"   => \$confirm,
@@ -320,8 +328,9 @@ sub read_config {
 	my ($prefix) = @_;
 
 	foreach my $setting (keys %config_bool_settings) {
-		my $target = $config_bool_settings{$setting}->[0];
-		$$target = Git::config_bool(@repo, "$prefix.$setting") unless (defined $$target);
+		my $target = $config_bool_settings{$setting};
+		my $v = Git::config_bool(@repo, "$prefix.$setting");
+		$$target = $v if defined $v;
 	}
 
 	foreach my $setting (keys %config_path_settings) {
@@ -333,15 +342,13 @@ sub read_config {
 			}
 		}
 		else {
-			$$target = Git::config_path(@repo, "$prefix.$setting") unless (defined $$target);
+			my $v = Git::config_path(@repo, "$prefix.$setting");
+			$$target = $v if defined $v;
 		}
 	}
 
 	foreach my $setting (keys %config_settings) {
 		my $target = $config_settings{$setting};
-		next if $setting eq "to" and defined $no_to;
-		next if $setting eq "cc" and defined $no_cc;
-		next if $setting eq "bcc" and defined $no_bcc;
 		if (ref($target) eq "ARRAY") {
 			unless (@$target) {
 				my @values = Git::config(@repo, "$prefix.$setting");
@@ -349,7 +356,8 @@ sub read_config {
 			}
 		}
 		else {
-			$$target = Git::config(@repo, "$prefix.$setting") unless (defined $$target);
+			my $v = Git::config(@repo, "$prefix.$setting");
+			$$target = $v if defined $v;
 		}
 	}
 
@@ -363,6 +371,11 @@ sub read_config {
 	}
 }
 
+$identity = Git::config(@repo, "sendemail.identity");
+read_config("sendemail.$identity") if defined $identity;
+read_config("sendemail");
+read_config("sendemail");
+
 # Begin by accumulating all the variables (defined above), that we will end up
 # needing, first, from the command line:
 
@@ -378,12 +391,12 @@ sub read_config {
                     "in-reply-to=s" => \$initial_in_reply_to,
 		    "reply-to=s" => \$reply_to,
 		    "subject=s" => \$initial_subject,
-		    "to=s" => \@initial_to,
+		    "to=s" => \@getopt_to,
 		    "to-cmd=s" => \$to_cmd,
 		    "no-to" => \$no_to,
-		    "cc=s" => \@initial_cc,
+		    "cc=s" => \@getopt_cc,
 		    "no-cc" => \$no_cc,
-		    "bcc=s" => \@initial_bcc,
+		    "bcc=s" => \@getopt_bcc,
 		    "no-bcc" => \$no_bcc,
 		    "chain-reply-to!" => \$chain_reply_to,
 		    "no-chain-reply-to" => sub {$chain_reply_to = 0},
@@ -434,6 +447,11 @@ sub read_config {
 		    "git-completion-helper" => \$git_completion_helper,
 	 );
 
+# Munge any "either config or getopt, not both" variables
+my @initial_to = @getopt_to ? @getopt_to : ($no_to ? () : @config_to);
+my @initial_cc = @getopt_cc ? @getopt_cc : ($no_cc ? () : @config_cc);
+my @initial_bcc = @getopt_bcc ? @getopt_bcc : ($no_bcc ? () : @config_bcc);
+
 usage() if $help;
 completion_helper() if $git_completion_helper;
 unless ($rc) {
@@ -447,16 +465,6 @@ sub read_config {
 	"(via command-line or configuration option)\n")
 	if defined $relogin_delay and not defined $batch_size;
 
-# read configuration from [sendemail "$identity"], fall back on [sendemail]
-$identity = Git::config(@repo, "sendemail.identity") unless (defined $identity);
-read_config("sendemail.$identity") if (defined $identity);
-read_config("sendemail");
-
-# fall back on builtin bool defaults
-foreach my $setting (values %config_bool_settings) {
-	${$setting->[0]} = $setting->[1] unless (defined (${$setting->[0]}));
-}
-
 # 'default' encryption is none -- this only prevents a warning
 $smtp_encryption = '' unless (defined $smtp_encryption);
 
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 1e3ac3c384..1b18201ce2 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1461,7 +1461,18 @@ test_expect_success $PREREQ '--transfer-encoding overrides sendemail.transferEnc
 	test -z "$(ls msgtxt*)"
 '
 
-test_expect_success $PREREQ 'sendemail.transferencoding=8bit' '
+test_expect_success $PREREQ 'sendemail.transferencoding=8bit via config' '
+	clean_fake_sendmail &&
+	git -c sendemail.transferencoding=8bit send-email \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		email-using-8bit \
+		2>errors >out &&
+	sed '1,/^$/d' msgtxt1 >actual &&
+	sed '1,/^$/d' email-using-8bit >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success $PREREQ 'sendemail.transferencoding=8bit via cli' '
 	clean_fake_sendmail &&
 	git send-email \
 		--transfer-encoding=8bit \

-- 
2.21.0.1020.gf2820cf01a
Previous: Ævar Arnfjörð BjarmasonNext: Eric Sunshine
Message 15 of 39 in “send-email: fix transferencoding config option”
  1. 1/1 send-email: fix transferencoding config optionHeinrich Schuchardt, Apr 9, 2019
  2. Jonathan NiederApr 9, 2019
  3. Heinrich SchuchardtApr 9, 2019
  4. Junio C HamanoApr 10, 2019
  5. Heinrich SchuchardtApr 10, 2019
  6. brian m. carlsonApr 10, 2019
  7. Re* [PATCH 1/1] send-email: fix transferencoding config optionJunio C Hamano, May 8, 2019
  8. 2/2 send-email: honor transferencoding config option againJunio C Hamano, May 8, 2019
  9. Junio C HamanoMay 8, 2019
  10. 0/2 send-email: set xfer encoding correctlyJunio C Hamano, May 8, 2019
  11. 0/3 send-email: fix cli->config parsing crazynessÆvar Arnfjörð Bjarmason, May 9, 2019
  12. Junio C HamanoMay 10, 2019
  13. 1/3 send-email: move the read_config() function above getoptsÆvar Arnfjörð Bjarmason, May 9, 2019
  14. 2/3 send-email: rename the @bcclist variable for consistencyÆvar Arnfjörð Bjarmason, May 9, 2019
  15. 3/3 send-email: do defaults -> config -> getopt in that orderÆvar Arnfjörð Bjarmason, May 9, 2019
  16. Eric SunshineMay 9, 2019
  17. Junio C HamanoMay 13, 2019
  18. brian m. carlsonMay 9, 2019
  19. Junio C HamanoMay 13, 2019
  20. Ævar Arnfjörð BjarmasonMay 13, 2019
  21. Stephen BoydMay 16, 2019
  22. Junio C HamanoMay 16, 2019
  23. Junio C HamanoMay 17, 2019
  24. 0/5 ab/send-email-transferencoding-fix-for-the-fixÆvar Arnfjörð Bjarmason, May 17, 2019
  25. 1/5 send-email: remove cargo-culted multi-patch pattern in testsÆvar Arnfjörð Bjarmason, May 17, 2019
  26. 2/5 send-email: fix broken transferEncoding testsÆvar Arnfjörð Bjarmason, May 17, 2019
  27. 3/5 send-email: document --no-[to|cc|bcc]Ævar Arnfjörð Bjarmason, May 17, 2019
  28. 4/5 send-email: fix regression in sendemail.identity parsingÆvar Arnfjörð Bjarmason, May 17, 2019
  29. Junio C HamanoMay 19, 2019
  30. Johannes SchindelinMay 22, 2019
  31. Johannes SchindelinMay 29, 2019
  32. 5/5 send-email: remove support for deprecated sendemail.smtpsslÆvar Arnfjörð Bjarmason, May 17, 2019
  33. 2/2 send-email: honor transferencoding config option againJunio C Hamano, May 8, 2019
  34. Eric SunshineMay 8, 2019
  35. Junio C HamanoMay 9, 2019
  36. brian m. carlsonMay 8, 2019
  37. 1/2 send-email: update the mechanism to set default configuration valuesJunio C Hamano, May 8, 2019
  38. brian m. carlsonApr 9, 2019
  39. Heinrich SchuchardtApr 9, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.