git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>,
	Brian M Carlson <sandals@crustytoothpaste.net>,
	git@vger.kernel.org
Subject: Re: [PATCH 1/1] send-email: fix transferencoding config option
Date: Wed, 10 Apr 2019 12:48:38 +0900	[thread overview]
Message-ID: <xmqq8swi34h5.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190409215856.GD92879@google.com> (Jonathan Nieder's message of "Tue, 9 Apr 2019 14:58:56 -0700")

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]
-- 
2.21.0-313-ge35b8cb8e2




  parent reply	other threads:[~2019-04-10  3:48 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-09 19:27 Heinrich Schuchardt
2019-04-09 21:58 ` Jonathan Nieder
2019-04-09 23:39   ` Heinrich Schuchardt
2019-04-10  3:48   ` Junio C Hamano [this message]
2019-04-10 20:40     ` Heinrich Schuchardt
2019-04-10 22:42       ` brian m. carlson
2019-05-08  8:18     ` Re* " Junio C Hamano
2019-05-08  8:20       ` [PATCH 2/2] send-email: honor transferencoding config option again Junio C Hamano
2019-05-08 10:13       ` Re* [PATCH 1/1] send-email: fix transferencoding config option Junio C Hamano
2019-05-08 10:56         ` [PATCH v2 0/2] send-email: set xfer encoding correctly Junio C Hamano
2019-05-09 11:48           ` [PATCH v3 0/3] send-email: fix cli->config parsing crazyness Ævar Arnfjörð Bjarmason
2019-05-10 13:50             ` Junio C Hamano
2019-05-09 11:48           ` [PATCH v3 1/3] send-email: move the read_config() function above getopts Ævar Arnfjörð Bjarmason
2019-05-09 11:48           ` [PATCH v3 2/3] send-email: rename the @bcclist variable for consistency Ævar Arnfjörð Bjarmason
2019-05-09 11:48           ` [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order Ævar Arnfjörð Bjarmason
2019-05-09 18:04             ` Eric Sunshine
2019-05-13  8:46               ` Junio C Hamano
2019-05-09 23:51             ` brian m. carlson
2019-05-13  8:50             ` Junio C Hamano
2019-05-13 21:13               ` Ævar Arnfjörð Bjarmason
2019-05-16 22:59             ` Stephen Boyd
2019-05-16 23:13               ` Junio C Hamano
2019-05-17  3:43                 ` Junio C Hamano
2019-05-17 19:55                   ` [PATCH 0/5] ab/send-email-transferencoding-fix-for-the-fix Ævar Arnfjörð Bjarmason
2019-05-17 19:55                   ` [PATCH 1/5] send-email: remove cargo-culted multi-patch pattern in tests Ævar Arnfjörð Bjarmason
2019-05-17 19:55                   ` [PATCH 2/5] send-email: fix broken transferEncoding tests Ævar Arnfjörð Bjarmason
2019-05-17 19:55                   ` [PATCH 3/5] send-email: document --no-[to|cc|bcc] Ævar Arnfjörð Bjarmason
2019-05-17 19:55                   ` [PATCH 4/5] send-email: fix regression in sendemail.identity parsing Ævar Arnfjörð Bjarmason
2019-05-19  1:29                     ` Junio C Hamano
2019-05-22 20:25                     ` Johannes Schindelin
2019-05-29  9:10                       ` Johannes Schindelin
2019-05-17 19:55                   ` [PATCH 5/5] send-email: remove support for deprecated sendemail.smtpssl Ævar Arnfjörð Bjarmason
2019-05-08 10:56         ` [PATCH v2 1/2] send-email: update the mechanism to set default configuration values Junio C Hamano
2019-05-08 10:56         ` [PATCH v2 2/2] send-email: honor transferencoding config option again Junio C Hamano
2019-05-08 21:24           ` Eric Sunshine
2019-05-09  6:47             ` Junio C Hamano
2019-05-08 23:12           ` brian m. carlson
2019-04-09 22:55 ` [PATCH 1/1] send-email: fix transferencoding config option brian m. carlson
2019-04-09 23:06   ` Heinrich Schuchardt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq8swi34h5.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    --cc=xypron.glpk@gmx.de \
    --subject='Re: [PATCH 1/1] send-email: fix transferencoding config option' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).