{"thread":{"id":"53881","subject":"[PATCH v2] git-send-email: die if sendmail.* config is set","startedAt":"2020-07-18T20:21:47Z","lastAt":"2020-07-20T17:41:45Z","messageCount":5,"participants":["Drew DeVault","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"401742","messageId":"20200718202142.111015-1-sir@cmpwn.com","threadId":"53881","inReplyTo":null,"subject":"[PATCH v2] git-send-email: die if sendmail.* config is set","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2020-07-18T20:21:42Z","receivedAt":"2020-07-18T20:21:47Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"I've seen several people mis-configure git send-email on their first\nattempt because they set the sendmail.* config options - not\nsendemail.*. This patch detects this mistake and bails out with a\nfriendly warning.\n\nSigned-off-by: Drew DeVault <sir@cmpwn.com>\n---\n Documentation/config/sendemail.txt |  5 +++++\n git-send-email.perl                |  8 ++++++++\n perl/Git.pm                        | 26 ++++++++++++++++++++++++++\n 3 files changed, 39 insertions(+)\n\ndiff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt\nindex 0006faf800..1726d5f85e 100644\n--- a/Documentation/config/sendemail.txt\n+++ b/Documentation/config/sendemail.txt\n@@ -61,3 +61,8 @@ sendemail.smtpBatchSize::\n sendemail.smtpReloginDelay::\n \tSeconds wait before reconnecting to smtp server.\n \tSee also the `--relogin-delay` option of linkgit:git-send-email[1].\n+\n+sendemail.forceSendmailVariables::\n+\tTo avoid common misconfiguration mistakes, linkgit:git-send-email[1]\n+\twill abort with a warning if any configuration options for \"sendmail\"\n+\texist. Set this variable to bypass the check.\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 36c47bae1d..1b186bc058 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -250,6 +250,7 @@ sub do_edit {\n my $use_xmailer = 1;\n my $validate = 1;\n my $target_xfer_encoding = 'auto';\n+my $forbid_sendmail_variables = 1;\n \n my %config_bool_settings = (\n     \"thread\" => \\$thread,\n@@ -263,6 +264,7 @@ sub do_edit {\n     \"multiedit\" => \\$multiedit,\n     \"annotate\" => \\$annotate,\n     \"xmailer\" => \\$use_xmailer,\n+    \"forbidsendmailvariables\" => \\$forbid_sendmail_variables,\n );\n \n my %config_settings = (\n@@ -478,6 +480,12 @@ sub read_config {\n     usage();\n }\n \n+if ($forbid_sendmail_variables && (scalar Git::config_regexp(\"sendmail.*\")) != 0) {\n+\tdie __(\"fatal: found configuration options for 'sendmail'\\n\" .\n+\t\t\"git-send-email is configured with the sendemail.* options - note the 'e'.\\n\" .\n+\t\t\"Set sendemail.forbidSendmailVariables to false to disable this check.\\n\");\n+}\n+\n die __(\"Cannot run git format-patch from outside a repository\\n\")\n \tif $format_patch and not $repo;\n \ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 54c9ed0dde..10df990959 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -723,6 +723,32 @@ sub config_int {\n \treturn scalar _config_common({'kind' => '--int'}, @_);\n }\n \n+=item config_regexp ( RE )\n+\n+Retrieve the list of configuration key names matching the regular\n+expression C<RE>. The return value is a list of strings matching\n+this regex.\n+\n+=cut\n+\n+sub config_regexp {\n+\tmy ($self, $regex) = _maybe_self(@_);\n+\ttry {\n+\t\tmy @cmd = ('config', '--name-only', '--get-regexp', $regex);\n+\t\tunshift @cmd, $self if $self;\n+\t\tmy @matches = command(@cmd);\n+\t\treturn @matches;\n+\t} catch Git::Error::Command with {\n+\t\tmy $E = shift;\n+\t\tif ($E->value() == 1) {\n+\t\t\tmy @matches = ();\n+\t\t\treturn @matches;\n+\t\t} else {\n+\t\t\tthrow $E;\n+\t\t}\n+\t};\n+}\n+\n # Common subroutine to implement bulk of what the config* family of methods\n # do. This currently wraps command('config') so it is not so fast.\n sub _config_common {\n-- \n2.27.0\n\n"},{"id":"401745","messageId":"xmqq8sfgbltq.fsf@gitster.c.googlers.com","threadId":"53881","inReplyTo":"20200718202142.111015-1-sir@cmpwn.com","subject":"Re: [PATCH v2] git-send-email: die if sendmail.* config is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-18T20:38:25Z","receivedAt":"2020-07-18T20:38:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Drew DeVault <sir@cmpwn.com> writes:\n\n> I've seen several people mis-configure git send-email on their first\n> attempt because they set the sendmail.* config options - not\n> sendemail.*. This patch detects this mistake and bails out with a\n> friendly warning.\n>\n> Signed-off-by: Drew DeVault <sir@cmpwn.com>\n> ---\n>  Documentation/config/sendemail.txt |  5 +++++\n>  git-send-email.perl                |  8 ++++++++\n>  perl/Git.pm                        | 26 ++++++++++++++++++++++++++\n>  3 files changed, 39 insertions(+)\n>\n> diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt\n> index 0006faf800..1726d5f85e 100644\n> --- a/Documentation/config/sendemail.txt\n> +++ b/Documentation/config/sendemail.txt\n> @@ -61,3 +61,8 @@ sendemail.smtpBatchSize::\n>  sendemail.smtpReloginDelay::\n>  \tSeconds wait before reconnecting to smtp server.\n>  \tSee also the `--relogin-delay` option of linkgit:git-send-email[1].\n> +\n> +sendemail.forceSendmailVariables::\n> +\tTo avoid common misconfiguration mistakes, linkgit:git-send-email[1]\n> +\twill abort with a warning if any configuration options for \"sendmail\"\n> +\texist. Set this variable to bypass the check.\n\nI am not sure if it is clear to readers what kind of forcing this\nrefers to.  At least it is not clear to me.\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 36c47bae1d..1b186bc058 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -250,6 +250,7 @@ sub do_edit {\n>  my $use_xmailer = 1;\n>  my $validate = 1;\n>  my $target_xfer_encoding = 'auto';\n> +my $forbid_sendmail_variables = 1;\n\nAh, probably s/force/forbid/ in the documentation above?  OK, then\nit makes sense.\n\n>  \n>  my %config_bool_settings = (\n>      \"thread\" => \\$thread,\n> @@ -263,6 +264,7 @@ sub do_edit {\n>      \"multiedit\" => \\$multiedit,\n>      \"annotate\" => \\$annotate,\n>      \"xmailer\" => \\$use_xmailer,\n> +    \"forbidsendmailvariables\" => \\$forbid_sendmail_variables,\n>  );\n>  \n>  my %config_settings = (\n> @@ -478,6 +480,12 @@ sub read_config {\n>      usage();\n>  }\n>  \n> +if ($forbid_sendmail_variables && (scalar Git::config_regexp(\"sendmail.*\")) != 0) {\n\nJudging from the way you wrote the \"config_regexp\" helper function,\nthe above regexp matches \"sendmailer.foo\", \"sendmailed.bar\", etc., I\nwould think, which probably is not what you intended.  \n\nI guess we can write \"sendmail[.].*\" or \"sendmail\\\\..*\" to ensure\nthat we are talking about (literally) \"sendmail.\" followed by\nanything?\n\n> +\tdie __(\"fatal: found configuration options for 'sendmail'\\n\" .\n> +\t\t\"git-send-email is configured with the sendemail.* options - note the 'e'.\\n\" .\n> +\t\t\"Set sendemail.forbidSendmailVariables to false to disable this check.\\n\");\n> +}\n\nOK.\n\nOther than the two minor nits, looking quite good.\n\nThanks.\n\n>  die __(\"Cannot run git format-patch from outside a repository\\n\")\n>  \tif $format_patch and not $repo;\n>  \n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 54c9ed0dde..10df990959 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -723,6 +723,32 @@ sub config_int {\n>  \treturn scalar _config_common({'kind' => '--int'}, @_);\n>  }\n>  \n> +=item config_regexp ( RE )\n> +\n> +Retrieve the list of configuration key names matching the regular\n> +expression C<RE>. The return value is a list of strings matching\n> +this regex.\n> +\n> +=cut\n> +\n> +sub config_regexp {\n> +\tmy ($self, $regex) = _maybe_self(@_);\n> +\ttry {\n> +\t\tmy @cmd = ('config', '--name-only', '--get-regexp', $regex);\n> +\t\tunshift @cmd, $self if $self;\n> +\t\tmy @matches = command(@cmd);\n> +\t\treturn @matches;\n> +\t} catch Git::Error::Command with {\n> +\t\tmy $E = shift;\n> +\t\tif ($E->value() == 1) {\n> +\t\t\tmy @matches = ();\n> +\t\t\treturn @matches;\n> +\t\t} else {\n> +\t\t\tthrow $E;\n> +\t\t}\n> +\t};\n> +}\n> +\n>  # Common subroutine to implement bulk of what the config* family of methods\n>  # do. This currently wraps command('config') so it is not so fast.\n>  sub _config_common {\n"},{"id":"401752","messageId":"xmqqv9ik9gwa.fsf@gitster.c.googlers.com","threadId":"53881","inReplyTo":"xmqq8sfgbltq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] git-send-email: die if sendmail.* config is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-19T06:07:49Z","receivedAt":"2020-07-19T06:07:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Drew DeVault <sir@cmpwn.com> writes:\n>\n>> I've seen several people mis-configure git send-email on their first\n>> attempt because they set the sendmail.* config options - not\n>> sendemail.*. This patch detects this mistake and bails out with a\n>> friendly warning.\n>>\n>> Signed-off-by: Drew DeVault <sir@cmpwn.com>\n>> ---\n>>  Documentation/config/sendemail.txt |  5 +++++\n>>  git-send-email.perl                |  8 ++++++++\n>>  perl/Git.pm                        | 26 ++++++++++++++++++++++++++\n>>  3 files changed, 39 insertions(+)\n\nOne more thing.  This should be fairly easy to protect from future\nbreakages by adding three new tests in t/t9001-send-email.sh script.\nOne would do something like\n\n\ttest_config sendmail.program sendmail &&\n\ttest_must_fail git send-email ... >err &&\n\ttest_i18ngrep \"found configuration options for .sendmail\" err\n\nas a positive test, the second would do\n\n\ttest_config sendmail.program sendmail &&\n\ttest_config sendemail.forbidsendmailvariables false &&\n\tgit send-email ...\n\nto make sure that escape hatch actually works and then the third\nwould do something like\n\n\ttest_config resendmail.program resendmail &&\n\tgit send-email ...\n\nto ensure that only variable whose name begins with \"sendmail.\"\ntriggers the error.\n\n>> +if ($forbid_sendmail_variables && (scalar Git::config_regexp(\"sendmail.*\")) != 0) {\n>\n> Judging from the way you wrote the \"config_regexp\" helper function,\n> the above regexp matches \"sendmailer.foo\", \"sendmailed.bar\", etc., I\n> would think, which probably is not what you intended.  \n>\n> I guess we can write \"sendmail[.].*\" or \"sendmail\\\\..*\" to ensure\n> that we are talking about (literally) \"sendmail.\" followed by\n> anything?\n\nI didn't know \"git config --get-regexp $regexp\" did not anchor the\nregular expression to the beginning or to the end.  In this case, we\ndo want to make sure the \"sendmail.\" substring literally appears at\nthe very beginning of the variable name, and because \"--get-regexp\"\ndoes not anchor the regular expression to the end, we do not need to\nadd an explicit \"anything goes\", i.e. \".*\" after it.\n\nIOW, \"^sendmail[.]\" is the minimal regexp we want to use.  We cannot\nafford to lose the \"^\" to reject \"resendmail.program\", and we do not\nhave to add \".*\" that would swallow the rest at the end.\n\nThanks.\n\n"},{"id":"401771","messageId":"20200720173336.GC2045458@coredump.intra.peff.net","threadId":"53881","inReplyTo":"20200718202142.111015-1-sir@cmpwn.com","subject":"Re: [PATCH v2] git-send-email: die if sendmail.* config is set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-20T17:33:36Z","receivedAt":"2020-07-20T17:33:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 18, 2020 at 04:21:42PM -0400, Drew DeVault wrote:\n\n> I've seen several people mis-configure git send-email on their first\n> attempt because they set the sendmail.* config options - not\n> sendemail.*. This patch detects this mistake and bails out with a\n> friendly warning.\n\nThis basically claims the \"sendmail.*\" namespace for send-email. Not\nstrictly, but if we're going to warn about anything set in it, it\neffectively shuts out other uses.\n\nI'm OK with that, but if we're going to do so, should we perhaps just\nsay \"sendmail.* is an alias for sendemail.*\"? Then rather than getting a\nwarning, this mistake would Just Work.\n\n-Peff\n"},{"id":"401772","messageId":"C4BMWIIK892D.25HUQKV9F98T6@homura","threadId":"53881","inReplyTo":"20200720173336.GC2045458@coredump.intra.peff.net","subject":"Re: [PATCH v2] git-send-email: die if sendmail.* config is set","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2020-07-20T17:40:51Z","receivedAt":"2020-07-20T17:41:45Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"On Mon Jul 20, 2020 at 9:33 AM EDT, Jeff King wrote:\n> This basically claims the \"sendmail.*\" namespace for send-email. Not\n> strictly, but if we're going to warn about anything set in it, it\n> effectively shuts out other uses.\n\nThe revised patch (v3 now) is less strict and offers an escape hatch via\nsendmail.forbidSendmailVariables. I'd prefer that over making sendmail.*\njust werk.\n"}]}