git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH 1/1] send-email: fix transferencoding config option
@ 2019-04-09 19:27 Heinrich Schuchardt
  2019-04-09 21:58 ` Jonathan Nieder
  2019-04-09 22:55 ` [PATCH 1/1] send-email: fix transferencoding config option brian m. carlson
  0 siblings, 2 replies; 39+ messages in thread
From: Heinrich Schuchardt @ 2019-04-09 19:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Brian M Carlson, git, 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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/1] send-email: fix transferencoding config option
  2019-04-09 19:27 [PATCH 1/1] send-email: fix transferencoding config option Heinrich Schuchardt
@ 2019-04-09 21:58 ` Jonathan Nieder
  2019-04-09 23:39   ` Heinrich Schuchardt
  2019-04-10  3:48   ` Junio C Hamano
  2019-04-09 22:55 ` [PATCH 1/1] send-email: fix transferencoding config option brian m. carlson
  1 sibling, 2 replies; 39+ messages in thread
From: Jonathan Nieder @ 2019-04-09 21:58 UTC (permalink / raw)
  To: Heinrich Schuchardt; +Cc: Junio C Hamano, Brian M Carlson, git

(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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/1] send-email: fix transferencoding config option
  2019-04-09 19:27 [PATCH 1/1] send-email: fix transferencoding config option Heinrich Schuchardt
  2019-04-09 21:58 ` Jonathan Nieder
@ 2019-04-09 22:55 ` brian m. carlson
  2019-04-09 23:06   ` Heinrich Schuchardt
  1 sibling, 1 reply; 39+ messages in thread
From: brian m. carlson @ 2019-04-09 22:55 UTC (permalink / raw)
  To: Heinrich Schuchardt; +Cc: Junio C Hamano, git

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/1] send-email: fix transferencoding config option
  2019-04-09 22:55 ` [PATCH 1/1] send-email: fix transferencoding config option brian m. carlson
@ 2019-04-09 23:06   ` Heinrich Schuchardt
  0 siblings, 0 replies; 39+ messages in thread
From: Heinrich Schuchardt @ 2019-04-09 23:06 UTC (permalink / raw)
  To: brian m. carlson, Junio C Hamano, git

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/1] send-email: fix transferencoding config option
  2019-04-09 21:58 ` Jonathan Nieder
@ 2019-04-09 23:39   ` Heinrich Schuchardt
  2019-04-10  3:48   ` Junio C Hamano
  1 sibling, 0 replies; 39+ messages in thread
From: Heinrich Schuchardt @ 2019-04-09 23:39 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Junio C Hamano, Brian M Carlson, git

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/1] send-email: fix transferencoding config option
  2019-04-09 21:58 ` Jonathan Nieder
  2019-04-09 23:39   ` Heinrich Schuchardt
@ 2019-04-10  3:48   ` Junio C Hamano
  2019-04-10 20:40     ` Heinrich Schuchardt
  2019-05-08  8:18     ` Re* " Junio C Hamano
  1 sibling, 2 replies; 39+ messages in thread
From: Junio C Hamano @ 2019-04-10  3:48 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Heinrich Schuchardt, Brian M Carlson, git

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




^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/1] send-email: fix transferencoding config option
  2019-04-10  3:48   ` Junio C Hamano
@ 2019-04-10 20:40     ` Heinrich Schuchardt
  2019-04-10 22:42       ` brian m. carlson
  2019-05-08  8:18     ` Re* " Junio C Hamano
  1 sibling, 1 reply; 39+ messages in thread
From: Heinrich Schuchardt @ 2019-04-10 20:40 UTC (permalink / raw)
  To: Junio C Hamano, Jonathan Nieder; +Cc: Brian M Carlson, git

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 1/1] send-email: fix transferencoding config option
  2019-04-10 20:40     ` Heinrich Schuchardt
