Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repl: catch \v and \r in new-line detection #54512

Merged
merged 4 commits into from
Sep 25, 2024

Conversation

RedYetiDev
Copy link
Member

Fixes #54504
Uses RegExpPrototypeExec (with \n, \r, and \v) rather than StringPrototypeIndexOf with \n.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Aug 22, 2024
@aduh95
Copy link
Contributor

aduh95 commented Aug 22, 2024

Can you add a test?

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.24%. Comparing base (76edde5) to head (500c5ee).
Report is 18 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54512      +/-   ##
==========================================
- Coverage   88.26%   88.24%   -0.03%     
==========================================
  Files         651      651              
  Lines      183894   183877      -17     
  Branches    35858    35857       -1     
==========================================
- Hits       162315   162255      -60     
- Misses      14882    14898      +16     
- Partials     6697     6724      +27     
Files with missing lines Coverage Δ
lib/internal/repl/utils.js 95.74% <100.00%> (ø)

... and 34 files with indirect coverage changes

@mikesamuel
Copy link
Contributor

@RedYetiDev Thanks for the quick fix.

@RedYetiDev
Copy link
Member Author

CC @nodejs/repl

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

Thank you for addressing this

Copy link
Member

@VoltrexKeyva VoltrexKeyva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RedYetiDev RedYetiDev added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 31, 2024
@RedYetiDev
Copy link
Member Author

Maybe this'll fix it?

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 4, 2024

Could someone start a new CI? Triagers can't start one without an approving review.

@RedYetiDev RedYetiDev added the review wanted PRs that need reviews. label Sep 10, 2024
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@RedYetiDev RedYetiDev added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed review wanted PRs that need reviews. labels Sep 19, 2024
@nodejs-github-bot
Copy link
Collaborator

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 22, 2024

CI LGTM. This should be good to commit-queue?

@H4ad H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 23, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 23, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/54512
✔  Done loading data for nodejs/node/pull/54512
----------------------------------- PR info ------------------------------------
Title      repl: catch `\v` and `\r` in new-line detection (#54512)
Author     Aviv Keller <redyetidev@gmail.com> (@RedYetiDev)
Branch     RedYetiDev:patch-106 -> nodejs:main
Labels     repl, author ready, needs-ci, commit-queue-squash
Commits    2
 - repl: catch `\v` and `\r` in new-line detection
 - disable if no inspector
Committers 2
 - RedYetiDev <38299977+RedYetiDev@users.noreply.github.com>
 - GitHub <noreply@github.com>
PR-URL: https://github.com/nodejs/node/pull/54512
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54512
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - disable if no inspector
   ℹ  This PR was created on Thu, 22 Aug 2024 20:48:50 GMT
   ✔  Approvals: 3
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/54512#pullrequestreview-2258269961
   ✔  - Mohammed Keyvanzadeh (@VoltrexKeyva): https://github.com/nodejs/node/pull/54512#pullrequestreview-2259289159
   ✔  - LiviaMedeiros (@LiviaMedeiros): https://github.com/nodejs/node/pull/54512#pullrequestreview-2274130019
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-09-21T15:51:28Z: https://ci.nodejs.org/job/node-test-pull-request/62627/
- Querying data for job/node-test-pull-request/62627/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/11001786432

@RedYetiDev
Copy link
Member Author

I reverted the change suggested because JSON.stringify('\v') !== '"\\v"', which breaks the length of the character when rendered.

@jakecastelli jakecastelli added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Sep 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024
@nodejs-github-bot
Copy link
Collaborator

@RedYetiDev
Copy link
Member Author

CI is 🟢 (yay!)

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 24, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 24, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/54512
✔  Done loading data for nodejs/node/pull/54512
----------------------------------- PR info ------------------------------------
Title      repl: catch `\v` and `\r` in new-line detection (#54512)
Author     Aviv Keller <redyetidev@gmail.com> (@RedYetiDev)
Branch     RedYetiDev:patch-106 -> nodejs:main
Labels     repl, author ready, needs-ci, commit-queue-squash
Commits    4
 - repl: catch `\v` and `\r` in new-line detection
 - disable if no inspector
 - fixup!
 - fixup! fixup!
Committers 1
 - RedYetiDev <38299977+RedYetiDev@users.noreply.github.com>
PR-URL: https://github.com/nodejs/node/pull/54512
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54512
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - repl: catch `\v` and `\r` in new-line detection
   ⚠  - disable if no inspector
   ⚠  - fixup!
   ⚠  - fixup! fixup!
   ℹ  This PR was created on Thu, 22 Aug 2024 20:48:50 GMT
   ✔  Approvals: 4
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/54512#pullrequestreview-2258269961
   ✔  - Mohammed Keyvanzadeh (@VoltrexKeyva): https://github.com/nodejs/node/pull/54512#pullrequestreview-2259289159
   ✔  - LiviaMedeiros (@LiviaMedeiros): https://github.com/nodejs/node/pull/54512#pullrequestreview-2274130019
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/54512#pullrequestreview-2323267187
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-09-24T15:06:29Z: https://ci.nodejs.org/job/node-test-pull-request/62735/
- Querying data for job/node-test-pull-request/62735/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/11021667751

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 25, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 25, 2024
@nodejs-github-bot nodejs-github-bot merged commit c6d20a0 into nodejs:main Sep 25, 2024
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c6d20a0

targos pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #54512
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 mentioned this pull request Oct 9, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#54512
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speculative execution in REPL shows TypeError overwriting input
10 participants