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.

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

Revert 7082 #7092

Closed
wants to merge 4 commits into from
Closed

Revert 7082 #7092

wants to merge 4 commits into from

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Jun 1, 2016

This reverts PR #7082 completely.
#7082 was landed too fast and did not have sufficient review time (under 10 hours instead of 48).

That PR also broke some things (testcases will follow in a separate PR).

/cc @bnoordhuis @trevnorris @jasnell
/cc @nodejs/ctc

ChALkeR added 4 commits June 1, 2016 16:29
This reverts commit 27e84dd.

nodejs#7082 was landed too fast and did
not have sufficient review time.

That PR also broke some things (testcases will follow separately).
This reverts commit 334ef4f.

nodejs#7082 was landed too fast and did
not have sufficient review time.

That PR also broke some things (testcases will follow separately).
This reverts commit c3cd453.

nodejs#7082 was landed too fast and did
not have sufficient review time.

That PR also broke some things (testcases will follow separately).
This reverts commit 0301ce9.

nodejs#7082 was landed too fast and did
not have sufficient review time.

That PR also broke some things (testcases will follow separately).
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jun 1, 2016
@bnoordhuis
Copy link
Member

What does it break? CI was green for #7082.

did not have sufficient review time (under 10 hours instead of 48).

I think that should be understood to refer to potentially contentious changes, API additions, etc. This was just general code cleanup in preparation for upcoming changes. If I had to wait two days for every such change, it wouldn't be finished this side of Christmas.

Last but not least, the two people who should review it, did and signed off on it.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 1, 2016

@bnoordhuis

What does it break? CI was green for #7082.

I cc-d you to the relevant issue.

@indutny
Copy link
Member

indutny commented Jun 1, 2016

@ChALkeR perhaps, the issue should be linked here?

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 1, 2016

I think that should be understood to refer to potentially contentious changes, API additions, etc. This was just general code cleanup in preparation for upcoming changes. If I had to wait two days for every such change, it wouldn't be finished this side of Christmas.

Ok, I could have misunderstood. But COLLABORATOR_GUIDE.md should be changed to reflect that, because I based my judgement on what is written there.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 1, 2016

@indutny It is.

@indutny
Copy link
Member

indutny commented Jun 1, 2016

@ChALkeR I can't see it, what exactly broke?

@jasnell
Copy link
Member

jasnell commented Jun 1, 2016

Looks like we are missing a regression test. Rather than revert this, we
can add the try-finally back and add the necessary regression test. I
missed it because I had assumed a regression test had already been added
and saw the green ci.

I do agree that this pr likely needed to wait a bit longer before landing.
On Jun 1, 2016 6:50 AM, "Сковорода Никита Андреевич" <
notifications@github.com> wrote:

@bnoordhuis https://github.com/bnoordhuis

What does it break? CI was green for #7082
#7082.

I cc-d you to the relevant issue.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7092 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eZuj48dDqF0S6AfNQvzv9dh6dAncks5qHY4NgaJpZM4IrjKF
.

@indutny
Copy link
Member

indutny commented Jun 1, 2016

I agree with @jasnell on this.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 1, 2016

@jasnell @indutny I agree that there should have been a regression test, and that's my fault that it wasn't added.

I initially waited for corresponding 4.x and 5.x releases before publishing that, but it's safe now. I will file a PR with a testcase in several minutes.

@Fishrock123
Copy link
Contributor

Where are links to what this broke? Did I miss them? I don't see them here.

@bnoordhuis
Copy link
Member

I do agree that this pr likely needed to wait a bit longer before landing.

I don't think the outcome would have been materially different. The issue is that a fix was landed without an accompanying regression test. It's good that @ChALkeR caught it quickly but that's really just luck.

@Fishrock123 https://github.com/nodejs/security/issues/46

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 1, 2016

Closing in favor of #7093.

@ChALkeR ChALkeR closed this Jun 1, 2016
@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 1, 2016

@bnoordhuis Just to be clear — I did not want to upset or annoy anyone when filing this PR.

I have considered two options — filing a follow-up fix or proposing a revert, and I have chosen the latter for the following reasons:

  1. I thought that if something got broken by some PR, then the quickest way to fix that short-term is just to revert the PR and give it more time to shape before landing back — so I anticipated that reverting would be the most straightforward fix.
  2. Again, trying to fix that asap, I have not reviewed the other changes in the PR at that time and I was not sure how they relate to the actual breaking change — and that would have been required for a clean fix.
  3. I believed that that fact that this got slipped, was an indication that the PR was not so trivial and needed more review time.

Sorry if I was a bit pushy with this, I had no such intent.

@bnoordhuis
Copy link
Member

Don't worry, I'm neither upset nor annoyed.

@rvagg
Copy link
Member

rvagg commented Jun 2, 2016

@ChALkeR fwiw I think you did the right thing, filling reverts even if you don't think they'll land is an appropriate way to get action on important breakages.

I also agree that it landed too quickly, even for things we think are simple you need to allow time to get eyes on the code and this is becoming increasingly important as we have more people because it's getting harder for us to be across all of the things that we need to be and catch-up times are extending for many of us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants