threads / patch / 60097

patchFix bug when more than one readline instance is used

Subject: [PATCH] Fix bug when more than one readline instance is used

## tl;dr

14 messages between Aug 10, 2023 and Aug 31, 2023. Diffs are folded; open one to read it.

replies: 13people: 3as markdown or json

Wesley Schwengle· Aug 10, 2023, 00:39 UTC · lore
The following error was emitted if one issued the command
    git send-email --compose 0001-my.patch

Can't locate object method "IN" via package "FakeTerm" at /home/wesleys/libexec/git-core/git-send-email line 997.

After added a warn in the relevant function that created the term it was obvious what happened:

Only one Term::ReadLine::Gnu instance is allowed. at /home/wesleys/libexec/git-core/git-send-email line 981.

When you supply no --to send-email asks you to whom you want to send the email to. This starts a term, the first Term::ReadLine::Gnu instance. The second time it wants to ask the user 'Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll):' and this causes FakeTerm to be loaded, but it doesn't have IN/OUT methods and thus fails.

The fix is to make $term global. If git chooses to drop perl 5.8 support and allows Perl 5.10, we could also use the state feature. Which would solve the problem without making $term global.

More or less the same logic happens in git-svn.perl so I fixed it there as well.

Signed-off-by: Wesley Schwengle <wesleys@opperschaap.net>
---
 git-send-email.perl | 4 +++-
 git-svn.perl        | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)
Show changes to 2 files +5 −1

git-send-email.perl, git-svn.perl

diff --git a/git-send-email.perl b/git-send-email.perl
index affbb88509..7fdcf9084a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -971,8 +971,10 @@ sub get_patch_subject {
 	do_edit(@files);
 }
 
