Page MenuHomePhabricator

[docs] Document our norms around reverts
ClosedPublic

Authored by reames on Wed, Mar 24, 3:48 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Thu, Mar 25, 2:44 PM
llvm/docs/DeveloperPolicy.rst
352
reames added inline comments.Thu, Mar 25, 2:56 PM
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.

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.

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).

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 added inline comments.
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?!" :)

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.

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 updated this revision to Diff 333579.Fri, Mar 26, 10:03 AM

Attempt to address comments from various reviewers.

reames marked 14 inline comments as done.Fri, Mar 26, 10:11 AM

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 added inline comments.Fri, Mar 26, 10:19 AM
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 resigned from this revision.Fri, Mar 26, 10:59 AM

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 updated this revision to Diff 333598.Fri, Mar 26, 11:12 AM
reames marked 3 inline comments as done.

minor typo fix

reames marked 2 inline comments as done.Fri, Mar 26, 11:17 AM

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.

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 added inline comments.
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 added inline comments.Mon, Mar 29, 6:56 AM
llvm/docs/DeveloperPolicy.rst
362

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

rupprecht resigned from this revision.Mon, Mar 29, 7:34 AM

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 updated this revision to Diff 333891.Mon, Mar 29, 8:55 AM
reames marked 2 inline comments as done.

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 added inline comments.Mon, Mar 29, 9:02 AM
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 added inline comments.Mon, Mar 29, 9:25 AM
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 updated this revision to Diff 334003.Mon, Mar 29, 3:43 PM

Incorporate Eric's feedback.

reames added inline comments.Mon, Mar 29, 3:44 PM
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.

I think this is starting to look pretty good!

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

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

I think so.

echristo added inline comments.Fri, Apr 2, 11:54 AM
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 added inline comments.Fri, Apr 2, 12:55 PM
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 added inline comments.Mon, Apr 5, 7:37 AM
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 added inline comments.Mon, Apr 5, 7:47 AM
llvm/docs/DeveloperPolicy.rst
330

Great suggestion, that would work for me. Eric?

lattner accepted this revision.Tue, Apr 6, 8:56 AM

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.

This revision is now accepted and ready to land.Tue, Apr 6, 8:56 AM
reames updated this revision to Diff 335588.Tue, Apr 6, 10:46 AM

Address final suggestions.

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

This revision was automatically updated to reflect the committed changes.