[docs] Document our norms around reverts

[reames]

Summary

This has come up a few times recently, and I was surprised to notice that we don't have anything in the docs.

I tried to stick to stuff that is uncontroversial in the community. If you see something here that 90+% of the community wouldn't agree with, please point it out. I'll remove it for now, and possibly return to individual points if warranted.

[smeenai]

llvm/docs/DeveloperPolicy.rst

319

Typo

332

Typo

[mehdi_amini]

Thanks! This looks like a great addition to the doc!

Note that we had a paragraph on revert here as well: https://llvm.org/docs/CodeReview.html#can-code-be-reviewed-after-it-is-committed ; I don't think anything is contradictory here, but I wonder if we need to consolidate or cross-reference in some way?

llvm/docs/DeveloperPolicy.rst

326

[jansvoboda11]

llvm/docs/DeveloperPolicy.rst

305

[probinson]

Please add words to the effect of: When re-landing a reverted patch, the commit message should be updated to indicate the problem that was addressed (bot failure, cite the PR, whatever).

(Separately, my personal peeve is that reverting a revert leads to headlines with a chain of "Revert "Revert "Revert ..." messages, which is really less than helpful. By adding this new section, I'll have a place to introduce a suggestion to set the message to "Reland "original message"" or some such.)

[ctetreau]

I think it's good to write this down. A revert could be interpreted as a hostile action, so the fact that they happen all the time around here should be written down.

[rupprecht]

As a whole, I think this patch is going in the right direction -- we definitely need more concrete guidance on reverting patches.

The decision tree here initially branches on whether the change is your own or someone else's change. Whoever authored/committed the patch certainly can come into play, but since the goal is to keep trunk in a good state, I think we should root the decision tree based on the content and impact of the change instead, e.g. deciding to revert a change based on the type of breakage (build failure, miscompile, performance degradation, compilation time/memory usage increase, etc.), how many platforms/users does it affect, what's the expected complexity/timeline of the fix, etc. That also emphasizes that the revert process is not personal, but just a matter of business keeping trunk green.

llvm/docs/DeveloperPolicy.rst

321

The two sections here seem to contradict each other:

• If you think the author would revert the change, you should revert the change. (Also, you should generally assume that the author would agree to a revert, based on guidance in the previous section that authors should usually revert if when asked).
• You should not revert a change until you've started a discussion

I think if you look at the details, they don't contract each other. But at a high level, they do. That should be cleared up.

330

Maybe it's just me, but I find the guidance in this section (and especially the term "third-party") a little odd and fragmenting here, as if there is a tier of better LLVM developers capable of deciding "I would revert this if I were the author", and developers outside that group are "other" LLVM developers that aren't trusted to revert patches without permission. Which is probably not what you're saying, so can you try to clarify this?

342–344

Given that the second sentence is saying it's OK to just have a promise of a test case, the wording of the first sentence to require a test case up front seems overly strict. "You should aim to have a public test case ready to share, or at least follow up with one in a reasonable time frame" might be capture it better.

342–346

Reality can sometimes be more complex than this. This is the _ideal_ case, yes. But sometimes it's not that simple, such as:

• A patch increases compile time significantly, which is more difficult to reduce that a plain crasher. Running creduce on a crasher is usually very easy because there's a simple yes/no answer (it crashes or it doesn't), whereas measuring compile time is difficult because the measurements are a lot noisier (maybe when doing an A/B compilation on a reducing test case, the second compile was only slower because something else on the system was busy, and now that reduction is on the wrong path). Sometimes these are infeasible to reduce, to the point where the burden shouldn't necessarily fall on the culprit finder.

Some cases where "near term" may be challenging to meet, depending on how long "near" is interpreted:

• A patch introduces a miscompile, which requires reducing a large test (with hundreds/thousands of dependencies) to a small example, and is not as easily reducible.
• A patch introduces a test failure, and it can take a while to figure out if it's a miscompile, or if it's just buggy code that happened to work "correctly" prior to the change

