[llvm] ce2a5fa - llvm/utils: guarantee revert_checker's revert ordering

[George Burgess IV via llvm-commits]

Author: George Burgess IV
Date: 2021-07-28T00:51:05Z
New Revision: ce2a5fa72be3fd1d606505f98d3831706c28cfa8

URL: https://github.com/llvm/llvm-project/commit/ce2a5fa72be3fd1d606505f98d3831706c28cfa8
DIFF: https://github.com/llvm/llvm-project/commit/ce2a5fa72be3fd1d606505f98d3831706c28cfa8.diff

LOG: llvm/utils: guarantee revert_checker's revert ordering

At the moment, the revert ordering from this tool is unspecified (though
it happens to be in `git log` order, so newest reverts come first).

>From the standpoint of tooling and users, this seems to be the opposite
of what we want by default: tools and users will generally try to apply
these reverts as cherry-picks. If two reverts in the list are close
enough to each other, if the reverts get applied out of order, we'll get
a merge conflict.

Rather than having `reverse`s for all tools (and mental reverses for
manual users), just guarantee an oldest-first output ordering for this
function.

Differential Revision: https://reviews.llvm.org/D106838

Added: 
    

Modified: 
    llvm/utils/revert_checker.py
    llvm/utils/revert_checker_test.py

Removed: 
    


################################################################################
diff  --git a/llvm/utils/revert_checker.py b/llvm/utils/revert_checker.py
index c61e4a3f9778..deb49412b1ec 100755
--- a/llvm/utils/revert_checker.py
+++ b/llvm/utils/revert_checker.py
@@ -170,7 +170,10 @@ def _find_common_parent_commit(git_dir: str, ref_a: str, ref_b: str) -> str:
 
 
 def find_reverts(git_dir: str, across_ref: str, root: str) -> List[Revert]:
-  """Finds reverts across `across_ref` in `git_dir`, starting from `root`."""
+  """Finds reverts across `across_ref` in `git_dir`, starting from `root`.
+
+  These reverts are returned in order of oldest reverts first.
+  """
   across_sha = _rev_parse(git_dir, across_ref)
   root_sha = _rev_parse(git_dir, root)
 
@@ -217,6 +220,10 @@ def find_reverts(git_dir: str, across_ref: str, root: str) -> List[Revert]:
       logging.error("%s claims to revert %s -- which isn't a commit -- %s", sha,
                     object_type, reverted_sha)
 
+  # Since `all_reverts` contains reverts in log order (e.g., newer comes before
+  # older), we need to reverse this to keep with our guarantee of older =
+  # earlier in the result.
+  all_reverts.reverse()
   return all_reverts
 
 

diff  --git a/llvm/utils/revert_checker_test.py b/llvm/utils/revert_checker_test.py
index a908e6675986..6573c25c1a0b 100755
--- a/llvm/utils/revert_checker_test.py
+++ b/llvm/utils/revert_checker_test.py
@@ -105,12 +105,12 @@ def test_known_reverts_across_arbitrary_llvm_rev(self) -> None:
         across_ref='c47f971694be0159ffddfee8a75ae515eba91439',
         root='9f981e9adf9c8d29bb80306daf08d2770263ade6')
     self.assertEqual(reverts, [
-        revert_checker.Revert(
-            sha='9f981e9adf9c8d29bb80306daf08d2770263ade6',
-            reverted_sha='4060016fce3e6a0b926ee9fc59e440a612d3a2ec'),
         revert_checker.Revert(
             sha='4e0fe038f438ae1679eae9e156e1f248595b2373',
             reverted_sha='65b21282c710afe9c275778820c6e3c1cf46734b'),
+        revert_checker.Revert(
+            sha='9f981e9adf9c8d29bb80306daf08d2770263ade6',
+            reverted_sha='4060016fce3e6a0b926ee9fc59e440a612d3a2ec'),
     ])