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

http: tidy up exposure of header validation #33371

Closed
wants to merge 1 commit into from
Closed

http: tidy up exposure of header validation #33371

wants to merge 1 commit into from

Conversation

osher
Copy link
Contributor

@osher osher commented May 12, 2020

Internal refactoring, following:
Following: #33119 (comment)

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

@osher
Copy link
Contributor Author

osher commented May 12, 2020

Ha. the change is completely internal - the tests even did not have to change 😀

@osher
Copy link
Contributor Author

osher commented May 12, 2020

@mscdex, @addaleax - as promised :)

@osher osher marked this pull request as ready for review May 12, 2020 15:33
@osher
Copy link
Contributor Author

osher commented May 12, 2020

mm. donno why the commit list is noisy...
I was following https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/syncing-a-fork

(first one on this for me as well, all my PRs so far started with editing a file online, and cloning the auto-generated repo)

if I'm doing something wrong - tell me, I'll fix it. even create a new PR if need be.

@addaleax
Copy link
Member

@osher Yes, we don’t do merge commits, and it’s unfortunate that the official Github docs don’t mention that git rebase may be preferential to git merge. In particular, our CI may fail if there are merge commits. (I’ll start one anyway because I don’t think this merge commit resolves a conflict, which is what I think would throw off CI, but it might fail anyway.)

If CI works, then I think you don’t need to update anything. If it doesn’t, you’ll need to update the branch again, but this time using git rebase rather than git merge.

@addaleax addaleax added the http Issues or PRs related to the http subsystem. label May 12, 2020
@nodejs-github-bot
Copy link
Collaborator

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

lpinca
lpinca reviewed May 13, 2020
View reviewed changes
lib/_http_outgoing.js
@@ -918,9 +918,8 @@ function(err, event) {
this.destroy(err);
};

OutgoingMessage.validateHeaderName = validateHeaderName;
Copy link
Member

Choose a reason for hiding this comment

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

I guess this makes this patch semver-major?

Copy link
Contributor Author

@osher osher May 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@lpinca - good spot. Here's the thing:

this line landed just about a week ago, and was decided to be done better as it is featured now in a discussion that continued after the landing in a friendly spirit.
It was also undocumented - it's an implementation detail.

If that's a problem - then I can leave that line.
LMK how to proceed.

@osher
Copy link
Contributor Author

osher commented May 14, 2020 •
edited
Loading

If CI works, then I think you don’t need to update anything. If it doesn’t, you’ll need to update the branch again, but this time using git rebase rather than git merge.

@addaleax

I updated the branch, just to learn how to do it. (edited: I think it also failed CI)
pls LMK if I'm on target or not...

What I basically did was

  1. since all changes are in a single commit - I created a patch from it using:
git format-patch -1 HEAD
  1. checkout the last commit from master, one before that auto-merge
  2. create a tmp branch on it, and apply the patch to it using
git am < 0001-http-tidy-up-exposure-of-header-validation.patch

(that's the file name I got)
4. validate the patch is there using git log
5. delete patch-1.1 and create it on tmp, and delete the tmp local branch.
6. force push patch-1.1

cheers

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 23, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 24, 2020 •
edited by BridgeAR
Loading

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

BridgeAR pushed a commit that referenced this pull request May 25, 2020
@osher @BridgeAR
http: tidy up exposure of header validation
e30a651 
PR-URL: #33371
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

Landed in e30a651

@BridgeAR BridgeAR closed this May 25, 2020
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@osher @codebytere
http: tidy up exposure of header validation
c2aad81 
PR-URL: #33371
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@osher @codebytere
http: tidy up exposure of header validation
de69354 
PR-URL: #33371
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Aug 17, 2020
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@osher @addaleax @nodejs-github-bot @BridgeAR @benjamingr @lpinca @codebytere