{"thread":{"id":"55662","subject":"[PATCH] git-send-email: use ! to indicate relative path to command","startedAt":"2021-05-11T18:37:15Z","lastAt":"2021-05-11T19:11:53Z","messageCount":4,"participants":["Gregory Anders","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"424168","messageId":"20210511183703.9488-1-greg@gpanders.com","threadId":"55662","inReplyTo":null,"subject":"[PATCH] git-send-email: use ! to indicate relative path to command","fromName":"Gregory Anders","fromEmail":"greg@gpanders.com","sentAt":"2021-05-11T18:37:03Z","receivedAt":"2021-05-11T18:37:15Z","isPatch":true,"sender":{"key":"greg@gpanders.com","avatar":"https://avatars.githubusercontent.com/u/8965202?v=4"},"body":"When the smtpServer config option is prefixed with a ! character, the\nvalue of the option should be interpreted as a command to look up on\nPATH.\n---\n\nPlease note that I am a total perl newbie. It's very likely that I did \nsomething suboptimally or in a non-idiomatic way. Please let me know if \nthat's the case.\n\n git-send-email.perl | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 175da07d94..dbc5a2f51c 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1492,7 +1492,11 @@ sub send_message {\n \n \tif ($dry_run) {\n \t\t# We don't want to send the email.\n-\t} elsif (file_name_is_absolute($smtp_server)) {\n+\t} elsif (file_name_is_absolute($smtp_server) || $smtp_server =~ /^!/) {\n+\t\tif ($smtp_server =~ s/^!//) {\n+\t\t\tmy $smtp_server = map {\"$_/$smtp_server\"} split /:/, $ENV{PATH};\n+\t\t}\n+\n \t\tmy $pid = open my $sm, '|-';\n \t\tdefined $pid or die $!;\n \t\tif (!$pid) {\n-- \n2.31.1.576.gc7e8ed1dea\n\n"},{"id":"424173","messageId":"YJrTlXzuuMoxeJay@coredump.intra.peff.net","threadId":"55662","inReplyTo":"20210511183703.9488-1-greg@gpanders.com","subject":"Re: [PATCH] git-send-email: use ! to indicate relative path to command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-11T18:57:25Z","receivedAt":"2021-05-11T18:57:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 11, 2021 at 12:37:03PM -0600, Gregory Anders wrote:\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 175da07d94..dbc5a2f51c 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1492,7 +1492,11 @@ sub send_message {\n>  \n>  \tif ($dry_run) {\n>  \t\t# We don't want to send the email.\n> -\t} elsif (file_name_is_absolute($smtp_server)) {\n> +\t} elsif (file_name_is_absolute($smtp_server) || $smtp_server =~ /^!/) {\n> +\t\tif ($smtp_server =~ s/^!//) {\n> +\t\t\tmy $smtp_server = map {\"$_/$smtp_server\"} split /:/, $ENV{PATH};\n> +\t\t}\n> +\n\nI don't think the new \"if\" block is doing what you expect:\n\n  - the result of \"map\" is a list, but you are assigning it to a scalar\n    (so you'll end up with the size of the list, which is really just\n    counting the number of elements in your $PATH). If you want to\n    search for a match in the PATH, you'd need to do something like:\n\n      for my $candidate (map { \"$_/$smtp_server\" } split /:/, $ENV{PATH}) {\n              if (-x $candidate) {\n\t              $smtp_server = $candidate;\n\t              last;\n\t      }\n      }\n\n     But see below.\n\n  - the bogus code in the conditional ends up doing nothing, since you\n    declare a new lexical version of $smtp_server (with \"my\"), shadowing\n    the outer variable.\n\nSo why does it work at all? Because the \"s/^!//\" in the \"if\" statement\nactually mutates $smtp_server to remove the \"!\". And then feeding that\nname into exec() below will do a lookup in PATH itself.\n\nSo a shorter version of the same thing is just:\n\n  ...\n  } elsif (file_name_is_absolute($smtp_server) || $smtp_server =~ s/^!//) {\n  ...\n\nwhich detects and mutates $smtp_server in the first place.\n\nHowever, it's probably not a good idea to change that variable, as it\nloses information. If we call into send_message() a second time, we\nwon't realize we're supposed to respect \"!\".\n\nSo perhaps something like (totally untested):\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 175da07d94..022dcf0999 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1492,11 +1492,14 @@ sub send_message {\n \n \tif ($dry_run) {\n \t\t# We don't want to send the email.\n-\t} elsif (file_name_is_absolute($smtp_server)) {\n+\t} elsif (file_name_is_absolute($smtp_server) || $smtp_server =~ /^!/) {\n+\t\tmy $prog = $smtp_server;\n+\t\t$prog =~ s/^!//;\n+\n \t\tmy $pid = open my $sm, '|-';\n \t\tdefined $pid or die $!;\n \t\tif (!$pid) {\n-\t\t\texec($smtp_server, @sendmail_parameters) or die $!;\n+\t\t\texec($prog, @sendmail_parameters) or die $!;\n \t\t}\n \t\tprint $sm \"$header\\n$message\";\n \t\tclose $sm or die $!;\n\n-Peff\n"},{"id":"424177","messageId":"YJrVBA4HP0HEqRLD@gpanders.com","threadId":"55662","inReplyTo":"YJrTlXzuuMoxeJay@coredump.intra.peff.net","subject":"Re: [PATCH] git-send-email: use ! to indicate relative path to command","fromName":"Gregory Anders","fromEmail":"greg@gpanders.com","sentAt":"2021-05-11T19:03:32Z","receivedAt":"2021-05-11T19:03:36Z","isPatch":true,"sender":{"key":"greg@gpanders.com","avatar":"https://avatars.githubusercontent.com/u/8965202?v=4"},"body":"I also noticed this after some quick testing and just sent a v2 right \nbefore seeing your reply.\n\nYour (untested) implementation seems much cleaner than mine, and I'm \nhappy to give that a try. Question: is it okay that we pass just a raw \ncommand name to exec instead of a full path? That is, is there any \nreason we need to first find the command in PATH *and then* pass it to \nexec (which is what my v2 implementation does)?\n"},{"id":"424181","messageId":"YJrW98FZN/N6u7un@coredump.intra.peff.net","threadId":"55662","inReplyTo":"YJrVBA4HP0HEqRLD@gpanders.com","subject":"Re: [PATCH] git-send-email: use ! to indicate relative path to command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-11T19:11:51Z","receivedAt":"2021-05-11T19:11:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 11, 2021 at 01:03:32PM -0600, Gregory Anders wrote:\n\n> I also noticed this after some quick testing and just sent a v2 right before\n> seeing your reply.\n> \n> Your (untested) implementation seems much cleaner than mine, and I'm happy\n> to give that a try. Question: is it okay that we pass just a raw command\n> name to exec instead of a full path? That is, is there any reason we need to\n> first find the command in PATH *and then* pass it to exec (which is what my\n> v2 implementation does)?\n\nI don't think so. Perl's exec() should do the PATH lookup itself. I was\nsurprised not to see this mentioned explicitly in the documentation, but\nit clearly does work. E.g., try:\n\n  perl -e 'exec(\"ls\")'\n\n-Peff\n"}]}