Skip to content

Commit 9d09f33

Browse files
committed
[llvm][utils] skip revert-checking reverts across branches
e2ba1b6 references that it reverts a commit that's not a parent of e2ba1b6. Functionally, this can (and demonstrably does) work(*), but from the standpoint of the revert checker, it's nonsense. Print a `logging.error` when it's detected. Tested by running the revert checker against a commit range that includes the aforementioned commit; the logging.error was fired appropriately. (*) - the specifics here are: - the _SHA_ that was referenced was on a non-main branch, but - the commit from the non-main branch was merged into the non-main branch from main - ...so the _functional_ commit being reverted was originally landed on main, but the _SHA_ referenced from main was from a branch that was cut before the reverted-commit was landed on main
1 parent e0f8898 commit 9d09f33

File tree

1 file changed

+29
-9
lines changed

1 file changed

+29
-9
lines changed

llvm/utils/revert_checker.py

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,12 @@ def _rev_parse(git_dir: str, ref: str) -> str:
211211

212212

213213
def _find_common_parent_commit(git_dir: str, ref_a: str, ref_b: str) -> str:
214-
"""Finds the closest common parent commit between `ref_a` and `ref_b`."""
214+
"""Finds the closest common parent commit between `ref_a` and `ref_b`.
215+
216+
Returns:
217+
A SHA. Note that `ref_a` will be returned if `ref_a` is a parent of
218+
`ref_b`, and vice-versa.
219+
"""
215220
return subprocess.check_output(
216221
["git", "-C", git_dir, "merge-base", ref_a, ref_b],
217222
encoding="utf-8",
@@ -341,16 +346,31 @@ def find_reverts(
341346
)
342347
continue
343348

344-
if object_type == "commit":
345-
all_reverts.append(Revert(sha, reverted_sha))
349+
if object_type != "commit":
350+
logging.error(
351+
"%s claims to revert the %s %s, which isn't a commit",
352+
sha,
353+
object_type,
354+
reverted_sha,
355+
)
356+
continue
357+
358+
# Rarely, reverts will cite SHAs on other branches (e.g., revert
359+
# commit says it reverts a commit with SHA ${X}, but ${X} is not a
360+
# parent of the revert). This can happen if e.g., the revert has
361+
# been mirrored to another branch. Treat them the same as
362+
# reverts of non-commits.
363+
if _find_common_parent_commit(git_dir, sha, reverted_sha) != reverted_sha:
364+
logging.error(
365+
"%s claims to revert %s, which is a commit that is not "
366+
"a parent of the revert",
367+
sha,
368+
reverted_sha,
369+
)
346370
continue
347371

348-
logging.error(
349-
"%s claims to revert %s -- which isn't a commit -- %s",
350-
sha,
351-
object_type,
352-
reverted_sha,
353-
)
372+
all_reverts.append(Revert(sha, reverted_sha))
373+
354374

355375
# Since `all_reverts` contains reverts in log order (e.g., newer comes before
356376
# older), we need to reverse this to keep with our guarantee of older =

0 commit comments

Comments
 (0)