revert_checker: guarantee a commit ordering

[george.burgess.iv]

Summary

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

[tstellar]

This makes sense to me.

llvm/utils/revert_checker.py

def _find_common_parent_commit(git_dir: str, ref_a: str, ref_b: str) -> str:
   """Finds the closest common parent commit between `ref_a` and `ref_b`."""
   return subprocess.check_output(
       ['git', '-C', git_dir, 'merge-base', ref_a, ref_b],
       encoding='utf-8',
   ).strip()
 
 
 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)
 
   common_ancestor = _find_common_parent_commit(git_dir, across_sha, root_sha)
   if common_ancestor != across_sha:
     raise ValueError(f"{across_sha} isn't an ancestor of {root_sha} "
                      '(common ancestor: {common_ancestor})')
 
for reverted_sha in resolved_reverts:
 
       if object_type == 'commit':
         all_reverts.append(Revert(sha, reverted_sha))
         continue
 
       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
 
 
 def _main() -> None:
   parser = argparse.ArgumentParser(
       description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter)
   parser.add_argument(
       'base_ref', help='Git ref or sha to check for reverts around.')

llvm/utils/revert_checker_test.py

class Test(unittest.TestCase):
 
   def test_known_reverts_across_arbitrary_llvm_rev(self) -> None:
     reverts = revert_checker.find_reverts(
         git_dir=get_llvm_project_path(),
         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'),
     ])
 
 
 if __name__ == '__main__':
   unittest.main()