From: Heinrich Schuchardt Date: Wed, 10 Apr 2019 20:40:51 GMT Subject: Re: [PATCH 1/1] send-email: fix transferencoding config option Message-ID: <668fc68f-4d94-39e8-256d-55229472761f@gmx.de> In-Reply-To: On 4/10/19 5:48 AM, Junio C Hamano wrote: > Jonathan Nieder 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..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..*", 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 > 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 > Signed-off-by: Junio C Hamano > --- > 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] >