+my $term;
 sub term {
-	my $term = eval {
+	return $term if $term;
+	$term = eval {
 		require Term::ReadLine;
 		$ENV{"GIT_SEND_EMAIL_NOTTY"}
 			? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT)
diff --git a/git-svn.perl b/git-svn.perl
index be987e316f..2813551e06 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -306,10 +306,12 @@ sub readline {
 	my $self = shift;
 	die "Cannot use readline on FakeTerm: $$self";
 }
+
 package main;
 
 my $term;
 sub term_init {
+	return $term if $term;
 	$term = eval {
 		require Term::ReadLine;
 		$ENV{"GIT_SVN_NOTTY"}

-- 
2.42.0.rc0.26.ga73c38ecaa
Jeff King· Aug 10, 2023, 00:49 UTC · re: Wesley Schwengle · lore

Re: [PATCH] Fix bug when more than one readline instance is used

On Wed, Aug 09, 2023 at 08:39:33PM -0400, Wesley Schwengle wrote:
Show 12 quoted lines
> The following error was emitted if one issued the command
> 
>     git send-email --compose 0001-my.patch
> 
> Can't locate object method "IN" via package "FakeTerm" at
> /home/wesleys/libexec/git-core/git-send-email line 997.
> 
> After added a warn in the relevant function that created the term it was
> obvious what happened:
> 
> Only one Term::ReadLine::Gnu instance is allowed. at
> /home/wesleys/libexec/git-core/git-send-email line 981.

I posted a similar fix yesterday, which is currently in 'next' via d42e4ca9f8:

  https://lore.kernel.org/git/20230808180935.GA2096901@coredump.intra.peff.net/
However...
> More or less the same logic happens in git-svn.perl so I fixed it there
> as well.

...I didn't touch git-svn.perl, and I agree it probably has the same problem (I didn't try it in practice, but any time ask() is called twice it will run into the same issue).

Do you want to prepare a patch on top removing the git-svn bits (probably all of FakeTerm, too)?

-Peff
Junio C Hamano· Aug 10, 2023, 01:05 UTC · re: Wesley Schwengle · lore

Re: [PATCH] Fix bug when more than one readline instance is used

Wesley Schwengle <wesleys@opperschaap.net> writes:
If I recall correctly, this was fixed by Peff yesterday?  
https://lore.kernel.org/git/20230808181531.GB2097200@coredump.intra.peff.net/
Show 16 quoted lines
> diff --git a/git-send-email.perl b/git-send-email.perl
> index affbb88509..7fdcf9084a 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -971,8 +971,10 @@ sub get_patch_subject {
>  	do_edit(@files);
>  }
>  
> +my $term;
>  sub term {
> -	my $term = eval {
> +	return $term if $term;
> +	$term = eval {
>  		require Term::ReadLine;
>  		$ENV{"GIT_SEND_EMAIL_NOTTY"}
>  			? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT)

The patch I queued yesterday wraps this lexical inside another block to hide it from the outside, but otherwise it should achieve the same goal.

Wesley Schwengle· Aug 10, 2023, 01:18 UTC · re: Jeff King · lore

[[PATCH v2]] Fix bug when more than one readline instance is used

A followup[^1] for git-svn.perl on d42e4ca9f8 where this bug was solved for git-send-email.perl

[^1]: https://lore.kernel.org/git/20230810004956.GA816605@coredump.intra.peff.net/T/#t
Signed-off-by: Wesley Schwengle <wesleys@opperschaap.net>
---
 git-svn.perl | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)
Show changes to git-svn.perl +9 −18
diff --git a/git-svn.perl b/git-svn.perl
index be987e316f..93f6538d61 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -297,27 +297,18 @@ sub _req_svn {
 		{} ],
 );
 
-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;
 
-my $term;
-sub term_init {
-	$term = eval {
+{
+	my $term;
+	sub term_init {
+		return $term if $term;
 		require Term::ReadLine;
-		$ENV{"GIT_SVN_NOTTY"}
-			? new Term::ReadLine 'git-svn', \*STDIN, \*STDOUT
-			: new Term::ReadLine 'git-svn';
-	};
-	if ($@) {
-		$term = new FakeTerm "$@: going non-interactive";
+		$term = $ENV{"GIT_SVN_NOTTY"}
+				? new Term::ReadLine 'git-svn', \*STDIN, \*STDOUT
+				: new Term::ReadLine 'git-svn';
+		};
+		return $term;
 	}
 }
 

-- 
2.42.0.rc0.26.ga73c38ecaa
Junio C Hamano· Aug 10, 2023, 14:31 UTC · re: Wesley Schwengle · lore

Re: [[PATCH v2]] Fix bug when more than one readline instance is used

Wesley Schwengle <wesleys@opperschaap.net> writes:
> Subject: Re: [[PATCH v2]] Fix bug when more than one readline instance is used

Thanks. Again, our convention is to make sure that, even only with the title, readers would know what the commit is about. The above does not even hint which part of the system the bug was about. By stealing from what Peff already has done, we can call this

    Subject: [PATCH v2] git-svn: avoid creating more than one Term::ReadLine object

to mimic c016726c (send-email: avoid creating more than one Term::ReadLine object, 2023-08-08). Also, please do not double the [brackets] around the "PATCH".

> A followup[^1] for git-svn.perl on d42e4ca9f8 where this bug was solved
> for git-send-email.perl
>
> [^1]: https://lore.kernel.org/git/20230810004956.GA816605@coredump.intra.peff.net/T/#t

Once a commit is in 'next', its commit object name will generally be stable, hence, taken as a whole, something like:

    git-svn: avoid creating more than one than one Term::ReadLine object
    Newer (v1.46) Term::ReadLine::Gnu would not like us to ask it to
    create multiple readline instances.  c016726c (send-email: avoid
    creating more than one Term::ReadLine object, 2023-08-08)
    adjusted git-send-email to this change.  Make the same
    adjustment to git-svn.
    While at it, drop the same FakeTerm hack, just like dfd46bae
    (send-email: drop FakeTerm hack, 2023-08-08) did, for exactly
    the same reason.

I'll queue the patch with the above commit log message for tonight, so unless you have improvements over it, there is no need to resend.

Thanks.
Show 45 quoted lines
> Signed-off-by: Wesley Schwengle <wesleys@opperschaap.net>
> ---
>  git-svn.perl | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index be987e316f..93f6538d61 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -297,27 +297,18 @@ sub _req_svn {
>  		{} ],
>  );
>  
> -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;
>  
> -my $term;
> -sub term_init {
> -	$term = eval {
> +{
> +	my $term;
> +	sub term_init {
> +		return $term if $term;
>  		require Term::ReadLine;
> -		$ENV{"GIT_SVN_NOTTY"}
> -			? new Term::ReadLine 'git-svn', \*STDIN, \*STDOUT
> -			: new Term::ReadLine 'git-svn';
> -	};
> -	if ($@) {
> -		$term = new FakeTerm "$@: going non-interactive";
> +		$term = $ENV{"GIT_SVN_NOTTY"}
> +				? new Term::ReadLine 'git-svn', \*STDIN, \*STDOUT
> +				: new Term::ReadLine 'git-svn';
> +		};
> +		return $term;
>  	}
>  }
Wesley· Aug 10, 2023, 15:14 UTC · re: Junio C Hamano · lore

Re: [[PATCH v2]] Fix bug when more than one readline instance is used

On 8/10/23 10:31, Junio C Hamano wrote:
Show 10 quoted lines
> Wesley Schwengle <wesleys@opperschaap.net> writes:
> 
>> Subject: Re: [[PATCH v2]] Fix bug when more than one readline instance is used
> 
> Thanks.  Again, our convention is to make sure that, even only with
> the title, readers would know what the commit is about.  The above
> does not even hint which part of the system the bug was about.  By
> stealing from what Peff already has done, we can call this
> 
>      Subject: [PATCH v2] git-svn: avoid creating more than one Term::ReadLine object
Ok. I'll keep that in mind for next time.
Show 17 quoted lines
> Once a commit is in 'next', its commit object name will generally be
> stable, hence, taken as a whole, something like:
> 
>      git-svn: avoid creating more than one than one Term::ReadLine object
> 
>      Newer (v1.46) Term::ReadLine::Gnu would not like us to ask it to
>      create multiple readline instances.  c016726c (send-email: avoid
>      creating more than one Term::ReadLine object, 2023-08-08)
>      adjusted git-send-email to this change.  Make the same
>      adjustment to git-svn.
> 
>      While at it, drop the same FakeTerm hack, just like dfd46bae
>      (send-email: drop FakeTerm hack, 2023-08-08) did, for exactly
>      the same reason.
> 
> I'll queue the patch with the above commit log message for tonight,
> so unless you have improvements over it, there is no need to resend.
Thank you for your patience and accepting the patch(es).

Cheers, Wesley

-- 
Wesley

Why not both?
Junio C Hamano· Aug 11, 2023, 01:01 UTC · re: Wesley Schwengle · lore

Re: [[PATCH v2]] Fix bug when more than one readline instance is used

Wesley Schwengle <wesleys@opperschaap.net> writes:
Show 11 quoted lines
> diff --git a/git-svn.perl b/git-svn.perl
> index be987e316f..93f6538d61 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> ...
> -	if ($@) {
> -		$term = new FakeTerm "$@: going non-interactive";
> +		$term = $ENV{"GIT_SVN_NOTTY"}
> +				? new Term::ReadLine 'git-svn', \*STDIN, \*STDOUT
> +				: new Term::ReadLine 'git-svn';
> +		};
This line with "};" on it should not be added, I think.
cf. https://github.com/git/git/actions/runs/5827208598/job/15802787783#step:5:74
> +		return $term;
>  	}
>  }
 git-svn.perl | 1 -
 1 file changed, 1 deletion(-)
Show changes to git-svn.perl +0 −1
diff --git a/git-svn.perl b/git-svn.perl
index 93f6538d61..e919c3f172 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -307,7 +307,6 @@ package main;
 		$term = $ENV{"GIT_SVN_NOTTY"}
 				? new Term::ReadLine 'git-svn', \*STDIN, \*STDOUT
 				: new Term::ReadLine 'git-svn';
-		};
 		return $term;
 	}
 }

