| Message ID | 20190409192733.10173-1-xypron.glpk@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] send-email: fix transferencoding config option | expand |
(thanks for cc-ing bmc!) Hi, Heinrich Schuchardt wrote: > Subject: send-email: fix transferencoding config option nit: "fix" doesn't tell me what was broken and what you improved about it. Here, I think you mean "respect transferencoding config option". > Since e67a228cd8a ("send-email: automatically determine transfer-encoding") > the value of sendmail.transferencoding is ignored because when parsing > the configuration $target_xfer_encoding is not initial anymore. 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. > Instead of initializing variable $target_xfer_encoding on definition we > have to set it to the default value of 'auto' if is initial after parsing > the configuration files. run-on sentence. I'm having trouble parsing this part. Can you start from the beginning and describe again what this does? In other words, tell me - What is the user-facing effect of the change? What workflow is it part of? - Any risks or complications? - Any technical details that might be interesting to the later reader? - What does this allow me to do that I couldn't do before? The code can speak for itself, so this should primarily focus on the intention behind the change. [...] > --- a/Documentation/git-send-email.txt > +++ b/Documentation/git-send-email.txt > @@ -146,7 +146,7 @@ Note that no attempts whatsoever are made to validate the encoding. > even more opaque. auto will use 8bit when possible, and quoted-printable > otherwise. > + > -Default is the value of the `sendemail.transferEncoding` configuration > +Default is the value of the `sendemail.transferencoding` configuration Unrelated change? [...] > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -239,7 +239,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() > > @@ -446,6 +446,8 @@ sub read_config { > $smtp_encryption = 'ssl'; > } > } > + > + $target_xfer_encoding = 'auto' unless (defined $target_xfer_encoding); Makes sense. Is there a way to cover this in tests (t/t9001-send-email.sh) so we can avoid regressing again? The rest looks good. Thanks for noticing, and hope that helps, Jonathan
On Tue, Apr 09, 2019 at 09:27:33PM +0200, Heinrich Schuchardt wrote: > diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt > index 1afe9fc858..884e776add 100644 > --- a/Documentation/git-send-email.txt > +++ b/Documentation/git-send-email.txt > @@ -146,7 +146,7 @@ Note that no attempts whatsoever are made to validate the encoding. > even more opaque. auto will use 8bit when possible, and quoted-printable > otherwise. > + > -Default is the value of the `sendemail.transferEncoding` configuration > +Default is the value of the `sendemail.transferencoding` configuration > value; if that is unspecified, default to `auto`. In Git, two-part config settings are case-insensitive. We traditionally write them in lower camel case because it's easier for people to read. Git will canonicalize the values when "git config" runs. So I don't think we should change this. > diff --git a/git-send-email.perl b/git-send-email.perl > index 8200d58cdc..0e23193939 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -239,7 +239,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() > > @@ -446,6 +446,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] Thanks for fixing this. I didn't realize that we only set values if the variable holding them is undef. Would you mind adding a test for this case so we won't regress it in the future?
On 4/10/19 12:55 AM, brian m. carlson wrote: > On Tue, Apr 09, 2019 at 09:27:33PM +0200, Heinrich Schuchardt wrote: >> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt >> index 1afe9fc858..884e776add 100644 >> --- a/Documentation/git-send-email.txt >> +++ b/Documentation/git-send-email.txt >> @@ -146,7 +146,7 @@ Note that no attempts whatsoever are made to validate the encoding. >> even more opaque. auto will use 8bit when possible, and quoted-printable >> otherwise. >> + >> -Default is the value of the `sendemail.transferEncoding` configuration >> +Default is the value of the `sendemail.transferencoding` configuration >> value; if that is unspecified, default to `auto`. > > In Git, two-part config settings are case-insensitive. We traditionally > write them in lower camel case because it's easier for people to read. > Git will canonicalize the values when "git config" runs. > > So I don't think we should change this. Thanks for the hint. > >> diff --git a/git-send-email.perl b/git-send-email.perl >> index 8200d58cdc..0e23193939 100755 >> --- a/git-send-email.perl >> +++ b/git-send-email.perl >> @@ -239,7 +239,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() >> >> @@ -446,6 +446,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] > > Thanks for fixing this. I didn't realize that we only set values if the > variable holding them is undef. Would you mind adding a test for this > case so we won't regress it in the future? > Nice challenge for my first patch for git :) I will give it a try. Best regards Heinrich
On 4/9/19 11:58 PM, Jonathan Nieder wrote: > (thanks for cc-ing bmc!) > Hi, > > Heinrich Schuchardt wrote: > >> Subject: send-email: fix transferencoding config option > > nit: "fix" doesn't tell me what was broken and what you improved about > it. Here, I think you mean "respect transferencoding config option". > >> Since e67a228cd8a ("send-email: automatically determine transfer-encoding") >> the value of sendmail.transferencoding is ignored because when parsing >> the configuration $target_xfer_encoding is not initial anymore. > > 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. > >> Instead of initializing variable $target_xfer_encoding on definition we >> have to set it to the default value of 'auto' if is initial after parsing >> the configuration files. > > run-on sentence. I'm having trouble parsing this part. > > Can you start from the beginning and describe again what this does? > In other words, tell me > > - What is the user-facing effect of the change? What workflow is it > part of? I am working with a repository which uses CRLF line endings. So when sending patches I should use an appropriate encoding. There should be two ways to do it: - call git-send-email with --transfer-encoding base64 - git config --global sendmail.transferencoding base64 Unfortunately the latter method did not show the expected result. The setting was simply ignored. > > - Any risks or complications? None that I am aware of. > > - Any technical details that might be interesting to the later reader? As I tried to explain above the setting is ignored because a variable is initialized too early. > > - What does this allow me to do that I couldn't do before? You can use a global setting for the transfer encoding. > > The code can speak for itself, so this should primarily focus on the > intention behind the change. > > [...] >> --- a/Documentation/git-send-email.txt >> +++ b/Documentation/git-send-email.txt >> @@ -146,7 +146,7 @@ Note that no attempts whatsoever are made to validate the encoding. >> even more opaque. auto will use 8bit when possible, and quoted-printable >> otherwise. >> + >> -Default is the value of the `sendemail.transferEncoding` configuration >> +Default is the value of the `sendemail.transferencoding` configuration > > Unrelated change? > > [...] >> --- a/git-send-email.perl >> +++ b/git-send-email.perl >> @@ -239,7 +239,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() >> >> @@ -446,6 +446,8 @@ sub read_config { >> $smtp_encryption = 'ssl'; >> } >> } >> + >> + $target_xfer_encoding = 'auto' unless (defined $target_xfer_encoding); > > Makes sense. > > Is there a way to cover this in tests (t/t9001-send-email.sh) so we > can avoid regressing again? I will give it a try. > > The rest looks good. > > Thanks for noticing, and hope that helps, > Jonathan > Thanks for reviewing. Best regards Heinrich
Jonathan Nieder <jrnieder@gmail.com> 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? -- >8 -- From: Heinrich Schuchardt <xypron.glpk@gmx.de> 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@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- 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]
On 4/10/19 5:48 AM, Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> 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@gmx.de> > 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@gmx.de> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > 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] >
On Wed, Apr 10, 2019 at 10:40:51PM +0200, Heinrich Schuchardt wrote: > 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())? Because nobody thought of it. The original rationale for "auto" was to replace "8bit" with something sane that worked due to long lines as an alternative to forcing people to choose it themselves. And as the commit message says: Choose quoted-printable instead of base64, since base64-encoded plain text is treated as suspicious by some spam filters. I'll try to see if I can write something up to handle this better. If quoted-printable works, I'll pick that; otherwise, I'll choose base64.
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 1afe9fc858..884e776add 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -146,7 +146,7 @@ Note that no attempts whatsoever are made to validate the encoding. even more opaque. auto will use 8bit when possible, and quoted-printable otherwise. + -Default is the value of the `sendemail.transferEncoding` configuration +Default is the value of the `sendemail.transferencoding` configuration value; if that is unspecified, default to `auto`. --xmailer:: diff --git a/git-send-email.perl b/git-send-email.perl index 8200d58cdc..0e23193939 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -239,7 +239,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() @@ -446,6 +446,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]
Since e67a228cd8a ("send-email: automatically determine transfer-encoding") the value of sendmail.transferencoding is ignored because when parsing the configuration $target_xfer_encoding is not initial anymore. Instead of initializing variable $target_xfer_encoding on definition we have to set it to the default value of 'auto' if is initial after parsing the configuration files. The documentation erroneously mentions the option as sendmail.transferEncoding. Fix that typo. Fixes: e67a228cd8a ("send-email: automatically determine transfer-encoding") Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- Documentation/git-send-email.txt | 2 +- git-send-email.perl | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) -- 2.20.1