[PATCH] D99305: [docs] Document our norms around reverts

Thu Mar 25 13:38:11 PDT 2021

reames added a comment.

Respond to rupprecht (see upcoming patch refresh as well)

================
Comment at: llvm/docs/DeveloperPolicy.rst:321
+
+When should you revert someone else's change?
+
----------------
rupprecht wrote:
> 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.
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.)

================
Comment at: llvm/docs/DeveloperPolicy.rst:330
+  opportunity enables this.
+* The other case for a third-party revert is for serious norm violations.  As a
+  general rule, you should never be reverting a patch for this unless you've
----------------
rupprecht wrote:
> 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?
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.

================
Comment at: llvm/docs/DeveloperPolicy.rst:342-344
+* You should have a publicly reproducible test case ready to share.  It is
+  not considered reasonable to revert without at least the promise to produce
+  a public test case in the near term.  We encourage sharing of test cases in
----------------
rupprecht wrote:
> rupprecht wrote:
> > 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.
> 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.
Good catch.  I tried to reword to address this, let me know if it needs further work.

================
Comment at: llvm/docs/DeveloperPolicy.rst:342-346
+* You should have a publicly reproducible test case ready to share.  It is
+  not considered reasonable to revert without at least the promise to produce
+  a public test case in the near term.  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.
----------------
reames wrote:
> rupprecht wrote:
> > rupprecht wrote:
> > > 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.
> > 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.
> Good catch.  I tried to reword to address this, let me know if it needs further work.
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.  

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99305/new/

https://reviews.llvm.org/D99305