-- 
2.42.0-rc1
Wesley· Aug 11, 2023, 01:09 UTC · re: Junio C Hamano · lore

Re: [[PATCH v2]] Fix bug when more than one readline instance is used

On 8/10/23 21:01, Junio C Hamano wrote:
Show 25 quoted lines
> Wesley Schwengle <wesleys@opperschaap.net> writes:
> 
> This line with "};" on it should not be added, I think.
> 
> cf. https://github.com/git/git/actions/runs/5827208598/job/15802787783#step:5:74
> 
>> +		return $term;
>>   	}
>>   }
> 
>   git-svn.perl | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index 93f6538d61..e919c3f172 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -307,7 +307,6 @@ package main;
>   		$term = $ENV{"GIT_SVN_NOTTY"}
>   				? new Term::ReadLine 'git-svn', \*STDIN, \*STDOUT
>   				: new Term::ReadLine 'git-svn';
> -		};
>   		return $term;
>   	}
>   }
You are 100% correct.

Cheers, Wesley

-- 
Wesley

Why not both?
Junio C Hamano· Aug 11, 2023, 05:30 UTC · re: Wesley · lore

Re: [[PATCH v2]] Fix bug when more than one readline instance is used

Wesley <wesleys@opperschaap.net> writes:
Show 24 quoted lines
> On 8/10/23 21:01, Junio C Hamano wrote:
>> Wesley Schwengle <wesleys@opperschaap.net> writes:
>> This line with "};" on it should not be added, I think.
>> cf. https://github.com/git/git/actions/runs/5827208598/job/15802787783#step:5:74
>> 
>>> +		return $term;
>>>   	}
>>>   }
>>   git-svn.perl | 1 -
>>   1 file changed, 1 deletion(-)
>> diff --git a/git-svn.perl b/git-svn.perl
>> index 93f6538d61..e919c3f172 100755
>> --- a/git-svn.perl
>> +++ b/git-svn.perl
>> @@ -307,7 +307,6 @@ package main;
>>   		$term = $ENV{"GIT_SVN_NOTTY"}
>>   				? new Term::ReadLine 'git-svn', \*STDIN, \*STDOUT
>>   				: new Term::ReadLine 'git-svn';
>> -		};
>>   		return $term;
>>   	}
>>   }
>
> You are 100% correct.

