| Message ID | 20190509114830.29647-4-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | send-email: set xfer encoding correctly | expand |
On Thu, May 9, 2019 at 7:48 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > 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 s/broke the broke/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.. s/\.\.$/./ > 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 s/it/it's/ > 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" ECANTPARSE "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>
On Thu, May 09, 2019 at 01:48:30PM +0200, Ævar Arnfjörð Bjarmason wrote: > @@ -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"); Did we really want to call this function twice with the same argument? If so, perhaps a comment is appropriate explaining why. In general, I'm not opposed to this approach, either. It does make it easier to handle parsing in general. However, having said that, I'm fine with either your series or Junio's.
Eric Sunshine <sunshine@sunshineco.com> writes: >> 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" > > ECANTPARSE "let's not set it was on the command-line" I took it as "let's not set it, if it was on the command-line".
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > # 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, ,,???
On Mon, May 13 2019, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> # 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, > > ,,??? Just a typo. I see you fixed this and the other issues noted in your push-out to "next", thanks! Sorry about not getting to this before that.
(Sorry for weird reply, I'm not subscribed and my MUA is not prepared for this) > 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. This broke my workflow. I specify --identity=<account> on the commandline and I want that to pick out my send-email config from my global .gitconfig file that corresponds to that identity. With this change, the config is parsed before the getopt part so --identity on the commandline is a nop and never looks into the config file to figure this out. So at least --identity is special in addition to --to,cc,bcc.
Stephen Boyd <swboyd@chromium.org> writes: >> 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. > > This broke my workflow. > > I specify --identity=<account> on the commandline and I want that to > pick out my send-email config from my global .gitconfig file that > corresponds to that identity. With this change, the config is parsed > before the getopt part so --identity on the commandline is a nop and > never looks into the config file to figure this out. So at least > --identity is special in addition to --to,cc,bcc. Ah, sorry that nobody noticed that case, but you are right. Because the ident is used as a part of the key to find identity-specific configuration values, if the command line gives one, we must have an access to it before we start reading the configuration. In that sense, it is more fundamental to special-case the option. We are past -rc0, so I am inclined to revert the change (and perhaps replace it with the other "fix" that did not break the parsing order like these patches did), with an expectation that a clever fix will be found later, *unless* a simple and correct fix is found quickly.
Junio C Hamano <gitster@pobox.com> writes: > Ah, sorry that nobody noticed that case, but you are right. Because > the ident is used as a part of the key to find identity-specific > configuration values, if the command line gives one, we must have an > access to it before we start reading the configuration. In that sense, > it is more fundamental to special-case the option. > > We are past -rc0, so I am inclined to revert the change (and perhaps > replace it with the other "fix" that did not break the parsing order > like these patches did), with an expectation that a clever fix will > be found later, *unless* a simple and correct fix is found quickly. Oops, spoke too soon. The topic hasn't escaped to 'master' yet, so I'll make sure it stays that way. Thanks for catching a regression before it gets too late.
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 \
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(-)