diff mbox series

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

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

Commit Message

Gregory Anders May 11, 2021, 11:49 p.m. UTC
The sendemail.smtpServer configuration option and the '--smtp-server'
command line option can name a program to use by providing an absolute
path to the program. However, an absolute path is not always portable
and it is often preferable to simply specify a program name and have
'git-send-email' find that program on $PATH.

For example, if a user wishes to use msmtp to send emails, they might
be able to simply use

    [sendemail]
            smtpServer = !msmtp

instead of using the full path. This is particularly useful when a
common git config file is used across multiple systems where the
absolute path to a given program may differ.

To that end, this patch allows both the configuration and command line
options to be prefixed with a '!' character to indicate that the
specified command should be found on $PATH, as if the user had entered
it directly on the command line.

Signed-off-by: Gregory Anders <greg@gpanders.com>
---
Diff from v4:

* Update the test with suggestions from Jeff King (this should fix 
  erroneous test failures caused by patch files being deleted by earlier 
  tests)
* Reword the commit message with feedback from Jeff King and Junio 
  Hamano

 Documentation/git-send-email.txt | 13 +++++++------
 git-send-email.perl              |  7 +++++--
 t/t9001-send-email.sh            | 12 ++++++++++++
 3 files changed, 24 insertions(+), 8 deletions(-)

Comments

brian m. carlson May 12, 2021, midnight UTC | #1
On 2021-05-11 at 23:49:35, Gregory Anders wrote:
> The sendemail.smtpServer configuration option and the '--smtp-server'
> command line option can name a program to use by providing an absolute
> path to the program. However, an absolute path is not always portable
> and it is often preferable to simply specify a program name and have
> 'git-send-email' find that program on $PATH.
> 
> For example, if a user wishes to use msmtp to send emails, they might
> be able to simply use
> 
>     [sendemail]
>             smtpServer = !msmtp
> 
> instead of using the full path. This is particularly useful when a
> common git config file is used across multiple systems where the
> absolute path to a given program may differ.
> 
> To that end, this patch allows both the configuration and command line
> options to be prefixed with a '!' character to indicate that the
> specified command should be found on $PATH, as if the user had entered
> it directly on the command line.

I think the idea of providing a way to invoke a sendmail-compatible mail
server is a good idea.

> Signed-off-by: Gregory Anders <greg@gpanders.com>
> ---
> Diff from v4:
> 
> * Update the test with suggestions from Jeff King (this should fix 
>   erroneous test failures caused by patch files being deleted by earlier 
>   tests)
> * Reword the commit message with feedback from Jeff King and Junio 
>   Hamano
> 
>  Documentation/git-send-email.txt | 13 +++++++------
>  git-send-email.perl              |  7 +++++--
>  t/t9001-send-email.sh            | 12 ++++++++++++
>  3 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 93708aefea..418e66c703 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -212,12 +212,13 @@ a password is obtained using 'git-credential'.
>  --smtp-server=<host>::
>  	If set, specifies the outgoing SMTP server to use (e.g.
>  	`smtp.example.com` or a raw IP address).  Alternatively it can
> -	specify a full pathname of a sendmail-like program instead;
> -	the program must support the `-i` option.  Default value can
> -	be specified by the `sendemail.smtpServer` configuration
> -	option; the built-in default is to search for `sendmail` in
> -	`/usr/sbin`, `/usr/lib` and $PATH if such program is
> -	available, falling back to `localhost` otherwise.
> +	specify a sendmail-like program instead, either by a full
> +	path-name or by prefixing the program name with `!`.  The
> +	program must support the `-i` option.  Default value can be
> +	specified by the `sendemail.smtpServer` configuration option;
> +	the built-in default is to search for `sendmail` in `/usr/sbin`,
> +	`/usr/lib` and $PATH if such program is available, falling back
> +	to `localhost` otherwise.

Elsewhere we use the ! syntax we invoke the shell, and I would suggest
that we do the same here.  That means we'll get PATH functionality by
default and we'll let people do a modicum of scripting if they like.
Jeff King May 12, 2021, 12:35 a.m. UTC | #2
On Wed, May 12, 2021 at 12:00:51AM +0000, brian m. carlson wrote:

> > +	specify a sendmail-like program instead, either by a full
> > +	path-name or by prefixing the program name with `!`.  The
> > +	program must support the `-i` option.  Default value can be
> > +	specified by the `sendemail.smtpServer` configuration option;
> > +	the built-in default is to search for `sendmail` in `/usr/sbin`,
> > +	`/usr/lib` and $PATH if such program is available, falling back
> > +	to `localhost` otherwise.
> 
> Elsewhere we use the ! syntax we invoke the shell, and I would suggest
> that we do the same here.  That means we'll get PATH functionality by
> default and we'll let people do a modicum of scripting if they like.

