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

Jonathan Nieder <jrnieder@gmail.com> writes:

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(-)