Skip to content

Replace patch-id rebase detection with merge-tree comparison #246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jerry-skydio merged 1 commit into from
May 19, 2026
Merged

Replace patch-id rebase detection with merge-tree comparison #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added revup/__init__.pyc
View file
Open in desktop
Binary file not shown.
168 changes: 90 additions & 78 deletions revup/git.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,6 @@

HEAD_COMMIT = GitCommitHash("HEAD")

GIT_DIFF_ARGS = [
"--no-pager",
"diff",
"--full-index",
"--no-color",
"--no-textconv",
"--no-ext-diff",
"-U1",
]

GIT_ENV_NOCONFIG = {
"HOME": "/dev/null",
"GIT_CONFIG_NOSYSTEM": "1",
Expand Down Expand Up @@ -167,7 +157,14 @@ async def get_editor() -> str:
ret = os.environ.get("GIT_EDITOR", os.environ.get("EDITOR", "nano"))
return ret

repo_root, git_dir, actual_version, email, editor, main_exists = await asyncio.gather(
(
repo_root,
git_dir,
actual_version,
email,
editor,
main_exists,
) = await asyncio.gather(
git_ctx.git_stdout("rev-parse", "--show-toplevel"),
git_ctx.git_stdout("rev-parse", "--path-format=absolute", "--git-dir"),
git_ctx.git_stdout("--version"),
Expand Down Expand Up @@ -266,6 +263,7 @@ def clear_cache(self) -> None:
self.fork_point.cache_clear() # pylint: disable=no-member
self.distance_to_fork_point.cache_clear() # pylint: disable=no-member
self.have_identical_trees.cache_clear() # pylint: disable=no-member
self.merge_tree.cache_clear() # pylint: disable=no-member

def get_scratch_dir(self) -> str:
"""
Expand Down Expand Up @@ -571,40 +569,86 @@ async def commit_tree(self, commit_info: CommitHeader) -> GitCommitHash:
"GIT_COMMITTER_DATE": commit_info.committer_date,
}
git_env = {k: v for k, v in git_env.items() if v != ""}
commit_tree_args = ["commit-tree", commit_info.tree, "-m", commit_info.commit_msg]
commit_tree_args = [
"commit-tree",
commit_info.tree,
"-m",
commit_info.commit_msg,
]
for p in commit_info.parents:
commit_tree_args.extend(["-p", p])
ret = await self.git_stdout(*commit_tree_args, env=git_env)
return GitCommitHash(ret)

async def get_patch_id(
@lru_cache(maxsize=None)
async def merge_tree(
self,
commit: GitCommitHash,
) -> str:
merge_base: GitCommitHash,
tree1: GitCommitHash,
tree2: GitCommitHash,
strategy: Optional[str] = None,
) -> Tuple[GitTreeHash, Optional[List[GitConflict]]]:
"""
Return a patch-id that uniquely identifies this commit's diff (but not its other metadata).
Run merge-tree and return the resulting tree hash and any conflicts.
Conflicts is None if merge succeeded (ret 0), or a list (possibly empty) on conflict.
"""
patch_source = (
[
self.git_path,
]
+ GIT_DIFF_ARGS
+ [
commit + "~",
commit,
]
)
ret = (
await self.sh.piped_sh(
patch_source,
[self.git_path, "patch-id", "--verbatim"],
env1=GIT_ENV_NOCONFIG,
env2=GIT_ENV_NOCONFIG,
args = [
"merge-tree",
"--write-tree",
"--messages",
"-z",
"--merge-base",
merge_base,
]
if strategy is not None:
args.extend([f"-X{strategy}"])
args.extend([tree1, tree2])

ret, stdout = await self.git(*args, no_config=True, raiseonerror=False)
if ret > 1:
raise RuntimeError(f"Unexpected / unknown error in git {args} {ret}")

sections = stdout.split("\0\0")
subsections = [s.split("\0") for s in sections]
tree_hash = GitTreeHash(subsections[0][0])

if ret == 0:
return tree_hash, None

conflicts: List[GitConflict] = []
informational = subsections[1]
i = 0
while i < len(informational) - 1:
num_paths = int(informational[i])
conflicts.append(
GitConflict(
informational[i + 1 + num_paths],
informational[i + 2 + num_paths].strip(),
informational[i + 1 : i + 1 + num_paths],
)
)
)[1].split()
# If the diff is empty, patch id will return nothing. We just use that as the patch-id since
# it fulfills the requirement of matching other empty diffs.
return ret[0] if ret else ""
i += num_paths + 3

return tree_hash, conflicts

async def commits_are_equivalent(
self,
local_commit: CommitHeader,
remote_commit: CommitHeader,
) -> bool:
"""
Check if local_commit produces the same diff as remote_commit by applying
local_commit onto remote_commit's parent via merge-tree and comparing trees.
"""
if not local_commit.parents or not remote_commit.parents:
return False

result_tree, conflicts = await self.merge_tree(
local_commit.parents[0],
local_commit.commit_id,
remote_commit.parents[0],
)
return conflicts is None and result_tree == remote_commit.tree

async def get_diff_summary(
self,
Expand All @@ -631,50 +675,15 @@ async def merge_tree_commit(
Perform a combined git merge-tree and commit-tree, returning a git commit hash. Raises
GitConflictException if there are conflicts while merging the trees.
"""
args = [
"merge-tree",
"--write-tree",
"--messages",
"-z",
"--merge-base",
merge_base,
]
if strategy is not None:
args.extend([f"-X{strategy}"])
args.extend([tree1, tree2])

ret, stdout = await self.git(*args, no_config=True, raiseonerror=False)

# See man page for git merge-tree for a complete breakdown of the output format
sections = stdout.split("\0\0")
subsections = [s.split("\0") for s in sections]
tree_hash = GitTreeHash(subsections[0][0])
tree_hash, conflicts = await self.merge_tree(merge_base, tree1, tree2, strategy)

if ret == 0 or ignore_conflicts:
if conflicts is None or ignore_conflicts:
new_commit_info.tree = tree_hash
return await self.commit_tree(new_commit_info)
elif ret == 1:
# conflicted_files would be subsections[0][1:] but we don't currently use this info
# (it corresponds to higher order files in cache).
informational = subsections[1]
# Information section is formatted as a list of conflicts:
# <num paths> <paths> <type> <message>
# We parse num paths first and loop through to extract the other fields.
e = GitConflictException(tree_hash)
i = 0
while i < len(informational) - 1:
num_paths = int(informational[i])
e.conflicts.append(
GitConflict(
informational[i + 1 + num_paths],
informational[i + 2 + num_paths].strip(),
informational[i + 1 : i + 1 + num_paths],
)
)
i += num_paths + 3
raise e
else:
raise RuntimeError(f"Unexpected / unknown error in git {args} {ret}")

e = GitConflictException(tree_hash)
e.conflicts = conflicts
raise e

async def dump_conflict(self, e: GitConflictException) -> None:
"""
Expand Down Expand Up @@ -728,7 +737,10 @@ async def synthetic_amend(
"""
commit_info = copy.deepcopy(commit_to_amend)
return await self.merge_tree_commit(
new_commit.commit_id, commit_info.commit_id, commit_info, new_commit.parents[0]
new_commit.commit_id,
commit_info.commit_id,
commit_info,
new_commit.parents[0],
)

async def synthetic_cherry_pick_from_commit(
Expand Down
40 changes: 10 additions & 30 deletions revup/topic_stack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,6 @@ class Review:
# List of commits the remote has for this review
remote_commits: List[git.CommitHeader] = field(default_factory=list)

# Corresponding output of git patch-id for each remote commit
remote_patch_ids: List[str] = field(default_factory=list)

# Existing PR details for this review. None if no PR currently exists
pr_info: Optional[PrInfo] = None

Expand Down Expand Up @@ -194,9 +191,6 @@ class Topic:
# Original commits included in this topic
original_commits: List[git.CommitHeader] = field(default_factory=list)

# Corresponding output of git patch-id for each commit
patch_ids: List[str] = field(default_factory=list)

# Tags for this topic (union of all tags for commits in the topic)
tags: Dict[str, Set[str]] = field(default_factory=lambda: defaultdict(set))

Expand Down Expand Up @@ -711,7 +705,7 @@ async def populate_relative_reviews(self, uploader: str, branch_format: str) ->

async def mark_rebases(self, skip_rebase: bool) -> None:
"""
Scan all topics and compare patch-ids to remote patch-ids. Appropriately mark any
Scan all topics and compare local commits to remote commits. Appropriately mark any
changes that are already merged, or where push can be skipped due to being rebases or
being identical.
"""
Expand Down Expand Up @@ -781,18 +775,12 @@ async def mark_rebases(self, skip_rebase: bool) -> None:
self.use_reordering_workaround = True

if review.pr_info is None:
# This is a new pr, no need to check patch ids
# This is a new pr, no need to check for rebase
review.is_pure_rebase = False
else:
assert (
review.pr_info.baseRefOid is not None and review.pr_info.headRefOid is not None
)
if not topic.patch_ids:
# Lazily load patch ids for the topic.
topic.patch_ids = await asyncio.gather(
*(self.git_ctx.get_patch_id(c.commit_id) for c in topic.original_commits)
)

review.remote_commits = git.parse_rev_list(
await self.git_ctx.rev_list(
review.pr_info.headRefOid,
Expand All @@ -802,14 +790,13 @@ async def mark_rebases(self, skip_rebase: bool) -> None:
)
)

review.remote_patch_ids = await asyncio.gather(
*(self.git_ctx.get_patch_id(c.commit_id) for c in review.remote_commits)
)

# This review is a rebase iff all commit diffs match
# Check rebase by applying each local commit onto the remote parent
# and comparing the resulting tree to the remote tree.
is_rebase = len(review.remote_commits) == len(topic.original_commits) and all(
local_id == remote_id
for local_id, remote_id in zip(topic.patch_ids, review.remote_patch_ids)
await asyncio.gather(*(
self.git_ctx.commits_are_equivalent(local, remote)
for local, remote in zip(topic.original_commits, review.remote_commits)
))
)
# This review is a "complete rebase" iff all commit diffs and metadata match
review.is_pure_rebase = is_rebase and all(
Expand Down Expand Up @@ -957,8 +944,8 @@ async def create_commits(self, trim_tags: bool) -> None:
next_parent = commit.commit_id
else:
# TODO: Potential optimization here: if remote_base_oid and base_ref are
# the same, we can use trees to pick the first N commits where patch-id
# is equal to remote patch-id
# the same, we can use trees to pick the first N commits where the
# local commit is equivalent to the remote commit
# TODO: We can parallelize independent chains of commit creation
try:
next_parent = await self.git_ctx.synthetic_cherry_pick_from_commit(
Expand Down Expand Up @@ -990,18 +977,11 @@ async def create_commits(self, trim_tags: bool) -> None:
# 1. The relative PR was closed without merging after being uploaded. We don't look
# at closed PRs, so we don't know whether that branch has changed. However actually
# building both branches could reveal that the resulting commit is the same.
# 2. A commit might not match the remote based on patch id, but applying the patch
# would result in the same commit as the remote. This could happen if a part of the
# patch is dependent on a local commit, but is a no-op when applied to the base.
review.push_status = PushStatus.NOCHANGE
if review.status == PrStatus.NEW:
# A PR marked as new but with a pr_info must have previously been merged, but
# marked as new when checking rebases. Return it to merged.
review.status = PrStatus.MERGED
# TODO: If 2 above were true *and* a rebase occurred this wouldn't catch it and
# an erroneous push / pr creation would happen. We'd have to compute patch ids again
# to catch this which is a bit inefficient for all reviews. Lets see how common this
# is / try to think of alternative solutions.

async def fetch_git_refs(self) -> None:
"""
Expand Down
42 changes: 42 additions & 0 deletions tests/test_upload.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,3 +1178,45 @@ async def test_commit_count_change_is_not_rebase(self):

assert not review.is_pure_rebase
assert review.push_status == PushStatus.PUSHED

@async_test
async def test_noop_on_base_detected_as_rebase(self):
"""A commit whose patch is a no-op when applied to the base should be detected as rebase.

This happens when a commit's diff depends on a preceding local commit but produces
the same tree as the base when cherry-picked independently. Patch-id would not match
the remote, but merge-tree correctly identifies the result as equivalent.
"""
async with GitTestEnvironment() as env:
# Root has a.txt so cherry-pick onto base won't conflict
await env.commit("root", {"root.txt": "r", "a.txt": "original"})
await env.git_ctx.git("branch", "origin/main", "HEAD")

# Local commit A modifies a.txt
await env.commit("setup\n\nTopic: setup", {"a.txt": "modified"})
# Local commit B reverts a.txt back — a real diff locally, but a no-op on base
await env.commit("revert\n\nTopic: alpha", {"a.txt": "original"})

# First upload: alpha's cherry-pick onto base (which has a.txt="original") produces
# a commit whose tree matches the base tree.
first = await run_upload_pipeline(env)
first_review = first.topics["alpha"].reviews["origin/main"]
remote_head = first_review.new_commits[-1]
remote_base = first_review.base_ref

# Second run with the same commits — should detect as rebase
topics = await run_upload_pipeline(env)
review = topics.topics["alpha"].reviews["origin/main"]
review.pr_info = PrInfo(
baseRef="main",
headRef=review.remote_head,
baseRefOid=remote_base,
headRefOid=remote_head,
body="",
title="",
state="OPEN",
)

await topics.mark_rebases(skip_rebase=True)

assert review.is_pure_rebase
Loading