diff mbox series

[02/12] send-email: avoid creating more than one Term::ReadLine object

Message ID 20240521195659.870714-3-gitster@pobox.com (mailing list archive)
State Superseded
Headers show
Series Fix various overly aggressive protections in 2.45.1 and friends | expand

Commit Message

Junio C Hamano May 21, 2024, 7:56 p.m. UTC
From: Jeff King <peff@peff.net>

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

[jc: cherry-picked from v2.42.0-rc2~6^2]

Signed-off-by: Jeff King <peff@peff.net>
Acked-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-send-email.perl   | 18 +++++++++++++-----
 t/t9001-send-email.sh |  5 +++--
 2 files changed, 16 insertions(+), 7 deletions(-)

Comments

Dragan Simic May 22, 2024, 8:15 a.m. UTC | #1
On 2024-05-21 21:56, Junio C Hamano wrote:
> From: Jeff King <peff@peff.net>
> 
> 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
> 
> [jc: cherry-picked from v2.42.0-rc2~6^2]
> 
> Signed-off-by: Jeff King <peff@peff.net>
> Acked-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Looking good to me.  Thanks for taking care of this issue.

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
>  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 72d876f0a0..ad51508790 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -917,11 +917,19 @@ sub get_patch_subject {
>  	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 1130ef21b3..0f08a9542b 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 <author@example.com>\$" msgtxt1 &&
> -		grep "^To: to@example.com\$" msgtxt1
> +		grep "^To: to@example.com\$" msgtxt1 &&
> +		grep "^In-Reply-To: <my-message-id@example.com>" msgtxt1
>  '
> 
>  test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '
diff mbox series

Patch

diff --git a/git-send-email.perl b/git-send-email.perl
index 72d876f0a0..ad51508790 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -917,11 +917,19 @@  sub get_patch_subject {
 	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 1130ef21b3..0f08a9542b 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 <author@example.com>\$" msgtxt1 &&
-		grep "^To: to@example.com\$" msgtxt1
+		grep "^To: to@example.com\$" msgtxt1 &&
+		grep "^In-Reply-To: <my-message-id@example.com>" msgtxt1
 '
 
 test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '