git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH v2] git-send-email: die if sendmail.* config is set
@ 2020-07-18 20:21 Drew DeVault
  2020-07-18 20:38 ` Junio C Hamano
  2020-07-20 17:33 ` Jeff King
  0 siblings, 2 replies; 8+ messages in thread
From: Drew DeVault @ 2020-07-18 20:21 UTC (permalink / raw)
  To: Junio C Hamano, git; +Cc: Drew DeVault

I've seen several people mis-configure git send-email on their first
attempt because they set the sendmail.* config options - not
sendemail.*. This patch detects this mistake and bails out with a
friendly warning.

Signed-off-by: Drew DeVault <sir@cmpwn.com>
---
 Documentation/config/sendemail.txt |  5 +++++
 git-send-email.perl                |  8 ++++++++
 perl/Git.pm                        | 26 ++++++++++++++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt
index 0006faf800..1726d5f85e 100644
--- a/Documentation/config/sendemail.txt
+++ b/Documentation/config/sendemail.txt
@@ -61,3 +61,8 @@ sendemail.smtpBatchSize::
 sendemail.smtpReloginDelay::
 	Seconds wait before reconnecting to smtp server.
 	See also the `--relogin-delay` option of linkgit:git-send-email[1].
+
+sendemail.forceSendmailVariables::
+	To avoid common misconfiguration mistakes, linkgit:git-send-email[1]
+	will abort with a warning if any configuration options for "sendmail"
+	exist. Set this variable to bypass the check.
diff --git a/git-send-email.perl b/git-send-email.perl
index 36c47bae1d..1b186bc058 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -250,6 +250,7 @@ sub do_edit {
 my $use_xmailer = 1;
 my $validate = 1;
 my $target_xfer_encoding = 'auto';
+my $forbid_sendmail_variables = 1;
 
 my %config_bool_settings = (
     "thread" => \$thread,
@@ -263,6 +264,7 @@ sub do_edit {
     "multiedit" => \$multiedit,
     "annotate" => \$annotate,
     "xmailer" => \$use_xmailer,
+    "forbidsendmailvariables" => \$forbid_sendmail_variables,
 );
 
 my %config_settings = (
@@ -478,6 +480,12 @@ sub read_config {
     usage();
 }
 
+if ($forbid_sendmail_variables && (scalar Git::config_regexp("sendmail.*")) != 0) {
+	die __("fatal: found configuration options for 'sendmail'\n" .
+		"git-send-email is configured with the sendemail.* options - note the 'e'.\n" .
+		"Set sendemail.forbidSendmailVariables to false to disable this check.\n");
+}
+
 die __("Cannot run git format-patch from outside a repository\n")
 	if $format_patch and not $repo;
 
diff --git a/perl/Git.pm b/perl/Git.pm
index 54c9ed0dde..10df990959 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -723,6 +723,32 @@ sub config_int {
 	return scalar _config_common({'kind' => '--int'}, @_);
 }
 
+=item config_regexp ( RE )
+
+Retrieve the list of configuration key names matching the regular
+expression C<RE>. The return value is a list of strings matching
+this regex.
+
+=cut
+
+sub config_regexp {
+	my ($self, $regex) = _maybe_self(@_);
+	try {
+		my @cmd = ('config', '--name-only', '--get-regexp', $regex);
+		unshift @cmd, $self if $self;
+		my @matches = command(@cmd);
+		return @matches;
+	} catch Git::Error::Command with {
+		my $E = shift;
+		if ($E->value() == 1) {
+			my @matches = ();
+			return @matches;
+		} else {
+			throw $E;
+		}
+	};
+}
+
 # Common subroutine to implement bulk of what the config* family of methods
 # do. This currently wraps command('config') so it is not so fast.
 sub _config_common {
-- 
2.27.0


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

* Re: [PATCH v2] git-send-email: die if sendmail.* config is set
  2020-07-18 20:21 [PATCH v2] git-send-email: die if sendmail.* config is set Drew DeVault
@ 2020-07-18 20:38 ` Junio C Hamano
  2020-07-19  6:07   ` Junio C Hamano
  2020-07-20 17:33 ` Jeff King
  1 sibling, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2020-07-18 20:38 UTC (permalink / raw)
  To: Drew DeVault; +Cc: git

Drew DeVault <sir@cmpwn.com> writes:

> I've seen several people mis-configure git send-email on their first
> attempt because they set the sendmail.* config options - not
> sendemail.*. This patch detects this mistake and bails out with a
> friendly warning.
>
> Signed-off-by: Drew DeVault <sir@cmpwn.com>
> ---
>  Documentation/config/sendemail.txt |  5 +++++
>  git-send-email.perl                |  8 ++++++++
>  perl/Git.pm                        | 26 ++++++++++++++++++++++++++
>  3 files changed, 39 insertions(+)
>
> diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt
> index 0006faf800..1726d5f85e 100644
> --- a/Documentation/config/sendemail.txt
> +++ b/Documentation/config/sendemail.txt
> @@ -61,3 +61,8 @@ sendemail.smtpBatchSize::
>  sendemail.smtpReloginDelay::
>  	Seconds wait before reconnecting to smtp server.
>  	See also the `--relogin-delay` option of linkgit:git-send-email[1].
> +
> +sendemail.forceSendmailVariables::
> +	To avoid common misconfiguration mistakes, linkgit:git-send-email[1]
> +	will abort with a warning if any configuration options for "sendmail"
> +	exist. Set this variable to bypass the check.

I am not sure if it is clear to readers what kind of forcing this
refers to.  At least it is not clear to me.

> diff --git a/git-send-email.perl b/git-send-email.perl
> index 36c47bae1d..1b186bc058 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -250,6 +250,7 @@ sub do_edit {
>  my $use_xmailer = 1;
>  my $validate = 1;
>  my $target_xfer_encoding = 'auto';
> +my $forbid_sendmail_variables = 1;

Ah, probably s/force/forbid/ in the documentation above?  OK, then
it makes sense.

>  
>  my %config_bool_settings = (
>      "thread" => \$thread,
> @@ -263,6 +264,7 @@ sub do_edit {
>      "multiedit" => \$multiedit,
>      "annotate" => \$annotate,
>      "xmailer" => \$use_xmailer,
> +    "forbidsendmailvariables" => \$forbid_sendmail_variables,
>  );
>  
>  my %config_settings = (
> @@ -478,6 +480,12 @@ sub read_config {
>      usage();
>  }
>  
> +if ($forbid_sendmail_variables && (scalar Git::config_regexp("sendmail.*")) != 0) {

Judging from the way you wrote the "config_regexp" helper function,
the above regexp matches "sendmailer.foo", "sendmailed.bar", etc., I
would think, which probably is not what you intended.  

I guess we can write "sendmail[.].*" or "sendmail\\..*" to ensure
that we are talking about (literally) "sendmail." followed by
anything?

> +	die __("fatal: found configuration options for 'sendmail'\n" .
> +		"git-send-email is configured with the sendemail.* options - note the 'e'.\n" .
> +		"Set sendemail.forbidSendmailVariables to false to disable this check.\n");
> +}

OK.

Other than the two minor nits, looking quite good.

Thanks.

>  die __("Cannot run git format-patch from outside a repository\n")
>  	if $format_patch and not $repo;
>  
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 54c9ed0dde..10df990959 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -723,6 +723,32 @@ sub config_int {
>  	return scalar _config_common({'kind' => '--int'}, @_);
>  }
>  
> +=item config_regexp ( RE )
> +
> +Retrieve the list of configuration key names matching the regular
> +expression C<RE>. The return value is a list of strings matching
> +this regex.
> +
> +=cut
> +
> +sub config_regexp {
> +	my ($self, $regex) = _maybe_self(@_);
> +	try {
> +		my @cmd = ('config', '--name-only', '--get-regexp', $regex);
> +		unshift @cmd, $self if $self;
> +		my @matches = command(@cmd);
> +		return @matches;
> +	} catch Git::Error::Command with {
> +		my $E = shift;
> +		if ($E->value() == 1) {
> +			my @matches = ();
> +			return @matches;
> +		} else {
> +			throw $E;
> +		}
> +	};
> +}
> +
>  # Common subroutine to implement bulk of what the config* family of methods
>  # do. This currently wraps command('config') so it is not so fast.
>  sub _config_common {

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

* Re: [PATCH v2] git-send-email: die if sendmail.* config is set
  2020-07-18 20:38 ` Junio C Hamano
@ 2020-07-19  6:07   ` Junio C Hamano
  0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2020-07-19  6:07 UTC (permalink / raw)
  To: Drew DeVault; +Cc: git

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

> Drew DeVault <sir@cmpwn.com> writes:
>
>> I've seen several people mis-configure git send-email on their first
>> attempt because they set the sendmail.* config options - not
>> sendemail.*. This patch detects this mistake and bails out with a
>> friendly warning.
>>
>> Signed-off-by: Drew DeVault <sir@cmpwn.com>
>> ---
>>  Documentation/config/sendemail.txt |  5 +++++
>>  git-send-email.perl                |  8 ++++++++
>>  perl/Git.pm                        | 26 ++++++++++++++++++++++++++
>>  3 files changed, 39 insertions(+)

One more thing.  This should be fairly easy to protect from future
breakages by adding three new tests in t/t9001-send-email.sh script.
One would do something like

	test_config sendmail.program sendmail &&
	test_must_fail git send-email ... >err &&
	test_i18ngrep "found configuration options for .sendmail" err

as a positive test, the second would do

	test_config sendmail.program sendmail &&
	test_config sendemail.forbidsendmailvariables false &&
	git send-email ...

to make sure that escape hatch actually works and then the third
would do something like

	test_config resendmail.program resendmail &&
	git send-email ...

to ensure that only variable whose name begins with "sendmail."
triggers the error.

>> +if ($forbid_sendmail_variables && (scalar Git::config_regexp("sendmail.*")) != 0) {
>
> Judging from the way you wrote the "config_regexp" helper function,
> the above regexp matches "sendmailer.foo", "sendmailed.bar", etc., I
> would think, which probably is not what you intended.  
>
> I guess we can write "sendmail[.].*" or "sendmail\\..*" to ensure
> that we are talking about (literally) "sendmail." followed by
> anything?

I didn't know "git config --get-regexp $regexp" did not anchor the
regular expression to the beginning or to the end.  In this case, we
do want to make sure the "sendmail." substring literally appears at
the very beginning of the variable name, and because "--get-regexp"
does not anchor the regular expression to the end, we do not need to
add an explicit "anything goes", i.e. ".*" after it.

IOW, "^sendmail[.]" is the minimal regexp we want to use.  We cannot
afford to lose the "^" to reject "resendmail.program", and we do not
have to add ".*" that would swallow the rest at the end.

Thanks.


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

* Re: [PATCH v2] git-send-email: die if sendmail.* config is set
  2020-07-18 20:21 [PATCH v2] git-send-email: die if sendmail.* config is set Drew DeVault
  2020-07-18 20:38 ` Junio C Hamano
@ 2020-07-20 17:33 ` Jeff King
  2020-07-20 17:40   ` Drew DeVault
  1 sibling, 1 reply; 8+ messages in thread
From: Jeff King @ 2020-07-20 17:33 UTC (permalink / raw)
  To: Drew DeVault; +Cc: Junio C Hamano, git

On Sat, Jul 18, 2020 at 04:21:42PM -0400, Drew DeVault wrote:

> I've seen several people mis-configure git send-email on their first
> attempt because they set the sendmail.* config options - not
> sendemail.*. This patch detects this mistake and bails out with a
> friendly warning.

This basically claims the "sendmail.*" namespace for send-email. Not
strictly, but if we're going to warn about anything set in it, it
effectively shuts out other uses.

I'm OK with that, but if we're going to do so, should we perhaps just
say "sendmail.* is an alias for sendemail.*"? Then rather than getting a
warning, this mistake would Just Work.

-Peff

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

* Re: [PATCH v2] git-send-email: die if sendmail.* config is set
  2020-07-20 17:33 ` Jeff King
@ 2020-07-20 17:40   ` Drew DeVault
  2020-07-24  0:42     ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Drew DeVault @ 2020-07-20 17:40 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git

On Mon Jul 20, 2020 at 9:33 AM EDT, Jeff King wrote:
> This basically claims the "sendmail.*" namespace for send-email. Not
> strictly, but if we're going to warn about anything set in it, it
> effectively shuts out other uses.

The revised patch (v3 now) is less strict and offers an escape hatch via
sendmail.forbidSendmailVariables. I'd prefer that over making sendmail.*
just werk.

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

* Re: [PATCH v2] git-send-email: die if sendmail.* config is set
  2020-07-20 17:40   ` Drew DeVault
@ 2020-07-24  0:42     ` Junio C Hamano
  2020-07-24  0:43       ` Drew DeVault
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2020-07-24  0:42 UTC (permalink / raw)
  To: Drew DeVault; +Cc: Jeff King, git

"Drew DeVault" <sir@cmpwn.com> writes:

> On Mon Jul 20, 2020 at 9:33 AM EDT, Jeff King wrote:
>> This basically claims the "sendmail.*" namespace for send-email. Not
>> strictly, but if we're going to warn about anything set in it, it
>> effectively shuts out other uses.
>
> The revised patch (v3 now) is less strict and offers an escape hatch via
> sendmail.forbidSendmailVariables. I'd prefer that over making sendmail.*
> just werk.

We are not in a hurry until the next cycle starts, but please send
the updated version out to the list before everybody forgets.

Thanks.

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

* Re: [PATCH v2] git-send-email: die if sendmail.* config is set
  2020-07-24  0:42     ` Junio C Hamano
@ 2020-07-24  0:43       ` Drew DeVault
  2020-07-24  0:59         ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Drew DeVault @ 2020-07-24  0:43 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git

Oh, I'm sorry, I thought I had already sent it out.

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

* Re: [PATCH v2] git-send-email: die if sendmail.* config is set
  2020-07-24  0:43       ` Drew DeVault
@ 2020-07-24  0:59         ` Junio C Hamano
  0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2020-07-24  0:59 UTC (permalink / raw)
  To: Drew DeVault; +Cc: Jeff King, git

"Drew DeVault" <sir@cmpwn.com> writes:

> Oh, I'm sorry, I thought I had already sent it out.

Thanks.  As long as nobody has dropped the ball, we are OK, and
again, we are not in a hurry right now.


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

end of thread, other threads:[~2020-07-24  0:59 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-18 20:21 [PATCH v2] git-send-email: die if sendmail.* config is set Drew DeVault
2020-07-18 20:38 ` Junio C Hamano
2020-07-19  6:07   ` Junio C Hamano
2020-07-20 17:33 ` Jeff King
2020-07-20 17:40   ` Drew DeVault
2020-07-24  0:42     ` Junio C Hamano
2020-07-24  0:43       ` Drew DeVault
2020-07-24  0:59         ` Junio C Hamano

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://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.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 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