* [PATCH v3 01/13] send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 02/13] send-email tests: test for boolean variables without a value Ævar Arnfjörð Bjarmason
` (13 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 175da07d94..cffdfdacfb 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;
--
2.32.0.rc0.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v3 02/13] send-email tests: test for boolean variables without a value
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 01/13] send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 03/13] send-email: remove non-working support for "sendemail.smtpssl" Ævar Arnfjörð Bjarmason
` (12 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
The Git.pm code does its own Perl-ifying of boolean variables, let's
ensure that empty values = true for boolean variables, as in the C
code.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t9001-send-email.sh | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 65b3035371..6bf79c816a 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1349,6 +1349,16 @@ test_expect_success $PREREQ 'sendemail.identity: bool variable fallback' '
! grep "X-Mailer" stdout
'
+test_expect_success $PREREQ 'sendemail.identity: bool variable without a value' '
+ git -c sendemail.xmailer \
+ send-email \
+ --dry-run \
+ --from="nobody@example.com" \
+ $patches >stdout &&
+ grep "To: default@example.com" stdout &&
+ grep "X-Mailer" stdout
+'
+
test_expect_success $PREREQ '--no-to overrides sendemail.to' '
git send-email \
--dry-run \
@@ -2073,6 +2083,18 @@ test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=true' '
do_xmailer_test 1 "--xmailer"
'
+test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer' '
+ test_when_finished "test_unconfig sendemail.xmailer" &&
+ cat >>.git/config <<-\EOF &&
+ [sendemail]
+ xmailer
+ EOF
+ test_config sendemail.xmailer true &&
+ do_xmailer_test 1 "" &&
+ do_xmailer_test 0 "--no-xmailer" &&
+ do_xmailer_test 1 "--xmailer"
+'
+
test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=false' '
test_config sendemail.xmailer false &&
do_xmailer_test 0 "" &&
@@ -2080,6 +2102,13 @@ test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=false' '
do_xmailer_test 1 "--xmailer"
'
+test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=' '
+ test_config sendemail.xmailer "" &&
+ do_xmailer_test 0 "" &&
+ do_xmailer_test 0 "--no-xmailer" &&
+ do_xmailer_test 1 "--xmailer"
+'
+
test_expect_success $PREREQ 'setup expected-list' '
git send-email \
--dry-run \
--
2.32.0.rc0.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v3 03/13] send-email: remove non-working support for "sendemail.smtpssl"
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 01/13] send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 02/13] send-email tests: test for boolean variables without a value Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 04/13] send-email: refactor sendemail.smtpencryption config parsing Ævar Arnfjörð Bjarmason
` (11 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 cffdfdacfb..24d73df5a3 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.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v3 04/13] send-email: refactor sendemail.smtpencryption config parsing
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-05-23 8:56 ` [PATCH v3 03/13] send-email: remove non-working support for "sendemail.smtpssl" Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 05/13] send-email: copy "config_regxp" into git-send-email.perl Ævar Arnfjörð Bjarmason
` (10 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 24d73df5a3..3d8362b5f0 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.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v3 05/13] send-email: copy "config_regxp" into git-send-email.perl
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-05-23 8:56 ` [PATCH v3 04/13] send-email: refactor sendemail.smtpencryption config parsing Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 06/13] send-email: lazily load config for a big speedup Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 3d8362b5f0..95a69d4c51 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -380,6 +380,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");
@@ -478,7 +496,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");
--
2.32.0.rc0.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v3 06/13] send-email: lazily load config for a big speedup
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-05-23 8:56 ` [PATCH v3 05/13] send-email: copy "config_regxp" into git-send-email.perl Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 07/13] send-email: lazily shell out to "git var" Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 95a69d4c51..d294681cf3 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;
@@ -398,9 +404,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,
@@ -411,8 +428,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
@@ -496,7 +513,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");
--
2.32.0.rc0.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v3 07/13] send-email: lazily shell out to "git var"
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-05-23 8:56 ` [PATCH v3 06/13] send-email: lazily load config for a big speedup Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 08/13] send-email: use function syntax instead of barewords Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 d294681cf3..56bd5b0e50 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -578,8 +578,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]);
@@ -767,7 +777,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 || '';
@@ -973,7 +983,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
@@ -1122,7 +1132,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.32.0.rc0.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v3 08/13] send-email: use function syntax instead of barewords
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-05-23 8:56 ` [PATCH v3 07/13] send-email: lazily shell out to "git var" Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 09/13] send-email: get rid of indirect object syntax Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 56bd5b0e50..83f764a8bf 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -696,7 +696,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...
@@ -782,7 +782,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.32.0.rc0.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v3 09/13] send-email: get rid of indirect object syntax
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-05-23 8:56 ` [PATCH v3 08/13] send-email: use function syntax instead of barewords Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 10/13] send-email: lazily load modules for a big speedup Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 83f764a8bf..f1542cb3ea 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.32.0.rc0.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v3 10/13] send-email: lazily load modules for a big speedup
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-05-23 8:56 ` [PATCH v3 09/13] send-email: get rid of indirect object syntax Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 11/13] perl: lazily load some common Git.pm setup code Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 71 +++++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 32 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index f1542cb3ea..1e9273fd4f 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);
@@ -309,9 +290,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";
@@ -592,11 +573,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;
@@ -645,10 +628,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 = $_) {
@@ -720,7 +704,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)) {
@@ -733,7 +718,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);
@@ -770,9 +756,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, $!);
@@ -879,6 +866,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};
@@ -886,6 +886,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);
@@ -1066,6 +1067,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);
}
@@ -1125,7 +1127,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++;
@@ -1295,6 +1298,7 @@ sub valid_fqdn {
sub maildomain_net {
my $maildomain;
+ require Net::Domain;
my $domain = Net::Domain::domainname();
$maildomain = $domain if valid_fqdn($domain);
@@ -1305,6 +1309,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;
@@ -1983,13 +1988,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.32.0.rc0.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v3 11/13] perl: lazily load some common Git.pm setup code
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2021-05-23 8:56 ` [PATCH v3 10/13] send-email: lazily load modules for a big speedup Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 12/13] send-email: move trivial config handling to Perl Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
perl/Git.pm | 35 ++++++++++++++++++-----------------
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index 73ebbf80cc..4d048f307b 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.32.0.rc0.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v3 12/13] send-email: move trivial config handling to Perl
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
` (10 preceding siblings ...)
2021-05-23 8:56 ` [PATCH v3 11/13] perl: lazily load some common Git.pm setup code Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-23 8:56 ` [PATCH v3 13/13] perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd() Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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.
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 <avarab@gmail.com>
diff --git a/git-send-email.perl b/git-send-email.perl
index 1e9273fd4f..1ea4d9589d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -324,7 +324,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;
@@ -353,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;
}
@@ -371,12 +373,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
@@ -389,8 +398,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
---
git-send-email.perl | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 1e9273fd4f..1ea4d9589d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -324,7 +324,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;
@@ -353,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;
}
@@ -371,12 +373,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
@@ -389,8 +398,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
--
2.32.0.rc0.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v3 13/13] perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd()
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
` (11 preceding siblings ...)
2021-05-23 8:56 ` [PATCH v3 12/13] send-email: move trivial config handling to Perl Ævar Arnfjörð Bjarmason
@ 2021-05-23 8:56 ` Ævar Arnfjörð Bjarmason
2021-05-24 1:15 ` [PATCH v3 00/13] send-email: various optimizations to speed up by >2x Junio C Hamano
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-23 8:56 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 2 +-
perl/Git.pm | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 1ea4d9589d..c6af05e8a3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -2007,7 +2007,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 4d048f307b..863bd80694 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;
--
2.32.0.rc0.406.g05cb3eebfc
^ permalink raw reply related [flat|nested] 108+ messages in thread
* Re: [PATCH v3 00/13] send-email: various optimizations to speed up by >2x
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
` (12 preceding siblings ...)
2021-05-23 8:56 ` [PATCH v3 13/13] perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd() Ævar Arnfjörð Bjarmason
@ 2021-05-24 1:15 ` Junio C Hamano
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
14 siblings, 0 replies; 108+ messages in thread
From: Junio C Hamano @ 2021-05-24 1:15 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Gregory Anders, Đoàn Trần Công Danh,
Jeff King, Eric Sunshine, Eric Wong, Felipe Contreras
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> A re-roll of v2[1] which fixes issues pointed out with v2, plus some
> others I noticed along the way.
"move trivial" step does not seem to apply to master, next, or jch
and it does not seem to apply to master...ab/send-email-optim
either.
On top of what commit were these patches designed to be applied?
Thanks.
^ permalink raw reply [flat|nested] 108+ messages in thread
* [PATCH v4 00/13] send-email: various optimizations to speed up by >2x
2021-05-23 8:56 ` [PATCH v3 00/13] " Ævar Arnfjörð Bjarmason
` (13 preceding siblings ...)
2021-05-24 1:15 ` [PATCH v3 00/13] send-email: various optimizations to speed up by >2x Junio C Hamano
@ 2021-05-24 7:52 ` Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 01/13] send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true Ævar Arnfjörð Bjarmason
` (14 more replies)
14 siblings, 15 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
This v4 fixes an issue in v3 where 12/13 had a "diff --git" as part of
the commit message (mistake during rebase/squash), which confused "git
am" in trying to apply a diff twice. See <xmqqwnrplyns.fsf@gitster.g>.
Ævar Arnfjörð Bjarmason (13):
send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true
send-email tests: test for boolean variables without a value
send-email: remove non-working support for "sendemail.smtpssl"
send-email: refactor sendemail.smtpencryption config parsing
send-email: copy "config_regxp" into git-send-email.perl
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 | 174 +++++++++++++++++++----------
perl/Git.pm | 35 +++---
t/t9001-send-email.sh | 29 +++++
4 files changed, 160 insertions(+), 81 deletions(-)
Range-diff against v3:
1: 71f890dc603 = 1: 7140847367c send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true
2: 707c2ca5563 = 2: d27f3b48f85 send-email tests: test for boolean variables without a value
3: 3bbd48dab23 = 3: a7a21b75f2e send-email: remove non-working support for "sendemail.smtpssl"
4: bed0f98d681 = 4: 7356a528589 send-email: refactor sendemail.smtpencryption config parsing
5: c12f69a4110 = 5: cce0f89143b send-email: copy "config_regxp" into git-send-email.perl
6: d1c233d2515 = 6: 8afe8661761 send-email: lazily load config for a big speedup
7: 4326c2f99c1 = 7: 491eefde6a2 send-email: lazily shell out to "git var"
8: e1fc71e3f90 = 8: 860156013f8 send-email: use function syntax instead of barewords
9: a806ce06f18 = 9: dd24f1249f5 send-email: get rid of indirect object syntax
10: aa11439789d = 10: 61e3e3c93c5 send-email: lazily load modules for a big speedup
11: b3b342b173b = 11: ada34374286 perl: lazily load some common Git.pm setup code
12: 950dc0f53dd ! 12: 3818000bfba send-email: move trivial config handling to Perl
@@ Commit message
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- diff --git a/git-send-email.perl b/git-send-email.perl
- index 1e9273fd4f..1ea4d9589d 100755
- --- a/git-send-email.perl
- +++ b/git-send-email.perl
- @@ -324,7 +324,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;
- @@ -353,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;
- }
- @@ -371,12 +373,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
- @@ -389,8 +398,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
-
## git-send-email.perl ##
@@ git-send-email.perl: sub read_config {
my $target = $config_bool_settings{$setting};
13: c1d7ea664ac = 13: d36b57e429f perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd()
--
2.32.0.rc1.385.g46e826f1e55
^ permalink raw reply [flat|nested] 108+ messages in thread
* [PATCH v4 01/13] send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:52 ` Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 02/13] send-email tests: test for boolean variables without a value Ævar Arnfjörð Bjarmason
` (13 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 175da07d946..cffdfdacfb9 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;
--
2.32.0.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v4 02/13] send-email tests: test for boolean variables without a value
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 01/13] send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:52 ` Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 03/13] send-email: remove non-working support for "sendemail.smtpssl" Ævar Arnfjörð Bjarmason
` (12 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
The Git.pm code does its own Perl-ifying of boolean variables, let's
ensure that empty values = true for boolean variables, as in the C
code.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t9001-send-email.sh | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 65b30353719..6bf79c816a6 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1349,6 +1349,16 @@ test_expect_success $PREREQ 'sendemail.identity: bool variable fallback' '
! grep "X-Mailer" stdout
'
+test_expect_success $PREREQ 'sendemail.identity: bool variable without a value' '
+ git -c sendemail.xmailer \
+ send-email \
+ --dry-run \
+ --from="nobody@example.com" \
+ $patches >stdout &&
+ grep "To: default@example.com" stdout &&
+ grep "X-Mailer" stdout
+'
+
test_expect_success $PREREQ '--no-to overrides sendemail.to' '
git send-email \
--dry-run \
@@ -2073,6 +2083,18 @@ test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=true' '
do_xmailer_test 1 "--xmailer"
'
+test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer' '
+ test_when_finished "test_unconfig sendemail.xmailer" &&
+ cat >>.git/config <<-\EOF &&
+ [sendemail]
+ xmailer
+ EOF
+ test_config sendemail.xmailer true &&
+ do_xmailer_test 1 "" &&
+ do_xmailer_test 0 "--no-xmailer" &&
+ do_xmailer_test 1 "--xmailer"
+'
+
test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=false' '
test_config sendemail.xmailer false &&
do_xmailer_test 0 "" &&
@@ -2080,6 +2102,13 @@ test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=false' '
do_xmailer_test 1 "--xmailer"
'
+test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=' '
+ test_config sendemail.xmailer "" &&
+ do_xmailer_test 0 "" &&
+ do_xmailer_test 0 "--no-xmailer" &&
+ do_xmailer_test 1 "--xmailer"
+'
+
test_expect_success $PREREQ 'setup expected-list' '
git send-email \
--dry-run \
--
2.32.0.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v4 03/13] send-email: remove non-working support for "sendemail.smtpssl"
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 01/13] send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 02/13] send-email tests: test for boolean variables without a value Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:52 ` Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 04/13] send-email: refactor sendemail.smtpencryption config parsing Ævar Arnfjörð Bjarmason
` (11 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 cffdfdacfb9..24d73df5a3e 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.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v4 04/13] send-email: refactor sendemail.smtpencryption config parsing
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-05-24 7:52 ` [PATCH v4 03/13] send-email: remove non-working support for "sendemail.smtpssl" Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:52 ` Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 05/13] send-email: copy "config_regxp" into git-send-email.perl Ævar Arnfjörð Bjarmason
` (10 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 24d73df5a3e..3d8362b5f07 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.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v4 05/13] send-email: copy "config_regxp" into git-send-email.perl
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-05-24 7:52 ` [PATCH v4 04/13] send-email: refactor sendemail.smtpencryption config parsing Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:52 ` Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 06/13] send-email: lazily load config for a big speedup Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 3d8362b5f07..95a69d4c51f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -380,6 +380,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");
@@ -478,7 +496,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");
--
2.32.0.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v4 06/13] send-email: lazily load config for a big speedup
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-05-24 7:52 ` [PATCH v4 05/13] send-email: copy "config_regxp" into git-send-email.perl Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:52 ` Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 07/13] send-email: lazily shell out to "git var" Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 95a69d4c51f..d294681cf39 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;
@@ -398,9 +404,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,
@@ -411,8 +428,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
@@ -496,7 +513,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");
--
2.32.0.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v4 07/13] send-email: lazily shell out to "git var"
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-05-24 7:52 ` [PATCH v4 06/13] send-email: lazily load config for a big speedup Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:52 ` Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 08/13] send-email: use function syntax instead of barewords Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 d294681cf39..56bd5b0e50c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -578,8 +578,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]);
@@ -767,7 +777,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 || '';
@@ -973,7 +983,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
@@ -1122,7 +1132,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.32.0.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v4 08/13] send-email: use function syntax instead of barewords
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-05-24 7:52 ` [PATCH v4 07/13] send-email: lazily shell out to "git var" Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:52 ` Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 09/13] send-email: get rid of indirect object syntax Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 56bd5b0e50c..83f764a8bf9 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -696,7 +696,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...
@@ -782,7 +782,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.32.0.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v4 09/13] send-email: get rid of indirect object syntax
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-05-24 7:52 ` [PATCH v4 08/13] send-email: use function syntax instead of barewords Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:52 ` Ævar Arnfjörð Bjarmason
2021-05-24 7:52 ` [PATCH v4 10/13] send-email: lazily load modules for a big speedup Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 83f764a8bf9..f1542cb3ea6 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.32.0.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v4 10/13] send-email: lazily load modules for a big speedup
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-05-24 7:52 ` [PATCH v4 09/13] send-email: get rid of indirect object syntax Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:52 ` Ævar Arnfjörð Bjarmason
2021-05-27 1:11 ` Junio C Hamano
2021-05-24 7:53 ` [PATCH v4 11/13] perl: lazily load some common Git.pm setup code Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
14 siblings, 1 reply; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 71 +++++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 32 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index f1542cb3ea6..1e9273fd4f5 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);
@@ -309,9 +290,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";
@@ -592,11 +573,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;
@@ -645,10 +628,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 = $_) {
@@ -720,7 +704,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)) {
@@ -733,7 +718,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);
@@ -770,9 +756,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, $!);
@@ -879,6 +866,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};
@@ -886,6 +886,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);
@@ -1066,6 +1067,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);
}
@@ -1125,7 +1127,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++;
@@ -1295,6 +1298,7 @@ sub valid_fqdn {
sub maildomain_net {
my $maildomain;
+ require Net::Domain;
my $domain = Net::Domain::domainname();
$maildomain = $domain if valid_fqdn($domain);
@@ -1305,6 +1309,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;
@@ -1983,13 +1988,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.32.0.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* Re: [PATCH v4 10/13] send-email: lazily load modules for a big speedup
2021-05-24 7:52 ` [PATCH v4 10/13] send-email: lazily load modules for a big speedup Ævar Arnfjörð Bjarmason
@ 2021-05-27 1:11 ` Junio C Hamano
2021-05-27 11:36 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 108+ messages in thread
From: Junio C Hamano @ 2021-05-27 1:11 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Gregory Anders, Đoàn Trần Công Danh,
Jeff King, Eric Sunshine, Eric Wong, Felipe Contreras
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> -use File::Temp qw/ tempdir tempfile /;
> -use File::Spec::Functions qw(catdir catfile);
> ...
> + require File::Spec;
> + push @files, grep { -f $_ } map { File::Spec->catfile($f, $_) }
> sort readdir $dh;
> ...
> - 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);
I suspect I would not be the only one who finds it curious to have
the distinction between the use of "->" in File::Spec->catfile() and
"::" in File::Temp::tempdir (and all the other function calls
touched by this patch).
Wouldn't it be more faithful to the original to do:
require File::Spec::Functions;
push @files, ... map { File::Spec::Functions::catfile($f, $_) }
in the first hunk? It would save the time readers of "git log -p"
has to spend on wondering why "catfile" is an oddball.
Of course "man File::Spec" would educate readers where the
difference comes from, but it is not immediately obvious to me why
we want to switch in a "lazily load for speedup" that is supposed to
be optimization-only, even if the class method ends up calling the
same underlying thing.
Thanks.
^ permalink raw reply [flat|nested] 108+ messages in thread
* Re: [PATCH v4 10/13] send-email: lazily load modules for a big speedup
2021-05-27 1:11 ` Junio C Hamano
@ 2021-05-27 11:36 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-27 11:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Gregory Anders, Đoàn Trần Công Danh,
Jeff King, Eric Sunshine, Eric Wong, Felipe Contreras
On Thu, May 27 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> -use File::Temp qw/ tempdir tempfile /;
>> -use File::Spec::Functions qw(catdir catfile);
>> ...
>> + require File::Spec;
>> + push @files, grep { -f $_ } map { File::Spec->catfile($f, $_) }
>> sort readdir $dh;
>> ...
>> - 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);
>
> I suspect I would not be the only one who finds it curious to have
> the distinction between the use of "->" in File::Spec->catfile() and
> "::" in File::Temp::tempdir (and all the other function calls
> touched by this patch).
>
> Wouldn't it be more faithful to the original to do:
>
> require File::Spec::Functions;
> push @files, ... map { File::Spec::Functions::catfile($f, $_) }
>
> in the first hunk? It would save the time readers of "git log -p"
> has to spend on wondering why "catfile" is an oddball.
>
> Of course "man File::Spec" would educate readers where the
> difference comes from, but it is not immediately obvious to me why
> we want to switch in a "lazily load for speedup" that is supposed to
> be optimization-only, even if the class method ends up calling the
> same underlying thing.
It was idiomatic to me so I didn't think of explaining it. Yes it looks
odd, but it's the most straightforward and correct thing to do in this
s/use/require/ change.
The reason to use the File::Spec::Functions wrapper is because you want
File::Spec class methods in your namespace. I.e. to do catfile() instead
of File::Spec->catfile().
To do that requires importing the "catfile" symbol. Since we're doing a
s/use/requires/ here we won't do that (symbols like that are
compile-time), so we'd need to call File::Spec::Functions::catfile() as
you point out.
Except the whole point of that Functions.pm package is to not need that
fully-qualified name, so once we're doing that we might as well call
File::Spec->catfile() instead.
The implementation of File::Spec::Functions is to literally give you a
generaler boilerplate function in your namespace that calls
File::Spec::catfile("File::Spec", <your argument here>), which is what
the "->" class method syntax does implicitly. Calling that wrapper
doesn't make sense here.
^ permalink raw reply [flat|nested] 108+ messages in thread
* [PATCH v4 11/13] perl: lazily load some common Git.pm setup code
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2021-05-24 7:52 ` [PATCH v4 10/13] send-email: lazily load modules for a big speedup Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:53 ` Ævar Arnfjörð Bjarmason
2021-05-24 7:53 ` [PATCH v4 12/13] send-email: move trivial config handling to Perl Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:53 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
perl/Git.pm | 35 ++++++++++++++++++-----------------
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index 73ebbf80cc6..4d048f307b3 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.32.0.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v4 12/13] send-email: move trivial config handling to Perl
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (10 preceding siblings ...)
2021-05-24 7:53 ` [PATCH v4 11/13] perl: lazily load some common Git.pm setup code Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:53 ` Ævar Arnfjörð Bjarmason
2021-05-27 15:57 ` Jeff King
2021-05-24 7:53 ` [PATCH v4 13/13] perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd() Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
14 siblings, 1 reply; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:53 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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.
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 <avarab@gmail.com>
---
git-send-email.perl | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 1e9273fd4f5..1ea4d9589d8 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -324,7 +324,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;
@@ -353,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;
}
@@ -371,12 +373,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
@@ -389,8 +398,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
--
2.32.0.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* Re: [PATCH v4 12/13] send-email: move trivial config handling to Perl
2021-05-24 7:53 ` [PATCH v4 12/13] send-email: move trivial config handling to Perl Ævar Arnfjörð Bjarmason
@ 2021-05-27 15:57 ` Jeff King
0 siblings, 0 replies; 108+ messages in thread
From: Jeff King @ 2021-05-27 15:57 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Eric Sunshine,
Eric Wong, Felipe Contreras
On Mon, May 24, 2021 at 09:53:01AM +0200, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 1e9273fd4f5..1ea4d9589d8 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -324,7 +324,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);
Thanks for addressing this. It looks like an undefined value will kick
back to the Git::config_bool() case. That's probably fine, as I'd think
it's relatively rare (and this is all just optimization anyway).
> @@ -353,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;
> }
I do wonder what happens for non-bool values here. I.e., would we return
a list that contains undef? Before your change, the value comes from
Git::config(), and now it comes from our pre-read $known_keys.
It seems fine either way:
$ git -c sendemail.smtpsslcertpath send-email -1
error: missing value for 'sendemail.smtpsslcertpath'
fatal: unable to parse command-line config
config --path --get sendemail.smtpsslcertpath: command returned error: 128
but I didn't carefully follow all the paths that config can take. So I
raise it only as a potential issue. Your response is hopefully "yes, I
thought of that, and it is fine." :)
-Peff
^ permalink raw reply [flat|nested] 108+ messages in thread
* [PATCH v4 13/13] perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd()
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (11 preceding siblings ...)
2021-05-24 7:53 ` [PATCH v4 12/13] send-email: move trivial config handling to Perl Ævar Arnfjörð Bjarmason
@ 2021-05-24 7:53 ` Ævar Arnfjörð Bjarmason
2021-05-27 16:00 ` [PATCH v4 00/13] send-email: various optimizations to speed up by >2x Jeff King
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-24 7:53 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 2 +-
perl/Git.pm | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 1ea4d9589d8..c6af05e8a31 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -2007,7 +2007,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 4d048f307b3..863bd80694c 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;
--
2.32.0.rc1.385.g46e826f1e55
^ permalink raw reply related [flat|nested] 108+ messages in thread
* Re: [PATCH v4 00/13] send-email: various optimizations to speed up by >2x
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (12 preceding siblings ...)
2021-05-24 7:53 ` [PATCH v4 13/13] perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd() Ævar Arnfjörð Bjarmason
@ 2021-05-27 16:00 ` Jeff King
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
14 siblings, 0 replies; 108+ messages in thread
From: Jeff King @ 2021-05-27 16:00 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Eric Sunshine,
Eric Wong, Felipe Contreras
On Mon, May 24, 2021 at 09:52:49AM +0200, Ævar Arnfjörð Bjarmason wrote:
> This v4 fixes an issue in v3 where 12/13 had a "diff --git" as part of
> the commit message (mistake during rebase/squash), which confused "git
> am" in trying to apply a diff twice. See <xmqqwnrplyns.fsf@gitster.g>.
I raised a probably-not-a-problem question in patch 12. Assuming it's
indeed not-a-problem, this all looks good to me.
Thanks for addressing my concerns about Git.pm stability.
-Peff
PS I hit some mild conflicts applying this on top of master because
7cbc0455cc (send-email: move "hooks_path" invocation to
git-send-email.perl, 2021-05-26) graduated in the meantime. I expect
Junio prefers it as you have it here, since he tends to use the
original base from previous rounds, but just a hint to any other
reviewers.
^ permalink raw reply [flat|nested] 108+ messages in thread
* [PATCH v5 00/13] send-email: various optimizations to speed up by >2x
2021-05-24 7:52 ` [PATCH v4 " Ævar Arnfjörð Bjarmason
` (13 preceding siblings ...)
2021-05-27 16:00 ` [PATCH v4 00/13] send-email: various optimizations to speed up by >2x Jeff King
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 01/13] send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true Ævar Arnfjörð Bjarmason
` (14 more replies)
14 siblings, 15 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
Hopefully the final iteration. Updates a commit message to explain why
I moved away from File::Spec::Functions, rebases on master, and
explains and deals with the "undef in config" issue Jeff King noted.
Ævar Arnfjörð Bjarmason (13):
send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true
send-email tests: test for boolean variables without a value
send-email: remove non-working support for "sendemail.smtpssl"
send-email: refactor sendemail.smtpencryption config parsing
send-email: copy "config_regxp" into git-send-email.perl
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 | 174 +++++++++++++++++++----------
perl/Git.pm | 32 +++---
t/t9001-send-email.sh | 29 +++++
4 files changed, 159 insertions(+), 79 deletions(-)
Range-diff against v4:
1: 7140847367c = 1: 81025b48f1c send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true
2: d27f3b48f85 = 2: 16277bd1082 send-email tests: test for boolean variables without a value
3: a7a21b75f2e = 3: e3e3e6415d2 send-email: remove non-working support for "sendemail.smtpssl"
4: 7356a528589 = 4: 961ca4c2b2a send-email: refactor sendemail.smtpencryption config parsing
5: cce0f89143b = 5: f2bd12728a1 send-email: copy "config_regxp" into git-send-email.perl
6: 8afe8661761 = 6: 4cf70c6f97e send-email: lazily load config for a big speedup
7: 491eefde6a2 = 7: bd0d9535718 send-email: lazily shell out to "git var"
8: 860156013f8 = 8: f1a879a8ae9 send-email: use function syntax instead of barewords
9: dd24f1249f5 = 9: 881b1093409 send-email: get rid of indirect object syntax
10: 61e3e3c93c5 ! 10: 9f21bc6e6f2 send-email: lazily load modules for a big speedup
@@ Commit message
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 <avarab@gmail.com>
## git-send-email.perl ##
@@ git-send-email.perl: sub maildomain_mta {
if (defined $smtp) {
my $domain = $smtp->domain;
@@ git-send-email.perl: 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,
+ require File::Spec;
-+ my $validate_hook = File::Spec->catfile($repo->hooks_path(),
++ my $validate_hook = File::Spec->catfile($hooks_path,
'sendemail-validate');
my $hook_error;
if (-x $validate_hook) {
11: ada34374286 ! 11: 66f68e38c16 perl: lazily load some common Git.pm setup code
@@ perl/Git.pm: sub get_tz_offset {
my $sign = qw( + + - )[ $gm <=> $t ];
return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
}
-@@ perl/Git.pm: 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;
- }
-
@@ perl/Git.pm: sub _temp_cache {
my $n = $name;
$n =~ s/\W/_/g; # no strange chars
12: 3818000bfba ! 12: f605b5ae49f send-email: move trivial config handling to Perl
@@ Commit message
"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
@@ git-send-email.perl: sub read_config {
- 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);
-- next unless defined $v;
+ my $v = $known_keys->{$key}->[0];
+ next unless defined $v;
next if $configured->{$setting}++;
$$target = $v;
- }
@@ git-send-email.perl: sub config_regexp {
my ($regex) = @_;
my @ret;
13: d36b57e429f = 13: aa3a2de7047 perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd()
--
2.32.0.rc1.458.gd885d4f985c
^ permalink raw reply [flat|nested] 108+ messages in thread
* [PATCH v5 01/13] send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 02/13] send-email tests: test for boolean variables without a value Ævar Arnfjörð Bjarmason
` (13 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 25be2ebd2af..1630e87cd4d 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;
--
2.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v5 02/13] send-email tests: test for boolean variables without a value
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 01/13] send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-07-10 23:23 ` Jeff King
2021-05-28 9:23 ` [PATCH v5 03/13] send-email: remove non-working support for "sendemail.smtpssl" Ævar Arnfjörð Bjarmason
` (12 subsequent siblings)
14 siblings, 1 reply; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
The Git.pm code does its own Perl-ifying of boolean variables, let's
ensure that empty values = true for boolean variables, as in the C
code.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t9001-send-email.sh | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 3b7540050ca..612de095fd7 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1368,6 +1368,16 @@ test_expect_success $PREREQ 'sendemail.identity: bool variable fallback' '
! grep "X-Mailer" stdout
'
+test_expect_success $PREREQ 'sendemail.identity: bool variable without a value' '
+ git -c sendemail.xmailer \
+ send-email \
+ --dry-run \
+ --from="nobody@example.com" \
+ $patches >stdout &&
+ grep "To: default@example.com" stdout &&
+ grep "X-Mailer" stdout
+'
+
test_expect_success $PREREQ '--no-to overrides sendemail.to' '
git send-email \
--dry-run \
@@ -2092,6 +2102,18 @@ test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=true' '
do_xmailer_test 1 "--xmailer"
'
+test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer' '
+ test_when_finished "test_unconfig sendemail.xmailer" &&
+ cat >>.git/config <<-\EOF &&
+ [sendemail]
+ xmailer
+ EOF
+ test_config sendemail.xmailer true &&
+ do_xmailer_test 1 "" &&
+ do_xmailer_test 0 "--no-xmailer" &&
+ do_xmailer_test 1 "--xmailer"
+'
+
test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=false' '
test_config sendemail.xmailer false &&
do_xmailer_test 0 "" &&
@@ -2099,6 +2121,13 @@ test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=false' '
do_xmailer_test 1 "--xmailer"
'
+test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=' '
+ test_config sendemail.xmailer "" &&
+ do_xmailer_test 0 "" &&
+ do_xmailer_test 0 "--no-xmailer" &&
+ do_xmailer_test 1 "--xmailer"
+'
+
test_expect_success $PREREQ 'setup expected-list' '
git send-email \
--dry-run \
--
2.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* Re: [PATCH v5 02/13] send-email tests: test for boolean variables without a value
2021-05-28 9:23 ` [PATCH v5 02/13] send-email tests: test for boolean variables without a value Ævar Arnfjörð Bjarmason
@ 2021-07-10 23:23 ` Jeff King
0 siblings, 0 replies; 108+ messages in thread
From: Jeff King @ 2021-07-10 23:23 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Eric Sunshine,
Eric Wong, Felipe Contreras
On Fri, May 28, 2021 at 11:23:41AM +0200, Ævar Arnfjörð Bjarmason wrote:
> +test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer' '
> + test_when_finished "test_unconfig sendemail.xmailer" &&
> + cat >>.git/config <<-\EOF &&
> + [sendemail]
> + xmailer
> + EOF
> + test_config sendemail.xmailer true &&
> + do_xmailer_test 1 "" &&
> + do_xmailer_test 0 "--no-xmailer" &&
> + do_xmailer_test 1 "--xmailer"
> +'
Is this test_config call in the middle of the snippet meant to be there?
It seems like it would wreck the test, since it will rewrite the file to
"xmailer = true". I'm guessing it's just leftover cruft from adapting
the earlier xmailer=true test?
(I wasn't looking carefully again at this series, but just happened to
be resolving a local conflict involving this hunk and noticed it).
-Peff
^ permalink raw reply [flat|nested] 108+ messages in thread
* [PATCH v5 03/13] send-email: remove non-working support for "sendemail.smtpssl"
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 01/13] send-email tests: support GIT_TEST_PERL_FATAL_WARNINGS=true Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 02/13] send-email tests: test for boolean variables without a value Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 04/13] send-email: refactor sendemail.smtpencryption config parsing Ævar Arnfjörð Bjarmason
` (11 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 1630e87cd4d..9a1a4898e36 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;
}
}
--
2.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v5 04/13] send-email: refactor sendemail.smtpencryption config parsing
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-05-28 9:23 ` [PATCH v5 03/13] send-email: remove non-working support for "sendemail.smtpssl" Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 05/13] send-email: copy "config_regxp" into git-send-email.perl Ævar Arnfjörð Bjarmason
` (10 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 9a1a4898e36..e2fe112aa50 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
--
2.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v5 05/13] send-email: copy "config_regxp" into git-send-email.perl
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-05-28 9:23 ` [PATCH v5 04/13] send-email: refactor sendemail.smtpencryption config parsing Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 06/13] send-email: lazily load config for a big speedup Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index e2fe112aa50..73e3d3fd26e 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");
--
2.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v5 06/13] send-email: lazily load config for a big speedup
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-05-28 9:23 ` [PATCH v5 05/13] send-email: copy "config_regxp" into git-send-email.perl Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 07/13] send-email: lazily shell out to "git var" Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 73e3d3fd26e..de62cbf2506 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");
--
2.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v5 07/13] send-email: lazily shell out to "git var"
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-05-28 9:23 ` [PATCH v5 06/13] send-email: lazily load config for a big speedup Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 08/13] send-email: use function syntax instead of barewords Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 de62cbf2506..38408ec75a8 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 '');
}
--
2.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v5 08/13] send-email: use function syntax instead of barewords
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-05-28 9:23 ` [PATCH v5 07/13] send-email: lazily shell out to "git var" Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 16:10 ` Felipe Contreras
2021-05-28 9:23 ` [PATCH v5 09/13] send-email: get rid of indirect object syntax Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
14 siblings, 1 reply; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 38408ec75a8..44dc3f6eb10 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(__ <<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...
@@ -792,7 +792,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.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* RE: [PATCH v5 08/13] send-email: use function syntax instead of barewords
2021-05-28 9:23 ` [PATCH v5 08/13] send-email: use function syntax instead of barewords Ævar Arnfjörð Bjarmason
@ 2021-05-28 16:10 ` Felipe Contreras
2021-05-29 8:17 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 108+ messages in thread
From: Felipe Contreras @ 2021-05-28 16:10 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
Æ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.
Agreed. Ruby has something similar:
foo arg1, arg2
foo(arg1, arg2)
As a C programmer I find the former a quirk of the language, and much
harder to read.
If it's a function call, present it as such.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 108+ messages in thread
* Re: [PATCH v5 08/13] send-email: use function syntax instead of barewords
2021-05-28 16:10 ` Felipe Contreras
@ 2021-05-29 8:17 ` Ævar Arnfjörð Bjarmason
2021-05-29 14:25 ` Felipe Contreras
0 siblings, 1 reply; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-29 8:17 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong
On Fri, May 28 2021, Felipe Contreras wrote:
> Æ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.
>
> Agreed. Ruby has something similar:
>
> foo arg1, arg2
> foo(arg1, arg2)
>
> As a C programmer I find the former a quirk of the language, and much
> harder to read.
>
> If it's a function call, present it as such.
I'm not too familiar with Ruby, but the important part here in Perl is
that it's not a "foo()" v.s. "foo" shortcut for a function invocation,
it's unambiguous whether that is a function and what its arguments are
until at that point in the program. Perl has dynamic grammar, you can't
parse it with a regular grammar without executing it.
^ permalink raw reply [flat|nested] 108+ messages in thread
* Re: [PATCH v5 08/13] send-email: use function syntax instead of barewords
2021-05-29 8:17 ` Ævar Arnfjörð Bjarmason
@ 2021-05-29 14:25 ` Felipe Contreras
0 siblings, 0 replies; 108+ messages in thread
From: Felipe Contreras @ 2021-05-29 14:25 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, Felipe Contreras
Cc: git, Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong
Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, May 28 2021, Felipe Contreras wrote:
>
> > Æ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.
> >
> > Agreed. Ruby has something similar:
> >
> > foo arg1, arg2
> > foo(arg1, arg2)
> >
> > As a C programmer I find the former a quirk of the language, and much
> > harder to read.
> >
> > If it's a function call, present it as such.
>
> I'm not too familiar with Ruby, but the important part here in Perl is
> that it's not a "foo()" v.s. "foo" shortcut for a function invocation,
> it's unambiguous whether that is a function and what its arguments are
> until at that point in the program. Perl has dynamic grammar, you can't
> parse it with a regular grammar without executing it.
Yes, that is the important part, but not the only part.
I'm saying in addition it's easier to read for most git.git developers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 108+ messages in thread
* [PATCH v5 09/13] send-email: get rid of indirect object syntax
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-05-28 9:23 ` [PATCH v5 08/13] send-email: use function syntax instead of barewords Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 10/13] send-email: lazily load modules for a big speedup Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 44dc3f6eb10..cc1027d8774 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.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v5 10/13] send-email: lazily load modules for a big speedup
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-05-28 9:23 ` [PATCH v5 09/13] send-email: get rid of indirect object syntax Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 15:55 ` Felipe Contreras
2021-05-28 9:23 ` [PATCH v5 11/13] perl: lazily load some common Git.pm setup code Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
14 siblings, 1 reply; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 71 +++++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 32 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index cc1027d8774..a8949c9d313 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();
--
2.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* RE: [PATCH v5 10/13] send-email: lazily load modules for a big speedup
2021-05-28 9:23 ` [PATCH v5 10/13] send-email: lazily load modules for a big speedup Ævar Arnfjörð Bjarmason
@ 2021-05-28 15:55 ` Felipe Contreras
2021-05-29 8:12 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 108+ messages in thread
From: Felipe Contreras @ 2021-05-28 15:55 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
Ævar Arnfjörð Bjarmason wrote:
> 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.
I would reference `man File::Spec` rather than an email.
And while this change makes sense, I think it should be split in two.
Instead of doing:
-use Term::ANSIColor;
-print color("reset"), "\n";
+require Term::ANSIColor;
+print Term::ANSIColor::color("reset"), "\n";
We could do this in one patch:
-print color("reset"), "\n";
+print Term::ANSIColor::color("reset"), "\n";
That is just no-op noise that we can mostly ignore in the review.
Then the actual change to require Term::ANSIColor selectively would be
much simpler to see.
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 108+ messages in thread
* Re: [PATCH v5 10/13] send-email: lazily load modules for a big speedup
2021-05-28 15:55 ` Felipe Contreras
@ 2021-05-29 8:12 ` Ævar Arnfjörð Bjarmason
2021-05-29 14:24 ` Felipe Contreras
0 siblings, 1 reply; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-29 8:12 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong
On Fri, May 28 2021, Felipe Contreras wrote:
> Ævar Arnfjörð Bjarmason wrote:
>> 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.
>
> I would reference `man File::Spec` rather than an email.
>
> And while this change makes sense, I think it should be split in two.
>
> Instead of doing:
>
> -use Term::ANSIColor;
> -print color("reset"), "\n";
> +require Term::ANSIColor;
> +print Term::ANSIColor::color("reset"), "\n";
>
> We could do this in one patch:
>
> -print color("reset"), "\n";
> +print Term::ANSIColor::color("reset"), "\n";
>
> That is just no-op noise that we can mostly ignore in the review.
>
> Then the actual change to require Term::ANSIColor selectively would be
> much simpler to see.
You mean do the change from imported functions in one commit, and then
sprinkle the "require" in another one?
I think it's clearer this way, you can't really assert that it worked as
intended (i.e. you have no more imports) without the s/use/require/g, so
it makes sense to do both as one atomic change.
^ permalink raw reply [flat|nested] 108+ messages in thread
* Re: [PATCH v5 10/13] send-email: lazily load modules for a big speedup
2021-05-29 8:12 ` Ævar Arnfjörð Bjarmason
@ 2021-05-29 14:24 ` Felipe Contreras
0 siblings, 0 replies; 108+ messages in thread
From: Felipe Contreras @ 2021-05-29 14:24 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, Felipe Contreras
Cc: git, Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong
Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, May 28 2021, Felipe Contreras wrote:
>
> > Ævar Arnfjörð Bjarmason wrote:
> >> 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.
> >
> > I would reference `man File::Spec` rather than an email.
> >
> > And while this change makes sense, I think it should be split in two.
> >
> > Instead of doing:
> >
> > -use Term::ANSIColor;
> > -print color("reset"), "\n";
> > +require Term::ANSIColor;
> > +print Term::ANSIColor::color("reset"), "\n";
> >
> > We could do this in one patch:
> >
> > -print color("reset"), "\n";
> > +print Term::ANSIColor::color("reset"), "\n";
> >
> > That is just no-op noise that we can mostly ignore in the review.
> >
> > Then the actual change to require Term::ANSIColor selectively would be
> > much simpler to see.
>
> You mean do the change from imported functions in one commit, and then
> sprinkle the "require" in another one?
Yes.
> I think it's clearer this way, you can't really assert that it worked as
> intended (i.e. you have no more imports) without the s/use/require/g, so
> it makes sense to do both as one atomic change.
It is clear, but there's some noise.
And I don't see how you can assert there are no more imports from this
patch, especially when the next patch gets rid of more imports.
Moreover, it's already the case that the code uses namespaces in some
cases: Net::Domain::domainname is one you did not have to convert. You
would be doing the same for all other instances.
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 108+ messages in thread
* [PATCH v5 11/13] perl: lazily load some common Git.pm setup code
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2021-05-28 9:23 ` [PATCH v5 10/13] send-email: lazily load modules for a big speedup Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 12/13] send-email: move trivial config handling to Perl Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
perl/Git.pm | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index 02eacef0c2a..5562c0cede2 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]);
}
@@ -1340,6 +1339,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");
@@ -1362,9 +1362,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.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v5 12/13] send-email: move trivial config handling to Perl
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
` (10 preceding siblings ...)
2021-05-28 9:23 ` [PATCH v5 11/13] perl: lazily load some common Git.pm setup code Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 9:23 ` [PATCH v5 13/13] perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd() Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index a8949c9d313..57911386835 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
--
2.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* [PATCH v5 13/13] perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd()
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
` (11 preceding siblings ...)
2021-05-28 9:23 ` [PATCH v5 12/13] send-email: move trivial config handling to Perl Ævar Arnfjörð Bjarmason
@ 2021-05-28 9:23 ` Ævar Arnfjörð Bjarmason
2021-05-28 16:13 ` [PATCH v5 00/13] send-email: various optimizations to speed up by >2x Felipe Contreras
2021-05-31 5:48 ` Jeff King
14 siblings, 0 replies; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-28 9:23 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
git-send-email.perl | 2 +-
perl/Git.pm | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 57911386835..0efe85c0b02 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 5562c0cede2..090a7df63fc 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;
--
2.32.0.rc1.458.gd885d4f985c
^ permalink raw reply related [flat|nested] 108+ messages in thread
* RE: [PATCH v5 00/13] send-email: various optimizations to speed up by >2x
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
` (12 preceding siblings ...)
2021-05-28 9:23 ` [PATCH v5 13/13] perl: nano-optimize by replacing Cwd::cwd() with Cwd::getcwd() Ævar Arnfjörð Bjarmason
@ 2021-05-28 16:13 ` Felipe Contreras
2021-05-31 5:48 ` Jeff King
14 siblings, 0 replies; 108+ messages in thread
From: Felipe Contreras @ 2021-05-28 16:13 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Jeff King,
Eric Sunshine, Eric Wong, Felipe Contreras,
Ævar Arnfjörð Bjarmason
Ævar Arnfjörð Bjarmason wrote:
> Hopefully the final iteration. Updates a commit message to explain why
> I moved away from File::Spec::Functions, rebases on master, and
> explains and deals with the "undef in config" issue Jeff King noted.
The series looks good to me, except the one comment about splitting one
patch, but not a deal-breaker.
For what it's worth:
Reviewed-by: Felipe Contreras <felipe.contreras@gmail.com>
--
Felipe Contreras
^ permalink raw reply [flat|nested] 108+ messages in thread
* Re: [PATCH v5 00/13] send-email: various optimizations to speed up by >2x
2021-05-28 9:23 ` [PATCH v5 " Ævar Arnfjörð Bjarmason
` (13 preceding siblings ...)
2021-05-28 16:13 ` [PATCH v5 00/13] send-email: various optimizations to speed up by >2x Felipe Contreras
@ 2021-05-31 5:48 ` Jeff King
2021-05-31 9:53 ` Ævar Arnfjörð Bjarmason
14 siblings, 1 reply; 108+ messages in thread
From: Jeff King @ 2021-05-31 5:48 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Eric Sunshine,
Eric Wong, Felipe Contreras
On Fri, May 28, 2021 at 11:23:39AM +0200, Ævar Arnfjörð Bjarmason wrote:
> Hopefully the final iteration. Updates a commit message to explain why
> I moved away from File::Spec::Functions, rebases on master, and
> explains and deals with the "undef in config" issue Jeff King noted.
Thanks. The solution was less invasive than I feared.
I guess here:
> @@ git-send-email.perl: sub read_config {
> - 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);
> -- next unless defined $v;
> + my $v = $known_keys->{$key}->[0];
> + next unless defined $v;
> next if $configured->{$setting}++;
> $$target = $v;
> - }
we'd ignore such undef values, whereas presumably before they'd have
triggered an error via Git::config(). I don't think it matters all that
much either way, though.
-Peff
^ permalink raw reply [flat|nested] 108+ messages in thread
* Re: [PATCH v5 00/13] send-email: various optimizations to speed up by >2x
2021-05-31 5:48 ` Jeff King
@ 2021-05-31 9:53 ` Ævar Arnfjörð Bjarmason
2021-05-31 14:38 ` Jeff King
0 siblings, 1 reply; 108+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-31 9:53 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Eric Sunshine,
Eric Wong, Felipe Contreras
On Mon, May 31 2021, Jeff King wrote:
> On Fri, May 28, 2021 at 11:23:39AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Hopefully the final iteration. Updates a commit message to explain why
>> I moved away from File::Spec::Functions, rebases on master, and
>> explains and deals with the "undef in config" issue Jeff King noted.
>
> Thanks. The solution was less invasive than I feared.
>
> I guess here:
>
>> @@ git-send-email.perl: sub read_config {
>> - 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);
>> -- next unless defined $v;
>> + my $v = $known_keys->{$key}->[0];
>> + next unless defined $v;
>> next if $configured->{$setting}++;
>> $$target = $v;
>> - }
>
> we'd ignore such undef values, whereas presumably before they'd have
> triggered an error via Git::config(). I don't think it matters all that
> much either way, though.
They didn't error before, they were treated the same as the empty
string.
This is because Git.pm uses "git config --get" to retrieve them, but we
now use "--list --null".
This ultimately comes down to a limitation of git-config's one-shot CLI
interface. You get the empty string and zero exit code for both of:
git -c foo.bar= config --get foo.bar
git -c foo.bar config --get foo.bar
Ignoring them here for list values is technically a behavior change if
we were treating this as a black box, but in reality we know that these
could only conceivable be useful for the bool values, for list values
like "list of e-mail addresses" it makes no sense to have that in a the
middle of a list, and we were implicitly ignoring them anyway before
when we processed the empty string.
^ permalink raw reply [flat|nested] 108+ messages in thread
* Re: [PATCH v5 00/13] send-email: various optimizations to speed up by >2x
2021-05-31 9:53 ` Ævar Arnfjörð Bjarmason
@ 2021-05-31 14:38 ` Jeff King
0 siblings, 0 replies; 108+ messages in thread
From: Jeff King @ 2021-05-31 14:38 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Gregory Anders,
Đoàn Trần Công Danh, Eric Sunshine,
Eric Wong, Felipe Contreras
On Mon, May 31, 2021 at 11:53:04AM +0200, Ævar Arnfjörð Bjarmason wrote:
> > I guess here:
> >
> >> @@ git-send-email.perl: sub read_config {
> >> - 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);
> >> -- next unless defined $v;
> >> + my $v = $known_keys->{$key}->[0];
> >> + next unless defined $v;
> >> next if $configured->{$setting}++;
> >> $$target = $v;
> >> - }
> >
> > we'd ignore such undef values, whereas presumably before they'd have
> > triggered an error via Git::config(). I don't think it matters all that
> > much either way, though.
>
> They didn't error before, they were treated the same as the empty
> string.
>
> This is because Git.pm uses "git config --get" to retrieve them, but we
> now use "--list --null".
>
> This ultimately comes down to a limitation of git-config's one-shot CLI
> interface. You get the empty string and zero exit code for both of:
>
> git -c foo.bar= config --get foo.bar
> git -c foo.bar config --get foo.bar
Ah, OK. Thanks for clarifying.
> Ignoring them here for list values is technically a behavior change if
> we were treating this as a black box, but in reality we know that these
> could only conceivable be useful for the bool values, for list values
> like "list of e-mail addresses" it makes no sense to have that in a the
> middle of a list, and we were implicitly ignoring them anyway before
> when we processed the empty string.
Right, it was the list one I was most puzzled by. But if we were already
conflating implicit-bool and the empty string, I think it's OK. I agree
that all of this is something that _shouldn't_ happen in the first
place, for well-formed config.
-Peff
^ permalink raw reply [flat|nested] 108+ messages in thread