From dfd46bae92ec25d71e7c8a2887e9508aaf211ee8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 8 Aug 2023 14:14:36 -0400 Subject: [PATCH 1/2] send-email: drop FakeTerm hack Back in 280242d1cc (send-email: do not barf when Term::ReadLine does not like your terminal, 2006-07-02), we added a fallback for when Term::ReadLine's constructor failed: we'd have a FakeTerm object instead, which would then die if anybody actually tried to call readline() on it. Since we instantiated the $term variable at program startup, we needed this workaround to let the program run in modes when we did not prompt the user. But later, in f4dc9432fd (send-email: lazily load modules for a big speedup, 2021-05-28), we started loading Term::ReadLine lazily only when ask() is called. So at that point we know we're trying to prompt the user, and we can just die if ReadLine instantiation fails, rather than making this fake object to lazily delay showing the error. This should be OK even if there is no tty (e.g., we're in a cron job), because Term::ReadLine will return a stub object in that case whose "IN" and "OUT" functions return undef. And since 5906f54e47 (send-email: don't attempt to prompt if tty is closed, 2009-03-31), we check for that case and skip prompting. And we can be sure that FakeTerm was not kicking in for such a situation, because it has actually been broken since that commit! It does not define "IN" or "OUT" methods, so perl would barf with an error. If FakeTerm was in use, we were neither honoring what 5906f54e47 tried to do, nor producing the readable message that 280242d1cc intended. So we're better off just dropping FakeTerm entirely, and letting the error reported by constructing Term::ReadLine through. Signed-off-by: Jeff King Acked-by: Taylor Blau Signed-off-by: Junio C Hamano --- git-send-email.perl | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index affbb88509..4cb6ee9327 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -26,18 +26,6 @@ use Git::I18N; Getopt::Long::Configure qw/ pass_through /; -package FakeTerm; -sub new { - my ($class, $reason) = @_; - return bless \$reason, shift; -} -sub readline { - my $self = shift; - die "Cannot use readline on FakeTerm: $$self"; -} -package main; - - sub usage { print <] @@ -972,16 +960,10 @@ EOT3 } sub term { - my $term = eval { - require Term::ReadLine; - $ENV{"GIT_SEND_EMAIL_NOTTY"} + require Term::ReadLine; + return $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 { From c016726c2deb84bcc6a7418efad92ef0562a8af3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 8 Aug 2023 14:15:31 -0400 Subject: [PATCH 2/2] send-email: avoid creating more than one Term::ReadLine object Every time git-send-email calls its ask() function to prompt the user, we call term(), which instantiates a new Term::ReadLine object. But in v1.46 of Term::ReadLine::Gnu (which provides the Term::ReadLine interface on some platforms), its constructor refuses to create a second instance[1]. So on systems with that version of the module, most git-send-email instances will fail (as we usually prompt for both "to" and "in-reply-to" unless the user provided them on the command line). We can fix this by keeping a single instance variable and returning it for each call to term(). In perl 5.10 and up, we could do that with a "state" variable. But since we only require 5.008, we'll do it the old-fashioned way, with a lexical "my" in its own scope. Note that the tests in t9001 detect this problem as-is, since the failure mode is for the program to die. But let's also beef up the "Prompting works" test to check that it correctly handles multiple inputs (if we had chosen to keep our FakeTerm hack in the previous commit, then the failure mode would be incorrectly ignoring prompts after the first). [1] For discussion of why multiple instances are forbidden, see: https://github.com/hirooih/perl-trg/issues/16 Signed-off-by: Jeff King Acked-by: Taylor Blau Signed-off-by: Junio C Hamano --- git-send-email.perl | 18 +++++++++++++----- t/t9001-send-email.sh | 5 +++-- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index 4cb6ee9327..897cea6564 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -959,11 +959,19 @@ EOT3 do_edit(@files); } -sub term { - require Term::ReadLine; - return $ENV{"GIT_SEND_EMAIL_NOTTY"} - ? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT) - : Term::ReadLine->new('git-send-email'); +{ + # Only instantiate one $term per program run, since some + # Term::ReadLine providers refuse to create a second instance. + my $term; + sub term { + require Term::ReadLine; + if (!defined $term) { + $term = $ENV{"GIT_SEND_EMAIL_NOTTY"} + ? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT) + : Term::ReadLine->new('git-send-email'); + } + return $term; + } } sub ask { diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 22fc908024..b5be8251ee 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -337,13 +337,14 @@ test_expect_success $PREREQ 'Show all headers' ' test_expect_success $PREREQ 'Prompting works' ' clean_fake_sendmail && (echo "to@example.com" && - echo "" + echo "my-message-id@example.com" ) | GIT_SEND_EMAIL_NOTTY=1 git send-email \ --smtp-server="$(pwd)/fake.sendmail" \ $patches \ 2>errors && grep "^From: A U Thor \$" msgtxt1 && - grep "^To: to@example.com\$" msgtxt1 + grep "^To: to@example.com\$" msgtxt1 && + grep "^In-Reply-To: " msgtxt1 ' test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '