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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 16, 2022

Fixes: #43740

PR-URL: #43741
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de
Reviewed-By: Geoffrey Booth webadmin@geoffreybooth.com
Reviewed-By: Matteo Collina matteo.collina@gmail.com

@nodejs-github-bot
Copy link
Collaborator

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
Member

@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 <ruben@bridgewater.de>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@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

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

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

@nodejs-github-bot
Copy link
Collaborator

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

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

@nodejs-github-bot
Copy link
Collaborator

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

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

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

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

@RafaelGSS
Copy link
Member

@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

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

@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

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 <ruben@bridgewater.de>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #44247
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@RafaelGSS
Copy link
Member

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

@richardlau
Copy link
Member

@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 <ruben@bridgewater.de>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #44247
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
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.

None yet
5 participants
@aduh95 @nodejs-github-bot @danielleadams @RafaelGSS @richardlau