| Message ID | 20210511234935.65147-1-greg@gpanders.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] git-send-email: use ! to indicate relative path to command | expand |
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.
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
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"
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
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.
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 --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 &&
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(-)