From cd5b33fbdc0299765278d0b607b64202603f0882 Mon Sep 17 00:00:00 2001 From: Gregory Anders Date: Fri, 14 May 2021 09:15:53 -0600 Subject: git-send-email: add option to specify sendmail command The sendemail.smtpServer configuration option and --smtp-server command line option both support using a sendmail-like program to send emails by specifying an absolute file path. However, this is not ideal for the following reasons: 1. It overloads the meaning of smtpServer (now a program is being used for the server?) 2. It doesn't allow for non-absolute paths, arguments, or arbitrary scripting Requiring an absolute path is bad for portability, as the same program may be in different locations on different systems. If a user wishes to pass arguments to their program, they have to use the smtpServerOption option, which is cumbersome (as it must be repeated for each option) and doesn't adhere to normal git conventions. Introduce a new configuration option sendemail.sendmailCmd as well as a command line option --sendmail-cmd that can be used to specify a command (with or without arguments) or shell expression to run to send email. The name of this option is consistent with --to-cmd and --cc-cmd. This invocation honors the user's $PATH so that absolute paths are not necessary. Arbitrary shell expressions are also supported, allowing users to do basic scripting. Give this option a higher precedence over --smtp-server and sendemail.smtpServer, as the new interface is more flexible. For backward compatibility, continue to support absolute paths in --smtp-server and sendemail.smtpServer. Signed-off-by: Gregory Anders Signed-off-by: Junio C Hamano --- git-send-email.perl | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index 1f425c0809..d9cff04067 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -70,6 +70,7 @@ sub usage { Sending: --envelope-sender * Email envelope sender. + --sendmail-cmd * Command to run to send email. --smtp-server * Outgoing SMTP server to use. The port is optional. Default 'localhost'. --smtp-server-option * Outgoing SMTP server option to use. @@ -243,6 +244,7 @@ sub do_edit { my (@suppress_cc); my ($auto_8bit_encoding); my ($compose_encoding); +my ($sendmail_cmd); # Variables with corresponding config settings & hardcoded defaults my ($debug_net_smtp) = 0; # Net::SMTP, see send_message() my $thread = 1; @@ -290,6 +292,7 @@ sub do_edit { "assume8bitencoding" => \$auto_8bit_encoding, "composeencoding" => \$compose_encoding, "transferencoding" => \$target_xfer_encoding, + "sendmailcmd" => \$sendmail_cmd, ); my %config_path_settings = ( @@ -423,6 +426,7 @@ sub read_config { "no-bcc" => \$no_bcc, "chain-reply-to!" => \$chain_reply_to, "no-chain-reply-to" => sub {$chain_reply_to = 0}, + "sendmail-cmd=s" => \$sendmail_cmd, "smtp-server=s" => \$smtp_server, "smtp-server-option=s" => \@smtp_server_options, "smtp-server-port=s" => \$smtp_server_port, @@ -996,16 +1000,19 @@ sub expand_one_alias { $reply_to = sanitize_address($reply_to); } -if (!defined $smtp_server) { +if (!defined $sendmail_cmd && !defined $smtp_server) { 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 = $_; + $sendmail_cmd = $_; last; } } - $smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug* + + if (!defined $sendmail_cmd) { + $smtp_server = 'localhost'; # could be 127.0.0.1, too... *shrug* + } } if ($compose && $compose > 0) { @@ -1485,11 +1492,17 @@ sub send_message { if ($dry_run) { # We don't want to send the email. - } elsif (file_name_is_absolute($smtp_server)) { + } elsif (defined $sendmail_cmd || file_name_is_absolute($smtp_server)) { my $pid = open my $sm, '|-'; defined $pid or die $!; if (!$pid) { - exec($smtp_server, @sendmail_parameters) or die $!; + if (defined $sendmail_cmd) { + exec ("sh", "-c", "$sendmail_cmd \"\$@\"", "-", @sendmail_parameters) + or die $!; + } else { + exec ($smtp_server, @sendmail_parameters) + or die $!; + } } print $sm "$header\n$message"; close $sm or die $!; @@ -1585,14 +1598,21 @@ sub send_message { printf($dry_run ? __("Dry-Sent %s\n") : __("Sent %s\n"), $subject); } else { print($dry_run ? __("Dry-OK. Log says:\n") : __("OK. Log says:\n")); - if (!file_name_is_absolute($smtp_server)) { + if (!defined $sendmail_cmd && !file_name_is_absolute($smtp_server)) { print "Server: $smtp_server\n"; print "MAIL FROM:<$raw_from>\n"; foreach my $entry (@recipients) { print "RCPT TO:<$entry>\n"; } } else { - print "Sendmail: $smtp_server ".join(' ',@sendmail_parameters)."\n"; + my $sm; + if (defined $sendmail_cmd) { + $sm = $sendmail_cmd; + } else { + $sm = $smtp_server; + } + + print "Sendmail: $sm ".join(' ',@sendmail_parameters)."\n"; } print $header, "\n"; if ($smtp) { -- cgit 1.2.3-korg From 5b719b7552b9eca486a3a6507e9397572d16481d Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Tue, 25 May 2021 01:14:24 +0200 Subject: send-email: fix missing error message regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a regression with the "the editor exited uncleanly, aborting everything" error message going missing after my d21616c0394 (git-send-email: refactor duplicate $? checks into a function, 2021-04-06). I introduced a $msg variable, but did not actually use it. This caused us to miss the optional error message supplied by the "do_edit" codepath. Fix that, and add tests to check that this works. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 12 +++++++++++- t/t9001-send-email.sh | 23 +++++++++++++++++++++-- 2 files changed, 32 insertions(+), 3 deletions(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index 175da07d94..170f42fe21 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -219,8 +219,18 @@ sub system_or_msg { my $exit_code = $? >> 8; return unless $signalled or $exit_code; + my @sprintf_args = ($args->[0], $exit_code); + if (defined $msg) { + # Quiet the 'redundant' warning category, except we + # need to support down to Perl 5.8, so we can't do a + # "no warnings 'redundant'", since that category was + # introduced in perl 5.22, and asking for it will die + # on older perls. + no warnings; + return sprintf($msg, @sprintf_args); + } return sprintf(__("fatal: command '%s' died with exit code %d"), - $args->[0], $exit_code); + @sprintf_args); } sub system_or_die { diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 65b3035371..2acf389837 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -644,14 +644,33 @@ test_expect_success $PREREQ 'In-Reply-To with --chain-reply-to' ' test_cmp expect actual ' +test_set_editor "$(pwd)/fake-editor" + +test_expect_success $PREREQ 'setup erroring fake editor' ' + write_script fake-editor <<-\EOF + echo >&2 "I am about to error" + exit 1 + EOF +' + +test_expect_success $PREREQ 'fake editor dies with error' ' + clean_fake_sendmail && + test_must_fail git send-email \ + --compose --subject foo \ + --from="Example " \ + --to=nobody@example.com \ + --smtp-server="$(pwd)/fake.sendmail" \ + $patches 2>err && + grep "I am about to error" err && + grep "the editor exited uncleanly, aborting everything" err +' + test_expect_success $PREREQ 'setup fake editor' ' write_script fake-editor <<-\EOF echo fake edit >>"$1" EOF ' -test_set_editor "$(pwd)/fake-editor" - test_expect_success $PREREQ '--compose works' ' clean_fake_sendmail && git send-email \ -- cgit 1.2.3-korg From 7cbc0455cc07702c5eeff1062c7e2a820758714f Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Wed, 26 May 2021 13:21:07 +0200 Subject: send-email: move "hooks_path" invocation to git-send-email.perl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the newly added "hooks_path" API in Git.pm to its only user in git-send-email.perl. This was added in c8243933c74 (git-send-email: Respect core.hooksPath setting, 2021-03-23), meaning that it hasn't yet made it into a non-rc release of git. The consensus with Git.pm is that we need to be considerate of out-of-tree users who treat it as a public documented interface. We should therefore be less willing to add new functionality to it, least we be stuck supporting it after our own uses for it disappear. In this case the git-send-email.perl hook invocation will probably be replaced by a future "git hook run" command, and in the commit preceding this one the "hooks_path" become nothing but a trivial wrapper for "rev-parse --git-path hooks" anyway (with no Cwd::abs_path() call), so let's just inline this command in git-send-email.perl itself. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 3 ++- perl/Git.pm | 12 ------------ 2 files changed, 2 insertions(+), 13 deletions(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index 175da07d94..a454020f01 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1949,7 +1949,8 @@ sub validate_patch { my ($fn, $xfer_encoding) = @_; if ($repo) { - my $validate_hook = catfile($repo->hooks_path(), + my $hooks_path = $repo->command_oneline('rev-parse', '--git-path', 'hooks'); + my $validate_hook = catfile($hooks_path, 'sendemail-validate'); my $hook_error; if (-x $validate_hook) { diff --git a/perl/Git.pm b/perl/Git.pm index df6280ebab..02eacef0c2 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -619,18 +619,6 @@ sub _prompt { sub repo_path { $_[0]->{opts}->{Repository} } -=item hooks_path () - -Return path to the hooks directory. Must be called on a repository instance. - -=cut - -sub hooks_path { - my ($self) = @_; - - my $dir = $self->command_oneline('rev-parse', '--git-path', 'hooks'); - return $dir; -} =item wc_path () -- cgit 1.2.3-korg From ecc4ee9cac60600f38542cebd940b4bd8b6497c8 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 28 May 2021 11:23:40 +0200 Subject: send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add support for the "GIT_TEST_PERL_FATAL_WARNINGS=true" test mode to "send-email". This was added to e.g. git-svn in 5338ed2b26 (perl: check for perl warnings while running tests, 2020-10-21), but not "send-email". Let's rectify that. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index 25be2ebd2a..1630e87cd4 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -18,7 +18,7 @@ use 5.008; use strict; -use warnings; +use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : (); use POSIX qw/strftime/; use Term::ReadLine; use Getopt::Long; -- cgit 1.2.3-korg From 671818ab0b8206481d38054477059fee46db0ba3 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 28 May 2021 11:23:42 +0200 Subject: send-email: remove non-working support for "sendemail.smtpssl" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the already dead code to support "sendemail.smtpssl" by finally removing the dead code supporting the configuration option. In f6bebd121ac (git-send-email: add support for TLS via Net::SMTP::SSL, 2008-06-25) the --smtp-ssl command-line option was documented as deprecated, later in 65180c66186 (List send-email config options in config.txt., 2009-07-22) the "sendemail.smtpssl" configuration option was also documented as such. Then in in 3ff15040e22 (send-email: fix regression in sendemail.identity parsing, 2019-05-17) I unintentionally removed support for it by introducing a bug in read_config(). As can be seen from the diff context we've already returned unless $enc i defined, so it's not possible for us to reach the "elsif" branch here. This code was therefore already dead since Git v2.23.0. So let's just remove it. We were already 11 years into a stated deprecation period of this variable when 3ff15040e22 landed, now it's around 13. Since it hasn't worked anyway for around 2 years it looks like we can safely remove it. The --smtp-ssl option is still deprecated, if someone cares they can follow-up and remove that too, but unlike the config option that one could still be in use in the wild. I'm just removing this code that's provably unused already. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- Documentation/config/sendemail.txt | 3 --- git-send-email.perl | 6 +----- 2 files changed, 1 insertion(+), 8 deletions(-) (limited to 'git-send-email.perl') diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt index cbc5af42fd..50baa5d6bf 100644 --- a/Documentation/config/sendemail.txt +++ b/Documentation/config/sendemail.txt @@ -8,9 +8,6 @@ sendemail.smtpEncryption:: See linkgit:git-send-email[1] for description. Note that this setting is not subject to the 'identity' mechanism. -sendemail.smtpssl (deprecated):: - Deprecated alias for 'sendemail.smtpEncryption = ssl'. - sendemail.smtpsslcertpath:: Path to ca-certificates (either a directory or a single file). Set it to an empty string to disable certificate verification. diff --git a/git-send-email.perl b/git-send-email.perl index 1630e87cd4..9a1a4898e3 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -393,11 +393,7 @@ sub read_config { my $enc = Git::config(@repo, $setting); return unless defined $enc; return if $configured->{$setting}++; - if (defined $enc) { - $smtp_encryption = $enc; - } elsif (Git::config_bool(@repo, "$prefix.smtpssl")) { - $smtp_encryption = 'ssl'; - } + $smtp_encryption = $enc; } } -- cgit 1.2.3-korg From 119974e9e7f3569400aa1950529f69431534a617 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 28 May 2021 11:23:43 +0200 Subject: send-email: refactor sendemail.smtpencryption config parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the removal of the support for sendemail.smtpssl in the preceding commit the parsing of sendemail.smtpencryption is no longer special, and can by moved to %config_settings. This gets us rid of an unconditional call to Git::config(), which as we'll see in subsequent commits matters for startup performance. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index 9a1a4898e3..e2fe112aa5 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -287,6 +287,7 @@ sub do_edit { ); my %config_settings = ( + "smtpencryption" => \$smtp_encryption, "smtpserver" => \$smtp_server, "smtpserverport" => \$smtp_server_port, "smtpserveroption" => \@smtp_server_options, @@ -387,14 +388,6 @@ sub read_config { $$target = $v; } } - - if (!defined $smtp_encryption) { - my $setting = "$prefix.smtpencryption"; - my $enc = Git::config(@repo, $setting); - return unless defined $enc; - return if $configured->{$setting}++; - $smtp_encryption = $enc; - } } # sendemail.identity yields to --identity. We must parse this -- cgit 1.2.3-korg From 2b110e9ba255120a2d78a5d2cccc842519e341f1 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 28 May 2021 11:23:44 +0200 Subject: send-email: copy "config_regxp" into git-send-email.perl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The config_regexp() function was added in dd84e528a3 (git-send-email: die if sendmail.* config is set, 2020-07-23) for use in git-send-email, and it's the only in-tree user of it. However, the consensus is that Git.pm is a public interface, so even though it's a recently added function we can't change it. So let's copy over a minimal version of it to git-send-email.perl itself. In a subsequent commit it'll be changed further for our own use. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index e2fe112aa5..73e3d3fd26 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -390,6 +390,24 @@ sub read_config { } } +sub config_regexp { + my ($regex) = @_; + my @ret; + eval { + @ret = Git::command( + 'config', + '--name-only', + '--get-regexp', + $regex, + ); + 1; + } or do { + # If we have no keys we're OK, otherwise re-throw + die $@ if $@->value != 1; + }; + return @ret; +} + # sendemail.identity yields to --identity. We must parse this # special-case first before the rest of the config is read. $identity = Git::config(@repo, "sendemail.identity"); @@ -488,7 +506,7 @@ sub read_config { usage(); } -if ($forbid_sendmail_variables && (scalar Git::config_regexp("^sendmail[.]")) != 0) { +if ($forbid_sendmail_variables && (scalar config_regexp("^sendmail[.]")) != 0) { die __("fatal: found configuration options for 'sendmail'\n" . "git-send-email is configured with the sendemail.* options - note the 'e'.\n" . "Set sendemail.forbidSendmailVariables to false to disable this check.\n"); -- cgit 1.2.3-korg From 9264d29bf6da9e1789d71807dd72bbd4fb447887 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 28 May 2021 11:23:45 +0200 Subject: send-email: lazily load config for a big speedup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reduce the time it takes git-send-email to get to even the most trivial of tasks (such as serving up its "-h" output) by first listing config keys that exist, and only then only call e.g. "git config --bool" on them if they do. Over a lot of runs this speeds the time to "-h" up for me from ~250ms to ~150ms, and the runtime of t9001-send-email.sh goes from ~25s to ~20s. This introduces a race condition where we'll do the "wrong" thing if a config key were to be inserted between us discovering the list and calling read_config(), i.e. we won't know about the racily added key. In theory this is a change in behavior, in practice it doesn't matter. The config_regexp() function being changed here was added in dd84e528a34 (git-send-email: die if sendmail.* config is set, 2020-07-23) for use by git-send-email. So we can change its odd return value in the case where no values are found by "git config". The difference in the *.pm code would matter if it was invoked in scalar context, but now it no longer is. Arguably this caching belongs in Git.pm itself, but in lieu of modifying it for all its callers let's only do this for "git send-email". The other big potential win would be "git svn", but unlike "git send-email" it doesn't check tens of config variables one at a time at startup (in my brief testing it doesn't check any). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index 73e3d3fd26..de62cbf250 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -347,11 +347,13 @@ sub signal_handler { # Read our sendemail.* config sub read_config { - my ($configured, $prefix) = @_; + my ($known_keys, $configured, $prefix) = @_; foreach my $setting (keys %config_bool_settings) { my $target = $config_bool_settings{$setting}; - my $v = Git::config_bool(@repo, "$prefix.$setting"); + my $key = "$prefix.$setting"; + next unless exists $known_keys->{$key}; + my $v = Git::config_bool(@repo, $key); next unless defined $v; next if $configured->{$setting}++; $$target = $v; @@ -359,8 +361,10 @@ sub read_config { foreach my $setting (keys %config_path_settings) { my $target = $config_path_settings{$setting}; + my $key = "$prefix.$setting"; + next unless exists $known_keys->{$key}; if (ref($target) eq "ARRAY") { - my @values = Git::config_path(@repo, "$prefix.$setting"); + my @values = Git::config_path(@repo, $key); next unless @values; next if $configured->{$setting}++; @$target = @values; @@ -375,14 +379,16 @@ sub read_config { foreach my $setting (keys %config_settings) { my $target = $config_settings{$setting}; + my $key = "$prefix.$setting"; + next unless exists $known_keys->{$key}; if (ref($target) eq "ARRAY") { - my @values = Git::config(@repo, "$prefix.$setting"); + my @values = Git::config(@repo, $key); next unless @values; next if $configured->{$setting}++; @$target = @values; } else { - my $v = Git::config(@repo, "$prefix.$setting"); + my $v = Git::config(@repo, $key); next unless defined $v; next if $configured->{$setting}++; $$target = $v; @@ -408,9 +414,20 @@ sub config_regexp { return @ret; } +# Save ourselves a lot of work of shelling out to 'git config' (it +# parses 'bool' etc.) by only doing so for config keys that exist. +my %known_config_keys; +{ + my @known_config_keys = config_regexp("^sende?mail[.]"); + @known_config_keys{@known_config_keys} = (); +} + # sendemail.identity yields to --identity. We must parse this # special-case first before the rest of the config is read. -$identity = Git::config(@repo, "sendemail.identity"); +{ + my $key = "sendemail.identity"; + $identity = Git::config(@repo, $key) if exists $known_config_keys{$key}; +} my $rc = GetOptions( "identity=s" => \$identity, "no-identity" => \$no_identity, @@ -421,8 +438,8 @@ sub config_regexp { # Now we know enough to read the config { my %configured; - read_config(\%configured, "sendemail.$identity") if defined $identity; - read_config(\%configured, "sendemail"); + read_config(\%known_config_keys, \%configured, "sendemail.$identity") if defined $identity; + read_config(\%known_config_keys, \%configured, "sendemail"); } # Begin by accumulating all the variables (defined above), that we will end up @@ -506,7 +523,7 @@ sub config_regexp { usage(); } -if ($forbid_sendmail_variables && (scalar config_regexp("^sendmail[.]")) != 0) { +if ($forbid_sendmail_variables && grep { /^sendmail/s } keys %known_config_keys) { die __("fatal: found configuration options for 'sendmail'\n" . "git-send-email is configured with the sendemail.* options - note the 'e'.\n" . "Set sendemail.forbidSendmailVariables to false to disable this check.\n"); -- cgit 1.2.3-korg From fef381e6dcbc328c4ccabaf4e8d4cdf19d1aba00 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 28 May 2021 11:23:46 +0200 Subject: send-email: lazily shell out to "git var" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Optimize git-send-email by only shelling out to "git var" if we need to. This is easily done by re-inventing our own small version of perl's Memoize module. I suppose I could just use Memoize itself, but in a subsequent patch I'll be micro-optimizing send-email's use of dependencies. Using Memoize is a measly extra 5-10 milliseconds, but as we'll see that'll end up mattering for us in the end. This brings the runtime of a plain "send-email" from around ~160-170ms to ~140m-150ms. The runtime of the tests is around the same, or around ~20s. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index de62cbf250..38408ec75a 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -588,8 +588,18 @@ sub config_regexp { } my ($repoauthor, $repocommitter); -($repoauthor) = Git::ident_person(@repo, 'author'); -($repocommitter) = Git::ident_person(@repo, 'committer'); +{ + my %cache; + my ($author, $committer); + my $common = sub { + my ($what) = @_; + return $cache{$what} if exists $cache{$what}; + ($cache{$what}) = Git::ident_person(@repo, $what); + return $cache{$what}; + }; + $repoauthor = sub { $common->('author') }; + $repocommitter = sub { $common->('committer') }; +} sub parse_address_line { return map { $_->format } Mail::Address->parse($_[0]); @@ -777,7 +787,7 @@ sub get_patch_subject { or die sprintf(__("Failed to open for writing %s: %s"), $compose_filename, $!); - my $tpl_sender = $sender || $repoauthor || $repocommitter || ''; + my $tpl_sender = $sender || $repoauthor->() || $repocommitter->() || ''; my $tpl_subject = $initial_subject || ''; my $tpl_in_reply_to = $initial_in_reply_to || ''; my $tpl_reply_to = $reply_to || ''; @@ -983,7 +993,7 @@ sub file_declares_8bit_cte { $sender =~ s/^\s+|\s+$//g; ($sender) = expand_aliases($sender); } else { - $sender = $repoauthor || $repocommitter || ''; + $sender = $repoauthor->() || $repocommitter->() || ''; } # $sender could be an already sanitized address @@ -1132,7 +1142,7 @@ sub make_message_id { $uniq = "$message_id_stamp-$message_id_serial"; my $du_part; - for ($sender, $repocommitter, $repoauthor) { + for ($sender, $repocommitter->(), $repoauthor->()) { $du_part = extract_valid_address(sanitize_address($_)); last if (defined $du_part and $du_part ne ''); } -- cgit 1.2.3-korg From 4adbf387bfd4b03b838282757e0bdf67e8f9fc8d Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 28 May 2021 11:23:47 +0200 Subject: send-email: use function syntax instead of barewords MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change calls like "__ 'foo'" to "__('foo')" so the Perl compiler doesn't have to guess that "__" is a function. This makes the code more readable. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index 38408ec75a..44dc3f6eb1 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -706,7 +706,7 @@ sub is_format_patch_arg { if (defined($format_patch)) { return $format_patch; } - die sprintf(__ < Date: Fri, 28 May 2021 11:23:48 +0200 Subject: send-email: get rid of indirect object syntax MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change indirect object syntax such as "new X ARGS" to "X->new(ARGS)". This allows perl to see what "new" is at compile-time without having loaded Term::ReadLine. This doesn't matter now, but will in a subsequent commit when we start lazily loading it. Let's do the same for the adjacent "FakeTerm" package for consistency, even though we're not going to conditionally load it. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index 44dc3f6eb1..cc1027d877 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -194,11 +194,11 @@ sub format_2822_time { my @repo = $repo ? ($repo) : (); my $term = eval { $ENV{"GIT_SEND_EMAIL_NOTTY"} - ? new Term::ReadLine 'git-send-email', \*STDIN, \*STDOUT - : new Term::ReadLine 'git-send-email'; + ? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT) + : Term::ReadLine->new('git-send-email'); }; if ($@) { - $term = new FakeTerm "$@: going non-interactive"; + $term = FakeTerm->new("$@: going non-interactive"); } # Behavior modification variables -- cgit 1.2.3-korg From f4dc9432fd287bde9100488943baf3c6a04d90d1 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 28 May 2021 11:23:49 +0200 Subject: send-email: lazily load modules for a big speedup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Optimize the time git-send-email takes to do even the simplest of things (such as serving up "-h") from around ~150ms to ~80ms-~90ms by lazily loading the modules it requires. Before this change Devel::TraceUse would report 99/97 used modules under NO_GETTEXT=[|Y], respectively. Now it's 52/37. It now takes ~15s to run t9001-send-email.sh, down from ~20s. Changing File::Spec::Functions::{catdir,catfile} to invoking class methods on File::Spec itself is idiomatic. See [1] for a more elaborate explanation, the resulting code behaves the same way, just without the now-pointless function wrapper. 1. http://lore.kernel.org/git/8735u8mmj9.fsf@evledraar.gmail.com Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 71 +++++++++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 32 deletions(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index cc1027d877..a8949c9d31 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -19,20 +19,10 @@ use 5.008; use strict; use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : (); -use POSIX qw/strftime/; -use Term::ReadLine; use Getopt::Long; -use Text::ParseWords; -use Term::ANSIColor; -use File::Temp qw/ tempdir tempfile /; -use File::Spec::Functions qw(catdir catfile); use Git::LoadCPAN::Error qw(:try); -use Cwd qw(abs_path cwd); use Git; use Git::I18N; -use Net::Domain (); -use Net::SMTP (); -use Git::LoadCPAN::Mail::Address; Getopt::Long::Configure qw/ pass_through /; @@ -166,7 +156,6 @@ sub format_2822_time { ); } -my $have_email_valid = eval { require Email::Valid; 1 }; my $smtp; my $auth; my $num_sent = 0; @@ -192,14 +181,6 @@ sub format_2822_time { my $repo = eval { Git->repository() }; my @repo = $repo ? ($repo) : (); -my $term = eval { - $ENV{"GIT_SEND_EMAIL_NOTTY"} - ? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT) - : Term::ReadLine->new('git-send-email'); -}; -if ($@) { - $term = FakeTerm->new("$@: going non-interactive"); -} # Behavior modification variables my ($quiet, $dry_run) = (0, 0); @@ -319,9 +300,9 @@ sub do_edit { # Handle Uncouth Termination sub signal_handler { - # Make text normal - print color("reset"), "\n"; + require Term::ANSIColor; + print Term::ANSIColor::color("reset"), "\n"; # SMTP password masked system "stty echo"; @@ -602,11 +583,13 @@ sub config_regexp { } sub parse_address_line { + require Git::LoadCPAN::Mail::Address; return map { $_->format } Mail::Address->parse($_[0]); } sub split_addrs { - return quotewords('\s*,\s*', 1, @_); + require Text::ParseWords; + return Text::ParseWords::quotewords('\s*,\s*', 1, @_); } my %aliases; @@ -655,10 +638,11 @@ sub parse_sendmail_aliases { s/\\"/"/g foreach @addr; $aliases{$alias} = \@addr }}}, - mailrc => sub { my $fh = shift; while (<$fh>) { + mailrc => sub { my $fh = shift; while (<$fh>) { if (/^alias\s+(\S+)\s+(.*?)\s*$/) { + require Text::ParseWords; # spaces delimit multiple addresses - $aliases{$1} = [ quotewords('\s+', 0, $2) ]; + $aliases{$1} = [ Text::ParseWords::quotewords('\s+', 0, $2) ]; }}}, pine => sub { my $fh = shift; my $f='\t[^\t]*'; for (my $x = ''; defined($x); $x = $_) { @@ -730,7 +714,8 @@ sub is_format_patch_arg { opendir my $dh, $f or die sprintf(__("Failed to opendir %s: %s"), $f, $!); - push @files, grep { -f $_ } map { catfile($f, $_) } + require File::Spec; + push @files, grep { -f $_ } map { File::Spec->catfile($f, $_) } sort readdir $dh; closedir $dh; } elsif ((-f $f or -p $f) and !is_format_patch_arg($f)) { @@ -743,7 +728,8 @@ sub is_format_patch_arg { if (@rev_list_opts) { die __("Cannot run git format-patch from outside a repository\n") unless $repo; - push @files, $repo->command('format-patch', '-o', tempdir(CLEANUP => 1), @rev_list_opts); + require File::Temp; + push @files, $repo->command('format-patch', '-o', File::Temp::tempdir(CLEANUP => 1), @rev_list_opts); } @files = handle_backup_files(@files); @@ -780,9 +766,10 @@ sub get_patch_subject { if ($compose) { # Note that this does not need to be secure, but we will make a small # effort to have it be unique + require File::Temp; $compose_filename = ($repo ? - tempfile(".gitsendemail.msg.XXXXXX", DIR => $repo->repo_path()) : - tempfile(".gitsendemail.msg.XXXXXX", DIR => "."))[1]; + File::Temp::tempfile(".gitsendemail.msg.XXXXXX", DIR => $repo->repo_path()) : + File::Temp::tempfile(".gitsendemail.msg.XXXXXX", DIR => "."))[1]; open my $c, ">", $compose_filename or die sprintf(__("Failed to open for writing %s: %s"), $compose_filename, $!); @@ -889,6 +876,19 @@ sub get_patch_subject { do_edit(@files); } +sub term { + my $term = eval { + require Term::ReadLine; + $ENV{"GIT_SEND_EMAIL_NOTTY"} + ? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT) + : Term::ReadLine->new('git-send-email'); + }; + if ($@) { + $term = FakeTerm->new("$@: going non-interactive"); + } + return $term; +} + sub ask { my ($prompt, %arg) = @_; my $valid_re = $arg{valid_re}; @@ -896,6 +896,7 @@ sub ask { my $confirm_only = $arg{confirm_only}; my $resp; my $i = 0; + my $term = term(); return defined $default ? $default : undef unless defined $term->IN and defined fileno($term->IN) and defined $term->OUT and defined fileno($term->OUT); @@ -1076,6 +1077,7 @@ sub extract_valid_address { return $address if ($address =~ /^($local_part_regexp)$/); $address =~ s/^\s*<(.*)>\s*$/$1/; + my $have_email_valid = eval { require Email::Valid; 1 }; if ($have_email_valid) { return scalar Email::Valid->address($address); } @@ -1135,7 +1137,8 @@ sub validate_address_list { sub make_message_id { my $uniq; if (!defined $message_id_stamp) { - $message_id_stamp = strftime("%Y%m%d%H%M%S.$$", gmtime(time)); + require POSIX; + $message_id_stamp = POSIX::strftime("%Y%m%d%H%M%S.$$", gmtime(time)); $message_id_serial = 0; } $message_id_serial++; @@ -1305,6 +1308,7 @@ sub valid_fqdn { sub maildomain_net { my $maildomain; + require Net::Domain; my $domain = Net::Domain::domainname(); $maildomain = $domain if valid_fqdn($domain); @@ -1315,6 +1319,7 @@ sub maildomain_mta { my $maildomain; for my $host (qw(mailhost localhost)) { + require Net::SMTP; my $smtp = Net::SMTP->new($host); if (defined $smtp) { my $domain = $smtp->domain; @@ -1994,13 +1999,15 @@ sub validate_patch { if ($repo) { my $hooks_path = $repo->command_oneline('rev-parse', '--git-path', 'hooks'); - my $validate_hook = catfile($hooks_path, + require File::Spec; + my $validate_hook = File::Spec->catfile($hooks_path, 'sendemail-validate'); my $hook_error; if (-x $validate_hook) { - my $target = abs_path($fn); + require Cwd; + my $target = Cwd::abs_path($fn); # The hook needs a correct cwd and GIT_DIR. - my $cwd_save = cwd(); + my $cwd_save = Cwd::cwd(); chdir($repo->wc_path() or $repo->repo_path()) or die("chdir: $!"); local $ENV{"GIT_DIR"} = $repo->repo_path(); -- cgit 1.2.3-korg From c95e3a3f0b8107b5dc7eac9dfdb9e5238280c9fb Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 28 May 2021 11:23:51 +0200 Subject: send-email: move trivial config handling to Perl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Optimize the startup time of git-send-email by using an amended config_regexp() function to retrieve the list of config keys and values we're interested in. For boolean keys we can handle the [true|false] case ourselves, and the "--get" case didn't need any parsing. Let's leave "--path" and other "--bool" cases to "git config". I'm not bothering with the "undef" or "" case (true and false, respectively), let's just punt on those and others and have "git config --type=bool" handle it. The "grep { defined } @values" here covers a rather subtle case. For list values such as sendemail.to it is possible as with any other config key to provide a plain "-c sendemail.to", i.e. to set the key as a boolean true. In that case the Git::config() API will return an empty string, but this new parser will correctly return "undef". However, that means we can end up with "undef" in the middle of a list. E.g. for sendemail.smtpserveroption in conjuction with sendemail.smtpserver as a path this would have produce a warning. For most of the other keys we'd behave the same despite the subtle change in the value, e.g. sendemail.to would behave the same because Mail::Address->parse() happens to return an empty list if fed "undef". For the boolean values we were already prepared to handle these variables being initialized as undef anyway. This brings the runtime of "git send-email" from ~60-~70ms to a very steady ~40ms on my test box. We now run just one "git config" invocation on startup instead of 8, the exact number will differ based on the local sendemail.* config. I happen to have 8 of those set. This brings the runtime of t9001-send-email.sh from ~13s down to ~12s for me. The change there is less impressive as many of those tests set various config values, and we're also getting to the point of diminishing returns for optimizing "git send-email" itself. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index a8949c9d31..5791138683 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -334,7 +334,11 @@ sub read_config { my $target = $config_bool_settings{$setting}; my $key = "$prefix.$setting"; next unless exists $known_keys->{$key}; - my $v = Git::config_bool(@repo, $key); + my $v = (@{$known_keys->{$key}} == 1 && + (defined $known_keys->{$key}->[0] && + $known_keys->{$key}->[0] =~ /^(?:true|false)$/s)) + ? $known_keys->{$key}->[0] eq 'true' + : Git::config_bool(@repo, $key); next unless defined $v; next if $configured->{$setting}++; $$target = $v; @@ -363,13 +367,13 @@ sub read_config { my $key = "$prefix.$setting"; next unless exists $known_keys->{$key}; if (ref($target) eq "ARRAY") { - my @values = Git::config(@repo, $key); - next unless @values; + my @values = @{$known_keys->{$key}}; + @values = grep { defined } @values; next if $configured->{$setting}++; @$target = @values; } else { - my $v = Git::config(@repo, $key); + my $v = $known_keys->{$key}->[0]; next unless defined $v; next if $configured->{$setting}++; $$target = $v; @@ -381,12 +385,19 @@ sub config_regexp { my ($regex) = @_; my @ret; eval { - @ret = Git::command( + my $ret = Git::command( 'config', - '--name-only', + '--null', '--get-regexp', $regex, ); + @ret = map { + # We must always return ($k, $v) here, since + # empty config values will be just "key\0", + # not "key\nvalue\0". + my ($k, $v) = split /\n/, $_, 2; + ($k, $v); + } split /\0/, $ret; 1; } or do { # If we have no keys we're OK, otherwise re-throw @@ -399,8 +410,10 @@ sub config_regexp { # parses 'bool' etc.) by only doing so for config keys that exist. my %known_config_keys; { - my @known_config_keys = config_regexp("^sende?mail[.]"); - @known_config_keys{@known_config_keys} = (); + my @kv = config_regexp("^sende?mail[.]"); + while (my ($k, $v) = splice @kv, 0, 2) { + push @{$known_config_keys{$k}} => $v; + } } # sendemail.identity yields to --identity. We must parse this -- cgit 1.2.3-korg From 17530b2ed2eac62706b8bbbcf93f62866f651ffd Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 28 May 2021 11:23:52 +0200 Subject: perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It has been pointed out[1] that cwd() invokes "pwd(1)" while getcwd() is a Perl-native XS function. For what we're using these for we can use getcwd(). The performance difference is miniscule, we're saving on the order of a millisecond or so, see [2] below for the benchmark. I don't think this matters in practice for optimizing git-send-email or perl execution (unlike the patches leading up to this one). But let's do it regardless of that, if only so we don't have to think about this as a low-hanging fruit anymore. 1. https://lore.kernel.org/git/20210512180517.GA11354@dcvr/ 2. $ perl -MBenchmark=:all -MCwd -wE 'cmpthese(10000, { getcwd => sub { getcwd }, cwd => sub { cwd }, pwd => sub { system "pwd >/dev/null" }})' (warning: too few iterations for a reliable count) Rate pwd cwd getcwd pwd 982/s -- -48% -100% cwd 1890/s 92% -- -100% getcwd 10000000000000000000/s 1018000000000000000% 529000000000000064% - Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- git-send-email.perl | 2 +- perl/Git.pm | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'git-send-email.perl') diff --git a/git-send-email.perl b/git-send-email.perl index 5791138683..0efe85c0b0 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -2020,7 +2020,7 @@ sub validate_patch { require Cwd; my $target = Cwd::abs_path($fn); # The hook needs a correct cwd and GIT_DIR. - my $cwd_save = Cwd::cwd(); + my $cwd_save = Cwd::getcwd(); chdir($repo->wc_path() or $repo->repo_path()) or die("chdir: $!"); local $ENV{"GIT_DIR"} = $repo->repo_path(); diff --git a/perl/Git.pm b/perl/Git.pm index 5562c0cede..090a7df63f 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -405,7 +405,7 @@ sub command_bidi_pipe { if ($self) { shift; require Cwd; - $cwd_save = Cwd::cwd(); + $cwd_save = Cwd::getcwd(); _setup_git_cmd_env($self); } require IPC::Open2; -- cgit 1.2.3-korg