Re: [PATCH] git-send-email: use ! to indicate relative path to command

From: Jeff King <peff@peff.net>
Date: May 11, 2021, 18:57 UTC
Message-ID: <YJrTlXzuuMoxeJay@coredump.intra.peff.net>
In-Reply-To: <20210511183703.9488-1-greg@gpanders.com>

On Tue, May 11, 2021 at 12:37:03PM -0600, Gregory Anders wrote:

Show 14 quoted lines

> 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