[PATCH] git-send-email: honor $PATH

[PATCH] git-send-email: honor $PATH

[Florian Klink]

This will search $PATH for a sendmail binary, instead of the (previously
fixed) list of paths.

Signed-off-by: Florian Klink <flokli@flokli.de>
---
 Documentation/git-send-email.txt | 5 ++---
 git-send-email.perl              | 3 ++-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index bac9014ac..b9b1f2c41 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -203,9 +203,8 @@ a password is obtained using 'git-credential'.
 	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 `/usr/sbin/sendmail` or
-	`/usr/lib/sendmail` if such program is available, or
-	`localhost` otherwise.
+	option; the built-in default is to search in $PATH if such program is
+	available, or `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 2208dcc21..8e357aeab 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -885,7 +885,8 @@ if (defined $initial_reply_to) {
 }
 
 if (!defined $smtp_server) {
-	foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
+	my @sendmail_paths = map {"$_/sendmail"} split /:/, $ENV{PATH};
+	foreach (@sendmail_paths) {
 		if (-x $_) {
 			$smtp_server = $_;
 			last;
-- 
2.15.0

Re: [PATCH] git-send-email: honor $PATH

[brian m. carlson]

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

On Sat, Nov 18, 2017 at 01:42:49PM +0100, Florian Klink wrote:
> This will search $PATH for a sendmail binary, instead of the (previously
> fixed) list of paths.
> 
> Signed-off-by: Florian Klink <flokli@flokli.de>
> ---
>  Documentation/git-send-email.txt | 5 ++---
>  git-send-email.perl              | 3 ++-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index bac9014ac..b9b1f2c41 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -203,9 +203,8 @@ a password is obtained using 'git-credential'.
>  	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 `/usr/sbin/sendmail` or
> -	`/usr/lib/sendmail` if such program is available, or
> -	`localhost` otherwise.
> +	option; the built-in default is to search in $PATH if such program is
> +	available, or `localhost` otherwise.

This patch adds support for PATH, but it also removes the fixed paths.
On many systems, unprivileged users don't have /usr/sbin in their PATH,
and I know of no systems which provide /usr/lib as a PATH value.
Therefore, it's possible that this change will break automatic detection
of sendmail for many users.

I think what you probably want to do is use entries in PATH first, and
leave the two old values as backups at the end.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

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

Re: [PATCH] git-send-email: honor $PATH

[Florian Klink]

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

>This patch adds support for PATH, but it also removes the fixed paths.
>On many systems, unprivileged users don't have /usr/sbin in their PATH,
>and I know of no systems which provide /usr/lib as a PATH value.
>Therefore, it's possible that this change will break automatic detection
>of sendmail for many users.
>
>I think what you probably want to do is use entries in PATH first, and
>leave the two old values as backups at the end.

You're perfectly right - forgot about /usr/sbin not in PATH for unprivileged
users. Will send a new patch which appends to the original list.

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

[PATCH v2] git-send-email: honor $PATH for sendmail binary

[Florian Klink]

This extends git-send-email to also consider sendmail binaries in $PATH,
in addition to the (fixed) list of /usr/sbin and /usr/lib.fixed) list of
paths.

Signed-off-by: Florian Klink <flokli@flokli.de>
---
 Documentation/git-send-email.txt | 6 +++---
 git-send-email.perl              | 4 +++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index bac9014ac..7af48f8eb 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -203,9 +203,9 @@ a password is obtained using 'git-credential'.
 	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 `/usr/sbin/sendmail` or
-	`/usr/lib/sendmail` if such program is available, or
-	`localhost` otherwise.
+	option; the built-in default is to search in $PATH,
+	then /usr/sbin and /usr/lib/sendmail afterwards 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 2208dcc21..570f04079 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -885,7 +885,9 @@ if (defined $initial_reply_to) {
 }
 
 if (!defined $smtp_server) {
-	foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
+	my @sendmail_paths = map {"$_/sendmail"} split /:/, $ENV{PATH};
+	push @sendmail_paths, qw( /usr/sbin/sendmail /usr/lib/sendmail );
+	foreach (@sendmail_paths) {
 		if (-x $_) {
 			$smtp_server = $_;
 			last;
-- 
2.15.0

Re: [PATCH] git-send-email: honor $PATH

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> This patch adds support for PATH, but it also removes the fixed paths.
> On many systems, unprivileged users don't have /usr/sbin in their PATH,
> and I know of no systems which provide /usr/lib as a PATH value.
> Therefore, it's possible that this change will break automatic detection
> of sendmail for many users.

It is more than possible ;-) that this change alone is a regression.

> I think what you probably want to do is use entries in PATH first, and
> leave the two old values as backups at the end.

I do not think it would make things worse if the change were to do
the two standard places first and then try elements on the $PATH;
split of $PATH needs to be done carefully (Windows?), though.  

I would feel a lot more worried about trying elements on the $PATH
first and then using the two standard places as fallback.  If the
order of addition matters at all, that would mean that trying
elements on $PATH first and then falling back to the two standard
places *will* change the behaviour---for the affected users, we used
to pick one of these two, but now we would pick something different.
sendmail is usually installed out of the way of $PATH for regular
users for a reason, so picking anything whose name happens to be
sendmail that is on $PATH does not sound right.

Of course, for users who do not have sendmail at one of the two
standard places _and_ has one on one of the directories on $PATH,
the order in which we check would not make a difference, so my
suggestion would be to do the other way around.

Re: [PATCH] git-send-email: honor $PATH

[Florian Klink]

On 17-11-19 10:04:58, Junio C Hamano wrote:
>"brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> This patch adds support for PATH, but it also removes the fixed paths.
>> On many systems, unprivileged users don't have /usr/sbin in their PATH,
>> and I know of no systems which provide /usr/lib as a PATH value.
>> Therefore, it's possible that this change will break automatic detection
>> of sendmail for many users.
>
>It is more than possible ;-) that this change alone is a regression.
>
>> I think what you probably want to do is use entries in PATH first, and
>> leave the two old values as backups at the end.
>
>I do not think it would make things worse if the change were to do
>the two standard places first and then try elements on the $PATH;
>split of $PATH needs to be done carefully (Windows?), though.

Support to detect sendmail binaries in windows' PATH seems a bit more complex.
The separator is different, and PATHEXT would need to be considered too.  I'm
not even sure if having a sendmail binary in PATH on windows is something usual
or if defaulting to smtp to localhost (what we currently do) is good enough (tm).
If we want to start parsing PATH under windows too, I'd suggest to use
File::Which instead of implementing it on our own.

>I would feel a lot more worried about trying elements on the $PATH
>first and then using the two standard places as fallback.  If the
>order of addition matters at all, that would mean that trying
>elements on $PATH first and then falling back to the two standard
>places *will* change the behaviour---for the affected users, we used
>to pick one of these two, but now we would pick something different.
>sendmail is usually installed out of the way of $PATH for regular
>users for a reason, so picking anything whose name happens to be
>sendmail that is on $PATH does not sound right.
>
>Of course, for users who do not have sendmail at one of the two
>standard places _and_ has one on one of the directories on $PATH,
>the order in which we check would not make a difference, so my
>suggestion would be to do the other way around.

I could happily provide a patch that does it the other way round, too. But let's
first decide on what to do with windows ;-)

Cheers,
Florian

Re: [PATCH] git-send-email: honor $PATH

[Florian Klink]

>Support to detect sendmail binaries in windows' PATH seems a bit more complex.
>The separator is different, and PATHEXT would need to be considered too.  I'm
>not even sure if having a sendmail binary in PATH on windows is something usual
>or if defaulting to smtp to localhost (what we currently do) is good enough (tm).
>If we want to start parsing PATH under windows too, I'd suggest to use
>File::Which instead of implementing it on our own.
>
>>I would feel a lot more worried about trying elements on the $PATH
>>first and then using the two standard places as fallback.  If the
>>order of addition matters at all, that would mean that trying
>>elements on $PATH first and then falling back to the two standard
>>places *will* change the behaviour---for the affected users, we used
>>to pick one of these two, but now we would pick something different.
>>sendmail is usually installed out of the way of $PATH for regular
>>users for a reason, so picking anything whose name happens to be
>>sendmail that is on $PATH does not sound right.
>>
>>Of course, for users who do not have sendmail at one of the two
>>standard places _and_ has one on one of the directories on $PATH,
>>the order in which we check would not make a difference, so my
>>suggestion would be to do the other way around.
>
>I could happily provide a patch that does it the other way round, too. But let's
>first decide on what to do with windows ;-)

Seems like there is not really much of motivation to try better in detecting
sendmail binaries in PATH on windows ;-)

Will send patch v3, which reverses the order as suggested by Junio shortly.

-- 
Florian

[PATCH v3] git-send-email: honor $PATH for sendmail binary

[Florian Klink]

This extends git-send-email to also consider sendmail binaries in $PATH
after checking the (fixed) list of /usr/sbin and /usr/lib, and before
falling back to localhost.

Signed-off-by: Florian Klink <flokli@flokli.de>
---
 Documentation/git-send-email.txt | 6 +++---
 git-send-email.perl              | 4 +++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index bac9014ac..44db25567 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -203,9 +203,9 @@ a password is obtained using 'git-credential'.
 	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 `/usr/sbin/sendmail` or
-	`/usr/lib/sendmail` if such program is available, or
-	`localhost` otherwise.
+	option; the built-in default is to search for `sendmail` in
+	`/usr/sbin`, `/usr/lib/sendmail` 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 2208dcc21..edcc6d346 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -885,7 +885,9 @@ if (defined $initial_reply_to) {
 }
 
 if (!defined $smtp_server) {
-	foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
+	my @sendmail_paths = qw( /usr/sbin/sendmail /usr/lib/sendmail );
+	push @sendmail_paths, map {"$_/sendmail"} split /:/, $ENV{PATH};
+	foreach (@sendmail_paths) {
 		if (-x $_) {
 			$smtp_server = $_;
 			last;
-- 
2.15.0

Re: [PATCH v3] git-send-email: honor $PATH for sendmail binary

[Junio C Hamano]

Florian Klink <flokli@flokli.de> writes:

> This extends git-send-email to also consider sendmail binaries in $PATH
> after checking the (fixed) list of /usr/sbin and /usr/lib, and before
> falling back to localhost.
>
> Signed-off-by: Florian Klink <flokli@flokli.de>
> ---

Thanks for an update.

In an ideal world where we were introducing git-send-email for the
first time without any existing users, we would certainly prefer
things on directories listed in $PATH, and use the two traditional
hardcoded places merely as fallback, but because we do have existing
users who have been relying on the code finding /usr/lib/sendmail
(even when they have something called 'sendmail' that they do not
want to use on their $PATH) and doing that ideal implementation
would break things for them.  Those who have /usr/lib/sendmail
installed that they do not want to use can continue to use
sendemail.smtpserver---if $PATH were searched first, they could
instead list the path that has their faviourite sendmail on it
without setting the configuration, but it does not change the fact
that they need to do _something_ anyway, so it is not too huge a
deal.

>  Documentation/git-send-email.txt | 6 +++---
>  git-send-email.perl              | 4 +++-
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index bac9014ac..44db25567 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -203,9 +203,9 @@ a password is obtained using 'git-credential'.
>  	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 `/usr/sbin/sendmail` or
> -	`/usr/lib/sendmail` if such program is available, or
> -	`localhost` otherwise.
> +	option; the built-in default is to search for `sendmail` in
> +	`/usr/sbin`, `/usr/lib/sendmail` and $PATH if such program is
> +	available, falling back to `localhost` otherwise.

"search for `sendmail` in `/usr/sbin`, `/usr/lib/sendmail`" would
mean we would not be happy with /usr/lib/sendmail but would be with
either /usr/sbin/sendmail or /usr/lib/sendmail/sendmail, which is
not what you wanted to say.  I'd do 's|/usr/lib/sendmail|/usr/lib|'
while queueing.

Thanks again.

>  --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 2208dcc21..edcc6d346 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -885,7 +885,9 @@ if (defined $initial_reply_to) {
>  }
>  
>  if (!defined $smtp_server) {
> -	foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
> +	my @sendmail_paths = qw( /usr/sbin/sendmail /usr/lib/sendmail );
> +	push @sendmail_paths, map {"$_/sendmail"} split /:/, $ENV{PATH};
> +	foreach (@sendmail_paths) {
>  		if (-x $_) {
>  			$smtp_server = $_;
>  			last;