Add Cowboy 0.8.x Support
gebi opened this issue · 11 comments
Please add support for cowboy 0.8.x which adds many useful features, additional security measurements and speedups.
See https://github.com/extend/cowboy/blob/master/CHANGELOG.md
Sockjs version for cowboy 0.8.3 is at https://github.com/gebi/sockjs-erlang/tree/master
Current master now passes all unit-test from sockjs-protocol-0.3.3.py cleanly.
(except for WebsocketHixie76 where support got removed in cowboy 0.8)
@gebi - someone will look into this asap. The maintainers have been out of the loop for a while, but that'll hopefully get addressed shortly. This one's high priority when things start moving again.
@hyperthunk feel free to merge from my master. The code is going into production shortly.
@gebi that's good to know, thanks. I'll close the issue once we've merged.
hi @hyperthunk , thx for merging! Though would it be possible to merge with real authorship information? Removing authorship of code, especially open source one is not nice! (especially for open source programmers where it's one of the only incentive to have).
I made a squashed down branch for you which is equal to your "Support for Cowboy 0.8.3" commit, but retains authorship information. https://github.com/gebi/sockjs-erlang/commits/t/cowboy_0.8.3
That's the branch I used to create the merge - I simply squashed the commits down. That original branch is not squashed - it retains dozens of commits. The definition of squashed is 1 commit. So it is cleanly rebased from master, but not squashed.
Why do I care about this? Because note that a number of those commits are not complete in and of themselves - there are partial fixes that span multiple commits. This breaks git bisect so that it's far more difficult to determine where a bug was really introduced. What is the solution to this? I'm not sure, because by squashing the commits, you are choosing 1 commit which wins, into which all the others are merged. That makes it kind of hard to attribute multiple authors.
I suppose an alternative would be for me to reverse that commit, then merge with --no-ff and simply describe the merge. Personally I find that pretty gross, since it not only presents an ugly history (full of tiny fix-commits that are only relevant on the original branch itself) and still won't bisect nicely without manual intervention. But at least you wouldn't loose the authorship information.
For the record, I do understand your point about attribution. I have re-worked the master branch via merges to include both yours and Ivan's contributions in a more obvious manner. In future I hope that we won't ignore pull requests for so long that patches get this big - kind of our own fault really! :)
Thanks again for all the work you've done bringing this together.
@hyperthunk uhm, yea you are right! git bisect will be ugly with that, i'm equally unsure about what should be done...
but wait have you merged both t/cowboy_0.8.x and t/cowboy_0.8.3 from my repo?
ugh... now i'm feeling bad for creating such a mess, only one of those branches should be considered for merge! They both represent the same commits, t/cowboy_0.8.3 was just nicely squashed together to make a prettier history for you.
Hm... and it was early bisect able as well (as it seems i've just missed to pin rebar.config from yurii to a specific revision instead of letting him choose HEAD.
@hyperthunk it seems you need to revert something anyway on sockjs-master, if that's the case i might just fix up the rebar thing for yurii so you can even have a fully bisect able history as well.
(sorry, my bad for not thinking about rebase in that case, normally i'm the one rejecting such things :(!
@gebi - it doesn't really matter, since the commit history was going to be a bit gross either way. IMHO it was already a bit messy, so I don't think we're the worst offenders here. For future reference, the best course of action is usually to avoid submitting patches that contain commits by multiple authors. That effectively becomes like merging back in a fork of a project, which is a much bigger deal than just merging a patch. I do realise that the whole point of your pull request was to bring together a number of forks and ignored pull requests, so it was going to be a bit painful whatever we did.
The history, as it stands, isn't too bad. It's not bisect-able, but then reverting and re-committing won't be either, so there's no point in doing that. I could of course, rewrite the history of master to fix this. But I'm not willing to do that because
a. this is the master branch, and shouldn't be treated that way
b. someone may have already fetched since I pushed those changes
c. I am not the owner of this repository (or the software in it) to make such a drastic decision
You shouldn't feel too bad, since I also made a mistake when I decided to squash all your commits and they came from multiple authors, it was remiss of me not to push that squashed patch and proffer your feedback on it before merging.
Let's just put this one down to experience. :)
@hyperthunk +1 for experience :)!
And thx for your merge!