[PATCH 0/9] send-email: various optimizations to speed up by >2x

This is on top of my just-submitted [1] which in turn is on top of
send-email work of mine sitting in "next".

I was meaning to hold off on these patches for a bit, but given the
concurrent on-list discussion about doing config discovery in
send-email I wanted to send this now.

This combines by not-picked-up[1] recent patches to remove the support
for the "sendemail.smtpssl" variable with the later patches showing
where that effort was really going.

As noted in the subject this speeds up git-send-email invocations by
~2x or more, and brings the very slow t9001 test from running in ~26s
on my box to ~12s. It's no longer consistently the slowest test I run.

This is basically done in two ways: We lazily invoke "git config" to
get config, before it's very eager, and deferring Perl compilation
with s/use/require/g.

1. https://lore.kernel.org/git/patch-1.1-92571a8cf7-20210512T094803Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-0.2-00000000000-20210411T144128Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (9):
  send-email: remove non-working support for "sendemail.smtpssl"
  send-email: refactor sendemail.smtpencryption config parsing
  send-email: lazily load config for a big speedup
  send-email: lazily shell out to "git var"
  send-email: use function syntax instead of barewords
  send-email: get rid of indirect object syntax
  send-email: lazily load modules for a big speedup
  perl: lazily load some common Git.pm setup code
  send-email: move trivial config handling to Perl

 Documentation/config/sendemail.txt |   3 -
 git-send-email.perl                | 145 +++++++++++++++++------------
 perl/Git.pm                        |  49 +++++-----
 3 files changed, 111 insertions(+), 86 deletions(-)

-- 
2.31.1.909.g789bb6d90e

Remove the already dead code to support "sendemail.smtssl" 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 <redacted>
---
 Documentation/config/sendemail.txt | 3 ---
 git-send-email.perl                | 6 +-----
 2 files changed, 1 insertion(+), 8 deletions(-)

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 175da07d94..3ef194fe2d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -383,11 +383,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;
 	}
 }
 

-- 
2.31.1.909.g789bb6d90e

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 <redacted>
---
 git-send-email.perl | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 3ef194fe2d..f9d859da3e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -277,6 +277,7 @@ sub do_edit {
 );
 
 my %config_settings = (
+    "smtpencryption" => \$smtp_encryption,
     "smtpserver" => \$smtp_server,
     "smtpserverport" => \$smtp_server_port,
     "smtpserveroption" => \@smtp_server_options,

@@ -377,14 +378,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

-- 
2.31.1.909.g789bb6d90e

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 <redacted>
---
 git-send-email.perl | 35 ++++++++++++++++++++++++++---------
 perl/Git.pm         |  4 ++--
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index f9d859da3e..5b0b7c33ec 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -337,11 +337,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;

@@ -349,8 +351,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;

@@ -365,14 +369,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;

@@ -380,9 +386,20 @@ sub read_config {
 	}
 }
 
+# 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 = Git::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,

@@ -393,8 +410,8 @@ sub read_config {
 # 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

@@ -478,7 +495,7 @@ sub read_config {
     usage();
 }
 
-if ($forbid_sendmail_variables && (scalar Git::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");

diff --git a/perl/Git.pm b/perl/Git.pm
index 73ebbf80cc..06a10b175a 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm

@@ -754,8 +754,8 @@ sub config_regexp {
 	} catch Git::Error::Command with {
 		my $E = shift;
 		if ($E->value() == 1) {
-			my @matches = ();
-			return @matches;
+			# Key(s) not found.
+			return;
 		} else {
 			throw $E;
 		}

-- 
2.31.1.909.g789bb6d90e

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 <redacted>
---
 git-send-email.perl | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5b0b7c33ec..9ff315f775 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -560,8 +560,18 @@ sub read_config {
 }
 
 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]);

@@ -749,7 +759,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 || '';

@@ -955,7 +965,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

@@ -1104,7 +1114,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 '');
 	}

