| Message ID | 20230810011831.1423208-1-wesleys@opperschaap.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [[PATCH,v2] ] Fix bug when more than one readline instance is used | expand |
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; > } > }
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 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; } }
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 <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) )
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
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.
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; } }
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(-)