{"thread":{"id":"60097","subject":"[PATCH] Fix bug when more than one readline instance is used","startedAt":"2023-08-10T00:43:55Z","lastAt":"2023-08-31T00:28:09Z","messageCount":14,"participants":["Wesley Schwengle","Jeff King","Junio C Hamano","Wesley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"480418","messageId":"20230810003939.1420306-1-wesleys@opperschaap.net","threadId":"60097","inReplyTo":null,"subject":"[PATCH] Fix bug when more than one readline instance is used","fromName":"Wesley Schwengle","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-08-10T00:39:33Z","receivedAt":"2023-08-10T00:43:55Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"The following error was emitted if one issued the command\n\n git send-email --compose 0001-my.patch\n\nCan't locate object method \"IN\" via package \"FakeTerm\" at\n/home/wesleys/libexec/git-core/git-send-email line 997.\n\nAfter added a warn in the relevant function that created the term it was\nobvious what happened:\n\nOnly one Term::ReadLine::Gnu instance is allowed. at\n/home/wesleys/libexec/git-core/git-send-email line 981.\n\nWhen you supply no --to send-email asks you to whom you want to send the\nemail to. This starts a term, the first Term::ReadLine::Gnu instance.\nThe second time it wants to ask the user 'Send this email?\n([y]es|[n]o|[e]dit|[q]uit|[a]ll):' and this causes FakeTerm to be\nloaded, but it doesn't have IN/OUT methods and thus fails.\n\nThe fix is to make $term global. If git chooses to drop perl 5.8 support\nand allows Perl 5.10, we could also use the state feature. Which would\nsolve the problem without making $term global.\n\nMore or less the same logic happens in git-svn.perl so I fixed it there\nas well.\n\nSigned-off-by: Wesley Schwengle \n---\n git-send-email.perl | 4 +++-\n git-svn.perl | 2 ++\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex affbb88509..7fdcf9084a 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -971,8 +971,10 @@ sub get_patch_subject {\n \tdo_edit(@files);\n }\n \n+my $term;\n sub term {\n-\tmy $term = eval {\n+\treturn $term if $term;\n+\t$term = eval {\n \t\trequire Term::ReadLine;\n \t\t$ENV{\"GIT_SEND_EMAIL_NOTTY\"}\n \t\t\t? Term::ReadLine->new('git-send-email', \\*STDIN, \\*STDOUT)\ndiff --git a/git-svn.perl b/git-svn.perl\nindex be987e316f..2813551e06 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -306,10 +306,12 @@ sub readline {\n \tmy $self = shift;\n \tdie \"Cannot use readline on FakeTerm: $$self\";\n }\n+\n package main;\n \n my $term;\n sub term_init {\n+\treturn $term if $term;\n \t$term = eval {\n \t\trequire Term::ReadLine;\n \t\t$ENV{\"GIT_SVN_NOTTY\"}\n-- \n2.42.0.rc0.26.ga73c38ecaa\n\n"},{"id":"480419","messageId":"20230810004956.GA816605@coredump.intra.peff.net","threadId":"60097","inReplyTo":"20230810003939.1420306-1-wesleys@opperschaap.net","subject":"Re: [PATCH] Fix bug when more than one readline instance is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-10T00:49:56Z","receivedAt":"2023-08-10T00:49:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 09, 2023 at 08:39:33PM -0400, Wesley Schwengle wrote:\n\n> The following error was emitted if one issued the command\n> \n> git send-email --compose 0001-my.patch\n> \n> Can't locate object method \"IN\" via package \"FakeTerm\" at\n> /home/wesleys/libexec/git-core/git-send-email line 997.\n> \n> After added a warn in the relevant function that created the term it was\n> obvious what happened:\n> \n> Only one Term::ReadLine::Gnu instance is allowed. at\n> /home/wesleys/libexec/git-core/git-send-email line 981.\n\nI posted a similar fix yesterday, which is currently in 'next' via\nd42e4ca9f8:\n\n https://lore.kernel.org/git/20230808180935.GA2096901@coredump.intra.peff.net/\n\nHowever...\n\n> More or less the same logic happens in git-svn.perl so I fixed it there\n> as well.\n\n...I didn't touch git-svn.perl, and I agree it probably has the same\nproblem (I didn't try it in practice, but any time ask() is called twice\nit will run into the same issue).\n\nDo you want to prepare a patch on top removing the git-svn bits\n(probably all of FakeTerm, too)?\n\n-Peff\n"},{"id":"480421","messageId":"xmqqil9nk8pu.fsf@gitster.g","threadId":"60097","inReplyTo":"20230810003939.1420306-1-wesleys@opperschaap.net","subject":"Re: [PATCH] Fix bug when more than one readline instance is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-10T01:05:33Z","receivedAt":"2023-08-10T01:05:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wesley Schwengle writes:\n\nIf I recall correctly, this was fixed by Peff yesterday? \n\nhttps://lore.kernel.org/git/20230808181531.GB2097200@coredump.intra.peff.net/\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index affbb88509..7fdcf9084a 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -971,8 +971,10 @@ sub get_patch_subject {\n> \tdo_edit(@files);\n> }\n> \n> +my $term;\n> sub term {\n> -\tmy $term = eval {\n> +\treturn $term if $term;\n> +\t$term = eval {\n> \t\trequire Term::ReadLine;\n> \t\t$ENV{\"GIT_SEND_EMAIL_NOTTY\"}\n> \t\t\t? Term::ReadLine->new('git-send-email', \\*STDIN, \\*STDOUT)\n\nThe patch I queued yesterday wraps this lexical inside another block\nto hide it from the outside, but otherwise it should achieve the\nsame goal.\n"},{"id":"480423","messageId":"20230810011831.1423208-1-wesleys@opperschaap.net","threadId":"60097","inReplyTo":"20230810004956.GA816605@coredump.intra.peff.net","subject":"[[PATCH v2]] Fix bug when more than one readline instance is used","fromName":"Wesley Schwengle","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-08-10T01:18:31Z","receivedAt":"2023-08-10T01:18:46Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"A followup[^1] for git-svn.perl on d42e4ca9f8 where this bug was solved\nfor git-send-email.perl\n\n[^1]: https://lore.kernel.org/git/20230810004956.GA816605@coredump.intra.peff.net/T/#t\n\nSigned-off-by: Wesley Schwengle \n---\n git-svn.perl | 27 +++++++++------------------\n 1 file changed, 9 insertions(+), 18 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex be987e316f..93f6538d61 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -297,27 +297,18 @@ sub _req_svn {\n \t\t{} ],\n );\n \n-package FakeTerm;\n-sub new {\n-\tmy ($class, $reason) = @_;\n-\treturn bless \\$reason, shift;\n-}\n-sub readline {\n-\tmy $self = shift;\n-\tdie \"Cannot use readline on FakeTerm: $$self\";\n-}\n package main;\n \n-my $term;\n-sub term_init {\n-\t$term = eval {\n+{\n+\tmy $term;\n+\tsub term_init {\n+\t\treturn $term if $term;\n \t\trequire Term::ReadLine;\n-\t\t$ENV{\"GIT_SVN_NOTTY\"}\n-\t\t\t? new Term::ReadLine 'git-svn', \\*STDIN, \\*STDOUT\n-\t\t\t: new Term::ReadLine 'git-svn';\n-\t};\n-\tif ($@) {\n-\t\t$term = new FakeTerm \"$@: going non-interactive\";\n+\t\t$term = $ENV{\"GIT_SVN_NOTTY\"}\n+\t\t\t\t? new Term::ReadLine 'git-svn', \\*STDIN, \\*STDOUT\n+\t\t\t\t: new Term::ReadLine 'git-svn';\n+\t\t};\n+\t\treturn $term;\n \t}\n }\n \n-- \n2.42.0.rc0.26.ga73c38ecaa\n\n"},{"id":"480435","messageId":"xmqqmsyzhsto.fsf@gitster.g","threadId":"60097","inReplyTo":"20230810011831.1423208-1-wesleys@opperschaap.net","subject":"Re: [[PATCH v2]] Fix bug when more than one readline instance is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-10T14:31:47Z","receivedAt":"2023-08-10T14:32:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wesley Schwengle writes:\n\n> Subject: Re: [[PATCH v2]] Fix bug when more than one readline instance is used\n\nThanks. Again, our convention is to make sure that, even only with\nthe title, readers would know what the commit is about. The above\ndoes not even hint which part of the system the bug was about. By\nstealing from what Peff already has done, we can call this\n\n Subject: [PATCH v2] git-svn: avoid creating more than one Term::ReadLine object\n\nto mimic c016726c (send-email: avoid creating more than one\nTerm::ReadLine object, 2023-08-08). Also, please do not double the\n[brackets] around the \"PATCH\".\n\n> A followup[^1] for git-svn.perl on d42e4ca9f8 where this bug was solved\n> for git-send-email.perl\n>\n> [^1]: https://lore.kernel.org/git/20230810004956.GA816605@coredump.intra.peff.net/T/#t\n\nOnce a commit is in 'next', its commit object name will generally be\nstable, hence, taken as a whole, something like:\n\n git-svn: avoid creating more than one than one Term::ReadLine object\n\n Newer (v1.46) Term::ReadLine::Gnu would not like us to ask it to\n create multiple readline instances. c016726c (send-email: avoid\n creating more than one Term::ReadLine object, 2023-08-08)\n adjusted git-send-email to this change. Make the same\n adjustment to git-svn.\n\n While at it, drop the same FakeTerm hack, just like dfd46bae\n (send-email: drop FakeTerm hack, 2023-08-08) did, for exactly\n the same reason.\n\nI'll queue the patch with the above commit log message for tonight,\nso unless you have improvements over it, there is no need to resend.\n\nThanks.\n\n> Signed-off-by: Wesley Schwengle \n> ---\n> git-svn.perl | 27 +++++++++------------------\n> 1 file changed, 9 insertions(+), 18 deletions(-)\n>\n> diff --git a/git-svn.perl b/git-svn.perl\n> index be987e316f..93f6538d61 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -297,27 +297,18 @@ sub _req_svn {\n> \t\t{} ],\n> );\n> \n> -package FakeTerm;\n> -sub new {\n> -\tmy ($class, $reason) = @_;\n> -\treturn bless \\$reason, shift;\n> -}\n> -sub readline {\n> -\tmy $self = shift;\n> -\tdie \"Cannot use readline on FakeTerm: $$self\";\n> -}\n> package main;\n> \n> -my $term;\n> -sub term_init {\n> -\t$term = eval {\n> +{\n> +\tmy $term;\n> +\tsub term_init {\n> +\t\treturn $term if $term;\n> \t\trequire Term::ReadLine;\n> -\t\t$ENV{\"GIT_SVN_NOTTY\"}\n> -\t\t\t? new Term::ReadLine 'git-svn', \\*STDIN, \\*STDOUT\n> -\t\t\t: new Term::ReadLine 'git-svn';\n> -\t};\n> -\tif ($@) {\n> -\t\t$term = new FakeTerm \"$@: going non-interactive\";\n> +\t\t$term = $ENV{\"GIT_SVN_NOTTY\"}\n> +\t\t\t\t? new Term::ReadLine 'git-svn', \\*STDIN, \\*STDOUT\n> +\t\t\t\t: new Term::ReadLine 'git-svn';\n> +\t\t};\n> +\t\treturn $term;\n> \t}\n> }\n"},{"id":"480439","messageId":"4839a610-89da-ae6c-efb0-5d638be5d1b0@opperschaap.net","threadId":"60097","inReplyTo":"xmqqmsyzhsto.fsf@gitster.g","subject":"Re: [[PATCH v2]] Fix bug when more than one readline instance is used","fromName":"Wesley","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-08-10T15:14:38Z","receivedAt":"2023-08-10T15:14:54Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"On 8/10/23 10:31, Junio C Hamano wrote:\n> Wesley Schwengle writes:\n> \n>> Subject: Re: [[PATCH v2]] Fix bug when more than one readline instance is used\n> \n> Thanks. Again, our convention is to make sure that, even only with\n> the title, readers would know what the commit is about. The above\n> does not even hint which part of the system the bug was about. By\n> stealing from what Peff already has done, we can call this\n> \n> Subject: [PATCH v2] git-svn: avoid creating more than one Term::ReadLine object\n\nOk. I'll keep that in mind for next time.\n\n> Once a commit is in 'next', its commit object name will generally be\n> stable, hence, taken as a whole, something like:\n> \n> git-svn: avoid creating more than one than one Term::ReadLine object\n> \n> Newer (v1.46) Term::ReadLine::Gnu would not like us to ask it to\n> create multiple readline instances. c016726c (send-email: avoid\n> creating more than one Term::ReadLine object, 2023-08-08)\n> adjusted git-send-email to this change. Make the same\n> adjustment to git-svn.\n> \n> While at it, drop the same FakeTerm hack, just like dfd46bae\n> (send-email: drop FakeTerm hack, 2023-08-08) did, for exactly\n> the same reason.\n> \n> I'll queue the patch with the above commit log message for tonight,\n> so unless you have improvements over it, there is no need to resend.\n\nThank you for your patience and accepting the patch(es).\n\nCheers,\nWesley\n\n\n-- \nWesley\n\nWhy not both?\n\n"},{"id":"480519","messageId":"xmqqcyzupf3b.fsf@gitster.g","threadId":"60097","inReplyTo":"20230810011831.1423208-1-wesleys@opperschaap.net","subject":"Re: [[PATCH v2]] Fix bug when more than one readline instance is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-11T01:01:12Z","receivedAt":"2023-08-11T01:01:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wesley Schwengle writes:\n\n> diff --git a/git-svn.perl b/git-svn.perl\n> index be987e316f..93f6538d61 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> ...\n> -\tif ($@) {\n> -\t\t$term = new FakeTerm \"$@: going non-interactive\";\n> +\t\t$term = $ENV{\"GIT_SVN_NOTTY\"}\n> +\t\t\t\t? new Term::ReadLine 'git-svn', \\*STDIN, \\*STDOUT\n> +\t\t\t\t: new Term::ReadLine 'git-svn';\n> +\t\t};\n\nThis line with \"};\" on it should not be added, I think.\n\ncf. https://github.com/git/git/actions/runs/5827208598/job/15802787783#step:5:74\n\n> +\t\treturn $term;\n> \t}\n> }\n\n git-svn.perl | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 93f6538d61..e919c3f172 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -307,7 +307,6 @@ package main;\n \t\t$term = $ENV{\"GIT_SVN_NOTTY\"}\n \t\t\t\t? new Term::ReadLine 'git-svn', \\*STDIN, \\*STDOUT\n \t\t\t\t: new Term::ReadLine 'git-svn';\n-\t\t};\n \t\treturn $term;\n \t}\n }\n-- \n2.42.0-rc1\n\n"},{"id":"480521","messageId":"8d683835-31d4-41f0-9d4e-90c95acbea28@opperschaap.net","threadId":"60097","inReplyTo":"xmqqcyzupf3b.fsf@gitster.g","subject":"Re: [[PATCH v2]] Fix bug when more than one readline instance is used","fromName":"Wesley","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-08-11T01:09:52Z","receivedAt":"2023-08-11T01:10:09Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"On 8/10/23 21:01, Junio C Hamano wrote:\n> Wesley Schwengle writes:\n> \n> This line with \"};\" on it should not be added, I think.\n> \n> cf. https://github.com/git/git/actions/runs/5827208598/job/15802787783#step:5:74\n> \n>> +\t\treturn $term;\n>> \t}\n>> }\n> \n> git-svn.perl | 1 -\n> 1 file changed, 1 deletion(-)\n> \n> diff --git a/git-svn.perl b/git-svn.perl\n> index 93f6538d61..e919c3f172 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -307,7 +307,6 @@ package main;\n> \t\t$term = $ENV{\"GIT_SVN_NOTTY\"}\n> \t\t\t\t? new Term::ReadLine 'git-svn', \\*STDIN, \\*STDOUT\n> \t\t\t\t: new Term::ReadLine 'git-svn';\n> -\t\t};\n> \t\treturn $term;\n> \t}\n> }\n\nYou are 100% correct.\n\nCheers,\nWesley\n\n-- \nWesley\n\nWhy not both?\n\n"},{"id":"480526","messageId":"xmqqwmy2no2e.fsf@gitster.g","threadId":"60097","inReplyTo":"8d683835-31d4-41f0-9d4e-90c95acbea28@opperschaap.net","subject":"Re: [[PATCH v2]] Fix bug when more than one readline instance is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-11T05:30:17Z","receivedAt":"2023-08-11T05:30:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wesley writes:\n\n> On 8/10/23 21:01, Junio C Hamano wrote:\n>> Wesley Schwengle writes:\n>> This line with \"};\" on it should not be added, I think.\n>> cf. https://github.com/git/git/actions/runs/5827208598/job/15802787783#step:5:74\n>> \n>>> +\t\treturn $term;\n>>> \t}\n>>> }\n>> git-svn.perl | 1 -\n>> 1 file changed, 1 deletion(-)\n>> diff --git a/git-svn.perl b/git-svn.perl\n>> index 93f6538d61..e919c3f172 100755\n>> --- a/git-svn.perl\n>> +++ b/git-svn.perl\n>> @@ -307,7 +307,6 @@ package main;\n>> \t\t$term = $ENV{\"GIT_SVN_NOTTY\"}\n>> \t\t\t\t? new Term::ReadLine 'git-svn', \\*STDIN, \\*STDOUT\n>> \t\t\t\t: new Term::ReadLine 'git-svn';\n>> -\t\t};\n>> \t\treturn $term;\n>> \t}\n>> }\n>\n> You are 100% correct.\n\nAnd embarrassingly, the above is not sufficient, as the way $term is\nused in git-send-email and git-svn are subtly different.\n\nI think we further need something like this on top, but my Perl is\nrusty.\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex e919c3f172..6033b97a0c 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -427,7 +427,7 @@ sub ask {\n \tmy $default = $arg{default};\n \tmy $resp;\n \tmy $i = 0;\n-\tterm_init() unless $term;\n+\tmy $term = term_init();\n \n \tif ( !( defined($term->IN)\n && defined( fileno($term->IN) )\n-- \n2.42.0-rc1\n\n"},{"id":"480538","messageId":"20230811145121.GB2303200@coredump.intra.peff.net","threadId":"60097","inReplyTo":"xmqqwmy2no2e.fsf@gitster.g","subject":"Re: [[PATCH v2]] Fix bug when more than one readline instance is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-11T14:51:21Z","receivedAt":"2023-08-11T14:51:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 10, 2023 at 10:30:17PM -0700, Junio C Hamano wrote:\n\n> And embarrassingly, the above is not sufficient, as the way $term is\n> used in git-send-email and git-svn are subtly different.\n> \n> I think we further need something like this on top, but my Perl is\n> rusty.\n> \n> diff --git a/git-svn.perl b/git-svn.perl\n> index e919c3f172..6033b97a0c 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -427,7 +427,7 @@ sub ask {\n> \tmy $default = $arg{default};\n> \tmy $resp;\n> \tmy $i = 0;\n> -\tterm_init() unless $term;\n> +\tmy $term = term_init();\n> \n> \tif ( !( defined($term->IN)\n> && defined( fileno($term->IN) )\n\nHmm. Isn't that an indication that git-svn is OK as-is?\n\nLooking at the version of git-svn.perl on the tip of master, I see we\ndeclare a global $term along with the initializer:\n\n my $term;\n sub term_init {\n $term = eval { ...etc... }\n\nAnd then later in ask we call term_init() only if it's uninitialized:\n\n sub ask {\n ...\n term_init() unless $term;\n\nSo those are looking at the same $term, and the result should only be\ninitialized once.\n\nIt could still benefit from cleaning up FakeTerm, since we lazily init\nthe object since 30d45f798d (git-svn: delay term initialization,\n2014-09-14). But I don't think there's a visible bug here with the new\nversion of Term::ReadLine::Gnu.\n\n-Peff\n"},{"id":"480547","messageId":"xmqqjzu1o97n.fsf@gitster.g","threadId":"60097","inReplyTo":"20230811145121.GB2303200@coredump.intra.peff.net","subject":"Re: [[PATCH v2]] Fix bug when more than one readline instance is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-11T16:05:48Z","receivedAt":"2023-08-11T16:05:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> diff --git a/git-svn.perl b/git-svn.perl\n>> index e919c3f172..6033b97a0c 100755\n>> --- a/git-svn.perl\n>> +++ b/git-svn.perl\n>> @@ -427,7 +427,7 @@ sub ask {\n>> \tmy $default = $arg{default};\n>> \tmy $resp;\n>> \tmy $i = 0;\n>> -\tterm_init() unless $term;\n>> +\tmy $term = term_init();\n>> \n>> \tif ( !( defined($term->IN)\n>> && defined( fileno($term->IN) )\n>\n> Hmm. Isn't that an indication that git-svn is OK as-is?\n\nYes. As long as we know they share the same kind of code structure\nto use the same library function that wants its callers to stick to\na singleton instance, there is a value in using the same structure\non the side of our callers, but yes, we can rely on the global $term\nfor it being a singleton.\n\n> It could still benefit from cleaning up FakeTerm, since we lazily init\n> the object since 30d45f798d (git-svn: delay term initialization,\n> 2014-09-14). But I don't think there's a visible bug here with the new\n> version of Term::ReadLine::Gnu.\n\nTrue. Let me drop the patch from the 'next down to master\nfast-track' candidate status.\n\nThanks.\n"},{"id":"481196","messageId":"xmqqa5u888lz.fsf_-_@gitster.g","threadId":"60097","inReplyTo":"xmqqjzu1o97n.fsf@gitster.g","subject":"[PATCH] git-svn: drop FakeTerm hack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-30T22:32:08Z","receivedAt":"2023-08-30T22:32:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n> ...\n>> It could still benefit from cleaning up FakeTerm, since we lazily init\n>> the object since 30d45f798d (git-svn: delay term initialization,\n>> 2014-09-14). But I don't think there's a visible bug here with the new\n>> version of Term::ReadLine::Gnu.\n>\n> True. Let me drop the patch from the 'next down to master\n> fast-track' candidate status.\n\nWe did the above but then everybody seems to have forgotten about\nit. Let's resurrect the topic. Here is my attempt.\n\n---- >8 ----\nFrom: Wesley Schwengle \nSubject: [PATCH] git-svn: drop FakeTerm hack\n\nDrop the FakeTerm hack, just like dfd46bae (send-email: drop\nFakeTerm hack, 2023-08-08) did, for exactly the same reason.\n\nSigned-off-by: Wesley Schwengle \nSigned-off-by: Junio C Hamano \n---\n git-svn.perl | 20 ++------------------\n 1 file changed, 2 insertions(+), 18 deletions(-)\n\ndiff --git c/git-svn.perl w/git-svn.perl\nindex be987e316f..4e8878f035 100755\n--- c/git-svn.perl\n+++ w/git-svn.perl\n@@ -297,28 +297,12 @@ sub _req_svn {\n \t\t{} ],\n );\n \n-package FakeTerm;\n-sub new {\n-\tmy ($class, $reason) = @_;\n-\treturn bless \\$reason, shift;\n-}\n-sub readline {\n-\tmy $self = shift;\n-\tdie \"Cannot use readline on FakeTerm: $$self\";\n-}\n-package main;\n-\n my $term;\n sub term_init {\n-\t$term = eval {\n-\t\trequire Term::ReadLine;\n-\t\t$ENV{\"GIT_SVN_NOTTY\"}\n+\trequire Term::ReadLine;\n+\t$term = $ENV{\"GIT_SVN_NOTTY\"}\n \t\t\t? new Term::ReadLine 'git-svn', \\*STDIN, \\*STDOUT\n \t\t\t: new Term::ReadLine 'git-svn';\n-\t};\n-\tif ($@) {\n-\t\t$term = new FakeTerm \"$@: going non-interactive\";\n-\t}\n }\n \n my $cmd;\n"},{"id":"481202","messageId":"20230831001325.GA2685726@coredump.intra.peff.net","threadId":"60097","inReplyTo":"xmqqa5u888lz.fsf_-_@gitster.g","subject":"Re: [PATCH] git-svn: drop FakeTerm hack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-31T00:13:25Z","receivedAt":"2023-08-31T00:13:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 30, 2023 at 03:32:08PM -0700, Junio C Hamano wrote:\n\n> > True. Let me drop the patch from the 'next down to master\n> > fast-track' candidate status.\n> \n> We did the above but then everybody seems to have forgotten about\n> it. Let's resurrect the topic. Here is my attempt.\n> \n> ---- >8 ----\n> From: Wesley Schwengle \n> Subject: [PATCH] git-svn: drop FakeTerm hack\n> \n> Drop the FakeTerm hack, just like dfd46bae (send-email: drop\n> FakeTerm hack, 2023-08-08) did, for exactly the same reason.\n\nYep, it looks good to me.\n\nOptionally you could add this to the commit message:\n\n It has been obsolete in git-svn since 30d45f798d (git-svn: delay term\n initialization, 2014-09-14). Note that unlike send-email, we already\n make sure to load Term::ReadLine only once. So this is just a cleanup,\n and not fixing any bug.\n\n-Peff\n"},{"id":"481203","messageId":"xmqqpm34ys17.fsf@gitster.g","threadId":"60097","inReplyTo":"20230831001325.GA2685726@coredump.intra.peff.net","subject":"Re: [PATCH] git-svn: drop FakeTerm hack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T00:28:04Z","receivedAt":"2023-08-31T00:28:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Optionally you could add this to the commit message:\n>\n> It has been obsolete in git-svn since 30d45f798d (git-svn: delay term\n> initialization, 2014-09-14). Note that unlike send-email, we already\n> make sure to load Term::ReadLine only once. So this is just a cleanup,\n> and not fixing any bug.\n\nThanks. That reads extremely well.\n"}]}