@ 2019-04-10 22:42       ` brian m. carlson
  0 siblings, 0 replies; 39+ messages in thread
From: brian m. carlson @ 2019-04-10 22:42 UTC (permalink / raw)
  To: Heinrich Schuchardt; +Cc: Junio C Hamano, Jonathan Nieder, git

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re* [PATCH 1/1] send-email: fix transferencoding config option
  2019-04-10  3:48   ` Junio C Hamano
  2019-04-10 20:40     ` Heinrich Schuchardt
@ 2019-05-08  8:18     ` Junio C Hamano
  2019-05-08  8:20       ` [PATCH 2/2] send-email: honor transferencoding config option again Junio C Hamano
  2019-05-08 10:13       ` Re* [PATCH 1/1] send-email: fix transferencoding config option Junio C Hamano
  1 sibling, 2 replies; 39+ messages in thread
From: Junio C Hamano @ 2019-05-08  8:18 UTC (permalink / raw)
  To: Heinrich Schuchardt; +Cc: Brian M Carlson, git, Jonathan Nieder

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 2/2] send-email: honor transferencoding config option again
  2019-05-08  8:18     ` Re* " Junio C Hamano
@ 2019-05-08  8:20       ` Junio C Hamano
  2019-05-08 10:13       ` Re* [PATCH 1/1] send-email: fix transferencoding config option Junio C Hamano
  1 sibling, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2019-05-08  8:20 UTC (permalink / raw)
  To: Heinrich Schuchardt; +Cc: Brian M Carlson, git, Jonathan Nieder

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: Re* [PATCH 1/1] send-email: fix transferencoding config option
  2019-05-08  8:18     ` Re* " Junio C Hamano
  2019-05-08  8:20       ` [PATCH 2/2] send-email: honor transferencoding config option again Junio C Hamano
@ 2019-05-08 10:13       ` Junio C Hamano
  2019-05-08 10:56         ` [PATCH v2 0/2] send-email: set xfer encoding correctly Junio C Hamano
                           ` (2 more replies)
  1 sibling, 3 replies; 39+ messages in thread
From: Junio C Hamano @ 2019-05-08 10:13 UTC (permalink / raw)
  To: Heinrich Schuchardt; +Cc: Brian M Carlson, git, Jonathan Nieder

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 0/2] send-email: set xfer encoding correctly
  2019-05-08 10:13       ` Re* [PATCH 1/1] send-email: fix transferencoding config option Junio C Hamano
@ 2019-05-08 10:56         ` Junio C Hamano
  2019-05-09 11:48           ` [PATCH v3 0/3] send-email: fix cli->config parsing crazyness Ævar Arnfjörð Bjarmason
                             ` (3 more replies)
  2019-05-08 10:56         ` [PATCH v2 1/2] send-email: update the mechanism to set default configuration values Junio C Hamano
  2019-05-08 10:56         ` [PATCH v2 2/2] send-email: honor transferencoding config option again Junio C Hamano
  2 siblings, 4 replies; 39+ messages in thread
From: Junio C Hamano @ 2019-05-08 10:56 UTC (permalink / raw)
  To: git; +Cc: Heinrich Schuchardt, Brian M Carlson, Jonathan Nieder

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 1/2] send-email: update the mechanism to set default  configuration values
  2019-05-08 10:13       ` Re* [PATCH 1/1] send-email: fix transferencoding config option Junio C Hamano
  2019-05-08 10:56         ` [PATCH v2 0/2] send-email: set xfer encoding correctly Junio C Hamano
