diff --git a/README.md b/README.md index 138c129..fa0cb17 100644 --- a/README.md +++ b/README.md @@ -98,6 +98,8 @@ It's useful only with Golang repositories, which require a `vendor` folder with *Note: Internally this is implemented using lifecycle hook script and is equivalent to passing `--post-rebase-hook _BUILTIN_/update_go_modules.sh` parameter.* +For repositories without a `vendor/` tree, use `--post-rebase-hook _BUILTIN_/update_go_modules_no_vendor.sh` instead (tidy/sync only, no vendoring). + ### Slack Webhook If you want to be notified in Slack about the status of recent rebases, you can set ``--slack-webhook` option. The value here is the path to a local file with the webhook url. diff --git a/rebasebot/bot.py b/rebasebot/bot.py index 9254bfc..6efe5d2 100755 --- a/rebasebot/bot.py +++ b/rebasebot/bot.py @@ -61,6 +61,10 @@ class PullRequestUpdateException(Exception): _MERGE_COMMIT_PARENT_COUNT = 2 _LOST_LINE_LOG_LIMIT = 10 _GO_MODULES_CARRY_COMMIT_MESSAGE = "UPSTREAM: : Updating and vendoring go modules after an upstream rebase" +# Raise rename similarity on cherry-picks so weak vendor/header matches are less +# likely; keep-versus-delete still uses the picked commit's real path list. +_CHERRY_PICK_FIND_RENAMES = "-Xfind-renames=70" +_CHERRY_PICK_STRATEGY_OPTIONS = ("-Xtheirs", _CHERRY_PICK_FIND_RENAMES) @dataclass(frozen=True) @@ -364,8 +368,9 @@ def _detect_conflicting_files(gitwd: git.Repo, sha: str) -> set: """ Probe a cherry-pick without -Xtheirs to detect which files conflict. - Attempts the cherry-pick with --no-commit (no merge strategy), records - any unmerged files, then resets to the original state. + Attempts the cherry-pick with --no-commit (no "theirs" strategy) using the + same find-renames threshold as the real apply, records any unmerged files, + then resets to the original state. Returns a set of filenames that had merge conflicts, or an empty set if the cherry-pick would apply cleanly. @@ -374,7 +379,7 @@ def _detect_conflicting_files(gitwd: git.Repo, sha: str) -> set: conflicted = set() try: - gitwd.git.cherry_pick(sha, "--no-commit") + gitwd.git.cherry_pick(sha, "--no-commit", _CHERRY_PICK_FIND_RENAMES) except git.GitCommandError: # Conflicts exist — record which files are unmerged try: @@ -396,6 +401,20 @@ def _detect_conflicting_files(gitwd: git.Repo, sha: str) -> set: return conflicted +def _unescape_git_path(path: str) -> str: + """Decode a git C-quoted path from --name-only / status --porcelain output.""" + if path.startswith('"') and path.endswith('"'): + path = path[1:-1] + path = path.encode("ascii").decode("unicode_escape").encode("latin1").decode(git.compat.defenc) + return path + + +def _picked_commit_paths(gitwd: git.Repo, sha: str) -> set[str]: + """Return paths touched by sha with rename detection disabled.""" + output = gitwd.git.diff_tree("--no-renames", "--no-commit-id", "--name-only", "-r", sha) + return {_unescape_git_path(line) for line in output.splitlines() if line} + + def _check_upstream_content_loss(gitwd: git.Repo, source_branch: str, only_files: set | None = None) -> list: """ After a cherry-pick with -Xtheirs, check whether any upstream content @@ -455,11 +474,11 @@ def _safe_cherry_pick( if conflict_policy != "auto": conflicted_files = _detect_conflicting_files(gitwd, sha) - # Phase 2: actual cherry-pick with -Xtheirs + # Phase 2: actual cherry-pick with -Xtheirs and find-renames threshold try: - gitwd.git.cherry_pick(f"{sha}", "-Xtheirs") + gitwd.git.cherry_pick(f"{sha}", *_CHERRY_PICK_STRATEGY_OPTIONS) except git.GitCommandError as ex: - if not _resolve_rebase_conflicts(gitwd): + if not _resolve_rebase_conflicts(gitwd, sha): raise RepoException(f"Git rebase failed: {ex}") from ex created_commit = gitwd.head.commit.hexsha != start_head @@ -654,7 +673,7 @@ def _prepare_rebase_branch(gitwd: git.Repo, source: GitHubBranch, dest: GitHubBr gitwd.git.checkout("-b", "rebase", commit) -def _resolve_conflict(gitwd: git.Repo) -> bool: +def _resolve_conflict(gitwd: git.Repo, sha: str) -> bool: status = gitwd.git.status(porcelain=True) if not status: @@ -663,7 +682,8 @@ def _resolve_conflict(gitwd: git.Repo) -> bool: return True # Conflict prefixes in porcelain mode that we can fix. - # In all next cases we delete the conflicting files. + # Delete-shaped conflicts: remove only if the path is in the picked commit's + # real (no-rename) path list; otherwise keep HEAD's version. # UD - Modified/Deleted # DU - Deleted/Modified # AU - Renamed/Deleted @@ -674,25 +694,43 @@ def _resolve_conflict(gitwd: git.Repo) -> bool: # Non-conflict status prefixes that we should ignore allowed_status_prefixes = ["M ", "D ", "A ", "R ", "C "] + picked_paths = _picked_commit_paths(gitwd, sha) unresolvable = False files_to_delete = [] + files_to_keep = [] for line in status.splitlines(): logging.info("Resolving conflict: %s", line) file_status = line[:3] if file_status in allowed_status_prefixes: - # There is a conflict we can't resolve + # Already staged non-conflict change — leave alone continue if file_status not in allowed_conflict_prefixes: # There is a conflict we can't resolve logging.info("Unresolvable conflict: %s", line) unresolvable = True - filename = line[3:].rstrip("\n") - # Special characters are escaped - if filename[0] == filename[-1] == '"': - filename = filename[1:-1] - filename = filename.encode("ascii").decode("unicode_escape").encode("latin1").decode(git.compat.defenc) - files_to_delete.append(filename) - logging.info("Deleting conflicting file: %s", filename) + continue + filename = _unescape_git_path(line[3:].rstrip("\n")) + if filename in picked_paths: + files_to_delete.append(filename) + else: + files_to_keep.append(filename) + + if files_to_keep: + logging.info( + "Keeping paths not in picked commit %s (false rename/delete): %s", + sha, + ", ".join(files_to_keep), + ) + if files_to_delete: + logging.info( + "Deleting paths present in picked commit %s: %s", + sha, + ", ".join(files_to_delete), + ) + + for keep_file in files_to_keep: + gitwd.git.checkout("HEAD", "--", keep_file) + gitwd.git.add(keep_file) for ud_file in files_to_delete: gitwd.git.rm(ud_file) @@ -703,21 +741,29 @@ def _resolve_conflict(gitwd: git.Repo) -> bool: logging.error("Unresolvable conflict. Aborting rebase.") return False + # If resolution left the index identical to HEAD, skip rather than + # creating an empty commit (common when vendor deletes are already applied + # and only false rename/delete conflicts remained). + if not gitwd.git.diff("HEAD") and not gitwd.git.diff("--cached"): + logging.info("Conflict resolution left no changes versus HEAD; skipping pick %s", sha) + gitwd.git.cherry_pick("--skip") + return True + gitwd.git.commit("--no-edit") return True -def _resolve_rebase_conflicts(gitwd: git.Repo) -> bool: +def _resolve_rebase_conflicts(gitwd: git.Repo, sha: str) -> bool: try: - if not _resolve_conflict(gitwd): + if not _resolve_conflict(gitwd, sha): return False logging.info("Conflict has been resolved. Continue rebase.") return True except git.GitCommandError: - return _resolve_rebase_conflicts(gitwd) + return _resolve_rebase_conflicts(gitwd, sha) def _cherrypick_art_pull_request( diff --git a/rebasebot/builtin-hooks/update_go_modules_no_vendor.sh b/rebasebot/builtin-hooks/update_go_modules_no_vendor.sh new file mode 100755 index 0000000..db9615a --- /dev/null +++ b/rebasebot/builtin-hooks/update_go_modules_no_vendor.sh @@ -0,0 +1,96 @@ +#!/bin/bash + +set -e # Exit immediately if a command exits with a non-zero status +set -o pipefail # Return the exit status of the last command in the pipe that failed + +stage_and_commit(){ + # If committer email and name is passed as environment variable then use it. + if [[ -z "$REBASEBOT_GIT_USERNAME" || -z "$REBASEBOT_GIT_EMAIL" ]]; then + author_flag=() + else + author_flag=(--author="$REBASEBOT_GIT_USERNAME <$REBASEBOT_GIT_EMAIL>") + fi + + if [[ -n $(git status --porcelain) ]]; then + git add -A + git commit "${author_flag[@]}" -q -m "UPSTREAM: : Updating go modules after an upstream rebase" + fi +} + +reset_go_mod_files() { + while IFS= read -r -d '' go_mod_file; do + local module_base_path + module_base_path=$(dirname "$go_mod_file") + + # Reset go.mod and go.sum to make sure they are the same as in the source + for filename in "go.mod" "go.sum"; do + local full_path="$module_base_path/$filename" + if [[ ! -f "$full_path" ]]; then + continue + fi + if ! git checkout "source/$REBASEBOT_SOURCE" -- "$full_path"; then + echo "go module at $module_base_path is downstream only, skip its resetting" + break + fi + done + done < <(find . -name 'go.mod' -print0) +} + +process_go_workspace_updates() { + echo "Performing go workspace modules update" + + for filename in "go.work" "go.work.sum"; do + if [[ ! -f "$filename" ]]; then + continue + fi + if ! git checkout "source/$REBASEBOT_SOURCE" -- "$filename"; then + echo "go.work is downstream only, which is not supported" >&2 + exit 1 + fi + done + + reset_go_mod_files + + echo "Running go work sync" + if ! go work sync; then + echo "Unable to run 'go work sync'" >&2 + exit 1 + fi + + stage_and_commit +} + +process_go_mod_updates() { + echo "Performing go modules update" + + reset_go_mod_files + + while IFS= read -r -d '' go_mod_file; do + local module_base_path + module_base_path=$(dirname "$go_mod_file") + + pushd "$module_base_path" > /dev/null || { echo "Failed to cd to $module_base_path" >&2; exit 1; } + + echo "Running go mod tidy for $module_base_path" + if ! go mod tidy; then + echo "Unable to run 'go mod tidy' in $module_base_path" >&2 + exit 1 + fi + + popd > /dev/null + done < <(find . -name 'go.mod' -print0) + + stage_and_commit +} + +# Check if the source branch environment variable is set +if [[ -z "$REBASEBOT_SOURCE" ]]; then + echo "The environment variable REBASEBOT_SOURCE is not set." >&2 + exit 1 +fi + +if [[ -f "go.work" ]]; then + process_go_workspace_updates +else + process_go_mod_updates +fi diff --git a/tests/test_bot.py b/tests/test_bot.py index daefc29..de7c937 100644 --- a/tests/test_bot.py +++ b/tests/test_bot.py @@ -140,6 +140,90 @@ def test_update_and_commit_empty(self, tmp_go_app_repo, monkeypatch): assert commits[0].message == "tidy and vendor go stuff\n" +class TestGoModNoVendor: + _COMMIT_MESSAGE = "UPSTREAM: : Updating go modules after an upstream rebase\n" + + def _args_stub(_, repo_dir, source) -> MagicMock: + args = MagicMock() + args.source = source + args.dest = GitHubBranch(repo_dir, "example", "foo", "dest") + args.rebase = GitHubBranch(repo_dir, "example", "foo", "rebase") + args.working_dir = repo_dir + args.git_username = "unittest" + args.git_email = "unit@test.org" + return args + + def test_update_and_commit_without_vendor(self, tmp_go_app_repo, monkeypatch): + repo_dir, repo = tmp_go_app_repo + + monkeypatch.chdir(repo_dir) + os.system("go mod init example.com/foo") + repo.git.add(all=True) + repo.git.commit("-m", "Init go module") + + source = GitHubBranch(repo_dir, "example", "foo", repo.active_branch.name) + repo.create_remote("source", source.url) + repo.remotes.source.fetch(source.branch) + + lifecycle_hooks._setup_environment_variables(self._args_stub(repo_dir, source)) + script = lifecycle_hooks.LifecycleHookScript("_BUILTIN_/update_go_modules_no_vendor.sh") + result = script() + + assert result.return_code == 0 + commits = list(repo.iter_commits()) + assert len(commits) == 3 + assert commits[0].message == self._COMMIT_MESSAGE + assert not os.path.isdir(os.path.join(repo_dir, "vendor")) + + def test_update_and_commit_go_workspace_without_vendor(self, tmp_go_app_repo, monkeypatch): + repo_dir, repo = tmp_go_app_repo + + monkeypatch.chdir(repo_dir) + os.system("go mod init example.com/foo") + os.system("go work init .") + repo.git.add(all=True) + repo.git.commit("-m", "Init go workspace") + + source = GitHubBranch(repo_dir, "example", "foo", repo.active_branch.name) + repo.create_remote("source", source.url) + repo.remotes.source.fetch(source.branch) + + lifecycle_hooks._setup_environment_variables(self._args_stub(repo_dir, source)) + script = lifecycle_hooks.LifecycleHookScript("_BUILTIN_/update_go_modules_no_vendor.sh") + result = script() + + assert result.return_code == 0 + assert not os.path.isdir(os.path.join(repo_dir, "vendor")) + commits = list(repo.iter_commits()) + # go work sync may or may not dirty the tree; never vendor. + if len(commits) == 3: + assert commits[0].message == self._COMMIT_MESSAGE + else: + assert len(commits) == 2 + assert commits[0].message == "Init go workspace\n" + + def test_update_fails_on_broken_go_mod(self, tmp_go_app_repo, monkeypatch): + repo_dir, repo = tmp_go_app_repo + + monkeypatch.chdir(repo_dir) + os.system("go mod init example.com/foo") + with open(os.path.join(repo_dir, "go.mod"), "w") as f: + f.write("this is not a valid go.mod\n") + repo.git.add(all=True) + repo.git.commit("-m", "Init broken go module") + + source = GitHubBranch(repo_dir, "example", "foo", repo.active_branch.name) + repo.create_remote("source", source.url) + repo.remotes.source.fetch(source.branch) + + lifecycle_hooks._setup_environment_variables(self._args_stub(repo_dir, source)) + script = lifecycle_hooks.LifecycleHookScript("_BUILTIN_/update_go_modules_no_vendor.sh") + result = script() + + assert result.return_code != 0 + assert not os.path.isdir(os.path.join(repo_dir, "vendor")) + + class TestCommitMessageTags: @pytest.mark.parametrize( "pr_is_merged,commit_message,tag_policy,expected", diff --git a/tests/test_conflict_policy.py b/tests/test_conflict_policy.py index 217b665..fefddb9 100644 --- a/tests/test_conflict_policy.py +++ b/tests/test_conflict_policy.py @@ -17,6 +17,7 @@ from __future__ import annotations import logging +import os from unittest.mock import MagicMock, patch from rebasebot import cli @@ -421,3 +422,166 @@ def test_auto_policy_returns_empty_content_loss(self, init_test_repositories, fa assert result.created_commit is True assert result.content_loss == [] + + +# Shared blob content used to provoke false rename/delete conflicts: a large +# downstream delete set plus a new upstream file with near-identical content. +_VENDOR_BLOB = 'package labels\n\nconst LabelKey = "app"\n' * 3 +_VENDOR_FILE_COUNT = 20 +_E2E_LABELS_FILE = "e2e_labels.go" +_E2E_LABELS_CONTENT = _VENDOR_BLOB + "// upstream e2e labels\n" + + +def _vendor_filename(index: int) -> str: + return f"vendor_{index:02d}.go" + + +def _vendor_content(index: int) -> str: + return _VENDOR_BLOB + f"// vendor file {index}\n" + + +def _prepare_working_repo(source, rebase, dest, fake_github_provider, tmpdir): + gitwd = _init_working_dir( + source=source, + dest=dest, + rebase=rebase, + github_app_provider=fake_github_provider, + git_username="test_rebasebot", + git_email="test@rebasebot.ocp", + workdir=tmpdir, + ) + gitwd.remotes.source.fetch(source.branch) + gitwd.remotes.dest.fetch(dest.branch) + _prepare_rebase_branch(gitwd, source, dest) + return gitwd + + +def _setup_vendor_files_on_source_and_dest(source, dest): + source_builder = CommitBuilder(source) + dest_builder = CommitBuilder(dest) + for i in range(_VENDOR_FILE_COUNT): + source_builder.add_file(_vendor_filename(i), _vendor_content(i)) + dest_builder.add_file(_vendor_filename(i), _vendor_content(i)) + source_builder.commit("add vendor stand-ins") + dest_builder.commit("UPSTREAM: : add vendor stand-ins") + + +class TestFalseRenameDeleteResolution: + """Regression tests for false rename/delete keep-versus-delete handling.""" + + def test_false_rename_delete_keeps_head_file(self, init_test_repositories, fake_github_provider, tmpdir): + """HEAD file not listed in the picked commit survives a false rename/delete conflict.""" + source, rebase, dest = init_test_repositories + _setup_vendor_files_on_source_and_dest(source, dest) + + source_drop = CommitBuilder(source) + for i in range(_VENDOR_FILE_COUNT): + source_drop.remove_file(_vendor_filename(i)) + source_drop.add_file(_E2E_LABELS_FILE, _E2E_LABELS_CONTENT).commit( + "upstream: drop vendor stand-ins, add e2e labels" + ) + + dest_pick = CommitBuilder(dest) + for i in range(_VENDOR_FILE_COUNT): + dest_pick.remove_file(_vendor_filename(i)) + # Extra path so the pick still has a real change after keeping the false rename. + carry = dest_pick.add_file("carry_marker.txt", "marker\n").commit("UPSTREAM: : remove vendor stand-ins") + + gitwd = _prepare_working_repo(source, rebase, dest, fake_github_provider, tmpdir) + assert _E2E_LABELS_FILE in gitwd.git.ls_files().splitlines() + + result = _safe_cherry_pick( + gitwd=gitwd, + sha=carry.hexsha, + source_branch=source.branch, + conflict_policy="auto", + commit_description=f"{carry.hexsha} - UPSTREAM: : remove vendor stand-ins", + ) + + assert result.created_commit is True + assert _E2E_LABELS_FILE in gitwd.git.ls_files().splitlines() + assert "carry_marker.txt" in gitwd.git.ls_files().splitlines() + with open(f"{gitwd.working_dir}/{_E2E_LABELS_FILE}", encoding="utf8") as f: + assert f.read() == _E2E_LABELS_CONTENT + + def test_legitimate_delete_removes_path(self, init_test_repositories, fake_github_provider, tmpdir): + """A path listed in the picked commit is still removed on modify/delete conflict.""" + source, rebase, dest = init_test_repositories + + CommitBuilder(source).update_file("test.go", "upstream modified content\n").commit("modify test.go") + carry = CommitBuilder(dest).remove_file("test.go").commit("UPSTREAM: : remove test.go") + + gitwd = _prepare_working_repo(source, rebase, dest, fake_github_provider, tmpdir) + assert "test.go" in gitwd.git.ls_files().splitlines() + + result = _safe_cherry_pick( + gitwd=gitwd, + sha=carry.hexsha, + source_branch=source.branch, + conflict_policy="auto", + commit_description=f"{carry.hexsha} - UPSTREAM: : remove test.go", + ) + + assert result.created_commit is True + assert "test.go" not in gitwd.git.ls_files().splitlines() + + def test_legitimate_delete_removes_non_ascii_path(self, init_test_repositories, fake_github_provider, tmpdir): + """Quoted non-ASCII paths in the picked commit still delete (path unescape must match).""" + source, rebase, dest = init_test_repositories + non_ascii_name = "café.txt" + + CommitBuilder(source).add_file(non_ascii_name, "upstream café\n").commit("add non-ascii file") + CommitBuilder(dest).add_file(non_ascii_name, "downstream café\n").commit( + "UPSTREAM: : add non-ascii file" + ) + CommitBuilder(source).update_file(non_ascii_name, "upstream modified café\n").commit( + "modify non-ascii file upstream" + ) + carry = CommitBuilder(dest).remove_file(non_ascii_name).commit("UPSTREAM: : remove non-ascii file") + + gitwd = _prepare_working_repo(source, rebase, dest, fake_github_provider, tmpdir) + non_ascii_path = os.path.join(gitwd.working_dir, non_ascii_name) + assert os.path.exists(non_ascii_path) + + result = _safe_cherry_pick( + gitwd=gitwd, + sha=carry.hexsha, + source_branch=source.branch, + conflict_policy="auto", + commit_description=f"{carry.hexsha} - UPSTREAM: : remove non-ascii file", + ) + + assert result.created_commit is True + assert not os.path.exists(non_ascii_path) + + def test_empty_after_resolution_skips_pick(self, init_test_repositories, fake_github_provider, tmpdir): + """Keeping false renames with no remaining changes skips instead of failing or empty-committing.""" + source, rebase, dest = init_test_repositories + _setup_vendor_files_on_source_and_dest(source, dest) + + source_drop = CommitBuilder(source) + for i in range(_VENDOR_FILE_COUNT): + source_drop.remove_file(_vendor_filename(i)) + source_drop.add_file(_E2E_LABELS_FILE, _E2E_LABELS_CONTENT).commit( + "upstream: drop vendor stand-ins, add e2e labels" + ) + + dest_pick = CommitBuilder(dest) + for i in range(_VENDOR_FILE_COUNT): + dest_pick.remove_file(_vendor_filename(i)) + carry = dest_pick.commit("UPSTREAM: : remove vendor stand-ins") + + gitwd = _prepare_working_repo(source, rebase, dest, fake_github_provider, tmpdir) + head_before = gitwd.head.commit.hexsha + + result = _safe_cherry_pick( + gitwd=gitwd, + sha=carry.hexsha, + source_branch=source.branch, + conflict_policy="auto", + commit_description=f"{carry.hexsha} - UPSTREAM: : remove vendor stand-ins", + ) + + assert result.created_commit is False + assert gitwd.head.commit.hexsha == head_before + assert _E2E_LABELS_FILE in gitwd.git.ls_files().splitlines()