{"thread":{"id":"60084","subject":"[PATCH 0/2] fix git-send-email with recent versions of Term::ReadLine::Gnu","startedAt":"2023-08-08T19:56:48Z","lastAt":"2023-08-08T21:18:01Z","messageCount":5,"participants":["Jeff King","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"480319","messageId":"20230808180935.GA2096901@coredump.intra.peff.net","threadId":"60084","inReplyTo":null,"subject":"[PATCH 0/2] fix git-send-email with recent versions of Term::ReadLine::Gnu","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-08T18:09:35Z","receivedAt":"2023-08-08T19:56:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Using git-send-email with the latest release of Term::ReadLine::Gnu\ncauses any invocations which prompt more than once to fail. Details are\nin the second patch, which fixes it. The first patch cleans up an\nobsolete workaround that obscured the root cause of the problem.\n\nThe affected version of Term::ReadLine::Gnu is v1.46, which hit Debian\nunstable in the past few weeks. So we should consider this for maint\nand perhaps even for the -rc candidates, since people will presumably\nstart running into this more and more.\n\n  [1/2]: send-email: drop FakeTerm hack\n  [2/2]: send-email: avoid creating more than one Term::ReadLine object\n\n git-send-email.perl   | 32 +++++++++++---------------------\n t/t9001-send-email.sh |  5 +++--\n 2 files changed, 14 insertions(+), 23 deletions(-)\n\n-Peff\n"},{"id":"480320","messageId":"20230808181531.GB2097200@coredump.intra.peff.net","threadId":"60084","inReplyTo":"20230808180935.GA2096901@coredump.intra.peff.net","subject":"[PATCH 2/2] send-email: avoid creating more than one Term::ReadLine object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-08T18:15:31Z","receivedAt":"2023-08-08T19:59:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Every time git-send-email calls its ask() function to prompt the user,\nwe call term(), which instantiates a new Term::ReadLine object. But in\nv1.46 of Term::ReadLine::Gnu (which provides the Term::ReadLine\ninterface on some platforms), its constructor refuses to create a second\ninstance[1]. So on systems with that version of the module, most\ngit-send-email instances will fail (as we usually prompt for both \"to\"\nand \"in-reply-to\" unless the user provided them on the command line).\n\nWe can fix this by keeping a single instance variable and returning it\nfor each call to term(). In perl 5.10 and up, we could do that with a\n\"state\" variable. But since we only require 5.008, we'll do it the\nold-fashioned way, with a lexical \"my\" in its own scope.\n\nNote that the tests in t9001 detect this problem as-is, since the\nfailure mode is for the program to die. But let's also beef up the\n\"Prompting works\" test to check that it correctly handles multiple\ninputs (if we had chosen to keep our FakeTerm hack in the previous\ncommit, then the failure mode would be incorrectly ignoring prompts\nafter the first).\n\n[1] For discussion of why multiple instances are forbidden, see:\n    https://github.com/hirooih/perl-trg/issues/16\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n git-send-email.perl   | 18 +++++++++++++-----\n t/t9001-send-email.sh |  5 +++--\n 2 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 4cb6ee9327..897cea6564 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -959,11 +959,19 @@ sub get_patch_subject {\n \tdo_edit(@files);\n }\n \n-sub term {\n-\trequire Term::ReadLine;\n-\treturn $ENV{\"GIT_SEND_EMAIL_NOTTY\"}\n-\t\t\t? Term::ReadLine->new('git-send-email', \\*STDIN, \\*STDOUT)\n-\t\t\t: Term::ReadLine->new('git-send-email');\n+{\n+\t# Only instantiate one $term per program run, since some\n+\t# Term::ReadLine providers refuse to create a second instance.\n+\tmy $term;\n+\tsub term {\n+\t\trequire Term::ReadLine;\n+\t\tif (!defined $term) {\n+\t\t\t$term = $ENV{\"GIT_SEND_EMAIL_NOTTY\"}\n+\t\t\t\t? Term::ReadLine->new('git-send-email', \\*STDIN, \\*STDOUT)\n+\t\t\t\t: Term::ReadLine->new('git-send-email');\n+\t\t}\n+\t\treturn $term;\n+\t}\n }\n \n sub ask {\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 48bf0af2ee..0662ff8120 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -337,13 +337,14 @@ test_expect_success $PREREQ 'Show all headers' '\n test_expect_success $PREREQ 'Prompting works' '\n \tclean_fake_sendmail &&\n \t(echo \"to@example.com\" &&\n-\t echo \"\"\n+\t echo \"my-message-id@example.com\"\n \t) | GIT_SEND_EMAIL_NOTTY=1 git send-email \\\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t$patches \\\n \t\t2>errors &&\n \t\tgrep \"^From: A U Thor <author@example.com>\\$\" msgtxt1 &&\n-\t\tgrep \"^To: to@example.com\\$\" msgtxt1\n+\t\tgrep \"^To: to@example.com\\$\" msgtxt1 &&\n+\t\tgrep \"^In-Reply-To: <my-message-id@example.com>\" msgtxt1\n '\n \n test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '\n-- \n2.42.0.rc0.376.g66bfc4f195\n"},{"id":"480321","messageId":"20230808181436.GA2097200@coredump.intra.peff.net","threadId":"60084","inReplyTo":"20230808180935.GA2096901@coredump.intra.peff.net","subject":"[PATCH 1/2] send-email: drop FakeTerm hack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-08T18:14:36Z","receivedAt":"2023-08-08T19:59:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Back in 280242d1cc (send-email: do not barf when Term::ReadLine does not\nlike your terminal, 2006-07-02), we added a fallback for when\nTerm::ReadLine's constructor failed: we'd have a FakeTerm object\ninstead, which would then die if anybody actually tried to call\nreadline() on it. Since we instantiated the $term variable at program\nstartup, we needed this workaround to let the program run in modes when\nwe did not prompt the user.\n\nBut later, in f4dc9432fd (send-email: lazily load modules for a big\nspeedup, 2021-05-28), we started loading Term::ReadLine lazily only when\nask() is called. So at that point we know we're trying to prompt the\nuser, and we can just die if ReadLine instantiation fails, rather than\nmaking this fake object to lazily delay showing the error.\n\nThis should be OK even if there is no tty (e.g., we're in a cron job),\nbecause Term::ReadLine will return a stub object in that case whose \"IN\"\nand \"OUT\" functions return undef. And since 5906f54e47 (send-email:\ndon't attempt to prompt if tty is closed, 2009-03-31), we check for that\ncase and skip prompting.\n\nAnd we can be sure that FakeTerm was not kicking in for such a\nsituation, because it has actually been broken since that commit! It\ndoes not define \"IN\" or \"OUT\" methods, so perl would barf with an error.\nIf FakeTerm was in use, we were neither honoring what 5906f54e47 tried\nto do, nor producing the readable message that 280242d1cc intended.\n\nSo we're better off just dropping FakeTerm entirely, and letting the\nerror reported by constructing Term::ReadLine through.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe smaller change is to implement:\n\n  sub IN { return undef }\n\nfor FakeTerm, which would then behave according to 5906f54e47's \"no tty\"\nbehavior. But after reading through the relevant history, I'm fairly\nconvinced that just dropping it is the right path. If it were actually\nkicking in ever, people would be complaining about the obscure\n\"$term->IN is not defined\" error message from perl.\n\n git-send-email.perl | 22 ++--------------------\n 1 file changed, 2 insertions(+), 20 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex affbb88509..4cb6ee9327 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -26,18 +26,6 @@\n \n Getopt::Long::Configure qw/ pass_through /;\n \n-package FakeTerm;\n-sub new {\n-\tmy ($class, $reason) = @_;\n-\treturn bless \\$reason, shift;\n-}\n-sub readline {\n-\tmy $self = shift;\n-\tdie \"Cannot use readline on FakeTerm: $$self\";\n-}\n-package main;\n-\n-\n sub usage {\n \tprint <<EOT;\n git send-email' [<options>] <file|directory>\n@@ -972,16 +960,10 @@ sub get_patch_subject {\n }\n \n sub term {\n-\tmy $term = eval {\n-\t\trequire Term::ReadLine;\n-\t\t$ENV{\"GIT_SEND_EMAIL_NOTTY\"}\n+\trequire Term::ReadLine;\n+\treturn $ENV{\"GIT_SEND_EMAIL_NOTTY\"}\n \t\t\t? Term::ReadLine->new('git-send-email', \\*STDIN, \\*STDOUT)\n \t\t\t: Term::ReadLine->new('git-send-email');\n-\t};\n-\tif ($@) {\n-\t\t$term = FakeTerm->new(\"$@: going non-interactive\");\n-\t}\n-\treturn $term;\n }\n \n sub ask {\n-- \n2.42.0.rc0.376.g66bfc4f195\n\n"},{"id":"480335","messageId":"ZNKihfNbEqJcCkXX@nand.local","threadId":"60084","inReplyTo":"20230808181531.GB2097200@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] send-email: avoid creating more than one Term::ReadLine object","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-08T20:16:05Z","receivedAt":"2023-08-08T20:58:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 08, 2023 at 02:15:31PM -0400, Jeff King wrote:\n> Every time git-send-email calls its ask() function to prompt the user,\n> we call term(), which instantiates a new Term::ReadLine object. But in\n> v1.46 of Term::ReadLine::Gnu (which provides the Term::ReadLine\n> interface on some platforms), its constructor refuses to create a second\n> instance[1]. So on systems with that version of the module, most\n> git-send-email instances will fail (as we usually prompt for both \"to\"\n> and \"in-reply-to\" unless the user provided them on the command line).\n\nNice one ;-). Everything you wrote here makes sense, as does\ninitializing the Term::ReadLine object only once.\n\n> Note that the tests in t9001 detect this problem as-is, since the\n> failure mode is for the program to die. But let's also beef up the\n> \"Prompting works\" test to check that it correctly handles multiple\n> inputs (if we had chosen to keep our FakeTerm hack in the previous\n> commit, then the failure mode would be incorrectly ignoring prompts\n> after the first).\n\nThanks for improving the test coverage while you're here.\n\nEverything here LGTM.\n\nThanks,\nTaylor\n"},{"id":"480337","messageId":"ZNKh2lzaRUjeU9AF@nand.local","threadId":"60084","inReplyTo":"20230808181436.GA2097200@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] send-email: drop FakeTerm hack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-08T20:13:14Z","receivedAt":"2023-08-08T21:18:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 08, 2023 at 02:14:36PM -0400, Jeff King wrote:\n> The smaller change is to implement:\n>\n>   sub IN { return undef }\n>\n> for FakeTerm, which would then behave according to 5906f54e47's \"no tty\"\n> behavior. But after reading through the relevant history, I'm fairly\n> convinced that just dropping it is the right path. If it were actually\n> kicking in ever, people would be complaining about the obscure\n> \"$term->IN is not defined\" error message from perl.\n\nAll makes sense. Thanks for the detailed commit message, as always :-).\n\nThanks,\nTaylor\n"}]}