Thanks for bringing that up. I agree it makes things more consistent
with other uses of "!", and certainly it's more flexible. It does
introduce an inconsistency with the absolute-path form, as I mentioned
in https://lore.kernel.org/git/YJsiKDNbKclFU00b@coredump.intra.peff.net/.

I don't know if that's a show-stopper or not. Certainly the
documentation can explain the difference, but it's nice to keep the
rules as simple as possible.

(My gut feeling is that consistency with other "!" places is more
important than consistency with the absolute-path form).

-Peff
Gregory Anders May 12, 2021, 12:45 a.m. UTC | #3
On Tue, 11 May 2021 20:35 -0400, Jeff King wrote:
>On Wed, May 12, 2021 at 12:00:51AM +0000, brian m. carlson wrote:
>
>> > +	specify a sendmail-like program instead, either by a full
>> > +	path-name or by prefixing the program name with `!`.  The
>> > +	program must support the `-i` option.  Default value can be
>> > +	specified by the `sendemail.smtpServer` configuration option;
>> > +	the built-in default is to search for `sendmail` in `/usr/sbin`,
>> > +	`/usr/lib` and $PATH if such program is available, falling back
>> > +	to `localhost` otherwise.
>>
>> Elsewhere we use the ! syntax we invoke the shell, and I would suggest
>> that we do the same here.  That means we'll get PATH functionality by
>> default and we'll let people do a modicum of scripting if they like.
>
>Thanks for bringing that up. I agree it makes things more consistent
>with other uses of "!", and certainly it's more flexible. It does
>introduce an inconsistency with the absolute-path form, as I mentioned
>in https://lore.kernel.org/git/YJsiKDNbKclFU00b@coredump.intra.peff.net/.
>
>I don't know if that's a show-stopper or not. Certainly the
>documentation can explain the difference, but it's nice to keep the
>rules as simple as possible.
>
>(My gut feeling is that consistency with other "!" places is more
>important than consistency with the absolute-path form).
>
>-Peff

We already have sendemail.smtpServerOption to add options:

     [sendemail]
             smtpServer = !msmtp
             smtpServerOption = -f
             smtpServerOption = greg@gpanders.com

I agree that it's not the prettiest and it's a little annoying to have 
to specify the option multiple times, but I thought it worth mentioning 
before considering another way to do the same thing.

I also am curious what other's thoughts are on Felipe's suggestion to 
add a sendemail.program option, which would altogether remove the need 
to further overload sendemail.smtpServer: 
https://lore.kernel.org/git/609b0017a323b_6064920888@natae.notmuch/

IMO if we want to add the capability to run an arbitrary shell command 
as the smtpServer, this makes more sense to add as a dedicated 
sendemail.program option that has that functionality baked right in:

     [sendemail]
             program = "msmtp -f greg@gpanders.com"
Jeff King May 12, 2021, 12:49 a.m. UTC | #4
On Tue, May 11, 2021 at 06:45:58PM -0600, Gregory Anders wrote:

> > > Elsewhere we use the ! syntax we invoke the shell, and I would suggest
> > > that we do the same here.  That means we'll get PATH functionality by
> > > default and we'll let people do a modicum of scripting if they like.
> > 
> > Thanks for bringing that up. I agree it makes things more consistent
> > with other uses of "!", and certainly it's more flexible. It does
> > introduce an inconsistency with the absolute-path form, as I mentioned
> > in https://lore.kernel.org/git/YJsiKDNbKclFU00b@coredump.intra.peff.net/.
> > 
> > I don't know if that's a show-stopper or not. Certainly the
> > documentation can explain the difference, but it's nice to keep the
> > rules as simple as possible.
> > 
> > (My gut feeling is that consistency with other "!" places is more
> > important than consistency with the absolute-path form).
> > 
> > -Peff
> 
> We already have sendemail.smtpServerOption to add options:
> 
>     [sendemail]
>             smtpServer = !msmtp
>             smtpServerOption = -f
>             smtpServerOption = greg@gpanders.com
> 
> I agree that it's not the prettiest and it's a little annoying to have to
> specify the option multiple times, but I thought it worth mentioning before
> considering another way to do the same thing.

Thanks for bringing that up. I agree that does give back some of the
flexibility, but it is inconsistent with most other parts of Git.

> I also am curious what other's thoughts are on Felipe's suggestion to add a
> sendemail.program option, which would altogether remove the need to further
> overload sendemail.smtpServer:
> https://lore.kernel.org/git/609b0017a323b_6064920888@natae.notmuch/
> 
> IMO if we want to add the capability to run an arbitrary shell command as
> the smtpServer, this makes more sense to add as a dedicated
> sendemail.program option that has that functionality baked right in:
> 
>     [sendemail]
>             program = "msmtp -f greg@gpanders.com"

