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

[Dragan Simic <dsimic@manjaro.org>]

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