And embarrassingly, the above is not sufficient, as the way $term is used in git-send-email and git-svn are subtly different.

I think we further need something like this on top, but my Perl is rusty.

Show changes to git-svn.perl +1 −1
diff --git a/git-svn.perl b/git-svn.perl
index e919c3f172..6033b97a0c 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -427,7 +427,7 @@ sub ask {
 	my $default = $arg{default};
 	my $resp;
 	my $i = 0;
-	term_init() unless $term;
+	my $term = term_init();
 
 	if ( !( defined($term->IN)
             && defined( fileno($term->IN) )

-- 
2.42.0-rc1
Jeff King· Aug 11, 2023, 14:51 UTC · re: Junio C Hamano · lore

Re: [[PATCH v2]] Fix bug when more than one readline instance is used

On Thu, Aug 10, 2023 at 10:30:17PM -0700, Junio C Hamano wrote:
Show 19 quoted lines
> And embarrassingly, the above is not sufficient, as the way $term is
> used in git-send-email and git-svn are subtly different.
> 
> I think we further need something like this on top, but my Perl is
> rusty.
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index e919c3f172..6033b97a0c 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -427,7 +427,7 @@ sub ask {
>  	my $default = $arg{default};
>  	my $resp;
>  	my $i = 0;
> -	term_init() unless $term;
> +	my $term = term_init();
>  
>  	if ( !( defined($term->IN)
>              && defined( fileno($term->IN) )
Hmm. Isn't that an indication that git-svn is OK as-is?

Looking at the version of git-svn.perl on the tip of master, I see we declare a global $term along with the initializer:

  my $term;
  sub term_init {
          $term = eval { ...etc... }
And then later in ask we call term_init() only if it's uninitialized:
  sub ask {
          ...
          term_init() unless $term;

So those are looking at the same $term, and the result should only be initialized once.

It could still benefit from cleaning up FakeTerm, since we lazily init the object since 30d45f798d (git-svn: delay term initialization, 2014-09-14). But I don't think there's a visible bug here with the new version of Term::ReadLine::Gnu.

-Peff
Junio C Hamano· Aug 11, 2023, 16:05 UTC · re: Jeff King · lore

Re: [[PATCH v2]] Fix bug when more than one readline instance is used

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
>> diff --git a/git-svn.perl b/git-svn.perl
>> index e919c3f172..6033b97a0c 100755
>> --- a/git-svn.perl
>> +++ b/git-svn.perl
>> @@ -427,7 +427,7 @@ sub ask {
>>  	my $default = $arg{default};
>>  	my $resp;
>>  	my $i = 0;
>> -	term_init() unless $term;
>> +	my $term = term_init();
>>  
>>  	if ( !( defined($term->IN)
>>              && defined( fileno($term->IN) )
>
> Hmm. Isn't that an indication that git-svn is OK as-is?

Yes. As long as we know they share the same kind of code structure to use the same library function that wants its callers to stick to a singleton instance, there is a value in using the same structure on the side of our callers, but yes, we can rely on the global $term for it being a singleton.

> It could still benefit from cleaning up FakeTerm, since we lazily init
> the object since 30d45f798d (git-svn: delay term initialization,
> 2014-09-14). But I don't think there's a visible bug here with the new
> version of Term::ReadLine::Gnu.

True. Let me drop the patch from the 'next down to master fast-track' candidate status.

Thanks.
Junio C Hamano· Aug 30, 2023, 22:32 UTC · re: Junio C Hamano · lore

[PATCH] git-svn: drop FakeTerm hack

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> Jeff King <peff@peff.net> writes:
> ...
>> It could still benefit from cleaning up FakeTerm, since we lazily init
>> the object since 30d45f798d (git-svn: delay term initialization,
>> 2014-09-14). But I don't think there's a visible bug here with the new
>> version of Term::ReadLine::Gnu.
>
> True.  Let me drop the patch from the 'next down to master
> fast-track' candidate status.

We did the above but then everybody seems to have forgotten about it. Let's resurrect the topic. Here is my attempt.

---- >8 ----
From: Wesley Schwengle <wesleys@opperschaap.net>
Subject: [PATCH] git-svn: drop FakeTerm hack

Drop the FakeTerm hack, just like dfd46bae (send-email: drop FakeTerm hack, 2023-08-08) did, for exactly the same reason.

Signed-off-by: Wesley Schwengle <wesleys@opperschaap.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-svn.perl | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)
Show changes to diff +2 −18
diff --git c/git-svn.perl w/git-svn.perl
index be987e316f..4e8878f035 100755
--- c/git-svn.perl
+++ w/git-svn.perl
@@ -297,28 +297,12 @@ sub _req_svn {
 		{} ],
 );
 
-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;
-
 my $term;
 sub term_init {
-	$term = eval {
-		require Term::ReadLine;
-		$ENV{"GIT_SVN_NOTTY"}
+	require Term::ReadLine;
+	$term = $ENV{"GIT_SVN_NOTTY"}
 			? new Term::ReadLine 'git-svn', \*STDIN, \*STDOUT
 			: new Term::ReadLine 'git-svn';
-	};
-	if ($@) {
-		$term = new FakeTerm "$@: going non-interactive";
-	}
 }
 
 my $cmd;
Jeff King· Aug 31, 2023, 00:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-svn: drop FakeTerm hack

On Wed, Aug 30, 2023 at 03:32:08PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> > True.  Let me drop the patch from the 'next down to master
> > fast-track' candidate status.
> 
> We did the above but then everybody seems to have forgotten about
> it.  Let's resurrect the topic.  Here is my attempt.
> 
> ---- >8 ----
> From: Wesley Schwengle <wesleys@opperschaap.net>
> Subject: [PATCH] git-svn: drop FakeTerm hack
> 
> Drop the FakeTerm hack, just like dfd46bae (send-email: drop
> FakeTerm hack, 2023-08-08) did, for exactly the same reason.
Yep, it looks good to me.
Optionally you could add this to the commit message:
  It has been obsolete in git-svn since 30d45f798d (git-svn: delay term
  initialization, 2014-09-14). Note that unlike send-email, we already
  make sure to load Term::ReadLine only once. So this is just a cleanup,
  and not fixing any bug.
-Peff
Junio C Hamano· Aug 31, 2023, 00:28 UTC · re: Jeff King · lore

Re: [PATCH] git-svn: drop FakeTerm hack

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> Optionally you could add this to the commit message:
>
>   It has been obsolete in git-svn since 30d45f798d (git-svn: delay term
>   initialization, 2014-09-14). Note that unlike send-email, we already
>   make sure to load Term::ReadLine only once. So this is just a cleanup,
>   and not fixing any bug.
Thanks.  That reads extremely well.

← back to recent threads