Our mails just crossed, but yeah, I think that would be a fine
direction.

-Peff
brian m. carlson May 12, 2021, 12:51 a.m. UTC | #5
On 2021-05-12 at 00:35:34, Jeff King wrote:
> On Wed, May 12, 2021 at 12:00:51AM +0000, brian m. carlson wrote:
> 
> > > +	specify a sendmail-like program instead, either by a full
> > > +	path-name or by prefixing the program name with `!`.  The
> > > +	program must support the `-i` option.  Default value can be
> > > +	specified by the `sendemail.smtpServer` configuration option;
> > > +	the built-in default is to search for `sendmail` in `/usr/sbin`,
> > > +	`/usr/lib` and $PATH if such program is available, falling back
> > > +	to `localhost` otherwise.
> > 
> > Elsewhere we use the ! syntax we invoke the shell, and I would suggest
> > that we do the same here.  That means we'll get PATH functionality by
> > default and we'll let people do a modicum of scripting if they like.
> 
> Thanks for bringing that up. I agree it makes things more consistent
> with other uses of "!", and certainly it's more flexible. It does
> introduce an inconsistency with the absolute-path form, as I mentioned
> in https://lore.kernel.org/git/YJsiKDNbKclFU00b@coredump.intra.peff.net/.
> 
> I don't know if that's a show-stopper or not. Certainly the
> documentation can explain the difference, but it's nice to keep the
> rules as simple as possible.

I think the minor incompatibility here is okay.  It would have been
nicer to be able to avoid it, but hindsight is always 20/20.

> (My gut feeling is that consistency with other "!" places is more
> important than consistency with the absolute-path form).

Yeah, I think the shell here can be very useful, because it lets you
configure something once in .gitconfig and handle system
incompatibilities (a purely hypothetical example):

  !f() { if [ "$(uname -s)" = Darwin ]; then sendmail "$@"; else postfix "$@"; fi; };f

People use this functionality all the time in other places: credential
helpers, editors (use a fancy graphical editor if supported, otherwise
Vim), and various other places we allow this syntax.
Junio C Hamano May 12, 2021, 3:29 a.m. UTC | #6
Gregory Anders <greg@gpanders.com> writes:

> We already have sendemail.smtpServerOption to add options:
>
>     [sendemail]
>             smtpServer = !msmtp
>             smtpServerOption = -f
>             smtpServerOption = greg@gpanders.com
>
> I agree that it's not the prettiest and it's a little annoying to have
> to specify the option multiple times, but I thought it worth
> mentioning before considering another way to do the same thing.

Well, then can't we just scrap this whole topic, like

    [sendemail]
	smtpServer = /usr/bin/env
	smtpServerOption = msmtp

would already work without adding '!' support ;-)

sendemail.command is also fine; make it invoke the command line via
the shell and we can gradually depreate the "server that is an
absolute path is the name of a program to talk to the server".
diff mbox series

Patch

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 93708aefea..418e66c703 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -212,12 +212,13 @@  a password is obtained using 'git-credential'.
 --smtp-server=<host>::
 	If set, specifies the outgoing SMTP server to use (e.g.
 	`smtp.example.com` or a raw IP address).  Alternatively it can
-	specify a full pathname of a sendmail-like program instead;
-	the program must support the `-i` option.  Default value can
-	be specified by the `sendemail.smtpServer` configuration
-	option; the built-in default is to search for `sendmail` in
-	`/usr/sbin`, `/usr/lib` and $PATH if such program is
-	available, falling back to `localhost` otherwise.
+	specify a sendmail-like program instead, either by a full
+	path-name or by prefixing the program name with `!`.  The
+	program must support the `-i` option.  Default value can be
+	specified by the `sendemail.smtpServer` configuration option;
+	the built-in default is to search for `sendmail` in `/usr/sbin`,
+	`/usr/lib` and $PATH if such program is available, falling back
+	to `localhost` otherwise.
 
 --smtp-server-port=<port>::
 	Specifies a port different from the default port (SMTP
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 $!;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 65b3035371..31d25b32b5 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2148,6 +2148,18 @@  test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '
 	test_cmp expected-list actual-list
 '
 
+test_expect_success $PREREQ 'test using a command for smtpServer with !' '
+	clean_fake_sendmail &&
+	PATH="$(pwd):$PATH" \
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="!fake.sendmail" \
+		HEAD~2 2>errors &&
+	test_path_is_file commandline1 &&
+	test_path_is_file commandline2
+'
+
 test_expect_success $PREREQ 'invoke hook' '
 	mkdir -p .git/hooks &&