-- 
2.31.1.909.g789bb6d90e

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 <redacted>
---
 git-send-email.perl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 9ff315f775..da46925aa0 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -678,7 +678,7 @@ sub is_format_patch_arg {
 		if (defined($format_patch)) {
 			return $format_patch;
 		}
-		die sprintf(__ <<EOF, $f, $f);
+		die sprintf(__(<<EOF), $f, $f);
 File '%s' exists but it could also be the range of commits
 to produce patches for.  Please disambiguate by...
 

@@ -764,7 +764,7 @@ sub get_patch_subject {
 	my $tpl_in_reply_to = $initial_in_reply_to || '';
 	my $tpl_reply_to = $reply_to || '';
 
-	print $c <<EOT1, Git::prefix_lines("GIT: ", __ <<EOT2), <<EOT3;
+	print $c <<EOT1, Git::prefix_lines("GIT: ", __(<<EOT2)), <<EOT3;
 From $tpl_sender # This line is ignored.
 EOT1
 Lines beginning in "GIT:" will be removed.

-- 
2.31.1.909.g789bb6d90e

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 <redacted>
---
 git-send-email.perl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index da46925aa0..f9c780ceed 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

-- 
2.31.1.909.g789bb6d90e

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 git-send-email.perl | 70 +++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 31 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index f9c780ceed..907dc1425f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -19,20 +19,10 @@
 use 5.008;
 use strict;
 use warnings;
-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);

@@ -309,9 +290,10 @@ sub do_edit {
 
 # Handle Uncouth Termination
 sub signal_handler {
+	require Term::ANSIColor;
 
 	# Make text normal
-	print color("reset"), "\n";
+	print Term::ANSIColor::color("reset"), "\n";
 
 	# SMTP password masked
 	system "stty echo";

@@ -574,11 +556,13 @@ sub read_config {
 }
 
 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;

@@ -627,10 +611,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 = $_) {

@@ -702,7 +687,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)) {

@@ -715,7 +701,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);

@@ -752,9 +739,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, $!);
 

@@ -861,6 +849,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};

@@ -868,6 +869,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);

@@ -1048,6 +1050,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);
 	}

@@ -1107,7 +1110,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++;

