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

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

[Heinrich Schuchardt]

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

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

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

[Jonathan Nieder]

(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

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

[Heinrich Schuchardt]

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

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

[Junio C Hamano]

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

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

[Heinrich Schuchardt]

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

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.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

So, here is a two-patch series that tries to do so, primarily done
to gauge if there still is the level of interest needed to make it
worth for us to pursue this topic.  Here is the first one; I'll send
the second one that takes advantage of this change separately (but
it should be trivial to imagine what that step would involve).

-- >8 --
Subject: [PATCH 1/2] send-email: update the mechanism to set default configuration values

The program has a good mechanism to specify the fallback default
values for boolean configuration variables after two invocations of
read_config() for "sendmail.$ident.$var" and "sendemail.$var" have
not found any configuration.  Imitate it so that we can set the
default values for non-boolean variables as well.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-send-email.perl | 51 ++++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index f4c07908d2..ca7faff094 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -250,28 +250,28 @@ sub do_edit {
 );
 
 my %config_settings = (
-    "smtpserver" => \$smtp_server,
-    "smtpserverport" => \$smtp_server_port,
-    "smtpserveroption" => \@smtp_server_options,
-    "smtpuser" => \$smtp_authuser,
-    "smtppass" => \$smtp_authpass,
-    "smtpdomain" => \$smtp_domain,
-    "smtpauth" => \$smtp_auth,
-    "smtpbatchsize" => \$batch_size,
-    "smtprelogindelay" => \$relogin_delay,
-    "to" => \@initial_to,
-    "tocmd" => \$to_cmd,
-    "cc" => \@initial_cc,
-    "cccmd" => \$cc_cmd,
-    "aliasfiletype" => \$aliasfiletype,
-    "bcc" => \@bcclist,
-    "suppresscc" => \@suppress_cc,
-    "envelopesender" => \$envelope_sender,
-    "confirm"   => \$confirm,
-    "from" => \$sender,
-    "assume8bitencoding" => \$auto_8bit_encoding,
-    "composeencoding" => \$compose_encoding,
-    "transferencoding" => \$target_xfer_encoding,
+    "smtpserver" => [\$smtp_server],
+    "smtpserverport" => [\$smtp_server_port],
+    "smtpserveroption" => [\@smtp_server_options],
+    "smtpuser" => [\$smtp_authuser],
+    "smtppass" => [\$smtp_authpass],
+    "smtpdomain" => [\$smtp_domain],
+    "smtpauth" => [\$smtp_auth],
+    "smtpbatchsize" => [\$batch_size],
+    "smtprelogindelay" => [\$relogin_delay],
+    "to" => [\@initial_to],
+    "tocmd" => [\$to_cmd],
+    "cc" => [\@initial_cc],
+    "cccmd" => [\$cc_cmd],
+    "aliasfiletype" => [\$aliasfiletype],
+    "bcc" => [\@bcclist],
+    "suppresscc" => [\@suppress_cc],
+    "envelopesender" => [\$envelope_sender],
+    "confirm"   => [\$confirm],
+    "from" => [\$sender],
+    "assume8bitencoding" => [\$auto_8bit_encoding],
+    "composeencoding" => [\$compose_encoding],
+    "transferencoding" => [\$target_xfer_encoding],
 );
 
 my %config_path_settings = (
@@ -446,6 +446,13 @@ sub read_config {
 	${$setting->[0]} = $setting->[1] unless (defined (${$setting->[0]}));
 }
 
+# fall back to builtin defaults
+for my $setting (values %config_settings) {
+	if (@$setting == 2 && !defined (${$setting->[0]})) {
+		${$setting->[0]} = $setting->[1];
+	}
+}
+
 # 'default' encryption is none -- this only prevents a warning
 $smtp_encryption = '' unless (defined $smtp_encryption);
 
-- 
2.21.0-777-g83232e3864

[PATCH 2/2] send-email: honor transferencoding config option again

[Junio C Hamano]

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 calling read_config() twice to parsing the
configuration files for "sendemail.transferencoding" and
"sendemail.$ident.transferencoding".

This is made trivial by the previous change.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * And this is the second one.  Totally untested, though.

 git-send-email.perl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index ca7faff094..7458a0d1ef 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()
 
@@ -271,7 +271,7 @@ sub do_edit {
     "from" => [\$sender],
     "assume8bitencoding" => [\$auto_8bit_encoding],
     "composeencoding" => [\$compose_encoding],
-    "transferencoding" => [\$target_xfer_encoding],
+    "transferencoding" => [\$target_xfer_encoding, 'auto'],
 );
 
 my %config_path_settings = (
-- 
2.21.0-777-g83232e3864

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> So, here is a two-patch series that tries to do so, primarily done
> to gauge if there still is the level of interest needed to make it
> worth for us to pursue this topic.  Here is the first one; I'll send
> the second one that takes advantage of this change separately (but
> it should be trivial to imagine what that step would involve).
>
> -- >8 --
> Subject: [PATCH 1/2] send-email: update the mechanism to set default configuration values
>
> The program has a good mechanism to specify the fallback default
> values for boolean configuration variables after two invocations of
> read_config() for "sendmail.$ident.$var" and "sendemail.$var" have
> not found any configuration.  Imitate it so that we can set the
> default values for non-boolean variables as well.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  git-send-email.perl | 51 ++++++++++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 22 deletions(-)

This one was embarrassingly buggy, and needs the following squashed
in.

Sorry about that.

 git-send-email.perl | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index ca7faff094..831947c7ed 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -411,7 +411,7 @@ sub read_config {
 	}
 
 	foreach my $setting (keys %config_settings) {
-		my $target = $config_settings{$setting};
+		my $target = $config_settings{$setting}->[0];
 		next if $setting eq "to" and defined $no_to;
 		next if $setting eq "cc" and defined $no_cc;
 		next if $setting eq "bcc" and defined $no_bcc;
@@ -447,10 +447,13 @@ sub read_config {
 }
 
 # fall back to builtin defaults
-for my $setting (values %config_settings) {
-	if (@$setting == 2 && !defined (${$setting->[0]})) {
-		${$setting->[0]} = $setting->[1];
-	}
+while (my ($name, $setting) = each %config_settings) {
+	next unless @$setting == 2;
+
+	my ($target, $default) = @$setting;
+	if (ref($target) eq "SCALAR") {
+		$$target = $default unless defined $target;
+	} # elsif ... for other types later.
 }
 
 # 'default' encryption is none -- this only prevents a warning

[PATCH v2 0/2] send-email: set xfer encoding correctly

[Junio C Hamano]

Earlier I sent a series of "oops that was wrong" which was rather
embarrassing.  This is what I have to replace Heinrich's patch that
was not quite right, while making it harder to make the same mistake
while defining fallback default values to configuration variables.

Junio C Hamano (2):
  send-email: update the mechanism to set default configuration values
  send-email: honor transferencoding config option again

 git-send-email.perl | 58 ++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 24 deletions(-)

-- 
2.21.0-777-g83232e3864

[PATCH v3 0/3] send-email: fix cli->config parsing crazyness

[Ævar Arnfjörð Bjarmason]

This is a proposed replacement for Junio's version of the
sendemail.transferencoding bugfix. As explained in 3/3 I think the
root cause is that we're needlessly doing the config->cli parsing in
the wrong order, so let's just fix that.

It fixes the bug at hand, and makes the coge less fragile for future
maintenance.

Ævar Arnfjörð Bjarmason (3):
  send-email: move the read_config() function above getopts
  send-email: rename the @bcclist variable for consistency
  send-email: do defaults -> config -> getopt in that order

 git-send-email.perl   | 179 ++++++++++++++++++++++--------------------
 t/t9001-send-email.sh |  13 ++-
 2 files changed, 105 insertions(+), 87 deletions(-)

-- 
2.21.0.1020.gf2820cf01a

Re: [PATCH v3 0/3] send-email: fix cli->config parsing crazyness

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This is a proposed replacement for Junio's version of the
> ... the
> root cause is that we're needlessly doing the config->cli parsing in
> the wrong order, so let's just fix that.

Yup, that's absolutely the right approach.  I just wanted to avoid
restructuring that heavily, but I have no problem as long as it is
somebody else, and somebody who is more competent in writing Perl
than I am, doing the heavy lifting ;-)

[PATCH v3 1/3] send-email: move the read_config() function above getopts

[Ævar Arnfjörð Bjarmason]

This is in preparation for a later change where we'll read the config
first before parsing command-line options. As the move detection will
show no lines (except one line of comment) is changed here, just moved
around.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl | 97 ++++++++++++++++++++++-----------------------
 1 file changed, 48 insertions(+), 49 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 24859a7bc3..0d87ed2b5d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -315,6 +315,54 @@ sub signal_handler {
 $SIG{TERM} = \&signal_handler;
 $SIG{INT}  = \&signal_handler;
 
+# Read our sendemail.* config
+sub read_config {
+	my ($prefix) = @_;
+
+	foreach my $setting (keys %config_bool_settings) {
+		my $target = $config_bool_settings{$setting}->[0];
+		$$target = Git::config_bool(@repo, "$prefix.$setting") unless (defined $$target);
+	}
+
+	foreach my $setting (keys %config_path_settings) {
+		my $target = $config_path_settings{$setting};
+		if (ref($target) eq "ARRAY") {
+			unless (@$target) {
+				my @values = Git::config_path(@repo, "$prefix.$setting");
+				@$target = @values if (@values && defined $values[0]);
+			}
+		}
+		else {
+			$$target = Git::config_path(@repo, "$prefix.$setting") unless (defined $$target);
+		}
+	}
+
+	foreach my $setting (keys %config_settings) {
+		my $target = $config_settings{$setting};
+		next if $setting eq "to" and defined $no_to;
+		next if $setting eq "cc" and defined $no_cc;
+		next if $setting eq "bcc" and defined $no_bcc;
+		if (ref($target) eq "ARRAY") {
+			unless (@$target) {
+				my @values = Git::config(@repo, "$prefix.$setting");
+				@$target = @values if (@values && defined $values[0]);
+			}
+		}
+		else {
+			$$target = Git::config(@repo, "$prefix.$setting") unless (defined $$target);
+		}
+	}
+
+	if (!defined $smtp_encryption) {
+		my $enc = Git::config(@repo, "$prefix.smtpencryption");
+		if (defined $enc) {
+			$smtp_encryption = $enc;
+		} elsif (Git::config_bool(@repo, "$prefix.smtpssl")) {
+			$smtp_encryption = 'ssl';
+		}
+	}
+}
+
 # Begin by accumulating all the variables (defined above), that we will end up
 # needing, first, from the command line:
 
@@ -399,55 +447,6 @@ sub signal_handler {
 	"(via command-line or configuration option)\n")
 	if defined $relogin_delay and not defined $batch_size;
 
-# Now, let's fill any that aren't set in with defaults:
-
-sub read_config {
-	my ($prefix) = @_;
-
-	foreach my $setting (keys %config_bool_settings) {
-		my $target = $config_bool_settings{$setting}->[0];
-		$$target = Git::config_bool(@repo, "$prefix.$setting") unless (defined $$target);
-	}
-
-	foreach my $setting (keys %config_path_settings) {
-		my $target = $config_path_settings{$setting};
-		if (ref($target) eq "ARRAY") {
-			unless (@$target) {
-				my @values = Git::config_path(@repo, "$prefix.$setting");
-				@$target = @values if (@values && defined $values[0]);
-			}
-		}
-		else {
-			$$target = Git::config_path(@repo, "$prefix.$setting") unless (defined $$target);
-		}
-	}
-
-	foreach my $setting (keys %config_settings) {
-		my $target = $config_settings{$setting};
-		next if $setting eq "to" and defined $no_to;
-		next if $setting eq "cc" and defined $no_cc;
-		next if $setting eq "bcc" and defined $no_bcc;
-		if (ref($target) eq "ARRAY") {
-			unless (@$target) {
-				my @values = Git::config(@repo, "$prefix.$setting");
-				@$target = @values if (@values && defined $values[0]);
-			}
-		}
-		else {
-			$$target = Git::config(@repo, "$prefix.$setting") unless (defined $$target);
-		}
-	}
-
-	if (!defined $smtp_encryption) {
-		my $enc = Git::config(@repo, "$prefix.smtpencryption");
-		if (defined $enc) {
-			$smtp_encryption = $enc;
-		} elsif (Git::config_bool(@repo, "$prefix.smtpssl")) {
-			$smtp_encryption = 'ssl';
-		}
-	}
-}
-
 # read configuration from [sendemail "$identity"], fall back on [sendemail]
 $identity = Git::config(@repo, "sendemail.identity") unless (defined $identity);
 read_config("sendemail.$identity") if (defined $identity);
-- 
2.21.0.1020.gf2820cf01a

[PATCH v3 2/3] send-email: rename the @bcclist variable for consistency

[Ævar Arnfjörð Bjarmason]

The "to" and "cc" variables are named @initial_{to,cc}, let's rename
this one to match them.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 0d87ed2b5d..48ed18a85c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -177,7 +177,7 @@ sub format_2822_time {
 my $re_encoded_word = qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/;
 
 # Variables we fill in automatically, or via prompting:
-my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh,
+my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@initial_bcc,$no_bcc,@xh,
 	$initial_in_reply_to,$reply_to,$initial_subject,@files,
 	$author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
 
@@ -272,7 +272,7 @@ sub do_edit {
     "cc" => \@initial_cc,
     "cccmd" => \$cc_cmd,
     "aliasfiletype" => \$aliasfiletype,
-    "bcc" => \@bcclist,
+    "bcc" => \@initial_bcc,
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
     "confirm"   => \$confirm,
@@ -383,7 +383,7 @@ sub read_config {
 		    "no-to" => \$no_to,
 		    "cc=s" => \@initial_cc,
 		    "no-cc" => \$no_cc,
-		    "bcc=s" => \@bcclist,
+		    "bcc=s" => \@initial_bcc,
 		    "no-bcc" => \$no_bcc,
 		    "chain-reply-to!" => \$chain_reply_to,
 		    "no-chain-reply-to" => sub {$chain_reply_to = 0},
@@ -940,7 +940,7 @@ sub expand_one_alias {
 
 @initial_to = process_address_list(@initial_to);
 @initial_cc = process_address_list(@initial_cc);
-@bcclist = process_address_list(@bcclist);
+@initial_bcc = process_address_list(@initial_bcc);
 
 if ($thread && !defined $initial_in_reply_to && $prompting) {
 	$initial_in_reply_to = ask(
@@ -1363,7 +1363,7 @@ sub send_message {
 		    }
 	       @cc);
 	my $to = join (",\n\t", @recipients);
-	@recipients = unique_email_list(@recipients,@cc,@bcclist);
+	@recipients = unique_email_list(@recipients,@cc,@initial_bcc);
 	@recipients = (map { extract_valid_address_or_die($_) } @recipients);
 	my $date = format_2822_time($time++);
 	my $gitversion = '@@GIT_VERSION@@';
-- 
2.21.0.1020.gf2820cf01a

[PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order

[Ævar Arnfjörð Bjarmason]

Change the git-send-email command-line argument parsing and config
reading code to parse those two in the right order. I.e. first we set
our hardcoded defaults, then we read our config, and finally we read
the command-line, with later sets overriding earlier sets.

This fixes a bug introduced in e67a228cd8 ("send-email: automatically
determine transfer-encoding", 2018-07-08). That change broke the broke
the reading of sendmail.transferencoding because it wasn't careful to
update our fragile code dealing with doing this in the previous
"defaults -> getopt -> config" order..

But as we can see from the history for this file doing it this way was
never what we actually wanted, it just something we grew organically
as of 5483c71d7a ("git-send-email: make options easier to configure.",
2007-06-27) and have been dealing with the fallout since, e.g. in
463b0ea22b ("send-email: Fix %config_path_settings handling",
2011-10-14).

As can be seen in this change the only place where we actually want to
do something clever is with the to/cc/bcc variables, where setting
them on the command-line (or using --no-{to,cc,bcc}) should clear out
values we grab from the config.

All the rest are things where the command-line should simply override
the config values, and by reading the config first the config code
doesn't need all this "let's not set it was on the command-line"
special-casing, as [1] shows we'd otherwise need to care about the
difference between whether something was a default or present in
config to fix the bug in e67a228cd8.

1. https://public-inbox.org/git/20190508105607.178244-2-gitster@pobox.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl   | 92 +++++++++++++++++++++++--------------------
 t/t9001-send-email.sh | 13 +++++-
 2 files changed, 62 insertions(+), 43 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 48ed18a85c..fab255249f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -177,11 +177,15 @@ sub format_2822_time {
 my $re_encoded_word = qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/;
 
 # Variables we fill in automatically, or via prompting:
-my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@initial_bcc,$no_bcc,@xh,
+my (@to,@cc,,@xh,$envelope_sender,
 	$initial_in_reply_to,$reply_to,$initial_subject,@files,
-	$author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
-
-my $envelope_sender;
+	$author,$sender,$smtp_authpass,$annotate,$compose,$time);
+# Things we either get from config, *or* are overridden on the
+# command-line.
+my ($no_cc, $no_to, $no_bcc);
+my (@config_to, @getopt_to);
+my (@config_cc, @getopt_cc);
+my (@config_bcc, @getopt_bcc);
 
 # Example reply to:
 #$initial_in_reply_to = ''; #<20050203173208.GA23964@foobar.com>';
@@ -228,33 +232,37 @@ sub do_edit {
 }
 
 # Variables with corresponding config settings
-my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
+my ($suppress_from, $signed_off_by_cc);
 my ($cover_cc, $cover_to);
 my ($to_cmd, $cc_cmd);
 my ($smtp_server, $smtp_server_port, @smtp_server_options);
 my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);
 my ($batch_size, $relogin_delay);
 my ($identity, $aliasfiletype, @alias_files, $smtp_domain, $smtp_auth);
-my ($validate, $confirm);
+my ($confirm);
 my (@suppress_cc);
 my ($auto_8bit_encoding);
 my ($compose_encoding);
-my $target_xfer_encoding = 'auto';
-
+# Variables with corresponding config settings & hardcoded defaults
 my ($debug_net_smtp) = 0;		# Net::SMTP, see send_message()
+my $thread = 1;
+my $chain_reply_to = 0;
+my $use_xmailer = 1;
+my $validate = 1;
+my $target_xfer_encoding = 'auto';
 
 my %config_bool_settings = (
-    "thread" => [\$thread, 1],
-    "chainreplyto" => [\$chain_reply_to, 0],
-    "suppressfrom" => [\$suppress_from, undef],
-    "signedoffbycc" => [\$signed_off_by_cc, undef],
-    "cccover" => [\$cover_cc, undef],
-    "tocover" => [\$cover_to, undef],
-    "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
-    "validate" => [\$validate, 1],
-    "multiedit" => [\$multiedit, undef],
-    "annotate" => [\$annotate, undef],
-    "xmailer" => [\$use_xmailer, 1]
+    "thread" => \$thread,
+    "chainreplyto" => \$chain_reply_to,
+    "suppressfrom" => \$suppress_from,
+    "signedoffbycc" => \$signed_off_by_cc,
+    "cccover" => \$cover_cc,
+    "tocover" => \$cover_to,
+    "signedoffcc" => \$signed_off_by_cc,
+    "validate" => \$validate,
+    "multiedit" => \$multiedit,
+    "annotate" => \$annotate,
+    "xmailer" => \$use_xmailer,
 );
 
 my %config_settings = (
@@ -267,12 +275,12 @@ sub do_edit {
     "smtpauth" => \$smtp_auth,
     "smtpbatchsize" => \$batch_size,
     "smtprelogindelay" => \$relogin_delay,
-    "to" => \@initial_to,
+    "to" => \@config_to,
     "tocmd" => \$to_cmd,
-    "cc" => \@initial_cc,
+    "cc" => \@config_cc,
     "cccmd" => \$cc_cmd,
     "aliasfiletype" => \$aliasfiletype,
-    "bcc" => \@initial_bcc,
+    "bcc" => \@config_bcc,
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
     "confirm"   => \$confirm,
@@ -320,8 +328,9 @@ sub read_config {
 	my ($prefix) = @_;
 
 	foreach my $setting (keys %config_bool_settings) {
-		my $target = $config_bool_settings{$setting}->[0];
-		$$target = Git::config_bool(@repo, "$prefix.$setting") unless (defined $$target);
+		my $target = $config_bool_settings{$setting};
+		my $v = Git::config_bool(@repo, "$prefix.$setting");
+		$$target = $v if defined $v;
 	}
 
 	foreach my $setting (keys %config_path_settings) {
@@ -333,15 +342,13 @@ sub read_config {
 			}
 		}
 		else {
-			$$target = Git::config_path(@repo, "$prefix.$setting") unless (defined $$target);
+			my $v = Git::config_path(@repo, "$prefix.$setting");
+			$$target = $v if defined $v;
 		}
 	}
 
 	foreach my $setting (keys %config_settings) {
 		my $target = $config_settings{$setting};
-		next if $setting eq "to" and defined $no_to;
-		next if $setting eq "cc" and defined $no_cc;
-		next if $setting eq "bcc" and defined $no_bcc;
 		if (ref($target) eq "ARRAY") {
 			unless (@$target) {
 				my @values = Git::config(@repo, "$prefix.$setting");
@@ -349,7 +356,8 @@ sub read_config {
 			}
 		}
 		else {
-			$$target = Git::config(@repo, "$prefix.$setting") unless (defined $$target);
+			my $v = Git::config(@repo, "$prefix.$setting");
+			$$target = $v if defined $v;
 		}
 	}
 
@@ -363,6 +371,11 @@ sub read_config {
 	}
 }
 
+$identity = Git::config(@repo, "sendemail.identity");
+read_config("sendemail.$identity") if defined $identity;
+read_config("sendemail");
+read_config("sendemail");
+
 # Begin by accumulating all the variables (defined above), that we will end up
 # needing, first, from the command line:
 
@@ -378,12 +391,12 @@ sub read_config {
                     "in-reply-to=s" => \$initial_in_reply_to,
 		    "reply-to=s" => \$reply_to,
 		    "subject=s" => \$initial_subject,
-		    "to=s" => \@initial_to,
+		    "to=s" => \@getopt_to,
 		    "to-cmd=s" => \$to_cmd,
 		    "no-to" => \$no_to,
-		    "cc=s" => \@initial_cc,
+		    "cc=s" => \@getopt_cc,
 		    "no-cc" => \$no_cc,
-		    "bcc=s" => \@initial_bcc,
+		    "bcc=s" => \@getopt_bcc,
 		    "no-bcc" => \$no_bcc,
 		    "chain-reply-to!" => \$chain_reply_to,
 		    "no-chain-reply-to" => sub {$chain_reply_to = 0},
@@ -434,6 +447,11 @@ sub read_config {
 		    "git-completion-helper" => \$git_completion_helper,
 	 );
 
+# Munge any "either config or getopt, not both" variables
+my @initial_to = @getopt_to ? @getopt_to : ($no_to ? () : @config_to);
+my @initial_cc = @getopt_cc ? @getopt_cc : ($no_cc ? () : @config_cc);
+my @initial_bcc = @getopt_bcc ? @getopt_bcc : ($no_bcc ? () : @config_bcc);
+
 usage() if $help;
 completion_helper() if $git_completion_helper;
 unless ($rc) {
@@ -447,16 +465,6 @@ sub read_config {
 	"(via command-line or configuration option)\n")
 	if defined $relogin_delay and not defined $batch_size;
 
-# read configuration from [sendemail "$identity"], fall back on [sendemail]
-$identity = Git::config(@repo, "sendemail.identity") unless (defined $identity);
-read_config("sendemail.$identity") if (defined $identity);
-read_config("sendemail");
-
-# fall back on builtin bool defaults
-foreach my $setting (values %config_bool_settings) {
-	${$setting->[0]} = $setting->[1] unless (defined (${$setting->[0]}));
-}
-
 # 'default' encryption is none -- this only prevents a warning
 $smtp_encryption = '' unless (defined $smtp_encryption);
 
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 1e3ac3c384..1b18201ce2 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1461,7 +1461,18 @@ test_expect_success $PREREQ '--transfer-encoding overrides sendemail.transferEnc
 	test -z "$(ls msgtxt*)"
 '
 
-test_expect_success $PREREQ 'sendemail.transferencoding=8bit' '
+test_expect_success $PREREQ 'sendemail.transferencoding=8bit via config' '
+	clean_fake_sendmail &&
+	git -c sendemail.transferencoding=8bit send-email \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		email-using-8bit \
+		2>errors >out &&
+	sed '1,/^$/d' msgtxt1 >actual &&
+	sed '1,/^$/d' email-using-8bit >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success $PREREQ 'sendemail.transferencoding=8bit via cli' '
 	clean_fake_sendmail &&
 	git send-email \
 		--transfer-encoding=8bit \
-- 
2.21.0.1020.gf2820cf01a

Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order

[Eric Sunshine]

On Thu, May 9, 2019 at 7:48 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Change the git-send-email command-line argument parsing and config
> reading code to parse those two in the right order. I.e. first we set
> our hardcoded defaults, then we read our config, and finally we read
> the command-line, with later sets overriding earlier sets.
>
> This fixes a bug introduced in e67a228cd8 ("send-email: automatically
> determine transfer-encoding", 2018-07-08). That change broke the broke

s/broke the broke/broke/

> the reading of sendmail.transferencoding because it wasn't careful to
> update our fragile code dealing with doing this in the previous
> "defaults -> getopt -> config" order..

s/\.\.$/./

> But as we can see from the history for this file doing it this way was
> never what we actually wanted, it just something we grew organically

s/it/it's/

> as of 5483c71d7a ("git-send-email: make options easier to configure.",
> 2007-06-27) and have been dealing with the fallout since, e.g. in
> 463b0ea22b ("send-email: Fix %config_path_settings handling",
> 2011-10-14).
>
> As can be seen in this change the only place where we actually want to
> do something clever is with the to/cc/bcc variables, where setting
> them on the command-line (or using --no-{to,cc,bcc}) should clear out
> values we grab from the config.
>
> All the rest are things where the command-line should simply override
> the config values, and by reading the config first the config code
> doesn't need all this "let's not set it was on the command-line"

ECANTPARSE "let's not set it was on the command-line"

> special-casing, as [1] shows we'd otherwise need to care about the
> difference between whether something was a default or present in
> config to fix the bug in e67a228cd8.
>
> 1. https://public-inbox.org/git/20190508105607.178244-2-gitster@pobox.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> All the rest are things where the command-line should simply override
>> the config values, and by reading the config first the config code
>> doesn't need all this "let's not set it was on the command-line"
>
> ECANTPARSE "let's not set it was on the command-line"

I took it as "let's not set it, if it was on the command-line".

Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On Thu, May 09, 2019 at 01:48:30PM +0200, Ævar Arnfjörð Bjarmason wrote:
> @@ -363,6 +371,11 @@ sub read_config {
>  	}
>  }
>  
> +$identity = Git::config(@repo, "sendemail.identity");
> +read_config("sendemail.$identity") if defined $identity;
> +read_config("sendemail");
> +read_config("sendemail");

Did we really want to call this function twice with the same argument?
If so, perhaps a comment is appropriate explaining why.

In general, I'm not opposed to this approach, either. It does make it
easier to handle parsing in general. However, having said that, I'm fine
with either your series or Junio's.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  # Variables we fill in automatically, or via prompting:
> -my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@initial_bcc,$no_bcc,@xh,
> +my (@to,@cc,,@xh,$envelope_sender,

,,???

Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order

[Ævar Arnfjörð Bjarmason]

On Mon, May 13 2019, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>>  # Variables we fill in automatically, or via prompting:
>> -my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@initial_bcc,$no_bcc,@xh,
>> +my (@to,@cc,,@xh,$envelope_sender,
>
> ,,???

Just a typo. I see you fixed this and the other issues noted in your
push-out to "next", thanks! Sorry about not getting to this before that.

Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order

[Stephen Boyd]

(Sorry for weird reply, I'm not subscribed and my MUA is not prepared
for this)

> Change the git-send-email command-line argument parsing and config
> reading code to parse those two in the right order. I.e. first we set
> our hardcoded defaults, then we read our config, and finally we read
> the command-line, with later sets overriding earlier sets.
> 
> This fixes a bug introduced in e67a228cd8 ("send-email: automatically
> determine transfer-encoding", 2018-07-08). That change broke the broke
> the reading of sendmail.transferencoding because it wasn't careful to
> update our fragile code dealing with doing this in the previous
> "defaults -> getopt -> config" order..
> 
> But as we can see from the history for this file doing it this way was
> never what we actually wanted, it just something we grew organically
> as of 5483c71d7a ("git-send-email: make options easier to configure.",
> 2007-06-27) and have been dealing with the fallout since, e.g. in
> 463b0ea22b ("send-email: Fix %config_path_settings handling",
> 2011-10-14).
> 
> As can be seen in this change the only place where we actually want to
> do something clever is with the to/cc/bcc variables, where setting
> them on the command-line (or using --no-{to,cc,bcc}) should clear out
> values we grab from the config.
> 
> All the rest are things where the command-line should simply override
> the config values, and by reading the config first the config code
> doesn't need all this "let's not set it was on the command-line"
> special-casing, as [1] shows we'd otherwise need to care about the
> difference between whether something was a default or present in
> config to fix the bug in e67a228cd8.

This broke my workflow.

I specify --identity=<account> on the commandline and I want that to
pick out my send-email config from my global .gitconfig file that
corresponds to that identity. With this change, the config is parsed
before the getopt part so --identity on the commandline is a nop and
never looks into the config file to figure this out. So at least
--identity is special in addition to --to,cc,bcc.

Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order

[Junio C Hamano]

Stephen Boyd <swboyd@chromium.org> writes:

>> As can be seen in this change the only place where we actually want to
>> do something clever is with the to/cc/bcc variables, where setting
>> them on the command-line (or using --no-{to,cc,bcc}) should clear out
>> values we grab from the config.
>> 
>> All the rest are things where the command-line should simply override
>> the config values, and by reading the config first the config code
>> doesn't need all this "let's not set it was on the command-line"
>> special-casing, as [1] shows we'd otherwise need to care about the
>> difference between whether something was a default or present in
>> config to fix the bug in e67a228cd8.
>
> This broke my workflow.
>
> I specify --identity=<account> on the commandline and I want that to
> pick out my send-email config from my global .gitconfig file that
> corresponds to that identity. With this change, the config is parsed
> before the getopt part so --identity on the commandline is a nop and
> never looks into the config file to figure this out. So at least
> --identity is special in addition to --to,cc,bcc.

Ah, sorry that nobody noticed that case, but you are right.  Because
the ident is used as a part of the key to find identity-specific
configuration values, if the command line gives one, we must have an
access to it before we start reading the configuration.  In that sense,
it is more fundamental to special-case the option.

We are past -rc0, so I am inclined to revert the change (and perhaps
replace it with the other "fix" that did not break the parsing order
like these patches did), with an expectation that a clever fix will
be found later, *unless* a simple and correct fix is found quickly.

Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ah, sorry that nobody noticed that case, but you are right.  Because
> the ident is used as a part of the key to find identity-specific
> configuration values, if the command line gives one, we must have an
> access to it before we start reading the configuration.  In that sense,
> it is more fundamental to special-case the option.
>
> We are past -rc0, so I am inclined to revert the change (and perhaps
> replace it with the other "fix" that did not break the parsing order
> like these patches did), with an expectation that a clever fix will
> be found later, *unless* a simple and correct fix is found quickly.

Oops, spoke too soon.  The topic hasn't escaped to 'master' yet, so
I'll make sure it stays that way.  Thanks for catching a regression
before it gets too late.

[PATCH 0/5] ab/send-email-transferencoding-fix-for-the-fix

[Ævar Arnfjörð Bjarmason]

This ab/send-email-transferencoding-fix-for-the-fix series goes on top
of the ab/send-email-transferencoding-fix merged into "next". It fixes
the sendemail.identity issue Stephen Boyd reported, and then some.

Maybe it would be more understandable to rewind the existing topic out
of next and produce a new rebased series, but I think this is actually
easier to review, and documents some of the tricky edge cases for
future contributors.

I did make sure that the new tests I added for existing behavior were
actually testing existing behavior, and not whatever bugs I got into
"next" by running the test on top of "master", only the expected (now
fixed) tests fail there, not any tests for existing sendemail.identity
behavior.

Ævar Arnfjörð Bjarmason (5):
  send-email: remove cargo-culted multi-patch pattern in tests
  send-email: fix broken transferEncoding tests
  send-email: document --no-[to|cc|bcc]
  send-email: fix regression in sendemail.identity parsing
  send-email: remove support for deprecated sendemail.smtpssl

 Documentation/config/sendemail.txt |   3 -
 Documentation/git-send-email.txt   |  11 ++-
 git-send-email.perl                |  78 ++++++++++--------
 t/t9001-send-email.sh              | 122 +++++++++++++++++++++--------
 4 files changed, 144 insertions(+), 70 deletions(-)

-- 
2.21.0.1020.gf2820cf01a

[PATCH 1/5] send-email: remove cargo-culted multi-patch pattern in tests

[Ævar Arnfjörð Bjarmason]

Change test code added in f434c083a0 ("send-email: add --no-cc,
--no-to, and --no-bcc", 2010-03-07) which blindly copied a pattern
from an earlier test added in 32ae83194b ("add a test for
git-send-email for non-threaded mails", 2009-06-12) where the
"$patches" variable was supplied more than once.

As it turns out we didn't need more than one "$patches" for the test
added in 32ae83194b either. The only tests that actually needed this
sort of invocation were the tests added in 54aae5e1a0 ("t9001:
send-email interation with --in-reply-to and --chain-reply-to",
2010-10-19).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t9001-send-email.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 1b18201ce2..13de44686b 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1204,7 +1204,7 @@ test_expect_success $PREREQ 'no in-reply-to and no threading' '
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
 		--no-thread \
-		$patches $patches >stdout &&
+		$patches >stdout &&
 	! grep "In-Reply-To: " stdout
 '
 
@@ -1224,7 +1224,7 @@ test_expect_success $PREREQ 'sendemail.to works' '
 	git send-email \
 		--dry-run \
 		--from="Example <nobody@example.com>" \
-		$patches $patches >stdout &&
+		$patches >stdout &&
 	grep "To: Somebody <somebody@ex.com>" stdout
 '
 
@@ -1234,7 +1234,7 @@ test_expect_success $PREREQ '--no-to overrides sendemail.to' '
 		--from="Example <nobody@example.com>" \
 		--no-to \
 		--to=nobody@example.com \
-		$patches $patches >stdout &&
+		$patches >stdout &&
 	grep "To: nobody@example.com" stdout &&
 	! grep "To: Somebody <somebody@ex.com>" stdout
 '
@@ -1245,7 +1245,7 @@ test_expect_success $PREREQ 'sendemail.cc works' '
 		--dry-run \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		$patches $patches >stdout &&
+		$patches >stdout &&
 	grep "Cc: Somebody <somebody@ex.com>" stdout
 '
 
@@ -1256,7 +1256,7 @@ test_expect_success $PREREQ '--no-cc overrides sendemail.cc' '
 		--no-cc \
 		--cc=bodies@example.com \
 		--to=nobody@example.com \
-		$patches $patches >stdout &&
+		$patches >stdout &&
 	grep "Cc: bodies@example.com" stdout &&
 	! grep "Cc: Somebody <somebody@ex.com>" stdout
 '
@@ -1268,7 +1268,7 @@ test_expect_success $PREREQ 'sendemail.bcc works' '
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
 		--smtp-server relay.example.com \
-		$patches $patches >stdout &&
+		$patches >stdout &&
 	grep "RCPT TO:<other@ex.com>" stdout
 '
 
@@ -1280,7 +1280,7 @@ test_expect_success $PREREQ '--no-bcc overrides sendemail.bcc' '
 		--bcc=bodies@example.com \
 		--to=nobody@example.com \
 		--smtp-server relay.example.com \
-		$patches $patches >stdout &&
+		$patches >stdout &&
 	grep "RCPT TO:<bodies@example.com>" stdout &&
 	! grep "RCPT TO:<other@ex.com>" stdout
 '
-- 
2.21.0.1020.gf2820cf01a

[PATCH 2/5] send-email: fix broken transferEncoding tests

[Ævar Arnfjörð Bjarmason]

I fixed a bug that had broken the reading of sendmail.transferEncoding
in 3494dfd3ee ("send-email: do defaults -> config -> getopt in that
order", 2019-05-09), but the test I added in that commit did nothing
to assert the bug had been fixed.

That issue originates in 8d81408435 ("git-send-email: add
--transfer-encoding option", 2014-11-25) which first added the
"sendemail.transferencoding=8bit".

That test has never done anything meaningful. It tested that the
"--transfer-encoding=8bit" option would turn on the 8bit
Transfer-Encoding, but that was the default at the time (and now). As
checking out 8d81408435 and editing the test to remove that option
will reveal, supplying it never did anything.

So when I copied it thinking it would work in 3494dfd3ee I copied a
previously broken test, although I was making sure it did the right
thing via da-hoc debugger inspection, so the bug was fixed.

So fix the test I added in 3494dfd3ee, as well as the long-standing
test added in 8d81408435. To test if we're actually setting the
Transfer-Encoding let's set it to 7bit, not 8bit, as 7bit will error
out on "email-using-8bit".

This means that we can remove the "sendemail.transferencoding=7bit
fails on 8bit data" test, since it was redundant, we now have other
tests that assert that that'll fail.

While I'm at it convert "git config <key> <value>" in the test setup
to just "-c <key>=<value>" on the command-line. Then we don't need to
cleanup after these tests, and there's no sense in asserting where
config values come from in these tests, we can take that as a given.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t9001-send-email.sh | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 13de44686b..61d484d1a6 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1437,10 +1437,10 @@ test_expect_success $PREREQ 'setup expect' '
 	EOF
 '
 
-test_expect_success $PREREQ 'sendemail.transferencoding=7bit fails on 8bit data' '
+test_expect_success $PREREQ '--transfer-encoding overrides sendemail.transferEncoding' '
 	clean_fake_sendmail &&
-	git config sendemail.transferEncoding 7bit &&
-	test_must_fail git send-email \
+	test_must_fail git -c sendemail.transferEncoding=8bit \
+		send-email \
 		--transfer-encoding=7bit \
 		--smtp-server="$(pwd)/fake.sendmail" \
 		email-using-8bit \
@@ -1449,11 +1449,10 @@ test_expect_success $PREREQ 'sendemail.transferencoding=7bit fails on 8bit data'
 	test -z "$(ls msgtxt*)"
 '
 
-test_expect_success $PREREQ '--transfer-encoding overrides sendemail.transferEncoding' '
+test_expect_success $PREREQ 'sendemail.transferEncoding via config' '
 	clean_fake_sendmail &&
-	git config sendemail.transferEncoding 8bit &&
-	test_must_fail git send-email \
-		--transfer-encoding=7bit \
+	test_must_fail git -c sendemail.transferEncoding=7bit \
+		send-email \
 		--smtp-server="$(pwd)/fake.sendmail" \
 		email-using-8bit \
 		2>errors >out &&
@@ -1461,27 +1460,15 @@ test_expect_success $PREREQ '--transfer-encoding overrides sendemail.transferEnc
 	test -z "$(ls msgtxt*)"
 '
 
-test_expect_success $PREREQ 'sendemail.transferencoding=8bit via config' '
+test_expect_success $PREREQ 'sendemail.transferEncoding via cli' '
 	clean_fake_sendmail &&
-	git -c sendemail.transferencoding=8bit send-email \
-		--smtp-server="$(pwd)/fake.sendmail" \
-		email-using-8bit \
-		2>errors >out &&
-	sed '1,/^$/d' msgtxt1 >actual &&
-	sed '1,/^$/d' email-using-8bit >expected &&
-	test_cmp expected actual
-'
-
-test_expect_success $PREREQ 'sendemail.transferencoding=8bit via cli' '
-	clean_fake_sendmail &&
-	git send-email \
-		--transfer-encoding=8bit \
+	test_must_fail git send-email \
+		--transfer-encoding=7bit \
 		--smtp-server="$(pwd)/fake.sendmail" \
 		email-using-8bit \
 		2>errors >out &&
-	sed '1,/^$/d' msgtxt1 >actual &&
-	sed '1,/^$/d' email-using-8bit >expected &&
-	test_cmp expected actual
+	grep "cannot send message as 7bit" errors &&
+	test -z "$(ls msgtxt*)"
 '
 
 test_expect_success $PREREQ 'setup expect' '
-- 
2.21.0.1020.gf2820cf01a

[PATCH 3/5] send-email: document --no-[to|cc|bcc]

[Ævar Arnfjörð Bjarmason]

These options added in f434c083a0 ("send-email: add --no-cc, --no-to,
and --no-bcc", 2010-03-07) were never documented.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-send-email.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 1afe9fc858..8100ff4b0f 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -278,6 +278,10 @@ must be used for each option.
 Automating
 ~~~~~~~~~~
 
+--no-[to|cc|bcc]::
+	Clears any list of "To:", "Cc:", "Bcc:" addresses previously
+	set via config.
+
 --to-cmd=<command>::
 	Specify a command to execute once per patch file which
 	should generate patch file specific "To:" entries.
-- 
2.21.0.1020.gf2820cf01a

[PATCH 4/5] send-email: fix regression in sendemail.identity parsing

[Ævar Arnfjörð Bjarmason]

Fix a regression in my recent 3494dfd3ee ("send-email: do defaults ->
config -> getopt in that order", 2019-05-09). I missed that the
$identity variable needs to be extracted from the command-line before
we do the config reading, as it determines which config variable we
should read first. See [1] for the report.

The sendemail.identity feature was added back in
34cc60ce2b ("send-email: Add support for SSL and SMTP-AUTH",
2007-09-03), there were no tests to assert that it worked properly.

So let's fix both the regression, and add some tests to assert that
this is being parsed properly. While I'm at it I'm adding a
--no-identity option to go with --[to|cc|bcc] variable, since the
semantics are similar. It's like to/cc/bcc except that unlike those we
don't support multiple identities, but we could now easily add it
support for it if anyone cares.

In just fixing the --identity command-line parsing bug I discovered
that a narrow fix to that wouldn't do. In read_config() we had a state
machine that would only set config values if they weren't set already,
and thus by proxy we wouldn't e.g. set "to" based on sendemail.to if
we'd seen sendemail.gmail.to before, with --identity=gmail.

I'd modified some of the relevant code in 3494dfd3ee, but just
reverting to that wouldn't do, since it would bring back the
regression fixed in that commit.

Refactor read_config() do what we actually mean here. We don't want to
set a given sendemail.VAR if a sendemail.$identity.VAR previously set
it. The old code was conflating this desire with the hardcoded
defaults for these variables, and as discussed in 3494dfd3ee that was
never going to work. Instead pass along the state of whether an
identity config set something before, as distinguished from the state
of the default just being false, or the default being a non-bool or
true (e.g. --transferencoding).

I'm still not happy with the test coverage here, e.g. there's nothing
testing sendemail.smtpEncryption, but I only have so much time to fix
this code.

1. https://public-inbox.org/git/5cddeb61.1c69fb81.47ed4.e648@mx.google.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-send-email.txt |  4 ++
 git-send-email.perl              | 60 ++++++++++++++++++++----------
 t/t9001-send-email.sh            | 64 ++++++++++++++++++++++++++++++++
 3 files changed, 108 insertions(+), 20 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 8100ff4b0f..a861934c69 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -282,6 +282,10 @@ Automating
 	Clears any list of "To:", "Cc:", "Bcc:" addresses previously
 	set via config.
 
+--no-identity::
+	Clears the previously read value of `sendemail.identity` set
+	via config, if any.
+
 --to-cmd=<command>::
 	Specify a command to execute once per patch file which
 	should generate patch file specific "To:" entries.
diff --git a/git-send-email.perl b/git-send-email.perl
index b2c4a77671..80cbbfd2b8 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -182,7 +182,7 @@ sub format_2822_time {
 	$author,$sender,$smtp_authpass,$annotate,$compose,$time);
 # Things we either get from config, *or* are overridden on the
 # command-line.
-my ($no_cc, $no_to, $no_bcc);
+my ($no_cc, $no_to, $no_bcc, $no_identity);
 my (@config_to, @getopt_to);
 my (@config_cc, @getopt_cc);
 my (@config_bcc, @getopt_bcc);
@@ -325,44 +325,53 @@ sub signal_handler {
 
 # Read our sendemail.* config
 sub read_config {
-	my ($prefix) = @_;
+	my ($configured, $prefix) = @_;
 
 	foreach my $setting (keys %config_bool_settings) {
 		my $target = $config_bool_settings{$setting};
 		my $v = Git::config_bool(@repo, "$prefix.$setting");
-		$$target = $v if defined $v;
+		next unless defined $v;
+		next if $configured->{$setting}++;
+		$$target = $v;
 	}
 
 	foreach my $setting (keys %config_path_settings) {
 		my $target = $config_path_settings{$setting};
 		if (ref($target) eq "ARRAY") {
-			unless (@$target) {
-				my @values = Git::config_path(@repo, "$prefix.$setting");
-				@$target = @values if (@values && defined $values[0]);
-			}
+			my @values = Git::config_path(@repo, "$prefix.$setting");
+			next unless @values;
+			next if $configured->{$setting}++;
+			@$target = @values;
 		}
 		else {
 			my $v = Git::config_path(@repo, "$prefix.$setting");
-			$$target = $v if defined $v;
+			next unless defined $v;
+			next if $configured->{$setting}++;
+			$$target = $v;
 		}
 	}
 
 	foreach my $setting (keys %config_settings) {
 		my $target = $config_settings{$setting};
 		if (ref($target) eq "ARRAY") {
-			unless (@$target) {
-				my @values = Git::config(@repo, "$prefix.$setting");
-				@$target = @values if (@values && defined $values[0]);
-			}
+			my @values = Git::config(@repo, "$prefix.$setting");
+			next unless @values;
+			next if $configured->{$setting}++;
+			@$target = @values;
 		}
 		else {
 			my $v = Git::config(@repo, "$prefix.$setting");
-			$$target = $v if defined $v;
+			next unless defined $v;
+			next if $configured->{$setting}++;
+			$$target = $v;
 		}
 	}
 
 	if (!defined $smtp_encryption) {
-		my $enc = Git::config(@repo, "$prefix.smtpencryption");
+		my $setting = "$prefix.smtpencryption";
+		my $enc = Git::config(@repo, $setting);
+		return unless defined $enc;
+		return if $configured->{$setting}++;
 		if (defined $enc) {
 			$smtp_encryption = $enc;
 		} elsif (Git::config_bool(@repo, "$prefix.smtpssl")) {
@@ -371,17 +380,30 @@ sub read_config {
 	}
 }
 
+# sendemail.identity yields to --identity. We must parse this
+# special-case first before the rest of the config is read.
 $identity = Git::config(@repo, "sendemail.identity");
-read_config("sendemail.$identity") if defined $identity;
-read_config("sendemail");
+my $rc = GetOptions(
+	"identity=s" => \$identity,
+	"no-identity" => \$no_identity,
+);
+usage() unless $rc;
+undef $identity if $no_identity;
+
+# Now we know enough to read the config
+{
+    my %configured;
+    read_config(\%configured, "sendemail.$identity") if defined $identity;
+    read_config(\%configured, "sendemail");
+}
 
 # Begin by accumulating all the variables (defined above), that we will end up
 # needing, first, from the command line:
 
 my $help;
 my $git_completion_helper;
-my $rc = GetOptions("h" => \$help,
-                    "dump-aliases" => \$dump_aliases);
+$rc = GetOptions("h" => \$help,
+                 "dump-aliases" => \$dump_aliases);
 usage() unless $rc;
 die __("--dump-aliases incompatible with other options\n")
     if !$help and $dump_aliases and @ARGV;
@@ -410,8 +432,6 @@ sub read_config {
 		    "smtp-debug:i" => \$debug_net_smtp,
 		    "smtp-domain:s" => \$smtp_domain,
 		    "smtp-auth=s" => \$smtp_auth,
-		    "no-smtp-auth" => sub {$smtp_auth = 'none'},
-		    "identity=s" => \$identity,
 		    "annotate!" => \$annotate,
 		    "no-annotate" => sub {$annotate = 0},
 		    "compose" => \$compose,
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 61d484d1a6..890e2874c3 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1228,6 +1228,61 @@ test_expect_success $PREREQ 'sendemail.to works' '
 	grep "To: Somebody <somebody@ex.com>" stdout
 '
 
+test_expect_success $PREREQ 'setup sendemail.identity' '
+	git config --replace-all sendemail.to "default@example.com" &&
+	git config --replace-all sendemail.isp.to "isp@example.com" &&
+	git config --replace-all sendemail.cloud.to "cloud@example.com"
+'
+
+test_expect_success $PREREQ 'sendemail.identity: reads the correct identity config' '
+	git -c sendemail.identity=cloud send-email \
+		--dry-run \
+		--from="nobody@example.com" \
+		$patches >stdout &&
+	grep "To: cloud@example.com" stdout
+'
+
+test_expect_success $PREREQ 'sendemail.identity: identity overrides sendemail.identity' '
+	git -c sendemail.identity=cloud send-email \
+		--identity=isp \
+		--dry-run \
+		--from="nobody@example.com" \
+		$patches >stdout &&
+	grep "To: isp@example.com" stdout
+'
+
+test_expect_success $PREREQ 'sendemail.identity: --no-identity clears previous identity' '
+	git -c sendemail.identity=cloud send-email \
+		--no-identity \
+		--dry-run \
+		--from="nobody@example.com" \
+		$patches >stdout &&
+	grep "To: default@example.com" stdout
+'
+
+test_expect_success $PREREQ 'sendemail.identity: bool identity variable existance overrides' '
+	git -c sendemail.identity=cloud \
+		-c sendemail.xmailer=true \
+		-c sendemail.cloud.xmailer=false \
+		send-email \
+		--dry-run \
+		--from="nobody@example.com" \
+		$patches >stdout &&
+	grep "To: cloud@example.com" stdout &&
+	! grep "X-Mailer" stdout
+'
+
+test_expect_success $PREREQ 'sendemail.identity: bool variable fallback' '
+	git -c sendemail.identity=cloud \
+		-c sendemail.xmailer=false \
+		send-email \
+		--dry-run \
+		--from="nobody@example.com" \
+		$patches >stdout &&
+	grep "To: cloud@example.com" stdout &&
+	! grep "X-Mailer" stdout
+'
+
 test_expect_success $PREREQ '--no-to overrides sendemail.to' '
 	git send-email \
 		--dry-run \
@@ -1785,6 +1840,15 @@ test_expect_success '--dump-aliases must be used alone' '
 	test_must_fail git send-email --dump-aliases --to=janice@example.com -1 refs/heads/accounting
 '
 
+test_expect_success 'aliases and sendemail.identity' '
+	test_must_fail git \
+		-c sendemail.identity=cloud \
+		-c sendemail.aliasesfile=default-aliases \
+		-c sendemail.cloud.aliasesfile=cloud-aliases \
+		send-email -1 2>stderr &&
+	test_i18ngrep "cloud-aliases" stderr
+'
+
 test_sendmail_aliases () {
 	msg="$1" && shift &&
 	expect="$@" &&
-- 
2.21.0.1020.gf2820cf01a

Re: [PATCH 4/5] send-email: fix regression in sendemail.identity parsing

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Fix a regression in my recent 3494dfd3ee ("send-email: do defaults ->
> config -> getopt in that order", 2019-05-09). I missed that the
> $identity variable needs to be extracted from the command-line before
> we do the config reading, as it determines which config variable we
> should read first. See [1] for the report.
> ...
> Refactor read_config() do what we actually mean here. We don't want to
> set a given sendemail.VAR if a sendemail.$identity.VAR previously set
> it. The old code was conflating this desire with the hardcoded
> defaults for these variables, and as discussed in 3494dfd3ee that was
> never going to work.

I am not sure if the "never going to work" claim is a correct one.
The "no hardcoded default in the variable, read command line, fill
missing ones from the two config files and finally apply the default
for the ones that are still missing" was cumbersome, error-prone
without a table, but did work.

It seems that no matter how we cut it, the cumbersomeness has to
exist, as long as the command line --identity needs to be taken care
of.  Without that complication, I really liked the base series---the
"set var to hardcoded default, overwrite with config and then
overwrite with command line, without having to check if we already
got value in an earlier step" was so much simpler and easy to
explain X-<.

> @@ -371,17 +380,30 @@ sub read_config {
> ...
>  my $help;
>  my $git_completion_helper;
> -my $rc = GetOptions("h" => \$help,
> -                    "dump-aliases" => \$dump_aliases);
> ...
> @@ -410,8 +432,6 @@ sub read_config {
> ...
>  		    "smtp-auth=s" => \$smtp_auth,
> -		    "no-smtp-auth" => sub {$smtp_auth = 'none'},
> -		    "identity=s" => \$identity,

You seem to be building on top of ab/send-email-transferencoding-fix
and something else, and these two hunks did not apply.  I think I
managed to wiggle the patch in correctly, though.

Thanks.

Re: [PATCH 4/5] send-email: fix regression in sendemail.identity parsing

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3225 bytes --]

Hi Ævar,

On Fri, 17 May 2019, Ævar Arnfjörð Bjarmason wrote:

> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 61d484d1a6..890e2874c3 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -1228,6 +1228,61 @@ test_expect_success $PREREQ 'sendemail.to works' '
>  	grep "To: Somebody <somebody@ex.com>" stdout
>  '
>
> +test_expect_success $PREREQ 'setup sendemail.identity' '
> +	git config --replace-all sendemail.to "default@example.com" &&
> +	git config --replace-all sendemail.isp.to "isp@example.com" &&
> +	git config --replace-all sendemail.cloud.to "cloud@example.com"
> +'
> +
> +test_expect_success $PREREQ 'sendemail.identity: reads the correct identity config' '
> +	git -c sendemail.identity=cloud send-email \
> +		--dry-run \
> +		--from="nobody@example.com" \
> +		$patches >stdout &&
> +	grep "To: cloud@example.com" stdout
> +'
> +
> +test_expect_success $PREREQ 'sendemail.identity: identity overrides sendemail.identity' '
> +	git -c sendemail.identity=cloud send-email \
> +		--identity=isp \
> +		--dry-run \
> +		--from="nobody@example.com" \
> +		$patches >stdout &&
> +	grep "To: isp@example.com" stdout
> +'
> +
> +test_expect_success $PREREQ 'sendemail.identity: --no-identity clears previous identity' '
> +	git -c sendemail.identity=cloud send-email \
> +		--no-identity \
> +		--dry-run \
> +		--from="nobody@example.com" \
> +		$patches >stdout &&
> +	grep "To: default@example.com" stdout
> +'
> +
> +test_expect_success $PREREQ 'sendemail.identity: bool identity variable existance overrides' '
> +	git -c sendemail.identity=cloud \
> +		-c sendemail.xmailer=true \
> +		-c sendemail.cloud.xmailer=false \
> +		send-email \
> +		--dry-run \
> +		--from="nobody@example.com" \
> +		$patches >stdout &&
> +	grep "To: cloud@example.com" stdout &&
> +	! grep "X-Mailer" stdout
> +'
> +
> +test_expect_success $PREREQ 'sendemail.identity: bool variable fallback' '
> +	git -c sendemail.identity=cloud \
> +		-c sendemail.xmailer=false \
> +		send-email \
> +		--dry-run \
> +		--from="nobody@example.com" \
> +		$patches >stdout &&
> +	grep "To: cloud@example.com" stdout &&
> +	! grep "X-Mailer" stdout
> +'
> +

These test cases all diligently use the `$PREREQ` prerequisite, but...

>  test_expect_success $PREREQ '--no-to overrides sendemail.to' '
>  	git send-email \
>  		--dry-run \
> @@ -1785,6 +1840,15 @@ test_expect_success '--dump-aliases must be used alone' '
>  	test_must_fail git send-email --dump-aliases --to=janice@example.com -1 refs/heads/accounting
>  '
>
> +test_expect_success 'aliases and sendemail.identity' '
> +	test_must_fail git \
> +		-c sendemail.identity=cloud \
> +		-c sendemail.aliasesfile=default-aliases \
> +		-c sendemail.cloud.aliasesfile=cloud-aliases \
> +		send-email -1 2>stderr &&
> +	test_i18ngrep "cloud-aliases" stderr
> +'
> +

This one is missing it. That breaks the Windows job in our Azure Pipeline
where we leave out all of the Perl bits (to accelerate the tests somewhat).

Ciao,
Dscho

>  test_sendmail_aliases () {
>  	msg="$1" && shift &&
>  	expect="$@" &&
> --
> 2.21.0.1020.gf2820cf01a
>
>
>

Re: [PATCH 4/5] send-email: fix regression in sendemail.identity parsing

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2572 bytes --]

Hi Junio & Ævar,

On Wed, 22 May 2019, Johannes Schindelin wrote:

> On Fri, 17 May 2019, Ævar Arnfjörð Bjarmason wrote:
>
> > [...]
> > +test_expect_success $PREREQ 'sendemail.identity: bool variable fallback' '
> > +	git -c sendemail.identity=cloud \
> > +		-c sendemail.xmailer=false \
> > +		send-email \
> > +		--dry-run \
> > +		--from="nobody@example.com" \
> > +		$patches >stdout &&
> > +	grep "To: cloud@example.com" stdout &&
> > +	! grep "X-Mailer" stdout
> > +'
> > +
>
> These test cases all diligently use the `$PREREQ` prerequisite, but...
>
> >  test_expect_success $PREREQ '--no-to overrides sendemail.to' '
> >  	git send-email \
> >  		--dry-run \
> > @@ -1785,6 +1840,15 @@ test_expect_success '--dump-aliases must be used alone' '
> >  	test_must_fail git send-email --dump-aliases --to=janice@example.com -1 refs/heads/accounting
> >  '
> >
> > +test_expect_success 'aliases and sendemail.identity' '
> > +	test_must_fail git \
> > +		-c sendemail.identity=cloud \
> > +		-c sendemail.aliasesfile=default-aliases \
> > +		-c sendemail.cloud.aliasesfile=cloud-aliases \
> > +		send-email -1 2>stderr &&
> > +	test_i18ngrep "cloud-aliases" stderr
> > +'
> > +
>
> This one is missing it. That breaks the Windows job in our Azure Pipeline
> where we leave out all of the Perl bits (to accelerate the tests somewhat).

For the record, this is still breaking our Azure Pipeline build.

Junio, would you terribly mind applying this on top (or fetching it from
the `shears/pu` branch at https://github.com/git-for-windows/git)?

-- snipsnap --
From 4c946f956d894676f68f5b130ab414d8ea75e97d Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Wed, 29 May 2019 11:09:23 +0200
Subject: [PATCH] SQUASH???

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t9001-send-email.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 7caab8160fcf..7c5ef114ac90 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1840,7 +1840,7 @@ test_expect_success '--dump-aliases must be used alone' '
 	test_must_fail git send-email --dump-aliases --to=janice@example.com -1 refs/heads/accounting
 '

-test_expect_success 'aliases and sendemail.identity' '
+test_expect_success $PREREQ 'aliases and sendemail.identity' '
 	test_must_fail git \
 		-c sendemail.identity=cloud \
 		-c sendemail.aliasesfile=default-aliases \
--
2.22.0.rc1.windows.1.19.g571a93d65ff3

[PATCH 5/5] send-email: remove support for deprecated sendemail.smtpssl

[Ævar Arnfjörð Bjarmason]

Remove the sendemail.smtpssl configuration variable and its associated
--smtp-ssl command-line option. This has been documented as deprecated
since f6bebd121a ("git-send-email: add support for TLS via
Net::SMTP::SSL", 2008-06-25) and 65180c6618 ("List send-email config
options in config.txt.", 2009-07-22), respectively. Waiting for 10
years should be enough.

This allows us to fix a special case in read_config(). We couldn't
just parse sendemail.smtpEncryption like everything else because we'd
need to fall back on sendemail.smtpssl. Now that we don't need to do
that we don't need this special case anymore. Let's still find out if
someone's using this and die() with a helpful message if that's the
case.

Because of my recent improvements to the command-line and config
parsing we can also revert fa835cd572 ("git-send-email: prevent
undefined variable warnings if no encryption is set", 2008-06-26),
since we now sensibly support setting defaults for these mixed config
& command-line options.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/sendemail.txt |  3 ---
 Documentation/git-send-email.txt   |  3 ---
 git-send-email.perl                | 26 +++++++++-----------------
 t/t9001-send-email.sh              |  9 +++++++++
 4 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt
index 0006faf800..d7855bff1f 100644
--- a/Documentation/config/sendemail.txt
+++ b/Documentation/config/sendemail.txt
@@ -8,9 +8,6 @@ sendemail.smtpEncryption::
 	See linkgit:git-send-email[1] for description.  Note that this
 	setting is not subject to the 'identity' mechanism.
 
-sendemail.smtpssl (deprecated)::
-	Deprecated alias for 'sendemail.smtpEncryption = ssl'.
-
 sendemail.smtpsslcertpath::
 	Path to ca-certificates (either a directory or a single file).
 	Set it to an empty string to disable certificate verification.
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index a861934c69..6cf5c32ce8 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -236,9 +236,6 @@ The --smtp-server-option option must be repeated for each option you want
 to pass to the server. Likewise, different lines in the configuration files
 must be used for each option.
 
---smtp-ssl::
-	Legacy alias for '--smtp-encryption ssl'.
-
 --smtp-ssl-cert-path::
 	Path to a store of trusted CA certificates for SMTP SSL/TLS
 	certificate validation (either a directory that has been processed
diff --git a/git-send-email.perl b/git-send-email.perl
index 80cbbfd2b8..b1ed45b907 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -77,7 +77,6 @@ sub usage {
     --smtp-user             <str>  * Username for SMTP-AUTH.
     --smtp-pass             <str>  * Password for SMTP-AUTH; not necessary.
     --smtp-encryption       <str>  * tls or ssl; anything else disables.
-    --smtp-ssl                     * Deprecated. Use '--smtp-encryption ssl'.
     --smtp-ssl-cert-path    <str>  * Path to ca-certificates (either directory or file).
                                      Pass an empty string to disable certificate
                                      verification.
@@ -236,7 +235,7 @@ sub do_edit {
 my ($cover_cc, $cover_to);
 my ($to_cmd, $cc_cmd);
 my ($smtp_server, $smtp_server_port, @smtp_server_options);
-my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);
+my ($smtp_authuser, $smtp_ssl_cert_path);
 my ($batch_size, $relogin_delay);
 my ($identity, $aliasfiletype, @alias_files, $smtp_domain, $smtp_auth);
 my ($confirm);
@@ -250,6 +249,9 @@ sub do_edit {
 my $use_xmailer = 1;
 my $validate = 1;
 my $target_xfer_encoding = 'auto';
+my $smtp_encryption = '';
+# Deprecated variables
+my $deprecated_smtp_ssl;
 
 my %config_bool_settings = (
     "thread" => \$thread,
@@ -266,10 +268,12 @@ sub do_edit {
 );
 
 my %config_settings = (
+    "smtpencryption" => \$smtp_encryption,
     "smtpserver" => \$smtp_server,
     "smtpserverport" => \$smtp_server_port,
     "smtpserveroption" => \@smtp_server_options,
     "smtpuser" => \$smtp_authuser,
+    "smtpssl" => \$deprecated_smtp_ssl,
     "smtppass" => \$smtp_authpass,
     "smtpdomain" => \$smtp_domain,
     "smtpauth" => \$smtp_auth,
@@ -366,18 +370,6 @@ sub read_config {
 			$$target = $v;
 		}
 	}
-
-	if (!defined $smtp_encryption) {
-		my $setting = "$prefix.smtpencryption";
-		my $enc = Git::config(@repo, $setting);
-		return unless defined $enc;
-		return if $configured->{$setting}++;
-		if (defined $enc) {
-			$smtp_encryption = $enc;
-		} elsif (Git::config_bool(@repo, "$prefix.smtpssl")) {
-			$smtp_encryption = 'ssl';
-		}
-	}
 }
 
 # sendemail.identity yields to --identity. We must parse this
@@ -426,7 +418,7 @@ sub read_config {
 		    "smtp-server-port=s" => \$smtp_server_port,
 		    "smtp-user=s" => \$smtp_authuser,
 		    "smtp-pass:s" => \$smtp_authpass,
-		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
+		    "smtp-ssl" => sub { $deprecated_smtp_ssl = 1 },
 		    "smtp-encryption=s" => \$smtp_encryption,
 		    "smtp-ssl-cert-path=s" => \$smtp_ssl_cert_path,
 		    "smtp-debug:i" => \$debug_net_smtp,
@@ -484,8 +476,8 @@ sub read_config {
 	"(via command-line or configuration option)\n")
 	if defined $relogin_delay and not defined $batch_size;
 
-# 'default' encryption is none -- this only prevents a warning
-$smtp_encryption = '' unless (defined $smtp_encryption);
+die __("Use of deprecated option --smtp-ssl (or smtp.smtpssl config), use --smtp-encryption=ssl (or sendemail.smtpEncryption=ssl) instead\n")
+	if $deprecated_smtp_ssl;
 
 # Set CC suppressions
 my(%suppress_cc);
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 890e2874c3..b5ccfa8737 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1849,6 +1849,15 @@ test_expect_success 'aliases and sendemail.identity' '
 	test_i18ngrep "cloud-aliases" stderr
 '
 
+test_expect_success 'deprecated --smtp-ssl (or sendemail.smtpssl=true)' '
+	test_must_fail git -c sendemail.smtpssl=true send-email -1 2>stderr &&
+	test_i18ngrep "deprecated option.*or.*config" stderr &&
+	test_must_fail git send-email --smtp-ssl -1 &&
+	test_i18ngrep "deprecated option.*or.*config" stderr &&
+	test_must_fail git -c sendemail.identity=test -c sendemail.test.smtpssl=true send-email -1 &&
+	test_i18ngrep "deprecated option.*or.*config" stderr
+'
+
 test_sendmail_aliases () {
 	msg="$1" && shift &&
 	expect="$@" &&
-- 
2.21.0.1020.gf2820cf01a

[PATCH v2 1/2] send-email: update the mechanism to set default configuration values

[Junio C Hamano]

The program has a good mechanism to specify the fallback default
values for boolean configuration variables after two invocations of
read_config() for "sendmail.$ident.$var" and "sendemail.$var" have
not found any configuration.  Imitate it so that we can set the
default values for non-boolean variables as well.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-send-email.perl | 56 ++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index f4c07908d2..98bc295c6e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -250,28 +250,28 @@ sub do_edit {
 );
 
 my %config_settings = (
-    "smtpserver" => \$smtp_server,
-    "smtpserverport" => \$smtp_server_port,
-    "smtpserveroption" => \@smtp_server_options,
-    "smtpuser" => \$smtp_authuser,
-    "smtppass" => \$smtp_authpass,
-    "smtpdomain" => \$smtp_domain,
-    "smtpauth" => \$smtp_auth,
-    "smtpbatchsize" => \$batch_size,
-    "smtprelogindelay" => \$relogin_delay,
-    "to" => \@initial_to,
-    "tocmd" => \$to_cmd,
-    "cc" => \@initial_cc,
-    "cccmd" => \$cc_cmd,
-    "aliasfiletype" => \$aliasfiletype,
-    "bcc" => \@bcclist,
-    "suppresscc" => \@suppress_cc,
-    "envelopesender" => \$envelope_sender,
-    "confirm"   => \$confirm,
-    "from" => \$sender,
-    "assume8bitencoding" => \$auto_8bit_encoding,
-    "composeencoding" => \$compose_encoding,
-    "transferencoding" => \$target_xfer_encoding,
+    "smtpserver" => [\$smtp_server],
+    "smtpserverport" => [\$smtp_server_port],
+    "smtpserveroption" => [\@smtp_server_options],
+    "smtpuser" => [\$smtp_authuser],
+    "smtppass" => [\$smtp_authpass],
+    "smtpdomain" => [\$smtp_domain],
+    "smtpauth" => [\$smtp_auth],
+    "smtpbatchsize" => [\$batch_size],
+    "smtprelogindelay" => [\$relogin_delay],
+    "to" => [\@initial_to],
+    "tocmd" => [\$to_cmd],
+    "cc" => [\@initial_cc],
+    "cccmd" => [\$cc_cmd],
+    "aliasfiletype" => [\$aliasfiletype],
+    "bcc" => [\@bcclist],
+    "suppresscc" => [\@suppress_cc],
+    "envelopesender" => [\$envelope_sender],
+    "confirm"   => [\$confirm],
+    "from" => [\$sender],
+    "assume8bitencoding" => [\$auto_8bit_encoding],
+    "composeencoding" => [\$compose_encoding],
+    "transferencoding" => [\$target_xfer_encoding],
 );
 
 my %config_path_settings = (
@@ -411,7 +411,7 @@ sub read_config {
 	}
 
 	foreach my $setting (keys %config_settings) {
-		my $target = $config_settings{$setting};
+		my $target = $config_settings{$setting}->[0];
 		next if $setting eq "to" and defined $no_to;
 		next if $setting eq "cc" and defined $no_cc;
 		next if $setting eq "bcc" and defined $no_bcc;
@@ -446,6 +446,16 @@ sub read_config {
 	${$setting->[0]} = $setting->[1] unless (defined (${$setting->[0]}));
 }
 
+# fall back to builtin defaults
+while (my ($name, $setting) = each %config_settings) {
+	next unless @$setting == 2;
+
+	my ($target, $default) = @$setting;
+	if (ref($target) eq "SCALAR") {
+		$$target = $default unless defined $$target;
+	} # elsif ... for other types later.
+}
+
 # 'default' encryption is none -- this only prevents a warning
 $smtp_encryption = '' unless (defined $smtp_encryption);
 
-- 
2.21.0-777-g83232e3864

[PATCH v2 2/2] send-email: honor transferencoding config option again

[Junio C Hamano]

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 calling read_config() twice to parsing the
configuration files for "sendemail.transferencoding" and
"sendemail.$ident.transferencoding".

It was made trivial to do so by the previous change.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-send-email.perl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 98bc295c6e..b9dd775b63 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()
 
@@ -271,7 +271,7 @@ sub do_edit {
     "from" => [\$sender],
     "assume8bitencoding" => [\$auto_8bit_encoding],
     "composeencoding" => [\$compose_encoding],
-    "transferencoding" => [\$target_xfer_encoding],
+    "transferencoding" => [\$target_xfer_encoding, 'auto'],
 );
 
 my %config_path_settings = (
-- 
2.21.0-777-g83232e3864

Re: [PATCH v2 2/2] send-email: honor transferencoding config option again

[Eric Sunshine]

On Wed, May 8, 2019 at 6:56 AM Junio C Hamano <gitster@pobox.com> wrote:
> Since e67a228cd8a ("send-email: automatically determine
> transfer-encoding"), the value of sendmail.transferencoding in the
> configuration file is ignored, because $target_xfer_encoding is

s/,//

> already defined read_config sub parses the configuration file.

s/defined/& when/ --or -- s/defined/& by the time/

> Instead of initializing variable $target_xfer_encoding to 'auto' on
> definition, we have to set it to the default value of 'auto' if is

s/if is/if it is/

> undefined after calling read_config() twice to parsing the

s/parsing/parse/

> configuration files for "sendemail.transferencoding" and

s/files/file/ (I think)

> "sendemail.$ident.transferencoding".
>
> It was made trivial to do so by the previous change.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH v2 2/2] send-email: honor transferencoding config option again

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, May 8, 2019 at 6:56 AM Junio C Hamano <gitster@pobox.com> wrote:
>> Since e67a228cd8a ("send-email: automatically determine
>> transfer-encoding"), the value of sendmail.transferencoding in the
>> configuration file is ignored, because $target_xfer_encoding is
>
> s/,//

(also all other typofixes are obviously correct)

Thanks.

Re: [PATCH v2 2/2] send-email: honor transferencoding config option again

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

On Wed, May 08, 2019 at 07:56:07PM +0900, Junio C Hamano wrote:
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 98bc295c6e..b9dd775b63 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()
>  
> @@ -271,7 +271,7 @@ sub do_edit {
>      "from" => [\$sender],
>      "assume8bitencoding" => [\$auto_8bit_encoding],
>      "composeencoding" => [\$compose_encoding],
> -    "transferencoding" => [\$target_xfer_encoding],
> +    "transferencoding" => [\$target_xfer_encoding, 'auto'],
>  );

These changes seem sane and the series makes the second one
straightforward. It's also a nice way to add defaults in the future, so
thanks for that.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]

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?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Heinrich Schuchardt]

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