Some other cases where this may not even make sense:

• If I've committed something that requires certain platforms, a test case may not even be useful to me at all -- e.g. I don't have a Windows machine, so I can't reproduce anything specific to that. I would rather just have some hint as to what went wrong (e.g. a stack trace) and an open line of communication with whoever is broken so I can say "does this patch fix your bug?"

I don't know that we need to enumerate every possibility, but we should clarify that this is an ideal, but maybe not a hard requirement.

[reames]

Starting by fixing typos, response to more substantial suggestions to follow.

[reames]

Address Mehdi's comment.

[reames]

In D99305#2650472, @probinson wrote:

Please add words to the effect of: When re-landing a reverted patch, the commit message should be updated to indicate the problem that was addressed (bot failure, cite the PR, whatever).

Incorporated.

(Separately, my personal peeve is that reverting a revert leads to headlines with a chain of "Revert "Revert "Revert ..." messages, which is really less than helpful. By adding this new section, I'll have a place to introduce a suggestion to set the message to "Reland "original message"" or some such.)

While I personally agree with this, I've seen some disagreement on this. Given that, I'll defer this addition to a separate change to keep this one simple and not controversial.

[reames]

Address probinson's comment

[reames]

In D99305#2651281, @rupprecht wrote:

As a whole, I think this patch is going in the right direction -- we definitely need more concrete guidance on reverting patches.

The decision tree here initially branches on whether the change is your own or someone else's change. Whoever authored/committed the patch certainly can come into play, but since the goal is to keep trunk in a good state, I think we should root the decision tree based on the content and impact of the change instead, e.g. deciding to revert a change based on the type of breakage (build failure, miscompile, performance degradation, compilation time/memory usage increase, etc.), how many platforms/users does it affect, what's the expected complexity/timeline of the fix, etc. That also emphasizes that the revert process is not personal, but just a matter of business keeping trunk green.

I personally feel this proposed reframing misses one of the really important parts of the proposed docs - that is, the interpersonal piece. To me, probably the single most important piece in the whole doc is the bit about reverts generally being a favor to the commit author. I really really hesitate to loose that.

I'm curious what others think, but I don't plan to take this suggestion unless the consensus from the community is that we should.

[reames]

Respond to rupprecht (see upcoming patch refresh as well)

llvm/docs/DeveloperPolicy.rst

321

I really don't see the contradiction. These two are written with different audiences in mind, and the former is deliberately inclusive of someone choosing not to revert and leaving the choice to the commit author. (Say because it would require potentially reverting a whole bunch of changes.)

330

I think this piece needs a lot more consideration and probably runs afoul of my "nothing controversial" rule. I dropped most of it for the moment, we can return to this in another review.

342–344

Good catch. I tried to reword to address this, let me know if it needs further work.

342–346

You raise some excellent points here. Please let me know if my attempted rewording is adequate. I tried to split two responsibilities out of what had previously been one. I think this better addresses the point you raised.

[reames]

Reword to try and address rupprecht points.

[hubert.reinterpretcast]

llvm/docs/DeveloperPolicy.rst

317–318

How portable and self-contained is this test case?

320

... doesn't the "further destabilize" note below actually apply here too?

323

This point along with the "if the author themselves would revert" below means that unilateral reverts without discussion is being encouraged here. I'm not in favour of action without discussion.

[hubert.reinterpretcast]

llvm/docs/DeveloperPolicy.rst

352

[reames]

llvm/docs/DeveloperPolicy.rst

317–318

I can't think of anyway to craft a useful guideline here (beyond what is already said elsewhere). If you have suggested wording, please share.

(Though, do you mind if we defer this to a future change? This seems minor compared to the rest of it.)

320

Judgement is always required. I don't think it's useful to repeat that in every guideline. Do you think I need to emphasize that more somewhere?

323

This is our long standing practice.

[hubert.reinterpretcast]

llvm/docs/DeveloperPolicy.rst

317–318

338

I think moving this part up (and reducing the assumptions below that the reverting party is not the author and also the assumptions that the action of "reverting" is taking place) would be an improvement.

[hubert.reinterpretcast]

llvm/docs/DeveloperPolicy.rst

323

Sure, but probably for specific reasons and not for all. Otherwise, there wouldn't be "requests" to revert at all (except from people with no commit access).

[hubert.reinterpretcast]

llvm/docs/DeveloperPolicy.rst

317–318

I think moving the "expectations" part up would help towards addressing my concern here.

320

For this too, I think moving the "expectations" part up will clarify the "priorities".

[JosephTremoulet]

llvm/docs/DeveloperPolicy.rst

306

FWIW, coming from a culture of gated check-ins where only the automation account has commit permissions and there's often complex infrastructure to manage the queue of pending changes, the thing I had to learn about LLVM and have to explain to newcomers is actually kind of the opposite of this first sentence -- that the LLVM community values the ability to freely commit above keeping inner-loop tests green at all times. And that this has some great benefits like facilitating breaking reviews/changes into small incremental pieces, fostering a culture of trust and accountability, etc. And that the commit rate in LLVM is quite high (looking at git stats for the past year, there were an average of 95 commits per day, with a peak of 172 commits in one day) to facilitate that, and adding commit barriers would impede that.

Then after all that is when we get to "how does testing work?" and that often it's only mostly green and there are breaks and reverts every day and it's not considered a big deal but rather just considered a fact of a healthily active codebase.

I don't know if and how much of that is worth including here, and apologies if I'm speaking out of turn and mischaracterizing the community, but it struck me that this seems to be written for the audience asking "how dare you revert my commit?!" and wanted to note there's another school out there more likely to ask "how dare you push straight to master?!" :)

[zero9178]

As a very new contributor to LLVM (and open source in general, truth be told), I would like to just quickly mention that I like this addition to the documentation a lot. Getting one's commit reverted is definitely a rather scary experience at first, but I think this documentation, and in particular the wording chosen, helps reinforce LLVM as a positive development environment. It has certainly helped me.

[lattner]

I'm also looking forward to this, I think it is a great addition. Lemme know when the writing converges and I'll do a final review, thanks!

[reames]

Attempt to address comments from various reviewers.

[reames]

Closing out inline comments and replying where it seems to make sense.

I'm getting slightly swamped by the volume of comments. If I miss replying to something you think needs more attention, please re-raise. It's entirely possible I missed something.

llvm/docs/DeveloperPolicy.rst

317–318

I don't get what you're going for here, and I think you might be getting too detailed for a high level guideline. If you have suggestions for specific wording, please make them.

317–318

I played with this, and it seemed to negatively impact readability overall for me. I'm going to leave the order as is.

See also the newly added first clause of the expectation section. I think that might help with the core concern.

320

The expectation section always applies. I don't want to start duplicating it everywhere. I think that would greatly harm readability.

323

Again, long standing practice. I'm explicitly not changing anything, I'm just documenting. I ask you move this point to the llvm-dev thread for broader discussion and possibly a follow up patch if warranted.

338

I decided moving the section was a net negative, but did try to reword a few of the clauses to be more explicit about it applying regardless of whether you were reverting your own patch or someone elses.

[mehdi_amini]

llvm/docs/DeveloperPolicy.rst

321

I suspect the sentence below "In general, if the author themselves would revert the change per these guidelines, we encourage other contributors to do so as a favor to the author" intends to say that if a bot is broken, the author should revert, but so should anyone who notice and identifies the faulty commit.

If you think the author would revert the change, you should revert the change.

Maybe the nuance is that this is not a "should" but a "may" here.

The difference I personally make is one of "urgency": if something is badly broken, I'll revert. If I have concerns and I wish for a revert, I'll express my concerns and ask the author to revert temporarily.

You should not revert a change until you've started a discussion

Was it in reference to an older version of the patch? The text says below: "Reverts should be reasonably timely. A change submitted two hours ago can be reverted without prior discussion".

[hubert.reinterpretcast]

I have just some typo/phrasing fixes to suggest. I think my other concerns have been addressed.

llvm/docs/DeveloperPolicy.rst

318–319

I really do think that at least the comma should be moved. I also suggest changing the ordering to clarify that the "test case" (as opposed to the thread) is the subject of the clause.

323

In conjunction with the expanded room for discretion below, I guess this works.

350

Fix typo.

362

Suggest not to have two periods for etc..

[reames]

minor typo fix

[reames]

Response to hubert.reinterpretcast

llvm/docs/DeveloperPolicy.rst

318–319

I've reread this sentence a couple of times, and don't see your point. Can you quote the exact change you're suggesting?

(I was never great at formal english grammar so I might be misparsing something.)

350

Already done. I noticed this too.

362

This is an ellipsis to indicate the list is not complete. I believe this is correct usage. Some sources say it must be three periods (not two), but I see both in regular usage.

[hubert.reinterpretcast]

llvm/docs/DeveloperPolicy.rst

318–319

I did, but maybe it's not showing for you?

My suggested rewrite is:
If a test case that demonstrates a problem is reported in the commit thread, please revert and investigate offline.

That is, I moved the comma to before the "please". I also moved the "demonstrates a problem" part to right after "test case" instead of being after "commit thread".

362

I've never heard of two period ellipses being considered formally correct. I think etc. is enough to indicate that the list is not complete.

[aprantl]

llvm/docs/DeveloperPolicy.rst

335

Perhaps add: As a courtesy please notify the author of the patch that you reverted it for them, either by commenting on the review or via email.

[probinson]

llvm/docs/DeveloperPolicy.rst

362

Agreed, ellipsis after "etc." is redundant. Also have never seen a two-period ellipsis.

[rupprecht]

I don't have anything more to add now, so resigning now & leaving final thoughts and wording nits to other reviewers.

llvm/docs/DeveloperPolicy.rst

336–337

I don't know if I have a specific suggestion that is better, but I'm not a fan of the wording here, especially with the "serious norm violations" wording.

While I agree with the general policy around timely reverts, starting a discussion before reverting someone else's change, etc., I also think that documenting these as "serious norm violations" unnecessarily escalates these cases when they do happen. If someone has reverted a change that falls into this category, there's a decent chance that it wasn't an intentional norms violation, and the discussion should start out with "Hey, can we talk more about this revert? I don't know if reverting was the right decision here" and not "This is a serious norms violation as documented in [link to these docs], please undo the revert".

[reames]

Rolled up grammar/wording edits.

llvm/docs/DeveloperPolicy.rst

335

I'd already had a sentence on this above. I expanded and made it a top level bullet point, does that address your concern?

[reames]

llvm/docs/DeveloperPolicy.rst

336–337

I want to avoid getting into the discussion of norm violation handling in this review. You raise several good points, and we should eventually document this, but I expect it to be an involved discussion. I really want the basics landed before opening that can of worms.

If anyone can think of wording to further soften this sentence, please throw it out. I can't think of anything without expanding this bullet a lot, and every attempt I've made ran into something I thought needed discussion I'm hoping to avoid on this review.

Maybe I should just strike that bullet for now entirely? The previous bullet reads reasonably on it's own as an answer to the question.

[echristo]

llvm/docs/DeveloperPolicy.rst

330

I think wording wise we can just remove the "... as a favor to the author." here and reword a little below...

332

"we generally consider reverting a normal part of development" or something in this vein perhaps?

336–337

I'd probably strike it. It's definitely part of the referring statement that got us here or you could just link to the developer policy and say something about that?

341

"Use your best judgment. If you're uncertain, please start an email on the commit thread asking for assistance." Perhaps?

343

"We aren't trying to enumerate every case, but rather give a set of guidelines."

345

"Know the context. Sometimes a patch is part of a chain and you may need to take all of that into context when making your revert."

365

s/strongly//

377

Honestly I think this might be better at the beginning of this. A framing reminder for people who come here after a patch has been reverted.

379

I'm not a fan of this sentence. I know where you're trying to get to though and I do like that, but it's a little sticky. I think it can be removed until we can come up with something better though :)

382

Perhaps begin this sentence with "Remember: "?

And maybe put it first?

[reames]

Incorporate Eric's feedback.

[reames]

llvm/docs/DeveloperPolicy.rst

330

I left this one as is. I took all your other edits, but taking this out seems to loose an important point to me.

[aprantl]

I think this is starting to look pretty good!

[lattner]

Are you ready for me to make a pass over it Phillip?

[reames]

In D99305#2659813, @lattner wrote:

Are you ready for me to make a pass over it Phillip?

I think so.

[echristo]

llvm/docs/DeveloperPolicy.rst

330

I followed up offline here with an article that would help illustrate my point, but for me not having the "I'm doing you a favor" is important. It's not a favor, it's just how we work. There's no obligation or reciprocity expected which the text gives the impression of.

[reames]

llvm/docs/DeveloperPolicy.rst

330

You did, my apologies for not acknowledging that.

I read the article you sent, it was definitely interesting, but it didn't change my take on this.

I'm going to defer to Chris on this. If he wants a change, I'll make it. If he's okay with the current wording, I'll leave it as is.

[probinson]

llvm/docs/DeveloperPolicy.rst

330

Maybe "courtesy" rather than "favor" would have less implication of an obligation? Overall we are expected to be courteous to one another.

[reames]

llvm/docs/DeveloperPolicy.rst

330

Great suggestion, that would work for me. Eric?

[lattner]

This looks really great to me Philip, thank you for documenting this for the community!

llvm/docs/DeveloperPolicy.rst

303

nit: I'd expand this out to "Patch reversion policy" or "Policy for reverting patches"

307–309

Wordsmithing, I'd recommend explaining why, so it doesn't sound capritious. Something like:

As such, we tend to make much heavier use of reverts to keep the tree healthy than some other open source projects, and our norms are a bit different.

374–375

A change submitted two years ago probably shouldn't be.

I'd go stronger:

A change submitted two years ago should not be.

[reames]

Address final suggestions.

This has been LGTMed. I'm going to wait until tomorrow for final objections, and commit if non are raised.

llvm/docs/CodeReview.rst

 likely-community-consensus requirements (as apply to all patch approvals) can
 be committed prior to an explicit review. In situations where there is any
 uncertainty, a patch should be reviewed prior to being committed.
 
 Please note that the developer responsible for a patch is also
 responsible for making all necessary review-related changes, including
 those requested during any post-commit review.
 
+.. _post_commit_review:
+
 Can Code Be Reviewed After It Is Committed?
 -------------------------------------------
 
 Post-commit review is encouraged, and can be accomplished using any of the
 tools detailed below. There is a strong expectation that authors respond
 promptly to post-commit feedback and address it. Failure to do so is cause for
-the patch to be reverted.
+the patch to be :ref:`reverted <revert_policy>`.
 
 If a community member expresses a concern about a recent commit, and this
 concern would have been significant enough to warrant a conversation during
 pre-commit review (including around the need for more design discussions),
 they may ask for a revert to the original author who is responsible to revert
 the patch promptly. Developers often disagree, and erring on the side of the
 developer asking for more review prevents any lingering disagreement over
 code in the tree. This does not indicate any fault from the patch author,

llvm/docs/DeveloperPolicy.rst

 
 * If the patch has been reviewed, add a link to its review page, as shown
   `here <https://www.llvm.org/docs/Phabricator.html#committing-a-change>`_.
 
 For minor violations of these recommendations, the community normally favors
 reminding the contributor of this policy over reverting. Minor corrections and
 omissions can be handled by sending a reply to the commits mailing list.
 
+.. _revert_policy:
+
+Patch reversion policy
+----------------------
+
+As a community, we strongly value having the tip of tree in a good state while
+allowing rapid iterative development.  As such, we tend to make much heavier
+use of reverts to keep the tree healthy than some other open source projects,
+and our norms are a bit different.
+
+How should you respond if someone reverted your change?
+
+* Remember, it is normal and healthy to have patches reverted.  Having a patch
+  reverted does not necessarily mean you did anything wrong.
+* We encourage explicitly thanking the person who reverted the patch for doing
+  the task on your behalf.
+* If you need more information to address the problem, please follow up in the
+  original commit thread with the reverting patch author.
+
+When should you revert your own change?
+
+* Any time you learn of a serious problem with a change, you should revert it.
+  We strongly encourage "revert to green" as opposed to "fixing forward".  We
+  encourage reverting first, investigating offline, and then reapplying the
+  fixed patch - possibly after another round of review if warranted.
+* If you break a buildbot in a way which can't be quickly fixed, please revert.
+* If a test case that demonstrates a problem is reported in the commit thread,
+  please revert and investigate offline.
+* If you receive substantial :ref:`post-commit review <post_commit_review>`
+  feedback, please revert and address said feedback before recommitting.
+  (Possibly after another round of review.)
+* If you are asked to revert by another contributor, please revert and discuss
+  the merits of the request offline (unless doing so would further destabilize
+  tip of tree).
+
+When should you revert someone else's change?
+
+* In general, if the author themselves would revert the change per these
+  guidelines, we encourage other contributors to do so as a courtesy to the
+  author.  This is one of the major cases where our norms differ from others;
+  we generally consider reverting a normal part of development.  We don't
+  expect contributors to be always available, and the assurance that a
+  problematic patch will be reverted and we can return to it at our next
+  opportunity enables this.
+
+What are the expectations around a revert?
+
+* Use your best judgment. If you're uncertain, please start an email on
+  the commit thread asking for assistance.  We aren't trying to enumerate
+  every case, but rather give a set of guidelines.
+* You should be sure that reverting the change improves the stability of tip
+  of tree.  Sometimes reverting one change in a series can worsen things
+  instead of improving them.  We expect reasonable judgment to ensure that
+  the proper patch or set of patches is being reverted.
+* The commit message for the reverting commit should explain why patch
+  is being reverted.
+* It is customary to respond to the original commit email mentioning the
+  revert.  This serves as both a notice to the original author that their
+  patch was reverted, and helps others following llvm-commits track context.
+* Ideally, you should have a publicly reproducible test case ready to share.  
+  Where possible, we encourage sharing of test cases in commit threads, or
+  in PRs.  We encourage the reverter to minimize the test case and to prune
+  dependencies where practical.  This even applies when reverting your own
+  patch; documenting the reasons for others who might be following along
+  is critical.
+* It is not considered reasonable to revert without at least the promise to
+  provide a means for the patch author to debug the root issue.  If a situation
+  arises where a public reproducer can not be shared for some reason (e.g.
+  requires hardware patch author doesn't have access to, sharp regression in
+  compile time of internal workload, etc.), the reverter is expected to be
+  proactive about working with the patch author to debug and test candidate
+  patches.
+* Reverts should be reasonably timely.  A change submitted two hours ago
+  can be reverted without prior discussion.  A change submitted two years ago
+  should not be.  Where exactly the transition point is is hard to say, but
+  it's probably in the handful of days in tree territory.  If you are unsure,
+  we encourage you to reply to the commit thread, give the author a bit to
+  respond, and then proceed with the revert if the author doesn't seem to be
+  actively responding.
+* When re-applying a reverted patch, the commit message should be updated to
+  indicate the problem that was addressed and how it was addressed.
+
 Obtaining Commit Access
 -----------------------
 
 We grant commit access to contributors with a track record of submitting high
 quality patches.  If you would like commit access, please send an email to
 `Chris <mailto:[e-mail]>`_ with your GitHub username.  This is true
 for former contributors with SVN access as well as new contributors.