@@ -1277,6 +1281,7 @@ sub valid_fqdn {
 sub maildomain_net {
 	my $maildomain;
 
+	require Net::Domain;
 	my $domain = Net::Domain::domainname();
 	$maildomain = $domain if valid_fqdn($domain);
 

@@ -1287,6 +1292,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;

@@ -1965,13 +1971,15 @@ sub validate_patch {
 	my ($fn, $xfer_encoding) = @_;
 
 	if ($repo) {
-		my $validate_hook = catfile($repo->hooks_path(),
+		require File::Spec;
+		my $validate_hook = File::Spec->catfile($repo->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();

-- 
2.31.1.909.g789bb6d90e

Instead of unconditionally requiring modules such as File::Spec, let's
only load them when needed. This speeds up code that only needs a
subset of the features Git.pm provides.

This brings a plain invocation of "git send-email" down from 52/37
loaded modules under NO_GETTEXT=[|Y] to 39/18, and it now takes
~60-~70ms instead of ~80-~90ms. The runtime of t9001-send-email.sh
test is down to ~13s from ~15s.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 perl/Git.pm | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 06a10b175a..f18852fb09 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm

@@ -11,9 +11,6 @@ package Git;
 use strict;
 use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
 
-use File::Temp ();
-use File::Spec ();
-
 BEGIN {
 
 our ($VERSION, @ISA, @EXPORT, @EXPORT_OK);

@@ -103,12 +100,9 @@ =head1 DESCRIPTION
 =cut
 
 
-use Carp qw(carp croak); # but croak is bad - throw instead
+sub carp { require Carp; goto &Carp::carp }
+sub croak { require Carp; goto &Carp::croak }
 use Git::LoadCPAN::Error qw(:try);
-use Cwd qw(abs_path cwd);
-use IPC::Open2 qw(open2);
-use Fcntl qw(SEEK_SET SEEK_CUR);
-use Time::Local qw(timegm);
 }
 
 

@@ -191,13 +185,15 @@ sub repository {
 			$dir = undef;
 		};
 
+		require Cwd;
 		if ($dir) {
+			require File::Spec;
 			File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;
-			$opts{Repository} = abs_path($dir);
+			$opts{Repository} = Cwd::abs_path($dir);
 
 			# If --git-dir went ok, this shouldn't die either.
 			my $prefix = $search->command_oneline('rev-parse', '--show-prefix');
-			$dir = abs_path($opts{Directory}) . '/';
+			$dir = Cwd::abs_path($opts{Directory}) . '/';
 			if ($prefix) {
 				if (substr($dir, -length($prefix)) ne $prefix) {
 					throw Error::Simple("rev-parse confused me - $dir does not have trailing $prefix");

@@ -223,7 +219,7 @@ sub repository {
 				throw Error::Simple("fatal: Not a git repository: $dir");
 			}
 
-			$opts{Repository} = abs_path($dir);
+			$opts{Repository} = Cwd::abs_path($dir);
 		}
 
 		delete $opts{Directory};

@@ -408,10 +404,12 @@ sub command_bidi_pipe {
 	my $cwd_save = undef;
 	if ($self) {
 		shift;
-		$cwd_save = cwd();
+		require Cwd;
+		$cwd_save = Cwd::cwd();
 		_setup_git_cmd_env($self);
 	}
-	$pid = open2($in, $out, 'git', @_);
+	require IPC::Open2;
+	$pid = IPC::Open2::open2($in, $out, 'git', @_);
 	chdir($cwd_save) if $cwd_save;
 	return ($pid, $in, $out, join(' ', @_));
 }

@@ -538,7 +536,8 @@ sub get_tz_offset {
 	my $t = shift || time;
 	my @t = localtime($t);
 	$t[5] += 1900;
-	my $gm = timegm(@t);
+	require Time::Local;
+	my $gm = Time::Local::timegm(@t);
 	my $sign = qw( + + - )[ $gm <=> $t ];
 	return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
 }

@@ -629,7 +628,8 @@ sub hooks_path {
 	my ($self) = @_;
 
 	my $dir = $self->command_oneline('rev-parse', '--git-path', 'hooks');
-	my $abs = abs_path($dir);
+	require Cwd;
+	my $abs = Cwd::abs_path($dir);
 	return $abs;
 }
 

@@ -1353,6 +1353,7 @@ sub _temp_cache {
 		my $n = $name;
 		$n =~ s/\W/_/g; # no strange chars
 
+		require File::Temp;
 		($$temp_fd, $fname) = File::Temp::tempfile(
 			"Git_${n}_XXXXXX", UNLINK => 1, DIR => $tmpdir,
 			) or throw Error::Simple("couldn't open new temp file");

@@ -1375,9 +1376,9 @@ sub temp_reset {
 
 	truncate $temp_fd, 0
 		or throw Error::Simple("couldn't truncate file");
-	sysseek($temp_fd, 0, SEEK_SET) and seek($temp_fd, 0, SEEK_SET)
+	sysseek($temp_fd, 0, Fcntl::SEEK_SET()) and seek($temp_fd, 0, Fcntl::SEEK_SET())
 		or throw Error::Simple("couldn't seek to beginning of file");
-	sysseek($temp_fd, 0, SEEK_CUR) == 0 and tell($temp_fd) == 0
+	sysseek($temp_fd, 0, Fcntl::SEEK_CUR()) == 0 and tell($temp_fd) == 0
 		or throw Error::Simple("expected file position to be reset");
 }
 

-- 
2.31.1.909.g789bb6d90e

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". As noted in a preceding commit
we're free to change the config_regexp() function, it's only used by
"git send-email".

This brings the runtime of "git send-email" from ~60-~70ms to a very
steady ~40ms on my test box. We no 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 <redacted>
---
 git-send-email.perl | 17 ++++++++++-------
 perl/Git.pm         | 10 +++++-----
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 907dc1425f..35ba19470d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -325,7 +325,10 @@ 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 &&
+			 $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;

@@ -354,14 +357,12 @@ 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}};
 			next if $configured->{$setting}++;
 			@$target = @values;
 		}
 		else {
-			my $v = Git::config(@repo, $key);
-			next unless defined $v;
+			my $v = $known_keys->{$key}->[0];
 			next if $configured->{$setting}++;
 			$$target = $v;
 		}

@@ -372,8 +373,10 @@ sub read_config {
 # parses 'bool' etc.) by only doing so for config keys that exist.
 my %known_config_keys;
 {
-	my @known_config_keys = Git::config_regexp("^sende?mail[.]");
-	@known_config_keys{@known_config_keys} = ();
+	my @kv = Git::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

diff --git a/perl/Git.pm b/perl/Git.pm
index f18852fb09..a9020d0d01 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm

@@ -739,18 +739,18 @@ sub config_int {
 =item config_regexp ( RE )
 
 Retrieve the list of configuration key names matching the regular
-expression C<RE>. The return value is a list of strings matching
-this regex.
+expression C<RE>. The return value is an ARRAY of key-value pairs.
 
 =cut
 
 sub config_regexp {
 	my ($self, $regex) = _maybe_self(@_);
 	try {
-		my @cmd = ('config', '--name-only', '--get-regexp', $regex);
+		my @cmd = ('config', '--null', '--get-regexp', $regex);
 		unshift @cmd, $self if $self;
-		my @matches = command(@cmd);
-		return @matches;
+		my $data = command(@cmd);
+		my (@kv) = map { split /\n/, $_, 2 } split /\0/, $data;
+		return @kv;
 	} catch Git::Error::Command with {
 		my $E = shift;
 		if ($E->value() == 1) {

-- 
2.31.1.909.g789bb6d90e

On 2021-05-12 15:48:17+0200, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

Remove the already dead code to support "sendemail.smtssl" by finally

s/smtssl/smtpssl/
 

-- 
Danh

Ævar Arnfjörð Bjarmason [off-list ref] wrote:

As noted in the subject this speeds up git-send-email invocations by
~2x or more, and brings the very slow t9001 test from running in ~26s
on my box to ~12s. It's no longer consistently the slowest test I run.

This is basically done in two ways: We lazily invoke "git config" to
get config, before it's very eager, and deferring Perl compilation
with s/use/require/g.

Nice.  I've been doing similar things elsewhere and hoping to
find time to get around to git-svn at some point.

Ævar Arnfjörð Bjarmason (9):
  send-email: remove non-working support for "sendemail.smtpssl"
  send-email: refactor sendemail.smtpencryption config parsing
  send-email: lazily load config for a big speedup
  send-email: lazily shell out to "git var"
  send-email: use function syntax instead of barewords
  send-email: get rid of indirect object syntax
  send-email: lazily load modules for a big speedup
  perl: lazily load some common Git.pm setup code
  send-email: move trivial config handling to Perl

I spotted some further optimizations for 7 and 8,
but otherwise consider this series:

Reviewed-by: Eric Wong <redacted>

Ævar Arnfjörð Bjarmason [off-list ref] wrote:

(This also applies to 7/9)

Cwd::cwd() execs /bin/pwd (at least on Perl 5.28.1 in Debian 10x).
There should be a benefit from using Cwd::getcwd() here, instead.

	strace -f -e execve perl -MCwd -E 'Cwd::cwd()

...confirms the execve is happening.

getcwd() takes 25ms vs 27ms of cwd() for me using the
"schedutil" CPU governor under Linux:

	time perl -MCwd -E 'Cwd::getcwd()'
	time perl -MCwd -E 'Cwd::cwd()'

On Wed, May 12, 2021 at 9:48 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

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". As noted in a preceding commit
we're free to change the config_regexp() function, it's only used by
"git send-email".

This brings the runtime of "git send-email" from ~60-~70ms to a very
steady ~40ms on my test box. We no run just one "git config"

s/no/now/

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

On Wed, May 12, 2021 at 03:48:25PM +0200, Ævar Arnfjörð Bjarmason wrote:

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". As noted in a preceding commit
we're free to change the config_regexp() function, it's only used by
"git send-email".

I think both of these cases should be safe.

It is a bit unfortunate to have to go through these contortions, but
this is definitely the best we can do for now. I think in the long run
it would be nice to have a "--stdin" mode for git-config, where we could
do something like:

  git config --stdin <<\EOF
  key=foo.bar
  type=bool
  default=false

  key=another.key
  type=color
  default=red
  EOF

But that doesn't exist yet, and using it would probably involve
rearranging send-email a bit (we would need an up-front list of all of
the keys we care about and their types). So I'm perfectly content with
this strategy in the meantime.

The Git.pm interface is public-facing, isn't it? Are we breaking
third-party callers of config_regexp here?

-Peff

On Wed, May 12, 2021 at 03:48:21PM +0200, Ævar Arnfjörð Bjarmason wrote:

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.

I think this is a good change. And while you do qualify it as "more"
readable, I think the main issue with readability here:

is how far the "EOF" marker is from the actual here-doc, syntactically
(plus the ugly indentation). I suspect this and other places might be
nicer to assign from the here-doc into a well-named variable.

But I think it's fine to leave for now (and I worry a bit that it may
turn into a rabbit hole, so we might be better to just leave it alone
anyway).

-Peff

On Wed, May 12, 2021 at 03:48:16PM +0200, Ævar Arnfjörð Bjarmason wrote:

This combines by not-picked-up[1] recent patches to remove the support
for the "sendemail.smtpssl" variable with the later patches showing
where that effort was really going.

As noted in the subject this speeds up git-send-email invocations by
~2x or more, and brings the very slow t9001 test from running in ~26s
on my box to ~12s. It's no longer consistently the slowest test I run.

Nice. I have observed that with a decent number of cores, the running
time of the entire test suite correlates strongly with the running time
of t9001. :)

Here are timings for individual tests run with "prove --state=slow,save".
(This is on an 8-core machine using -j32, skipping cvs/svn/p4 tests,
and using a tmpfs via --root). The timings were computed with:

  perl -MYAML -e '
    $_ = do { local $/; <> };
    # prove puts this non-YAML cruft at the end
    s/\.\.\.$//s;

    my $t = YAML::Load($_)->{tests};
    print "$_->[1] $_->[0]\n" for
      sort { $b->[1] <=> $a->[1] }
      map { [$_, $t->{$_}->{elapsed}] }
      keys(%$t);
  ' t/.prove | head

Before your patches, the whole sweet takes ~60-63s, and the top timings
(from a 63s run) are:

    63.2607979774475 t9001-send-email.sh
    51.742644071579 t0027-auto-crlf.sh
    37.7909920215607 t3070-wildmatch.sh
    27.09605717659 t7610-mergetool.sh
    24.7028169631958 t7112-reset-submodule.sh
    24.5535898208618 t5572-pull-submodule.sh
    23.8404550552368 t9500-gitweb-standalone-no-errors.sh
    22.3544380664825 t7400-submodule-basic.sh
    21.7017750740051 t5510-fetch.sh
    21.4575610160828 t3305-notes-fanout.sh

Now after, which takes ~54-59s (this is from a 54s run):

  46.796669960022 t0027-auto-crlf.sh
  32.5747599601746 t3070-wildmatch.sh
  21.5069420337677 t7610-mergetool.sh
  20.8392388820648 t1701-racy-split-index.sh
  19.7403028011322 t5572-pull-submodule.sh
  19.7386808395386 t9001-send-email.sh
  19.4622302055359 t7112-reset-submodule.sh
  18.9555768966675 t9500-gitweb-standalone-no-errors.sh
  18.0672709941864 t7400-submodule-basic.sh
  17.641391992569 t5510-fetch.sh

I have some messy patches to split t9001 into two segments. They were
waiting to get polished, but perhaps I can just discard them now. :)

Some side notes for those interested in timing the test suite:

  - If I run t9001 standalone, it goes much faster, of course; the CPU
    throttles down when we're running all the tests in parallel.

  - Those are with "-x --verbose-log", which is nice for catching flaky
    results. Dropping those seems to shave a few seconds off.

  - A big chunk of time for t0027 and t3070 is spent running the sed-based
    chain-linting for their huge tables of auto-generated tests (1400+
    and 1800+ respectively). Dropping the sed linting for just those
    tests knocks off about 30 CPU-seconds.

-Peff

On Wed, May 12, 2021 at 03:48:16PM +0200, Ævar Arnfjörð Bjarmason wrote:

As noted in the subject this speeds up git-send-email invocations by
~2x or more, and brings the very slow t9001 test from running in ~26s
on my box to ~12s. It's no longer consistently the slowest test I run.

This is basically done in two ways: We lazily invoke "git config" to
get config, before it's very eager, and deferring Perl compilation
with s/use/require/g.

Splitting my reply, since the other one got deep into test-suite timing
details.

The techniques here look overall pretty reasonable. I think the module
lazy-loading makes the overall code a _little_ uglier, but IMHO the
speedup you're getting is worth it (I was surprised how much of the
improvement comes from that versus avoiding git-config subprocesses).

My only concern is changing the interface of Git::config_regexp() in the
final patch. Do we need to have a config_regexp_with_values() to avoid
breaking third-party users of the module?

-Peff

Jeff King wrote:

It is a bit unfortunate to have to go through these contortions, but
this is definitely the best we can do for now. I think in the long run
it would be nice to have a "--stdin" mode for git-config, where we could
do something like:

  git config --stdin <<\EOF
  key=foo.bar
  type=bool
  default=false

  key=another.key
  type=color
  default=red
  EOF

Why do we even have to specify the type? Shouldn't there be a registry
of configurations (a schema), so that all users don't have to do this?

-- 
Felipe Contreras

On Thu, May 13, 2021 at 02:04:08AM -0500, Felipe Contreras wrote:

Jeff King wrote:

quoted

It is a bit unfortunate to have to go through these contortions, but
this is definitely the best we can do for now. I think in the long run
it would be nice to have a "--stdin" mode for git-config, where we could
do something like:

  git config --stdin <<\EOF
  key=foo.bar
  type=bool
  default=false

  key=another.key
  type=color
  default=red
  EOF

Why do we even have to specify the type? Shouldn't there be a registry
of configurations (a schema), so that all users don't have to do this?

One of the purposes of git-config is to serve third-party scripts that
store their own config keys that Git does not know about. So we can't
know the set of all possible types that will be asked about.

Obviously we could have git-config know internally about all of the keys
other parts of Git would ask about. But generally we have pushed that
knowledge out to the users of the keys, rather than any kind of central
registry.

-Peff

On Thu, May 13 2021, Felipe Contreras wrote:

Jeff King wrote:

quoted

It is a bit unfortunate to have to go through these contortions, but
this is definitely the best we can do for now. I think in the long run
it would be nice to have a "--stdin" mode for git-config, where we could
do something like:

  git config --stdin <<\EOF
  key=foo.bar
  type=bool
  default=false

  key=another.key
  type=color
  default=red
  EOF

Why do we even have to specify the type? Shouldn't there be a registry
of configurations (a schema), so that all users don't have to do this?

Yes, we should be moving towards that. Currently we're not there, the
closest we've got is the generated config-list.h.

If we closed that loop properly you should be able to do:

    git config --get --type=dwim clean.requireForce # or auto, or no --type

Or whatever, instead of:

    git config --get --type=bool clean.requireForce

But we're not there yet, there's also edge cases here and there that
make a plain exhaustive registry hard, and send-email is one of them.

In its case the value of sendemail.identity=something (if any)
determines if/how e.g. sendemail.something.smtpEncryption is
interpreted.

I think we can do it with just a list of config variables where the
variable is either a string or a regex (in this case,
"^sendemail(?:[^.]+\.)?\.smtpEncryption$"), but I haven't tried. There
may be other tricky special-cases.

On Wed, May 12 2021, Jeff King wrote:

On Wed, May 12, 2021 at 03:48:16PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

As noted in the subject this speeds up git-send-email invocations by
~2x or more, and brings the very slow t9001 test from running in ~26s
on my box to ~12s. It's no longer consistently the slowest test I run.

This is basically done in two ways: We lazily invoke "git config" to
get config, before it's very eager, and deferring Perl compilation
with s/use/require/g.

Splitting my reply, since the other one got deep into test-suite timing
details.

The techniques here look overall pretty reasonable. I think the module
lazy-loading makes the overall code a _little_ uglier, but IMHO the
speedup you're getting is worth it (I was surprised how much of the
improvement comes from that versus avoiding git-config subprocesses).

Yeah, it's mostly uglier, but I think it's worth it. Some parts are
better afterwards though, i.e. the SOME_RARE_BARE_WORD is now
Module::It::Is::In::SOME_RARE_BARE_WORD, which makes it easier to
understand where it's from.

My only concern is changing the interface of Git::config_regexp() in the
final patch. Do we need to have a config_regexp_with_values() to avoid
breaking third-party users of the module?

As noted in 3/9 I don't think we need to worry about it, it's recently
introduced (a few months) API in Git.pm for send-email itself. I think
we can just change it.

In general I think it's unfortunate that we have (at least in principle)
a "public by default" module like Git.pm that's mostly for our own use.

This series doesn't try to deal with that in general at all, I'm
somewhat of the opinion that we should just fork it at this
point. I.e. have a Git.pm we freeze in time, and a Git/Ours.pm that's
going to be the private API.

I stopped with these optimizations at the point of refactoring away
Error.pm, which is a large contributor to compilation time, but as long
as it's a public API that can't be done without changing the public
API. If all we needed to worry about was send-email, git-svn etc. just
changing it to Perl-native exceptions would be trivial.

On Thu, May 13, 2021 at 09:37:36AM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

My only concern is changing the interface of Git::config_regexp() in the
final patch. Do we need to have a config_regexp_with_values() to avoid
breaking third-party users of the module?

As noted in 3/9 I don't think we need to worry about it, it's recently
introduced (a few months) API in Git.pm for send-email itself. I think
we can just change it.

Ah, thanks for pointing that out. I _thought_ I had seen you mention it
earlier, but when I went back to look I couldn't find it.

I'm not entirely convinced, though. I agree it's probably not heavily
used, but the existing interface was shipped in three releases already
(v2.29 and up).

In general I think it's unfortunate that we have (at least in principle)
a "public by default" module like Git.pm that's mostly for our own use.

I'd certainly agree with that sentiment. :)

This series doesn't try to deal with that in general at all, I'm
somewhat of the opinion that we should just fork it at this
point. I.e. have a Git.pm we freeze in time, and a Git/Ours.pm that's
going to be the private API.

I stopped with these optimizations at the point of refactoring away
Error.pm, which is a large contributor to compilation time, but as long
as it's a public API that can't be done without changing the public
API. If all we needed to worry about was send-email, git-svn etc. just
changing it to Perl-native exceptions would be trivial.

Yeah, I don't have any real problem with that, as long as we don't break
third-party scripts that we've promised not to. I'd even be OK with
deprecating Git.pm and eventually phasing it out, if we think it's a
maintenance burden.

-Peff

Jeff King wrote:

On Thu, May 13, 2021 at 02:04:08AM -0500, Felipe Contreras wrote:

quoted

Jeff King wrote:

quoted

It is a bit unfortunate to have to go through these contortions, but
this is definitely the best we can do for now. I think in the long run
it would be nice to have a "--stdin" mode for git-config, where we could
do something like:

  git config --stdin <<\EOF
  key=foo.bar
  type=bool
  default=false

  key=another.key
  type=color
  default=red
  EOF

Why do we even have to specify the type? Shouldn't there be a registry
of configurations (a schema), so that all users don't have to do this?

One of the purposes of git-config is to serve third-party scripts that
store their own config keys that Git does not know about. So we can't
know the set of all possible types that will be asked about.

Yes, I know, I maintain several tools that have such configurations. For
those you would need to specify the type (or find some way to install
the schema so that git parses it).

But I'm talking about git.git configurations. If you don't specify the
type in --stdin it should fetch it from some database. That would be
much more user-friendly.

-- 
Felipe Contreras

On Thu, May 13 2021, Felipe Contreras wrote:

Jeff King wrote:

quoted

On Thu, May 13, 2021 at 02:04:08AM -0500, Felipe Contreras wrote:

quoted

Jeff King wrote:

quoted

It is a bit unfortunate to have to go through these contortions, but
this is definitely the best we can do for now. I think in the long run
it would be nice to have a "--stdin" mode for git-config, where we could
do something like:

  git config --stdin <<\EOF
  key=foo.bar
  type=bool
  default=false

  key=another.key
  type=color
  default=red
  EOF

Why do we even have to specify the type? Shouldn't there be a registry
of configurations (a schema), so that all users don't have to do this?

One of the purposes of git-config is to serve third-party scripts that
store their own config keys that Git does not know about. So we can't
know the set of all possible types that will be asked about.

Yes, I know, I maintain several tools that have such configurations. For
those you would need to specify the type (or find some way to install
the schema so that git parses it).

But I'm talking about git.git configurations. If you don't specify the
type in --stdin it should fetch it from some database. That would be
much more user-friendly.

For what it's worth my idea of hacking a plumbing thingy for
git-send-email here before ultimately deciding that my simpler caching
approach was easier and gave me 95% of the win, was to just teach it a
mode where it spews out all config variables \0-delimited with all
possible interpretations of it. I.e.:

    some.variable	123	bool	true	path	123	string	123	[...]

It's rather cheap to do the "interpret this as --type=X for me" on the
C-level, so we might as well spew out all possible interpretations.

That means that any external tool would be guaranteed to only need one
"git config" invocation to parse any of its config, i.e. in a case where
variable X decides if variable Y is a bool or path or whatever. They'd
already have all possible values.

Something like:

	git config -l -z --type=bool,path
	git config -l -z --type=ALL

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 <redacted>
---
 Documentation/config/sendemail.txt | 3 ---
 git-send-email.perl                | 6 +-----
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt
index cbc5af42fdf..50baa5d6bfb 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 175da07d946..3ef194fe2da 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -383,11 +383,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;
 	}
 }
 

-- 
2.32.0.rc0.405.g5d387561bb3

A re-roll of [1], the work I based v1 on top of has landed in master,
so this has been rebased on master.

The changes here are minor, just a typo fix / commit message
clarification, moving "require" closer to where it's used, and finally
a new 10/10 patch to s/cwd/getcwd/g.

As noted in the commit message I don't think that'll make any
difference in practice. The "time" Eric posted was for loading Cwd.pm
and then doing cwd() or getcwd(), but when we run it we've already
paid the cost of loading Cwd.pm. But it was an easy change to make, so
let's make it anyway.

1. https://lore.kernel.org/git/cover-0.9-0000000000-20210512T132955Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (10):
  send-email: remove non-working support for "sendemail.smtpssl"
  send-email: refactor sendemail.smtpencryption config parsing
  send-email: lazily load config for a big speedup
  send-email: lazily shell out to "git var"
  send-email: use function syntax instead of barewords
  send-email: get rid of indirect object syntax
  send-email: lazily load modules for a big speedup
  perl: lazily load some common Git.pm setup code
  send-email: move trivial config handling to Perl
  perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd()

 Documentation/config/sendemail.txt |   3 -
 git-send-email.perl                | 146 +++++++++++++++++------------
 perl/Git.pm                        |  49 +++++-----
 3 files changed, 111 insertions(+), 87 deletions(-)

Range-diff against v1:
 1:  92571a8cf7f <  -:  ----------- Makefile: make PERL_DEFINES recursively expanded
 2:  85b706d43fc !  1:  8474acae689 send-email: remove non-working support for "sendemail.smtpssl"
    @@ Metadata
      ## Commit message ##
         send-email: remove non-working support for "sendemail.smtpssl"
     
    -    Remove the already dead code to support "sendemail.smtssl" by finally
    +    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
 3:  c22af817f10 =  2:  b87f53adbed send-email: refactor sendemail.smtpencryption config parsing
 4:  1e14d322535 =  3:  1b27a393ae3 send-email: lazily load config for a big speedup
 5:  e1df469d5fe =  4:  acee22b77d2 send-email: lazily shell out to "git var"
 6:  8846d40fc02 =  5:  f317cd1c01e send-email: use function syntax instead of barewords
 7:  0dde0e14ef6 =  6:  fc27024f838 send-email: get rid of indirect object syntax
 8:  55a0b07062f !  7:  f86f5453d7a send-email: lazily load modules for a big speedup
    @@ git-send-email.perl: sub do_edit {
      
      # Handle Uncouth Termination
      sub signal_handler {
    -+	require Term::ANSIColor;
    - 
    +-
      	# Make text normal
     -	print color("reset"), "\n";
    ++	require Term::ANSIColor;
     +	print Term::ANSIColor::color("reset"), "\n";
      
      	# SMTP password masked
 9:  2312346f71e =  8:  86641377c0d perl: lazily load some common Git.pm setup code
10:  0d87c9a5a37 !  9:  895c9e29a96 send-email: move trivial config handling to Perl
    @@ Commit message
     
         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.
    +    values we're interested in. See the earlier "send-email: lazily load
    +    config for a big speedup" commit for why changing its interface is OK.
     
         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
    @@ Commit message
         "git send-email".
     
         This brings the runtime of "git send-email" from ~60-~70ms to a very
    -    steady ~40ms on my test box. We no run just one "git config"
    +    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.
     
 -:  ----------- > 10:  97455f993d5 perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd()
-- 
2.32.0.rc0.405.g5d387561bb3

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 <redacted>
---
 git-send-email.perl | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 3ef194fe2da..f9d859da3ed 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -277,6 +277,7 @@ sub do_edit {
 );
 
 my %config_settings = (
+    "smtpencryption" => \$smtp_encryption,
     "smtpserver" => \$smtp_server,
     "smtpserverport" => \$smtp_server_port,
     "smtpserveroption" => \@smtp_server_options,

@@ -377,14 +378,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

-- 
2.32.0.rc0.405.g5d387561bb3

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 <redacted>
---
 git-send-email.perl | 35 ++++++++++++++++++++++++++---------
 perl/Git.pm         |  4 ++--
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index f9d859da3ed..5b0b7c33ec6 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -337,11 +337,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;

@@ -349,8 +351,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;

@@ -365,14 +369,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;

@@ -380,9 +386,20 @@ sub read_config {
 	}
 }
 
+# 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 = Git::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,

@@ -393,8 +410,8 @@ sub read_config {
 # 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

@@ -478,7 +495,7 @@ sub read_config {
     usage();
 }
 
-if ($forbid_sendmail_variables && (scalar Git::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");

diff --git a/perl/Git.pm b/perl/Git.pm
index 73ebbf80cc6..06a10b175a7 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm

@@ -754,8 +754,8 @@ sub config_regexp {
 	} catch Git::Error::Command with {
 		my $E = shift;
 		if ($E->value() == 1) {
-			my @matches = ();
-			return @matches;
+			# Key(s) not found.
+			return;
 		} else {
 			throw $E;
 		}

-- 
2.32.0.rc0.405.g5d387561bb3

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 <redacted>
---
 git-send-email.perl | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5b0b7c33ec6..9ff315f775c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -560,8 +560,18 @@ sub read_config {
 }
 
 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]);

@@ -749,7 +759,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 || '';

@@ -955,7 +965,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

@@ -1104,7 +1114,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 '');
 	}