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

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

[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.

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

Remove the PATH lookup, since Perl's exec() does this already.

Thanks to Jeff King for the assist.

 git-send-email.perl | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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 $!;
-- 
2.31.1.576.g06d230b258

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

[Jeff King]

On Tue, May 11, 2021 at 01:15:11PM -0600, Gregory Anders wrote:

> 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.

This tells us "what", but the commit message is a good place to describe
"why". That helps reviewers now understand what you're trying to
accomplish, and why this is a good way to do it rather than some other
patch.

IMHO the most important "why" here is that there currently is no way to
specify a local smtp server program without using a full path.

I think this is a good direction to fix it, though for anybody just
seeing this patch, I'd call attention to the nearby thread (and the one
it links to):

  https://lore.kernel.org/git/YJrH8uqzapnpNEsb@gpanders.com/

>  git-send-email.perl | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

We'd probably want a test here, as well; see t/t9001-send-email.sh.

We implicitly test the absolute-path behavior in that script because
pass "--smtp-server=$(pwd)/fake.sendmail" in lots of places. But we'd
probably want a new test block that checks that:

  PATH=$(pwd):$PATH \
  git send-email --smtp-server="!fake.sendmail"

does what you expect.

-Peff

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

[Felipe Contreras]

Gregory Anders wrote:
> --- 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 =~ /^!/) {

Is there any particular reason why we are checking for an absolute path?

-- 
Felipe Contreras

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

[Gregory Anders]

Presently, the smtpServer config option can use a sendmail-like program
by providing an absolute path to the program as the value for this
option. 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 the smtpServer config option to be
prefixed with a ! character that implements the above behavior; namely,
the specified value (sans the ! character) is used as if the user had
entered it directly on the command line.

See also https://lore.kernel.org/git/YJrH8uqzapnpNEsb@gpanders.com/.

Signed-off-by: Gregory Anders <greg@gpanders.com>
---
 Documentation/git-send-email.txt | 13 +++++++------
 git-send-email.perl              |  7 +++++--
 t/t9001-send-email.sh            | 10 ++++++++++
 3 files changed, 22 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.
 
 --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..14cc61ace7 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2225,4 +2225,14 @@ test_expect_success $PREREQ 'test forbidSendmailVariables behavior override' '
 		HEAD^
 '
 
+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" \
+		$patches 2>errors
+'
+
 test_done
-- 
2.31.1.576.g2f8a831619

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

[Gregory Anders]

On Tue, 11 May 2021 15:03 -0500, Felipe Contreras wrote:
>Is there any particular reason why we are checking for an absolute 
>path?

The existing behavior is that the value of smtpServer is a hostname of a 
server, *unless* it is an absolute path in which case it is interpreted 
as a program to use to send the mail (e.g. a sendmail alternative or 
msmtp).

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

[Jeff King]

On Tue, May 11, 2021 at 02:40:44PM -0600, Gregory Anders wrote:

> Presently, the smtpServer config option can use a sendmail-like program
> by providing an absolute path to the program as the value for this
> option. 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 the smtpServer config option to be
> prefixed with a ! character that implements the above behavior; namely,
> the specified value (sans the ! character) is used as if the user had
> entered it directly on the command line.

Thanks, this justifies the change quite nicely, I think.

> See also https://lore.kernel.org/git/YJrH8uqzapnpNEsb@gpanders.com/.

IMHO this link could be dropped. I referenced it earlier in the thread
since there wasn't much context in your commit message. In general we
prefer to copy that context into the commit, so it doesn't depend on
having access to the list. And you already did that quite nicely, so the
link tells us nothing new. :)

> 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.

Ah, thanks for remembering to update the documentation. This looks good.

> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 65b3035371..14cc61ace7 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -2225,4 +2225,14 @@ test_expect_success $PREREQ 'test forbidSendmailVariables behavior override' '
>  		HEAD^
>  '
>  
> +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" \
> +		$patches 2>errors
> +'

How do we know that we successfully ran our fake.sendmail script? I
guess probably send-email would error out, but should we confirm that it
touched commandline1, etc?

