GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — repl: handle buffered string logic on finish by antsmartian · Pull Request #24389 · nodejs/node

Conversation

@antsmartian

@antsmartian antsmartian commented Nov 16, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Fixes : #24385

Looks like clearBufferedCommand will be called on almost all flows. Hence history was broken. Sorry about that, but I take this as learning and see why history test cases didn't catch up this in first place. I will spend some time this weekend to add few more test cases on to repl history which caused this regression.

Can some one ping repl team to make it land faster?

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and noinkling reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. labels Nov 16, 2018
@Trott

Trott commented Nov 16, 2018

Copy link
Copy Markdown
Member

@nodejs/repl
antsmartian reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@antsmartian

antsmartian commented Nov 16, 2018

Copy link
Copy Markdown
Contributor Author

cc @vsemozhetbyt
vsemozhetbyt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vsemozhetbyt

vsemozhetbyt commented Nov 16, 2018

Copy link
Copy Markdown
Contributor

Who do we need to cc to not forget including this fix in the security release that will be created soon?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@antsmartian

antsmartian commented Nov 16, 2018

Copy link
Copy Markdown
Contributor Author

@vsemozhetbyt Oh if this gets included in that release, it will be great :) Thanks for the follow up.
vsemozhetbyt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented Nov 17, 2018

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1647/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jdalton
jdalton reviewed Nov 18, 2018
View reviewed changes
Comment thread lib/readline.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@antsmartian
repl: handle buffered string logic on finish
2c179f3
@antsmartian
antsmartian force-pushed the repl-history branch from 8e33b3a to 2c179f3 Compare November 18, 2018 03:38
jasnell
jasnell approved these changes Nov 19, 2018
View reviewed changes
Fishrock123
Fishrock123 approved these changes Nov 19, 2018
View reviewed changes
@vsemozhetbyt vsemozhetbyt added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Nov 19, 2018
@vsemozhetbyt

vsemozhetbyt commented Nov 19, 2018

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/18764/
antsmartian reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@antsmartian

antsmartian commented Nov 19, 2018

Copy link
Copy Markdown
Contributor Author

@vsemozhetbyt You are super fast :)
vsemozhetbyt reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented Nov 19, 2018

Copy link
Copy Markdown
Contributor

Bleh, some unrelated ARM failure.

Thanks, landed in eb42c1e.

(Hopefully I did that right... been an awfully long time since I landed anything...)
antsmartian reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123 Fishrock123 closed this Nov 19, 2018
Fishrock123 pushed a commit that referenced this pull request Nov 19, 2018
@antsmartian @Fishrock123
repl: handle buffered string logic on finish
eb42c1e 
Looks like `clearBufferedCommand` will be called on almost all flows.
Hence history was broken.

PR-URL: #24389
Fixes: #24385
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Nov 19, 2018
test-cluster-worker-isdead crash #24496
Closed
@Fishrock123

Fishrock123 commented Nov 19, 2018

Copy link
Copy Markdown
Contributor

Made an issue about the ARM failure (a crash): #24496
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fishrock123 pushed a commit that referenced this pull request Nov 19, 2018
@antsmartian @Fishrock123
repl: handle buffered string logic on finish
c11d345 
Looks like `clearBufferedCommand` will be called on almost all flows.
Hence history was broken.

PR-URL: #24389
Fixes: #24385
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Fishrock123

Fishrock123 commented Nov 19, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Landed on 11.x in c11d345
vsemozhetbyt and antsmartian reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request Nov 19, 2018
@antsmartian @targos
repl: handle buffered string logic on finish
682b3d5 
Looks like `clearBufferedCommand` will be called on almost all flows.
Hence history was broken.

PR-URL: #24389
Fixes: #24385
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@antsmartian

antsmartian commented Nov 20, 2018

Copy link
Copy Markdown
Contributor Author

Thanks @Fishrock123
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@antsmartian
antsmartian deleted the repl-history branch November 20, 2018 02:21
@targos

targos commented Nov 20, 2018

Copy link
Copy Markdown
Member

@Fishrock123 v11.x should only be updated when a release is merged. Can you force push to remove the commit?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

targos commented Nov 20, 2018

Copy link
Copy Markdown
Member

I fixed it, but had to temporarily unprotect the release branches to do so.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos targos mentioned this pull request Nov 20, 2018
Require pull request reviews before merging to release branches nodejs/Release#392
Closed
@targos

targos commented Nov 20, 2018

Copy link
Copy Markdown
Member

@Fishrock123 No worries, btw :) I opened an issue in the Release repo to discuss a way to prevent this mistake in the future: nodejs/Release#392
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@antsmartian

antsmartian commented Nov 20, 2018

Copy link
Copy Markdown
Contributor Author

@targos Thanks. Not sure if this commit bit (#22153) is backported to 10.x stages. If so, we need to backport this fix to 10.x as well.
targos reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos targos mentioned this pull request Nov 20, 2018
repl: Adding code for handling multiline history #22153
Closed
2 of 3 tasks
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@antsmartian @rvagg
repl: handle buffered string logic on finish
bd8be40 
Looks like `clearBufferedCommand` will be called on almost all flows.
Hence history was broken.

PR-URL: #24389
Fixes: #24385
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
v11.4.0 proposal #24854
Merged
4 tasks done
This was referenced Dec 7, 2018
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#261
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#262
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#263
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#264
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#265
Closed
@codebytere codebytere added the dont-land-on-v10.x label Jan 13, 2019
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@antsmartian @refack
repl: handle buffered string logic on finish
ce5021e 
Looks like `clearBufferedCommand` will be called on almost all flows.
Hence history was broken.

PR-URL: nodejs#24389
Fixes: nodejs#24385
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
+2 more reviewers
@jdalton jdalton jdalton left review comments
@Fishrock123 Fishrock123 Fishrock123 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@antsmartian @Trott @vsemozhetbyt @Fishrock123 @targos @jdalton @jasnell @codebytere @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.