Skip to content

[v18.x backport] lib: reset RegExp statics before running user code#44247

Closed
aduh95 wants to merge 1 commit intonodejs:v18.x-stagingfrom
aduh95:backport-regexp-statics-great-reset
Closed

[v18.x backport] lib: reset RegExp statics before running user code#44247
aduh95 wants to merge 1 commit intonodejs:v18.x-stagingfrom
aduh95:backport-regexp-statics-great-reset

Conversation

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 16, 2022

Fixes: #43740

PR-URL: #43741
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Geoffrey Booth [email protected]
Reviewed-By: Matteo Collina [email protected]

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 16, 2022

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Aug 16, 2022
@aduh95
Copy link
Contributor Author

aduh95 commented Aug 16, 2022

It looks like the lint failure was introduced by 5cb5c65.

@aduh95 aduh95 added the blocked PRs that are blocked by other issues or PRs. label Aug 16, 2022
This was referenced Aug 16, 2022
Merged
Closed
@danielleadams
Copy link
Contributor

danielleadams commented Aug 17, 2022

@aduh95 I had to rest v18.x-staging, sorry but this will need a rebase

@aduh95
lib: reset RegExp statics before running user code
aa76869 
Fixes: nodejs#43740

PR-URL: nodejs#43741
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@aduh95 aduh95 force-pushed the backport-regexp-statics-great-reset branch from 48d92db to aa76869 Compare August 23, 2022 16:58
@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed blocked PRs that are blocked by other issues or PRs. labels Aug 23, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 23, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/46185/

This was referenced Aug 24, 2022
Open
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 5, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/46422/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 5, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/46428/

@github-actions github-actions bot mentioned this pull request Sep 6, 2022
Open
17 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 6, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/46432/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 6, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/46444/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 6, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/46455/

@github-actions github-actions bot mentioned this pull request Sep 7, 2022
Open
15 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 7, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/46461/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 7, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/46462/

@github-actions github-actions bot mentioned this pull request Sep 8, 2022
Open
12 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 8, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/46487/

This was referenced Sep 9, 2022
Open
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 16, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/46619/

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 16, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 16, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 16, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/46620/

@RafaelGSS
Copy link
Member

RafaelGSS commented Sep 23, 2022

@aduh95 Is it ready to land? I'm about to including it to the v18 proposal

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 23, 2022

It is ready to land, thanks for taking care of this :)

@RafaelGSS
Copy link
Member

RafaelGSS commented Sep 23, 2022

It is ready to land, thanks for taking care of this :)

Can you amend the Backport-PR-URL metadata to your commit message?

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 23, 2022

It is ready to land, thanks for taking care of this :)

Can you amend the Backport-PR-URL metadata to your commit message?

Sure I can, but I find it surprising as of all the backport PRs I've opened, I've never touched the commit message, and it was taken care of upon landing – also if I push another commit, that means we'd probably need another CI run, which I'd rather avoid. Are you sure that's not something that ncu takes care of?

@RafaelGSS
Copy link
Member

RafaelGSS commented Sep 23, 2022

@nodejs/releasers Do ncu handle it by default? I'm just asking because following the https://github.com/nodejs/node/blob/main/doc/contributing/backporting-to-release-lines.md#how-to-submit-a-backport-pull-request step 9.v says:

Amend the commit message and include a Backport-PR-URL: metadata and re-push the change to your fork.

I think I can include that metadata while merging using git node land X, but I'm not quite sure if that's the correct workflow.

@RafaelGSS
Copy link
Member

RafaelGSS commented Sep 23, 2022

Landed in 687ffcc

@RafaelGSS RafaelGSS closed this Sep 23, 2022
RafaelGSS pushed a commit that referenced this pull request Sep 23, 2022
@aduh95 @RafaelGSS
lib: reset RegExp statics before running user code
687ffcc 
Fixes: #43740

Backport-PR-URL: #43741
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
PR-URL: #44247
Reviewed-By: Rafael Gonzaga <[email protected]>
@RafaelGSS
Copy link
Member

RafaelGSS commented Sep 23, 2022

@aduh95 just realized that I can do it manually using ncu. Sorry for the inconvenience.

@richardlau
Copy link
Member

richardlau commented Sep 23, 2022

@nodejs/releasers Do ncu handle it by default? I'm just asking because following the https://github.com/nodejs/node/blob/main/doc/contributing/backporting-to-release-lines.md#how-to-submit-a-backport-pull-request step 9.v says:

Amend the commit message and include a Backport-PR-URL: metadata and re-push the change to your fork.

I think I can include that metadata while merging using git node land X, but I'm not quite sure if that's the correct workflow.

Use --backport with git node land (nodejs/node-core-utils#383).

@RafaelGSS RafaelGSS mentioned this pull request Sep 23, 2022
Closed
@aduh95 aduh95 deleted the backport-regexp-statics-great-reset branch September 24, 2022 12:30
@RafaelGSS RafaelGSS mentioned this pull request Sep 26, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
@aduh95 @RafaelGSS
lib: reset RegExp statics before running user code
d8d34ae 
Fixes: #43740

Backport-PR-URL: #43741
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
PR-URL: #44247
Reviewed-By: Rafael Gonzaga <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aduh95 @nodejs-github-bot @danielleadams @RafaelGSS @richardlau