It also seems to fail for me. I think $patches isn't valid at this point
in the script, as the patch files have been removed. Switching to just
HEAD~2 doesn't seem to work either, as earlier tests set up a hook which
rejects it. I think we could pass HEAD^ to send just one path, but I
thought it would be nice to confirm that sending multiple works (i.e.,
avoiding the bug we discussed earlier).

I think the existing hook tests are somewhat poor to leave this
confusing state. But the easiest thing to me is to just bump our test up
a bit, like:

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 14cc61ace7..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 &&
 
@@ -2225,14 +2237,4 @@ test_expect_success $PREREQ 'test forbidSendmailVariables behavior override' '
 		HEAD^
 '
 
-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" \
-		$patches 2>errors
-'
-
 test_done

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

[Felipe Contreras]

Gregory Anders wrote:
> On Tue, 11 May 2021 15:03 -0500, Felipe Contreras wrote:
> >Is there any particular reason why we are checking for an absolute 
> >path?
> 
> The existing behavior is that the value of smtpServer is a hostname of a 
> server, *unless* it is an absolute path in which case it is interpreted 
> as a program to use to send the mail (e.g. a sendmail alternative or 
> msmtp).

I see. That's unfortunate.

It would be much better to have sendemail.program, and then we wouldn't
needto deal with these workarounds.

-- 
Felipe Contreras

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

[Gregory Anders]

On Tue, 11 May 2021 17:39 -0400, Jeff King wrote:
>It also seems to fail for me. I think $patches isn't valid at this 
>point in the script, as the patch files have been removed. Switching to 
>just HEAD~2 doesn't seem to work either, as earlier tests set up a hook 
>which rejects it. I think we could pass HEAD^ to send just one path, 
>but I thought it would be nice to confirm that sending multiple works 
>(i.e., avoiding the bug we discussed earlier).

I had to play around with this too (being unfamiliar with how this test 
harness worked). It would pass if I ran the first ~20 tests (20 is not 
significant, it's just a number I chose to ensure that whatever 
necessary setup needed to happen was done), but would fail otherwise. 
Moving the test up seems fine, I put it at the bottom simply because I 
wasn't sure where else to put it.

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

[Gregory Anders]

On Tue, 11 May 2021 17:07 -0500, Felipe Contreras wrote:
>It would be much better to have sendemail.program, and then we wouldn't
>need to deal with these workarounds.

Frankly I agree. Should I modify my patch to add this new option instead 
of modifying the behavior of smtpServer? Obviously the smtpServer option 
would need to preserve its current behavior for backward compatibility.

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

[Junio C Hamano]

Gregory Anders <greg@gpanders.com> writes:

> Presently, the smtpServer config option can use a sendmail-like program

s/Presently, t/T/;  Also if I understand it correctly this is not
just about configuration varible but equally applies to the command
line option --smtp-server, so mention both of them in their full
name, i.e.

    The sendemail.smtpServer configuration variable and the
    "--smtp-server" command line option can name a program to use by
    providing ...

> by providing an absolute path to the program as the value for this
> option. 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.

Nicely explained.

> To that end, this patch allows the smtpServer config option to be
> prefixed with a ! character that implements the above behavior; namely,
> the specified value (sans the ! character) is used as if the user had
> entered it directly on the command line.

	Allow the value of the configuration variable and the
	command line option to be prefixed with a '!' to signal that
	the specified command needs to be looked up on $PATH.

> See also https://lore.kernel.org/git/YJrH8uqzapnpNEsb@gpanders.com/.

You summarised the problem description in that message pretty well
in the proposed log message (which is much better than leaving only
a URL and forcing readers to go there), and I suspect there aren't
much readers can learn from by seeing also the article, though.

> +	} 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 $!;
>  		}

Looking good.

Thanks.

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

[Gregory Anders]

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

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 &&
 
-- 
2.31.1

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3159 bytes --]

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.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Jeff King]

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

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

[Gregory Anders]

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"

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

[Jeff King]

On Tue, May 11, 2021 at 04:19:46PM -0600, Gregory Anders wrote:

