Skip to content

Commit 3494dfd

Browse files
avargitster
avar
authored and
gitster
committed
send-email: do defaults -> config -> getopt in that order
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 e67a228 ("send-email: automatically determine transfer-encoding", 2018-07-08). That change broke the reading of sendmail.transferencoding because it wasn't careful to update the code to parse them 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's just something we grew organically as of 5483c71 ("git-send-email: make options easier to configure.", 2007-06-27) and have been dealing with the fallout since, e.g. in 463b0ea ("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, if 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 e67a228. 1. https://public-inbox.org/git/[email protected]/ Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent e60f6d1 commit 3494dfd

File tree

2 files changed

+61
-43
lines changed

2 files changed

+61
-43
lines changed

‎git-send-email.perl

Lines changed: 49 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -169,11 +169,15 @@ sub format_2822_time {
169169
my $re_encoded_word = qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/;
170170

171171
# Variables we fill in automatically, or via prompting:
172-
my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@initial_bcc,$no_bcc,@xh,
172+
my (@to,@cc,@xh,$envelope_sender,
173173
$initial_in_reply_to,$reply_to,$initial_subject,@files,
174-
$author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
175-
176-
my $envelope_sender;
174+
$author,$sender,$smtp_authpass,$annotate,$compose,$time);
175+
# Things we either get from config, *or* are overridden on the
176+
# command-line.
177+
my ($no_cc, $no_to, $no_bcc);
178+
my (@config_to, @getopt_to);
179+
my (@config_cc, @getopt_cc);
180+
my (@config_bcc, @getopt_bcc);
177181

178182
# Example reply to:
179183
#$initial_in_reply_to = ''; #<[email protected]>';
@@ -220,33 +224,37 @@ sub do_edit {
220224
}
221225

222226
# Variables with corresponding config settings
223-
my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
227+
my ($suppress_from, $signed_off_by_cc);
224228
my ($cover_cc, $cover_to);
225229
my ($to_cmd, $cc_cmd);
226230
my ($smtp_server, $smtp_server_port, @smtp_server_options);
227231
my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);
228232
my ($batch_size, $relogin_delay);
229233
my ($identity, $aliasfiletype, @alias_files, $smtp_domain, $smtp_auth);
230-
my ($validate, $confirm);
234+
my ($confirm);
231235
my (@suppress_cc);
232236
my ($auto_8bit_encoding);
233237
my ($compose_encoding);
234-
my $target_xfer_encoding = 'auto';
235-
238+
# Variables with corresponding config settings & hardcoded defaults
236239
my ($debug_net_smtp) = 0; # Net::SMTP, see send_message()
240+
my $thread = 1;
241+
my $chain_reply_to = 0;
242+
my $use_xmailer = 1;
243+
my $validate = 1;
244+
my $target_xfer_encoding = 'auto';
237245

238246
my %config_bool_settings = (
239-
"thread" => [\$thread, 1],
240-
"chainreplyto" => [\$chain_reply_to, 0],
241-
"suppressfrom" => [\$suppress_from, undef],
242-
"signedoffbycc" => [\$signed_off_by_cc, undef],
243-
"cccover" => [\$cover_cc, undef],
244-
"tocover" => [\$cover_to, undef],
245-
"signedoffcc" => [\$signed_off_by_cc, undef], # Deprecated
246-
"validate" => [\$validate, 1],
247-
"multiedit" => [\$multiedit, undef],
248-
"annotate" => [\$annotate, undef],
249-
"xmailer" => [\$use_xmailer, 1]
247+
"thread" => \$thread,
248+
"chainreplyto" => \$chain_reply_to,
249+
"suppressfrom" => \$suppress_from,
250+
"signedoffbycc" => \$signed_off_by_cc,
251+
"cccover" => \$cover_cc,
252+
"tocover" => \$cover_to,
253+
"signedoffcc" => \$signed_off_by_cc,
254+
"validate" => \$validate,
255+
"multiedit" => \$multiedit,
256+
"annotate" => \$annotate,
257+
"xmailer" => \$use_xmailer,
250258
);
251259

252260
my %config_settings = (
@@ -259,12 +267,12 @@ sub do_edit {
259267
"smtpauth" => \$smtp_auth,
260268
"smtpbatchsize" => \$batch_size,
261269
"smtprelogindelay" => \$relogin_delay,
262-
"to" => \@initial_to,
270+
"to" => \@config_to,
263271
"tocmd" => \$to_cmd,
264-
"cc" => \@initial_cc,
272+
"cc" => \@config_cc,
265273
"cccmd" => \$cc_cmd,
266274
"aliasfiletype" => \$aliasfiletype,
267-
"bcc" => \@initial_bcc,
275+
"bcc" => \@config_bcc,
268276
"suppresscc" => \@suppress_cc,
269277
"envelopesender" => \$envelope_sender,
270278
"confirm" => \$confirm,
@@ -312,8 +320,9 @@ sub read_config {
312320
my ($prefix) = @_;
313321

314322
foreach my $setting (keys %config_bool_settings) {
315-
my $target = $config_bool_settings{$setting}->[0];
316-
$$target = Git::config_bool(@repo, "$prefix.$setting") unless (defined $$target);
323+
my $target = $config_bool_settings{$setting};
324+
my $v = Git::config_bool(@repo, "$prefix.$setting");
325+
$$target = $v if defined $v;
317326
}
318327

319328
foreach my $setting (keys %config_path_settings) {
@@ -325,23 +334,22 @@ sub read_config {
325334
}
326335
}
327336
else {
328-
$$target = Git::config_path(@repo, "$prefix.$setting") unless (defined $$target);
337+
my $v = Git::config_path(@repo, "$prefix.$setting");
338+
$$target = $v if defined $v;
329339
}
330340
}
331341

332342
foreach my $setting (keys %config_settings) {
333343
my $target = $config_settings{$setting};
334-
next if $setting eq "to" and defined $no_to;
335-
next if $setting eq "cc" and defined $no_cc;
336-
next if $setting eq "bcc" and defined $no_bcc;
337344
if (ref($target) eq "ARRAY") {
338345
unless (@$target) {
339346
my @values = Git::config(@repo, "$prefix.$setting");
340347
@$target = @values if (@values && defined $values[0]);
341348
}
342349
}
343350
else {
344-
$$target = Git::config(@repo, "$prefix.$setting") unless (defined $$target);
351+
my $v = Git::config(@repo, "$prefix.$setting");
352+
$$target = $v if defined $v;
345353
}
346354
}
347355

@@ -355,6 +363,10 @@ sub read_config {
355363
}
356364
}
357365

366+
$identity = Git::config(@repo, "sendemail.identity");
367+
read_config("sendemail.$identity") if defined $identity;
368+
read_config("sendemail");
369+
358370
# Begin by accumulating all the variables (defined above), that we will end up
359371
# needing, first, from the command line:
360372

@@ -369,12 +381,12 @@ sub read_config {
369381
"in-reply-to=s" => \$initial_in_reply_to,
370382
"reply-to=s" => \$reply_to,
371383
"subject=s" => \$initial_subject,
372-
"to=s" => \@initial_to,
384+
"to=s" => \@getopt_to,
373385
"to-cmd=s" => \$to_cmd,
374386
"no-to" => \$no_to,
375-
"cc=s" => \@initial_cc,
387+
"cc=s" => \@getopt_cc,
376388
"no-cc" => \$no_cc,
377-
"bcc=s" => \@initial_bcc,
389+
"bcc=s" => \@getopt_bcc,
378390
"no-bcc" => \$no_bcc,
379391
"chain-reply-to!" => \$chain_reply_to,
380392
"no-chain-reply-to" => sub {$chain_reply_to = 0},
@@ -423,6 +435,11 @@ sub read_config {
423435
"relogin-delay=i" => \$relogin_delay,
424436
);
425437

438+
# Munge any "either config or getopt, not both" variables
439+
my @initial_to = @getopt_to ? @getopt_to : ($no_to ? () : @config_to);
440+
my @initial_cc = @getopt_cc ? @getopt_cc : ($no_cc ? () : @config_cc);
441+
my @initial_bcc = @getopt_bcc ? @getopt_bcc : ($no_bcc ? () : @config_bcc);
442+
426443
usage() if $help;
427444
unless ($rc) {
428445
usage();
@@ -435,16 +452,6 @@ sub read_config {
435452
"(via command-line or configuration option)\n")
436453
if defined $relogin_delay and not defined $batch_size;
437454

438-
# read configuration from [sendemail "$identity"], fall back on [sendemail]
439-
$identity = Git::config(@repo, "sendemail.identity") unless (defined $identity);
440-
read_config("sendemail.$identity") if (defined $identity);
441-
read_config("sendemail");
442-
443-
# fall back on builtin bool defaults
444-
foreach my $setting (values %config_bool_settings) {
445-
${$setting->[0]} = $setting->[1] unless (defined (${$setting->[0]}));
446-
}
447-
448455
# 'default' encryption is none -- this only prevents a warning
449456
$smtp_encryption = '' unless (defined $smtp_encryption);
450457

‎t/t9001-send-email.sh

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1433,7 +1433,18 @@ test_expect_success $PREREQ '--transfer-encoding overrides sendemail.transferEnc
14331433
test -z "$(ls msgtxt*)"
14341434
'
14351435

1436-
test_expect_success $PREREQ 'sendemail.transferencoding=8bit' '
1436+
test_expect_success $PREREQ 'sendemail.transferencoding=8bit via config' '
1437+
clean_fake_sendmail &&
1438+
git -c sendemail.transferencoding=8bit send-email \
1439+
--smtp-server="$(pwd)/fake.sendmail" \
1440+
email-using-8bit \
1441+
2>errors >out &&
1442+
sed '1,/^$/d' msgtxt1 >actual &&
1443+
sed '1,/^$/d' email-using-8bit >expected &&
1444+
test_cmp expected actual
1445+
'
1446+
1447+
test_expect_success $PREREQ 'sendemail.transferencoding=8bit via cli' '
14371448
clean_fake_sendmail &&
14381449
git send-email \
14391450
--transfer-encoding=8bit \

0 commit comments

Comments
 (0)