@ 2019-05-08 10:56         ` Junio C Hamano
  2019-05-08 10:56         ` [PATCH v2 2/2] send-email: honor transferencoding config option again Junio C Hamano
  2 siblings, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2019-05-08 10:56 UTC (permalink / raw)
  To: git; +Cc: Heinrich Schuchardt, Brian M Carlson, Jonathan Nieder

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 2/2] send-email: honor transferencoding config option again
  2019-05-08 10:13       ` Re* [PATCH 1/1] send-email: fix transferencoding config option Junio C Hamano
  2019-05-08 10:56         ` [PATCH v2 0/2] send-email: set xfer encoding correctly Junio C Hamano
  2019-05-08 10:56         ` [PATCH v2 1/2] send-email: update the mechanism to set default configuration values Junio C Hamano
@ 2019-05-08 10:56         ` Junio C Hamano
  2019-05-08 21:24           ` Eric Sunshine
  2019-05-08 23:12           ` brian m. carlson
  2 siblings, 2 replies; 39+ messages in thread
From: Junio C Hamano @ 2019-05-08 10:56 UTC (permalink / raw)
  To: git; +Cc: Heinrich Schuchardt, Brian M Carlson, Jonathan Nieder

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 2/2] send-email: honor transferencoding config option again
  2019-05-08 10:56         ` [PATCH v2 2/2] send-email: honor transferencoding config option again Junio C Hamano
@ 2019-05-08 21:24           ` Eric Sunshine
  2019-05-09  6:47             ` Junio C Hamano
  2019-05-08 23:12           ` brian m. carlson
  1 sibling, 1 reply; 39+ messages in thread
From: Eric Sunshine @ 2019-05-08 21:24 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git List, Heinrich Schuchardt, Brian M Carlson, Jonathan Nieder

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>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 2/2] send-email: honor transferencoding config option again
  2019-05-08 10:56         ` [PATCH v2 2/2] send-email: honor transferencoding config option again Junio C Hamano
  2019-05-08 21:24           ` Eric Sunshine
@ 2019-05-08 23:12           ` brian m. carlson
  1 sibling, 0 replies; 39+ messages in thread
From: brian m. carlson @ 2019-05-08 23:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Heinrich Schuchardt, Jonathan Nieder

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 2/2] send-email: honor transferencoding config option again
  2019-05-08 21:24           ` Eric Sunshine
@ 2019-05-09  6:47             ` Junio C Hamano
  0 siblings, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2019-05-09  6:47 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Git List, Heinrich Schuchardt, Brian M Carlson, Jonathan Nieder

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.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 0/3] send-email: fix cli->config parsing crazyness
  2019-05-08 10:56         ` [PATCH v2 0/2] send-email: set xfer encoding correctly Junio C Hamano
@ 2019-05-09 11:48           ` Ævar Arnfjörð Bjarmason
  2019-05-10 13:50             ` Junio C Hamano
  2019-05-09 11:48           ` [PATCH v3 1/3] send-email: move the read_config() function above getopts Ævar Arnfjörð Bjarmason
                             ` (2 subsequent siblings)
  3 siblings, 1 reply; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-09 11:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Heinrich Schuchardt, Brian M Carlson,
	Jonathan Nieder, Eric Sunshine,
	Æ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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 1/3] send-email: move the read_config() function above getopts
  2019-05-08 10:56         ` [PATCH v2 0/2] send-email: set xfer encoding correctly Junio C Hamano
  2019-05-09 11:48           ` [PATCH v3 0/3] send-email: fix cli->config parsing crazyness Ævar Arnfjörð Bjarmason
@ 2019-05-09 11:48           ` Ævar Arnfjörð Bjarmason
  2019-05-09 11:48           ` [PATCH v3 2/3] send-email: rename the @bcclist variable for consistency Ævar Arnfjörð Bjarmason
  2019-05-09 11:48           ` [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order Ævar Arnfjörð Bjarmason
  3 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-09 11:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Heinrich Schuchardt, Brian M Carlson,
	Jonathan Nieder, Eric Sunshine,
	Æ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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 2/3] send-email: rename the @bcclist variable for consistency
  2019-05-08 10:56         ` [PATCH v2 0/2] send-email: set xfer encoding correctly Junio C Hamano
  2019-05-09 11:48           ` [PATCH v3 0/3] send-email: fix cli->config parsing crazyness Ævar Arnfjörð Bjarmason
  2019-05-09 11:48           ` [PATCH v3 1/3] send-email: move the read_config() function above getopts Ævar Arnfjörð Bjarmason
