{"thread":{"id":"53878","subject":"[PATCH] git-send-email: die if sendmail.* config is set","startedAt":"2020-07-18T13:33:17Z","lastAt":"2020-07-18T20:10:33Z","messageCount":4,"participants":["Drew DeVault","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"401728","messageId":"20200718132311.27248-1-sir@cmpwn.com","threadId":"53878","inReplyTo":null,"subject":"[PATCH] git-send-email: die if sendmail.* config is set","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2020-07-18T13:23:11Z","receivedAt":"2020-07-18T13:33:17Z","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 git-send-email.perl |  6 ++++++\n perl/Git.pm         | 26 ++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 36c47bae1d..8e42ba00c1 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -478,6 +478,12 @@ sub read_config {\n     usage();\n }\n \n+if ((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\"Assuming this is a mistake and bailing out.\\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":"401734","messageId":"xmqqpn8sbt0w.fsf@gitster.c.googlers.com","threadId":"53878","inReplyTo":"20200718132311.27248-1-sir@cmpwn.com","subject":"Re: [PATCH] git-send-email: die if sendmail.* config is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-18T18:02:55Z","receivedAt":"2020-07-18T18:03:02Z","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\nIt is not very friendly thing to do, though.\n\nIt just closes the door for anybody to add something that works\nindependent from \"git send-email\", to which \"sendmail.*\" variables\nmay be appropriate knob to use.\n\nDemoting the \"die\" to \"warn\" is OK, and limiting the check to\nvariables that actually has corresponding and likely-misspelt\n\"sendemail.*\" counterparts would be even better, but \"you are not\nallowed to have any 'sendmail.*' variables, ever\" is way too much,\nI am afraid.\n\n> Signed-off-by: Drew DeVault <sir@cmpwn.com>\n> ---\n>  git-send-email.perl |  6 ++++++\n>  perl/Git.pm         | 26 ++++++++++++++++++++++++++\n>  2 files changed, 32 insertions(+)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 36c47bae1d..8e42ba00c1 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -478,6 +478,12 @@ sub read_config {\n>      usage();\n>  }\n>  \n> +if ((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\"Assuming this is a mistake and bailing out.\\n\");\n> +}\n\n"},{"id":"401737","messageId":"C49YLQZUZ4RG.129ZY8FQEYDYU@homura","threadId":"53878","inReplyTo":"xmqqpn8sbt0w.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] git-send-email: die if sendmail.* config is set","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2020-07-18T18:25:41Z","receivedAt":"2020-07-18T18:27:58Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"On Sat Jul 18, 2020 at 7:02 AM EDT, Junio C Hamano wrote:\n> It is not very friendly thing to do, though.\n>\n> It just closes the door for anybody to add something that works\n> independent from \"git send-email\", to which \"sendmail.*\" variables\n> may be appropriate knob to use.\n>\n> Demoting the \"die\" to \"warn\" is OK, and limiting the check to\n> variables that actually has corresponding and likely-misspelt\n> \"sendemail.*\" counterparts would be even better, but \"you are not\n> allowed to have any 'sendmail.*' variables, ever\" is way too much,\n> I am afraid.\n\nI originally had this just a warning, but it can be difficult to see.\nThere's a lot of text printed from git send-email, a lot of it looks\nlike diagnostics, and it can be easy to lose a message in there. And if\nyou pass --annotate, which I encourage people to do, your editor\nimmediately covers up the warning.\n\nI wonder if a happy medium would be adding a config option which\nsquelches the message? If sendemail.squelchConfigError is set, for\nexample. If not, I'll experiment with a noisy warning.\n"},{"id":"401741","messageId":"xmqqh7u4bn4d.fsf@gitster.c.googlers.com","threadId":"53878","inReplyTo":"C49YLQZUZ4RG.129ZY8FQEYDYU@homura","subject":"Re: [PATCH] git-send-email: die if sendmail.* config is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-18T20:10:26Z","receivedAt":"2020-07-18T20:10:33Z","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 originally had this just a warning, but it can be difficult to see.\n> There's a lot of text printed from git send-email, a lot of it looks\n> like diagnostics, and it can be easy to lose a message in there. And if\n> you pass --annotate, which I encourage people to do, your editor\n> immediately covers up the warning.\n\nOK.\n\n> I wonder if a happy medium would be adding a config option which\n> squelches the message? If sendemail.squelchConfigError is set, for\n> example.\n\nI think with \"squelches the message\" replaced with \"bypasses the\ncheck\", it would probably be a good idea.  It may be that a\nreasonable behaviour would be to check for any \"sendemail.*\" and\ndie, like your version does, by default, an optional opt-out\nconfiguration variable (sendemail.forbidSendmailVariables) to\ndisable the check entirely, and then add a hint that mentions the\nvariable to the \"die\" message in your patch.\n\n\n"}]}