[PATCH] D106838: revert_checker: guarantee a commit ordering

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv created this revision.
george.burgess.iv added reviewers: tstellar, probinson.
george.burgess.iv requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

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 most tools (and mental reverses for manual users), just guarantee an oldest-first output ordering for this function.


https://reviews.llvm.org/D106838

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


Index: llvm/utils/revert_checker_test.py
===================================================================
--- llvm/utils/revert_checker_test.py
+++ llvm/utils/revert_checker_test.py
@@ -105,12 +105,12 @@
         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'),
     ])
 
 
Index: llvm/utils/revert_checker.py
===================================================================
--- llvm/utils/revert_checker.py
+++ llvm/utils/revert_checker.py
@@ -170,7 +170,10 @@
 
 
 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 @@
       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
 
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D106838.361855.patch
Type: text/x-patch
Size: 1844 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210727/96393a41/attachment.bin>