@ 2019-05-09 11:48           ` Ævar Arnfjörð Bjarmason
  2019-05-09 11:48           ` [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order Ævar Arnfjörð Bjarmason
  3 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-09 11:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Heinrich Schuchardt, Brian M Carlson,
	Jonathan Nieder, Eric Sunshine,
	Æ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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order
  2019-05-08 10:56         ` [PATCH v2 0/2] send-email: set xfer encoding correctly Junio C Hamano
                             ` (2 preceding siblings ...)
  2019-05-09 11:48           ` [PATCH v3 2/3] send-email: rename the @bcclist variable for consistency Ævar Arnfjörð Bjarmason
@ 2019-05-09 11:48           ` Ævar Arnfjörð Bjarmason
  2019-05-09 18:04             ` Eric Sunshine
                               ` (3 more replies)
  3 siblings, 4 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-09 11:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Heinrich Schuchardt, Brian M Carlson,
	Jonathan Nieder, Eric Sunshine,
	Æ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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order
  2019-05-09 11:48           ` [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order Ævar Arnfjörð Bjarmason
@ 2019-05-09 18:04             ` Eric Sunshine
  2019-05-13  8:46               ` Junio C Hamano
  2019-05-09 23:51             ` brian m. carlson
                               ` (2 subsequent siblings)
  3 siblings, 1 reply; 39+ messages in thread
From: Eric Sunshine @ 2019-05-09 18:04 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Git List, Junio C Hamano, Heinrich Schuchardt, Brian M Carlson,
	Jonathan Nieder

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>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order
  2019-05-09 11:48           ` [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order Ævar Arnfjörð Bjarmason
  2019-05-09 18:04             ` Eric Sunshine
@ 2019-05-09 23:51             ` brian m. carlson
  2019-05-13  8:50             ` Junio C Hamano
  2019-05-16 22:59             ` Stephen Boyd
  3 siblings, 0 replies; 39+ messages in thread
From: brian m. carlson @ 2019-05-09 23:51 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Heinrich Schuchardt, Jonathan Nieder, Eric Sunshine

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/3] send-email: fix cli->config parsing crazyness
  2019-05-09 11:48           ` [PATCH v3 0/3] send-email: fix cli->config parsing crazyness Ævar Arnfjörð Bjarmason
@ 2019-05-10 13:50             ` Junio C Hamano
  0 siblings, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2019-05-10 13:50 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Heinrich Schuchardt, Brian M Carlson, Jonathan Nieder,
	Eric Sunshine

Æ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 ;-)


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order
  2019-05-09 18:04             ` Eric Sunshine
@ 2019-05-13  8:46               ` Junio C Hamano
  0 siblings, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2019-05-13  8:46 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Ævar Arnfjörð Bjarmason, Git List,
	Heinrich Schuchardt, Brian M Carlson, Jonathan Nieder

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".

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order
  2019-05-09 11:48           ` [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order Ævar Arnfjörð Bjarmason
  2019-05-09 18:04             ` Eric Sunshine
  2019-05-09 23:51             ` brian m. carlson
@ 2019-05-13  8:50             ` Junio C Hamano
  2019-05-13 21:13               ` Ævar Arnfjörð Bjarmason
  2019-05-16 22:59             ` Stephen Boyd
  3 siblings, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2019-05-13  8:50 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Heinrich Schuchardt, Brian M Carlson, Jonathan Nieder,
	Eric Sunshine

Æ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,

