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

Forward port from v1.x #1559

Closed
wants to merge 2 commits into from
Closed

Forward port from v1.x #1559

wants to merge 2 commits into from

Conversation

Fishrock123
Copy link
Contributor

Related PRs: #1520, #1517

I was under the impression we were going to be landing everything in master and then backport any fixes, unless something doesn't apply to master.

/cc @iojs/tc, esp @trevnorris & @indutny

@Fishrock123 Fishrock123 added the meta Issues and PRs related to the general management of the project. label Apr 29, 2015
@Fishrock123 Fishrock123 added this to the 2.0.0 milestone Apr 29, 2015
This was referenced Apr 29, 2015
Closed
Merged
@Fishrock123
Copy link
Contributor Author

If I could get some quick review here that'd be great. It'd be good to get these into 2.0.0 so that we don't have divergence.

@bnoordhuis
Copy link
Member

Rubber-stamp LGTM. Maybe run the CI just in case.

@Fishrock123
Copy link
Contributor Author

Good idea.

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/638/

@Fishrock123 Fishrock123 self-assigned this Apr 29, 2015
@Fishrock123
Copy link
Contributor Author

It would be nice to have some sort of metadata to point to PRs for porting patches. Hmm.

@chrisdickinson
Copy link
Contributor

@Fishrock123 So, in this case I believe the appropriate set of operations would be:

git checkout master
git merge v1.x
git checkout v1.x
git cherry-pick <missing-commits>

trevnorris and others added 2 commits April 30, 2015 17:53
@trevnorris @Fishrock123
buffer: fix copy() segfault with zero arguments
30fb284 
Buffer#copy() immediately does a ToObject() on the first argument before
it checks if it's even an Object. This causes
Object::HasIndexedPropertiesInExternalArrayData() to be run on nothing,
triggering the segfault. Instead run HasInstance() on the args Value.
Which will check if it's actually an Object, before checking if it
contains data.

Fixes: nodejs#1519
PR-URL: nodejs#1520
PORT-PR-URL: nodejs#1559
Reviewed-by: Evan Lucas <evanlucas@me.com>
@indutny @Fishrock123
deps: update http_parser to 2.5.0
9f45e5e 
PR-URL: nodejs#1517
PORT-PR-URL: nodejs#1559
Reviewed-By: Brian White <mscdex@mscdex.net>
@Fishrock123 Fishrock123 force-pushed the forward-port-from-v1.x branch from 074ed63 to 9f45e5e Compare April 30, 2015 21:54
@Fishrock123
Copy link
Contributor Author

@chrisdickinson Some things just got back-ported into v1.x though, won't that cause conflicts now? :/ Or do I just merge with the "ours" rule?

@chrisdickinson
Copy link
Contributor

Ideally this would have gone in first so that the backport commits wouldn't get duplicated.

@chrisdickinson
Copy link
Contributor

I'm going to take a look and see if git chokes on this – I suspect it won't, but it's surprised me before :)

@Fishrock123
Copy link
Contributor Author

@chrisdickinson any update? Either way the commits should really make it into 2.0.0

@chrisdickinson
Copy link
Contributor

git merge seems to work but produces something of an ugly history with duplicated commits. I'm leaning towards cherry-picking these commits into master at present.

@Fishrock123 Fishrock123 mentioned this pull request May 1, 2015
Closed
@Fishrock123
Copy link
Contributor Author

@chrisdickinson would it work if we did a merge from a branch or something from a point where only these two commits existed in v1.x? Or is a cherry-pick still cleaner?

If cherry-pick is cleaner, is this meta-data reasonable?

@chrisdickinson
Copy link
Contributor

The metadata looks reasonable. Try a merge from $(git merge-base master v1.x)+onlythose2changes, but if it doesn't work out, or otherwise produces a complicated history – see git log --pretty=oneline --graph after merging – you should cherry-pick.

@Fishrock123
Copy link
Contributor Author

@chrisdickinson the result of that is this: https://github.com/Fishrock123/io.js/commits/master-with-v1.x-merge

I'd personally prefer to cherry-pick, but that may not be the best option.

@Fishrock123
Copy link
Contributor Author

The commit history from git log --pretty=oneline --graph looks like this:

*   daa05ca9448b5ef09740883f8833c21cfa228746 Merge branch 'v1.x'
|\  
| * b3a7da10919445c40f1f36868e529bd2f6ea7269 deps: update http_parser to 2.5.0
| * 5404cbc74533e2d52861221201b166e7c9c015ec buffer: fix copy() segfault with zero arguments
| * 2f6986ee4693b2001f34c98957f337f0cff6204d Working on v1.8.2
* | b4ad5d7050581fe615dab0ec8ef23c83bd965acb doc: improve http.request and https.request opts
* | 550c2638c0885f9cbb1022f8f5234015e21836fe tls: use `SSL_set_cert_cb` for async SNI/OCSP
(... commits ...)
* | 26327757f8b342eb91df71f1fdf34a855e29aa71 doc: update AUTHORS list
* |   59a5c98f2c987080634ed078c9868e23e8c55820 Merge v1.8.1.
|\ \

@chrisdickinson
Copy link
Contributor

Looking over it, the merge commit should work. Thanks for catching these commits!

@Fishrock123 Fishrock123 mentioned this pull request May 1, 2015
Merged
@Fishrock123
Copy link
Contributor Author

replaced by #1582

@Fishrock123 Fishrock123 closed this May 2, 2015
@Fishrock123 Fishrock123 deleted the forward-port-from-v1.x branch May 12, 2015 15:38
@B020239 B020239 mentioned this pull request Mar 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Fishrock123 Fishrock123

Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Fishrock123 @bnoordhuis @chrisdickinson @trevnorris @indutny