On 4/10/19 5:48 AM, Junio C Hamano wrote:
Jonathan Nieder <jrnieder@xxxxxxxxx> writes:
nit: I was confused when first reading this, since I read "the
configuration $target_xfer_encoding" as a single phrase. A comma
after "configuration" might help.
...
run-on sentence. I'm having trouble parsing this part.
I had the same issue with the wording. Without addressing other
parts of the suggestions in the thread (like describing the
motivating use case, and protecting this with the test), here is
what I have tentatively queued.
As all the $scalar variables that are referenced by %config_settings
etc. all potentially share this issue, I wonder if it makes sense to
have a validation at the very beginning of the read_config sub,
something along the lines of....
sub read_config {
my ($prefix) = @_;
while (my ($k, $v) = each %config_bool_settings) {
if (defined $$v) {
die "BUG: \%config_bool_settings{$k} is not undef\n";
}
}
... similarly for %config_path_settings and %config_settings ...
... then the original code ...
foreach my $setting (keys %config_bool_settings) {
...
}
By the way, if we look more closely to the two callsites of
read_config(), however, we realize that Heinrich's patch is a wrong
solution to the problem.
What happens when "sendemail.<ident>.xferencoding" is not set, but
"sendemail.xferencoding" is, with the updated code? The "ah, the
configuration file did not define the xfer-encoding, so let's set it
to auto" at the end of read_config is done still too early. After
checking "sendemail.<ident>.*", the code added by the patch under
review assigns 'auto' to $target_xfer_encoding and this assignment
causes "sendemail.xferencoding" to be ignored, just like BMC's bug.
In other words, the patch is reproducing the same bug it is
attempting to fix; a quick-and-dirty and obvious band-aid is to move
the assignment of 'auto' further down, outside the read_config()
sub, after two calls to the sub is made by the caller, but singling
this single variable out is very unsatisfactory.
I wonder if we can follow the pattern used by the code to handle the
fallback for %config_bool_settings we can see immediately after
these two calls to read_config()? That is, each of the element in
the %config_* hash is not merely a pointer to where the value is
stored, but also knows what the default fallback value should be,
and a loop _in the caller of_ read_config(), after it finishes
making calls to the read_config function, fills in the missing
default?
Sounds reasonable. But including the tests requested nothing I could
easily shoulder.
Just a quite different thought:
'auto' should discover a safe transfer encoding. Why does 'auto' not
discover that a patch contains carriage returns and should be base64
encoded (see subroutine apply_transfer_encoding())?
Best regards
Heinrich
-- >8 --
From: Heinrich Schuchardt <xypron.glpk@xxxxxx>
Date: Tue, 9 Apr 2019 21:27:33 +0200
Subject: [PATCH] send-email: honor transferencoding config option again
Since e67a228cd8a ("send-email: automatically determine
transfer-encoding"), the value of sendmail.transferencoding in the
configuration file is ignored, because $target_xfer_encoding is
already defined read_config sub parses the configuration file.
Instead of initializing variable $target_xfer_encoding to 'auto' on
definition, we have to set it to the default value of 'auto' if is
undefined after parsing the configuration files.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@xxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
git-send-email.perl | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index f4c07908d2..db32cddbde 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -231,7 +231,7 @@ sub do_edit {
my (@suppress_cc);
my ($auto_8bit_encoding);
my ($compose_encoding);
-my $target_xfer_encoding = 'auto';
+my ($target_xfer_encoding);
my ($debug_net_smtp) = 0; # Net::SMTP, see send_message()
@@ -434,6 +434,8 @@ sub read_config {
$smtp_encryption = 'ssl';
}
}
+
+ $target_xfer_encoding = 'auto' unless (defined $target_xfer_encoding);
}
# read configuration from [sendemail "$identity"], fall back on [sendemail]
