Stream: t-compiler

Topic: to revert or not to revert that is the question


pnkfelix (Nov 14 2019 at 15:55, on Zulip):

one easy way to go would be that if the PR author themself approves of a revert, then it shouldn't require any other input beyond a single rustc reviewer to r+ the revert

centril (Nov 14 2019 at 15:56, on Zulip):

Well, to give an example, I would I think feel differently if we finally made e.g. https://github.com/rust-lang/rust/pull/59546 not require a -Z flag and then it gets reverted due to a perf regression in two companies.
It's not just about trust; it's also about priorities, and they can be different.

pnkfelix (Nov 14 2019 at 15:56, on Zulip):

so I guess the scenario I'm concerned about is when we want to take action and the original PR author is unavailable or does not support the revert.

centril (Nov 14 2019 at 15:56, on Zulip):

but in general I don't think this will be an issue

pnkfelix (Nov 14 2019 at 15:56, on Zulip):

Okay well let me table the question for now

pnkfelix (Nov 14 2019 at 15:57, on Zulip):

maybe we can dedicate a separate topic to it for async discussion

centril (Nov 14 2019 at 15:57, on Zulip):

(split the topic now?)

pnkfelix (Nov 14 2019 at 15:57, on Zulip):

we can either now do lightning fast WG checkin(s), or look at another nominated issue

pnkfelix (Nov 14 2019 at 15:57, on Zulip):

oh yeah let me do that

pnkfelix (Nov 14 2019 at 16:12, on Zulip):

BTW, Part of the reason why I wanted us to have some guidelines here

nikomatsakis (Nov 14 2019 at 16:12, on Zulip):

Sorry I was AFK for most of this. My take is that we should make it policy that people can revert easily if they think it's warranted. My reasons are:

I think that people will naturally be less likely to revert a major change (like fixing the infinite loop UB) than they would be to minor updates that cause problems, and adding a "minimum bar of folks" to check-in is just kind of bureaucracy we don't need.

What would probably be helpful is some directions for the steps to take when reverting, and in particular some kind of scheme (nominations?) to help make sure the reverted PR gets discussed -- maybe even a template for the comment that helps to ensure that the PR author knows this is not a personal strike against their code. I think people will understand.

Basically, I think we should have a page on forge detailed our "revert policy" that includes a copy-and-paste comment linking to it and explaining what's happening.

pnkfelix (Nov 14 2019 at 16:13, on Zulip):

BTW, Part of the reason why I wanted us to have some guidelines here

... was because I observed an interesting back-and-forth on the revert PR #66378

pnkfelix (Nov 14 2019 at 16:14, on Zulip):

where eddyb r+'ed the revert, and the PR author themself r-'ed it because I (pnkfelix) had nominated the associated issue for discussion at the triage meeting.

pnkfelix (Nov 14 2019 at 16:14, on Zulip):

I took the initiative and r+'ed it again

rkruppe (Nov 14 2019 at 16:15, on Zulip):

for completeness: I discussed that out of line with eddyb, asking if they were aware of the nomination and wanted to r+ regardless. they weren't and didn't so we agreed to r-

pnkfelix (Nov 14 2019 at 16:15, on Zulip):

rkruppe (revert PR author) was probably just exercising caution; not wanting to step on toes of the original PR author

pnkfelix (Nov 14 2019 at 16:16, on Zulip):

for completeness: I discussed that out of line with eddyb, asking if they were aware of the nomination and wanted to r+ regardless. they weren't and didn't so we agreed to r-

Ah thank you, that is useful context.

pnkfelix (Nov 14 2019 at 16:16, on Zulip):

So it sounds like @nikomatsakis is at least in favor of letting any one T-compiler member have authority to revert

pnkfelix (Nov 14 2019 at 16:17, on Zulip):

based on their comment above

pnkfelix (Nov 14 2019 at 16:18, on Zulip):

And really, I'm not actually opposed to this; specifically because in practice, I think the T-compiler members will end up discussing reverts amongst themselves anyway before acting

centril (Nov 14 2019 at 16:20, on Zulip):

I think that will work well almost always

pnkfelix (Nov 14 2019 at 16:23, on Zulip):

So it seems like the main action item is to follow-up on @nikomatsakis 's suggestion: document steps to take when reverting, including scheme to ensure the reverted PR gets discussed and "maybe even a template for the comment that helps to ensure that the PR author knows this is not a personal strike against their code."

Last update: Dec 12 2019 at 01:15UTC