git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] git-send-email: use ! to indicate relative path to command
@ 2021-05-11 18:37 Gregory Anders
  2021-05-11 18:57 ` Jeff King
  0 siblings, 1 reply; 4+ messages in thread
From: Gregory Anders @ 2021-05-11 18:37 UTC (permalink / raw)
  To: git; +Cc: Gregory Anders

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(-)

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) {
-- 
2.31.1.576.gc7e8ed1dea


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] git-send-email: use ! to indicate relative path to command
  2021-05-11 18:37 [PATCH] git-send-email: use ! to indicate relative path to command Gregory Anders
@ 2021-05-11 18:57 ` Jeff King
  2021-05-11 19:03   ` Gregory Anders
  0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2021-05-11 18:57 UTC (permalink / raw)
  To: Gregory Anders; +Cc: git

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] git-send-email: use ! to indicate relative path to command
  2021-05-11 18:57 ` Jeff King
@ 2021-05-11 19:03   ` Gregory Anders
  2021-05-11 19:11     ` Jeff King
  0 siblings, 1 reply; 4+ messages in thread
From: Gregory Anders @ 2021-05-11 19:03 UTC (permalink / raw)
  To: Jeff King; +Cc: git

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)?

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] git-send-email: use ! to indicate relative path to command
  2021-05-11 19:03   ` Gregory Anders
@ 2021-05-11 19:11     ` Jeff King
  0 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2021-05-11 19:11 UTC (permalink / raw)
  To: Gregory Anders; +Cc: git

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-05-11 19:11 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-11 18:37 [PATCH] git-send-email: use ! to indicate relative path to command Gregory Anders
2021-05-11 18:57 ` Jeff King
2021-05-11 19:03   ` Gregory Anders
2021-05-11 19:11     ` Jeff King

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).