> On Tue, 11 May 2021 17:07 -0500, Felipe Contreras wrote:
> > It would be much better to have sendemail.program, and then we wouldn't
> > need to deal with these workarounds.
> 
> Frankly I agree. Should I modify my patch to add this new option instead of
> modifying the behavior of smtpServer? Obviously the smtpServer option would
> need to preserve its current behavior for backward compatibility.

Yeah, that was mentioned in the thread I linked earlier. I think it
would be a fine solution, too. It would probably make sense for it to
use the shell, as suggested elsewhere, and to call it "smtp-command" for
consistency with other parts of Git (I'm thinking particularly of
GIT_SSH versus GIT_SSH_COMMAND, where the latter was introduced to fix
the defect that the former could not provide any arguments).

-Peff

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

[Jeff King]

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2072 bytes --]

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.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Felipe Contreras]

Jeff King wrote:
> On Tue, May 11, 2021 at 04:19:46PM -0600, Gregory Anders wrote:
> > Frankly I agree. Should I modify my patch to add this new option instead of
> > modifying the behavior of smtpServer? Obviously the smtpServer option would
> > need to preserve its current behavior for backward compatibility.
> 
> Yeah, that was mentioned in the thread I linked earlier. I think it
> would be a fine solution, too. It would probably make sense for it to
> use the shell, as suggested elsewhere, and to call it "smtp-command" for
> consistency with other parts of Git (I'm thinking particularly of
> GIT_SSH versus GIT_SSH_COMMAND, where the latter was introduced to fix
> the defect that the former could not provide any arguments).

But it would be "smtpserver-command", and I don't think that the best
naming, because it doesn't necessarily have anything to do with SMTP, or
a server.

I think simply sendemail.command is perfectly fine.

-- 
Felipe Contreras

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

[Jeff King]

On Tue, May 11, 2021 at 08:08:50PM -0500, Felipe Contreras wrote:

> > Yeah, that was mentioned in the thread I linked earlier. I think it
> > would be a fine solution, too. It would probably make sense for it to
> > use the shell, as suggested elsewhere, and to call it "smtp-command" for
> > consistency with other parts of Git (I'm thinking particularly of
> > GIT_SSH versus GIT_SSH_COMMAND, where the latter was introduced to fix
> > the defect that the former could not provide any arguments).
> 
> But it would be "smtpserver-command", and I don't think that the best
> naming, because it doesn't necessarily have anything to do with SMTP, or
> a server.
> 
> I think simply sendemail.command is perfectly fine.

Aren't there many other "commands" run by send-email, like --to-cmd and
--cc-cmd? It probably should indicate somehow that it is the command for
sending mail. I agree it does not have to say "SMTP". If it is meant to
be compatible with sendmail, then maybe "sendemail.sendmailCommand" and
"--sendmail-cmd" would work.

-Peff

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

[Felipe Contreras]

Jeff King wrote:
> On Tue, May 11, 2021 at 08:08:50PM -0500, Felipe Contreras wrote:
> 
> > > Yeah, that was mentioned in the thread I linked earlier. I think it
> > > would be a fine solution, too. It would probably make sense for it to
> > > use the shell, as suggested elsewhere, and to call it "smtp-command" for
> > > consistency with other parts of Git (I'm thinking particularly of
> > > GIT_SSH versus GIT_SSH_COMMAND, where the latter was introduced to fix
> > > the defect that the former could not provide any arguments).
> > 
> > But it would be "smtpserver-command", and I don't think that the best
> > naming, because it doesn't necessarily have anything to do with SMTP, or
> > a server.
> > 
> > I think simply sendemail.command is perfectly fine.
> 
> Aren't there many other "commands" run by send-email, like --to-cmd and
> --cc-cmd? It probably should indicate somehow that it is the command for
> sending mail. I agree it does not have to say "SMTP". If it is meant to
> be compatible with sendmail, then maybe "sendemail.sendmailCommand" and
> "--sendmail-cmd" would work.

Yes, although I find sendemail.sendmailCommand awfully redundant.
I would prefer sendemail.mainCommand, but to me sendemail.command
implies it's the main command as opposed to all ther other commands.

