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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht requested changes to this revision.
rupprecht added a comment.
This revision now requires changes to proceed.

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.



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


================
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
----------------
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?


================
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:
> 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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99305