,,???

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order
  2019-05-13  8:50             ` Junio C Hamano
@ 2019-05-13 21:13               ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-13 21:13 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Heinrich Schuchardt, Brian M Carlson, Jonathan Nieder,
	Eric Sunshine


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.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order
  2019-05-09 11:48           ` [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order Ævar Arnfjörð Bjarmason
                               ` (2 preceding siblings ...)
  2019-05-13  8:50             ` Junio C Hamano
@ 2019-05-16 22:59             ` Stephen Boyd
  2019-05-16 23:13               ` Junio C Hamano
  3 siblings, 1 reply; 39+ messages in thread
From: Stephen Boyd @ 2019-05-16 22:59 UTC (permalink / raw)
  To: avarab; +Cc: git, gitster, jrnieder, sandals, sunshine, xypron.glpk

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order
  2019-05-16 22:59             ` Stephen Boyd
@ 2019-05-16 23:13               ` Junio C Hamano
  2019-05-17  3:43                 ` Junio C Hamano
  0 siblings, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2019-05-16 23:13 UTC (permalink / raw)
  To: Stephen Boyd; +Cc: avarab, git, jrnieder, sandals, sunshine, xypron.glpk

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.


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 3/3] send-email: do defaults -> config -> getopt in that order
  2019-05-16 23:13               ` Junio C Hamano
@ 2019-05-17  3:43                 ` Junio C Hamano
  2019-05-17 19:55                   ` [PATCH 0/5] ab/send-email-transferencoding-fix-for-the-fix Ævar Arnfjörð Bjarmason
                                     ` (5 more replies)
  0 siblings, 6 replies; 39+ messages in thread
From: Junio C Hamano @ 2019-05-17  3:43 UTC (permalink / raw)
  To: Stephen Boyd; +Cc: avarab, git, jrnieder, sandals, sunshine, xypron.glpk

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.



^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 0/5] ab/send-email-transferencoding-fix-for-the-fix
  2019-05-17  3:43                 ` Junio C Hamano
@ 2019-05-17 19:55                   ` Ævar Arnfjörð Bjarmason
  2019-05-17 19:55                   ` [PATCH 1/5] send-email: remove cargo-culted multi-patch pattern in tests Ævar Arnfjörð Bjarmason
                                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-17 19:55 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Stephen Boyd, jrnieder, sandals,
	sunshine, xypron.glpk, Paolo Bonzini,
	Æ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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 1/5] send-email: remove cargo-culted multi-patch pattern in tests
  2019-05-17  3:43                 ` Junio C Hamano
  2019-05-17 19:55                   ` [PATCH 0/5] ab/send-email-transferencoding-fix-for-the-fix Ævar Arnfjörð Bjarmason
@ 2019-05-17 19:55                   ` Ævar Arnfjörð Bjarmason
  2019-05-17 19:55                   ` [PATCH 2/5] send-email: fix broken transferEncoding tests Ævar Arnfjörð Bjarmason
                                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-17 19:55 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Stephen Boyd, jrnieder, sandals,
	sunshine, xypron.glpk, Paolo Bonzini,
	Æ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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 2/5] send-email: fix broken transferEncoding tests
  2019-05-17  3:43                 ` Junio C Hamano
  2019-05-17 19:55                   ` [PATCH 0/5] ab/send-email-transferencoding-fix-for-the-fix Ævar Arnfjörð Bjarmason
  2019-05-17 19:55                   ` [PATCH 1/5] send-email: remove cargo-culted multi-patch pattern in tests Ævar Arnfjörð Bjarmason
@ 2019-05-17 19:55                   ` Ævar Arnfjörð Bjarmason
  2019-05-17 19:55                   ` [PATCH 3/5] send-email: document --no-[to|cc|bcc] Ævar Arnfjörð Bjarmason
                                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-17 19:55 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Stephen Boyd, jrnieder, sandals,
	sunshine, xypron.glpk, Paolo Bonzini,
	Æ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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 3/5] send-email: document --no-[to|cc|bcc]
  2019-05-17  3:43                 ` Junio C Hamano
                                     ` (2 preceding siblings ...)
  2019-05-17 19:55                   ` [PATCH 2/5] send-email: fix broken transferEncoding tests Ævar Arnfjörð Bjarmason