Just like there's many presidents in USA (of companies, organizations,
and student unions), but when you say "the president of USA" it's
understood which president you are talking about.

Cheers.

-- 
Felipe Contreras

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

[Gregory Anders]

On Tue, 11 May 2021 20:52 -0500, Felipe Contreras wrote:
>Jeff King wrote:
>> On Tue, May 11, 2021 at 08:08:50PM -0500, Felipe Contreras wrote:
>>
>> > > Yeah, that was mentioned in the thread I linked earlier. I think it
>> > > would be a fine solution, too. It would probably make sense for it to
>> > > use the shell, as suggested elsewhere, and to call it "smtp-command" for
>> > > consistency with other parts of Git (I'm thinking particularly of
>> > > GIT_SSH versus GIT_SSH_COMMAND, where the latter was introduced to fix
>> > > the defect that the former could not provide any arguments).
>> >
>> > But it would be "smtpserver-command", and I don't think that the best
>> > naming, because it doesn't necessarily have anything to do with SMTP, or
>> > a server.
>> >
>> > I think simply sendemail.command is perfectly fine.
>>
>> Aren't there many other "commands" run by send-email, like --to-cmd and
>> --cc-cmd? It probably should indicate somehow that it is the command for
>> sending mail. I agree it does not have to say "SMTP". If it is meant to
>> be compatible with sendmail, then maybe "sendemail.sendmailCommand" and
>> "--sendmail-cmd" would work.
>
>Yes, although I find sendemail.sendmailCommand awfully redundant.
>I would prefer sendemail.mainCommand, but to me sendemail.command
>implies it's the main command as opposed to all ther other commands.
>
>Just like there's many presidents in USA (of companies, organizations,
>and student unions), but when you say "the president of USA" it's
>understood which president you are talking about.
>
>Cheers.
>
>-- 
>Felipe Contreras

I agree with Jeff here. While I also find sendemail.sendmailCommand 
redundant, it makes more sense when used as a command line option:

     git send-email --sendmail-cmd <cmd>

Conversely, `--command` is more ambiguous and less clear. Explicitly 
using `sendmailCommand` makes it clear that the user is specifying a 
command that is compatible with the `sendmail` program.

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

[Junio C Hamano]

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".

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

[Felipe Contreras]

Gregory Anders wrote:
> On Tue, 11 May 2021 20:52 -0500, Felipe Contreras wrote:
> >> > I think simply sendemail.command is perfectly fine.
> >>
> >> Aren't there many other "commands" run by send-email, like --to-cmd and
> >> --cc-cmd? It probably should indicate somehow that it is the command for
> >> sending mail. I agree it does not have to say "SMTP". If it is meant to
> >> be compatible with sendmail, then maybe "sendemail.sendmailCommand" and
> >> "--sendmail-cmd" would work.
> >
> >Yes, although I find sendemail.sendmailCommand awfully redundant.
> >I would prefer sendemail.mainCommand, but to me sendemail.command
> >implies it's the main command as opposed to all ther other commands.
> >
> >Just like there's many presidents in USA (of companies, organizations,
> >and student unions), but when you say "the president of USA" it's
> >understood which president you are talking about.
> 
> I agree with Jeff here. While I also find sendemail.sendmailCommand 
> redundant, it makes more sense when used as a command line option:
> 
>      git send-email --sendmail-cmd <cmd>
> 
> Conversely, `--command` is more ambiguous and less clear. Explicitly 
> using `sendmailCommand` makes it clear that the user is specifying a 
> command that is compatible with the `sendmail` program.

So would `git send-email --sendmail <cmd>`.

To make it clear, I think the options are:

 1. `git send-email --sendmail <cmd>`: `sendemail.sendmail`
 2. `git send-email --command <cmd>`: `sendemail.command`

I lean towards #2 since I think most people do a configuration and
forget about it it; they will rarely use the --command argument.

What I'm really against is:

  sendemail.sendmail-command.sendmail = sendmail

Cheers.

-- 
Felipe Contreras