diff mbox series

git-send-email: use ! to indicate relative path to command

Message ID 20210511183703.9488-1-greg@gpanders.com (mailing list archive)
State Superseded
Headers show
Series git-send-email: use ! to indicate relative path to command | expand

Commit Message

Gregory Anders May 11, 2021, 6:37 p.m. UTC 
When the smtpServer config option is prefixed with a ! character, the
value of the option should be interpreted as a command to look up on
PATH.
---

Please note that I am a total perl newbie. It's very likely that I did 
something suboptimally or in a non-idiomatic way. Please let me know if 
that's the case.

 git-send-email.perl | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Jeff King May 11, 2021, 6:57 p.m. UTC | #1 
On Tue, May 11, 2021 at 12:37:03PM -0600, Gregory Anders wrote:

> diff --git a/git-send-email.perl b/git-send-email.perl
> index 175da07d94..dbc5a2f51c 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1492,7 +1492,11 @@ sub send_message {
>  
>  	if ($dry_run) {
>  		# We don't want to send the email.
> -	} elsif (file_name_is_absolute($smtp_server)) {
> +	} elsif (file_name_is_absolute($smtp_server) || $smtp_server =~ /^!/) {
> +		if ($smtp_server =~ s/^!//) {
> +			my $smtp_server = map {"$_/$smtp_server"} split /:/, $ENV{PATH};
> +		}
> +

I don't think the new "if" block is doing what you expect:

  - the result of "map" is a list, but you are assigning it to a scalar
    (so you'll end up with the size of the list, which is really just
    counting the number of elements in your $PATH). If you want to
    search for a match in the PATH, you'd need to do something like:

      for my $candidate (map { "$_/$smtp_server" } split /:/, $ENV{PATH}) {
              if (-x $candidate) {
	              $smtp_server = $candidate;
	              last;
	      }
      }

     But see below.

  - the bogus code in the conditional ends up doing nothing, since you
    declare a new lexical version of $smtp_server (with "my"), shadowing
    the outer variable.

So why does it work at all? Because the "s/^!//" in the "if" statement
actually mutates $smtp_server to remove the "!". And then feeding that
name into exec() below will do a lookup in PATH itself.

So a shorter version of the same thing is just:

  ...
  } elsif (file_name_is_absolute($smtp_server) || $smtp_server =~ s/^!//) {
  ...

which detects and mutates $smtp_server in the first place.

However, it's probably not a good idea to change that variable, as it
loses information. If we call into send_message() a second time, we
won't realize we're supposed to respect "!".

So perhaps something like (totally untested):

diff --git a/git-send-email.perl b/git-send-email.perl
index 175da07d94..022dcf0999 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1492,11 +1492,14 @@ sub send_message {
 
 	if ($dry_run) {
 		# We don't want to send the email.
-	} elsif (file_name_is_absolute($smtp_server)) {
+	} elsif (file_name_is_absolute($smtp_server) || $smtp_server =~ /^!/) {
+		my $prog = $smtp_server;
+		$prog =~ s/^!//;
+
 		my $pid = open my $sm, '|-';
 		defined $pid or die $!;
 		if (!$pid) {
-			exec($smtp_server, @sendmail_parameters) or die $!;
+			exec($prog, @sendmail_parameters) or die $!;
 		}
 		print $sm "$header\n$message";
 		close $sm or die $!;

-Peff
Gregory Anders May 11, 2021, 7:03 p.m. UTC | #2 
I also noticed this after some quick testing and just sent a v2 right 
before seeing your reply.

Your (untested) implementation seems much cleaner than mine, and I'm 
happy to give that a try. Question: is it okay that we pass just a raw 
command name to exec instead of a full path? That is, is there any 
reason we need to first find the command in PATH *and then* pass it to 
exec (which is what my v2 implementation does)?
Jeff King May 11, 2021, 7:11 p.m. UTC | #3 
On Tue, May 11, 2021 at 01:03:32PM -0600, Gregory Anders wrote:

> I also noticed this after some quick testing and just sent a v2 right before
> seeing your reply.
> 
> Your (untested) implementation seems much cleaner than mine, and I'm happy
> to give that a try. Question: is it okay that we pass just a raw command
> name to exec instead of a full path? That is, is there any reason we need to
> first find the command in PATH *and then* pass it to exec (which is what my
> v2 implementation does)?

I don't think so. Perl's exec() should do the PATH lookup itself. I was
surprised not to see this mentioned explicitly in the documentation, but
it clearly does work. E.g., try:

  perl -e 'exec("ls")'

-Peff
diff mbox series

Patch

diff --git a/git-send-email.perl b/git-send-email.perl
index 175da07d94..dbc5a2f51c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1492,7 +1492,11 @@  sub send_message {
 
 	if ($dry_run) {
 		# We don't want to send the email.
-	} elsif (file_name_is_absolute($smtp_server)) {
+	} elsif (file_name_is_absolute($smtp_server) || $smtp_server =~ /^!/) {
+		if ($smtp_server =~ s/^!//) {
+			my $smtp_server = map {"$_/$smtp_server"} split /:/, $ENV{PATH};
+		}
+
 		my $pid = open my $sm, '|-';
 		defined $pid or die $!;
 		if (!$pid) {