Re: [PATCH 1/1] send-email: fix transferencoding config option

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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]
>