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

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

[Wesley Schwengle]

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(-)

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

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

[Jeff King]

On Wed, Aug 09, 2023 at 08:39:33PM -0400, Wesley Schwengle wrote:

> 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

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

[Junio C Hamano]

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/

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

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

[Wesley Schwengle]

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(-)

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

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

[Junio C Hamano]

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.

> 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;
>  	}
>  }

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

[Wesley]

On 8/10/23 10:31, Junio C Hamano wrote:
> 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.

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

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

[Junio C Hamano]

Wesley Schwengle <wesleys@opperschaap.net> writes:

> 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(-)

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

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

[Wesley]

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.

Cheers,
Wesley

-- 
Wesley

Why not both?

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

[Junio C Hamano]

Wesley <wesleys@opperschaap.net> writes:

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

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

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

[Jeff King]

On Thu, Aug 10, 2023 at 10:30:17PM -0700, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

[PATCH] git-svn: drop FakeTerm hack

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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(-)

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;

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

[Jeff King]

On Wed, Aug 30, 2023 at 03:32:08PM -0700, Junio C Hamano wrote:

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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