@ 2019-05-17 19:55                   ` Ævar Arnfjörð Bjarmason
  2019-05-17 19:55                   ` [PATCH 4/5] send-email: fix regression in sendemail.identity parsing Ævar Arnfjörð Bjarmason
  2019-05-17 19:55                   ` [PATCH 5/5] send-email: remove support for deprecated sendemail.smtpssl Ævar Arnfjörð Bjarmason
  5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-17 19:55 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Stephen Boyd, jrnieder, sandals,
	sunshine, xypron.glpk, Paolo Bonzini,
	Æ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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 4/5] send-email: fix regression in sendemail.identity parsing
  2019-05-17  3:43                 ` Junio C Hamano
                                     ` (3 preceding siblings ...)
  2019-05-17 19:55                   ` [PATCH 3/5] send-email: document --no-[to|cc|bcc] Ævar Arnfjörð Bjarmason
@ 2019-05-17 19:55                   ` Ævar Arnfjörð Bjarmason
  2019-05-19  1:29                     ` Junio C Hamano
  2019-05-22 20:25                     ` Johannes Schindelin
  2019-05-17 19:55                   ` [PATCH 5/5] send-email: remove support for deprecated sendemail.smtpssl Ævar Arnfjörð Bjarmason
  5 siblings, 2 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-17 19:55 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Stephen Boyd, jrnieder, sandals,
	sunshine, xypron.glpk, Paolo Bonzini,
	Æ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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 5/5] send-email: remove support for deprecated sendemail.smtpssl
  2019-05-17  3:43                 ` Junio C Hamano
                                     ` (4 preceding siblings ...)
  2019-05-17 19:55                   ` [PATCH 4/5] send-email: fix regression in sendemail.identity parsing Ævar Arnfjörð Bjarmason
@ 2019-05-17 19:55                   ` Ævar Arnfjörð Bjarmason
  5 siblings, 0 replies; 39+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-17 19:55 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Stephen Boyd, jrnieder, sandals,
	sunshine, xypron.glpk, Paolo Bonzini,
	Æ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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 4/5] send-email: fix regression in sendemail.identity parsing
  2019-05-17 19:55                   ` [PATCH 4/5] send-email: fix regression in sendemail.identity parsing Ævar Arnfjörð Bjarmason
@ 2019-05-19  1:29                     ` Junio C Hamano
  2019-05-22 20:25                     ` Johannes Schindelin
  1 sibling, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2019-05-19  1:29 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Stephen Boyd, jrnieder, sandals, sunshine,
	xypron.glpk, Paolo Bonzini

Æ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.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 4/5] send-email: fix regression in sendemail.identity parsing
  2019-05-17 19:55                   ` [PATCH 4/5] send-email: fix regression in sendemail.identity parsing Ævar Arnfjörð Bjarmason
  2019-05-19  1:29                     ` Junio C Hamano
@ 2019-05-22 20:25                     ` Johannes Schindelin
  2019-05-29  9:10                       ` Johannes Schindelin
  1 sibling, 1 reply; 39+ messages in thread
From: Johannes Schindelin @ 2019-05-22 20:25 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Jeff King, Stephen Boyd, jrnieder, sandals,
	sunshine, xypron.glpk, Paolo Bonzini

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 4/5] send-email: fix regression in sendemail.identity parsing
  2019-05-22 20:25                     ` Johannes Schindelin
@ 2019-05-29  9:10                       ` Johannes Schindelin
  0 siblings, 0 replies; 39+ messages in thread
From: Johannes Schindelin @ 2019-05-29  9:10 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Jeff King, Stephen Boyd, jrnieder, sandals,
	sunshine, xypron.glpk, Paolo Bonzini

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2019-05-29  9:11 UTC | newest]

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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git