From: Simon Glass <simon.glass@canonical.com> Break long lines to comply with 80-character limit and remove trailing whitespace across agent.py, control.py, database.py, gitlab_api.py, and __main__.py Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/__main__.py | 22 ++++++++++++---------- tools/pickman/agent.py | 13 ++++++++----- tools/pickman/control.py | 12 ++++++++---- tools/pickman/database.py | 11 +++++++---- tools/pickman/gitlab_api.py | 15 ++++++++++----- 5 files changed, 45 insertions(+), 28 deletions(-) diff --git a/tools/pickman/__main__.py b/tools/pickman/__main__.py index 05e0f7fb6bb..d11734f1f25 100755 --- a/tools/pickman/__main__.py +++ b/tools/pickman/__main__.py @@ -60,24 +60,24 @@ def parse_args(argv): subparsers.add_parser('compare', help='Compare branches') - count_merges = subparsers.add_parser('count-merges', - help='Count remaining merges to process') + count_merges = subparsers.add_parser( + 'count-merges', help='Count remaining merges to process') count_merges.add_argument('source', help='Source branch name') subparsers.add_parser('list-sources', help='List tracked source branches') - next_merges = subparsers.add_parser('next-merges', - help='Show next N merges to be applied') + next_merges = subparsers.add_parser( + 'next-merges', help='Show next N merges to be applied') next_merges.add_argument('source', help='Source branch name') next_merges.add_argument('-c', '--count', type=int, default=10, help='Number of merges to show (default: 10)') - next_set = subparsers.add_parser('next-set', - help='Show next set of commits to cherry-pick') + next_set = subparsers.add_parser( + 'next-set', help='Show next set of commits to cherry-pick') next_set.add_argument('source', help='Source branch name') - review_cmd = subparsers.add_parser('review', - help='Check open MRs and handle comments') + review_cmd = subparsers.add_parser( + 'review', help='Check open MRs and handle comments') review_cmd.add_argument('-r', '--remote', default='ci', help='Git remote (default: ci)') @@ -95,7 +95,8 @@ def parse_args(argv): help='Run step repeatedly until stopped') poll_cmd.add_argument('source', help='Source branch name') poll_cmd.add_argument('-i', '--interval', type=int, default=300, - help='Interval between steps in seconds (default: 300)') + help='Interval between steps in seconds ' + '(default: 300)') poll_cmd.add_argument('-m', '--max-mrs', type=int, default=5, help='Max open MRs allowed (default: 5)') poll_cmd.add_argument('-r', '--remote', default='ci', @@ -115,7 +116,8 @@ def parse_args(argv): test_cmd = subparsers.add_parser('test', help='Run tests') test_cmd.add_argument('-P', '--processes', type=int, - help='Number of processes to run tests (default: all)') + help='Number of processes to run tests ' + '(default: all)') test_cmd.add_argument('-T', '--test-coverage', action='store_true', help='Run tests and check for 100%% coverage') test_cmd.add_argument('-v', '--verbosity', type=int, default=1, diff --git a/tools/pickman/agent.py b/tools/pickman/agent.py index 4570312a97c..9b37428af3e 100644 --- a/tools/pickman/agent.py +++ b/tools/pickman/agent.py @@ -83,7 +83,8 @@ async def run(commits, source, branch_name, repo_path=None): Steps to follow: 1. First run 'git status' to check the repository state is clean -2. Create and checkout a new branch based on ci/master: git checkout -b {branch_name} ci/master +2. Create and checkout a new branch based on ci/master: + git checkout -b {branch_name} ci/master 3. Cherry-pick each commit in order: - For regular commits: git cherry-pick -x <hash> - For merge commits (identified by "Merge" in subject): git cherry-pick -x -m 1 --allow-empty <hash> @@ -94,9 +95,10 @@ Steps to follow: - Show the conflicting files - Try to resolve simple conflicts automatically - For complex conflicts, describe what needs manual resolution and abort - - When fix-ups are needed, amend the commit to add a one-line note at the end - of the commit message describing the changes made -5. After ALL cherry-picks complete, verify with 'git log --oneline -n {len(commits) + 2}' + - When fix-ups are needed, amend the commit to add a one-line note at the + end of the commit message describing the changes made +5. After ALL cherry-picks complete, verify with + 'git log --oneline -n {len(commits) + 2}' Ensure all {len(commits)} commits are present. 6. Run 'buildman -L --board sandbox -w -o /tmp/pickman' to verify the build 7. Report the final status including: @@ -287,7 +289,8 @@ def build_review_prompt(mr_iid, branch_name, task_desc, context_section, comment_steps = """ - Make the requested changes to the code - Amend the relevant commit or create a fixup commit""" - return f"""Task for merge request !{mr_iid} (branch: {branch_name}): {task_desc} + return f"""Task for merge request !{mr_iid} (branch: {branch_name}): +{task_desc} {context_section}{comment_section}{rebase_section} Steps to follow: 1. Checkout the branch: git checkout {branch_name} diff --git a/tools/pickman/control.py b/tools/pickman/control.py index b14c830f1d4..7a52b99a9ed 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -601,7 +601,8 @@ def get_history(fname, source, commits, branch_name, conv_log): fhandle.write(content) # Generate commit message - commit_msg = f'pickman: Record cherry-pick of {len(commits)} commits from {source}\n\n' + commit_msg = (f'pickman: Record cherry-pick of {len(commits)} commits ' + f'from {source}\n\n') commit_msg += '\n'.join(f'- {c.short_hash} {c.subject}' for c in commits) return content, commit_msg @@ -806,7 +807,8 @@ def execute_apply(dbs, source, commits, branch_name, args): # pylint: disable=t target = args.target # Use merge commit subject as title (last commit is the merge) title = f'[pickman] {commits[-1].subject}' - # Description matches .pickman-history entry (summary + conversation) + # Description matches .pickman-history entry + # (summary + conversation) summary = format_history_summary(source, commits, branch_name) description = f'{summary}\n\n### Conversation log\n{conv_log}' @@ -1083,7 +1085,8 @@ Comments addressed: # Commit the history file run_git(['add', '-f', HISTORY_FILE]) - run_git(['commit', '-m', f'pickman: Record review handling for {branch_name}']) + run_git(['commit', '-m', + f'pickman: Record review handling for {branch_name}']) def do_review(args, dbs): @@ -1126,7 +1129,8 @@ def parse_mr_description(desc): desc (str): MR description text Returns: - tuple: (source_branch, last_commit_hash) or (None, None) if not parseable + tuple: (source_branch, last_commit_hash) or (None, None) + if not parseable """ # Extract source branch from "## date: source_branch" line source_match = re.search(r'^## [^:]+: (.+)$', desc, re.MULTILINE) diff --git a/tools/pickman/database.py b/tools/pickman/database.py index b8da21caf58..f584fe14c21 100644 --- a/tools/pickman/database.py +++ b/tools/pickman/database.py @@ -292,8 +292,9 @@ class Database: # pylint: disable=too-many-public-methods cherry_hash) or None if not found """ res = self.execute( - 'SELECT id, chash, source_id, mergereq_id, subject, author, status, ' - 'cherry_hash FROM pcommit WHERE chash = ?', (chash,)) + 'SELECT id, chash, source_id, mergereq_id, subject, author, ' + 'status, cherry_hash FROM pcommit WHERE chash = ?', + (chash,)) return res.fetchone() def commit_get_by_source(self, source_id, status=None): @@ -344,11 +345,13 @@ class Database: # pylint: disable=too-many-public-methods """ if cherry_hash: self.execute( - 'UPDATE pcommit SET status = ?, cherry_hash = ? WHERE chash = ?', + 'UPDATE pcommit SET status = ?, cherry_hash = ? ' + 'WHERE chash = ?', (status, cherry_hash, chash)) else: self.execute( - 'UPDATE pcommit SET status = ? WHERE chash = ?', (status, chash)) + 'UPDATE pcommit SET status = ? WHERE chash = ?', + (status, chash)) def commit_set_mergereq(self, chash, mergereq_id): """Set the merge request for a commit diff --git a/tools/pickman/gitlab_api.py b/tools/pickman/gitlab_api.py index 0ccdf8defb2..94af6880b7c 100644 --- a/tools/pickman/gitlab_api.py +++ b/tools/pickman/gitlab_api.py @@ -132,7 +132,8 @@ def parse_url(url): Examples: - git@gitlab.com:group/project.git -> ('gitlab.com', 'group/project') - - https://gitlab.com/group/project.git -> ('gitlab.com', 'group/project') + - https://gitlab.com/group/project.git -> + ('gitlab.com', 'group/project') """ # SSH format: git@gitlab.com:group/project.git ssh_match = re.match(r'git@([^:]+):(.+?)(?:\.git)?$', url) @@ -211,7 +212,8 @@ def push_branch(remote, branch, force=False, skip_ci=True): args.extend(['-o', 'ci.skip']) if force: if have_remote_ref: - args.append(f'--force-with-lease=refs/remotes/{remote}/{branch}') + args.append( + f'--force-with-lease=refs/remotes/{remote}/{branch}') else: args.append('--force') args.extend([push_target, f'HEAD:{branch}']) @@ -349,7 +351,8 @@ def get_open_pickman_mrs(remote): remote (str): Remote name Returns: - list: List of dicts with 'iid', 'title', 'web_url', 'source_branch' keys, + list: List of dicts with 'iid', 'title', 'web_url', 'source_branch' + keys, or None on failure """ return get_pickman_mrs(remote, state='opened') @@ -594,7 +597,8 @@ def check_permissions(remote): # pylint: disable=too-many-return-statements token = get_token() if not token: tout.error('No GitLab token configured') - tout.error('Set token in ~/.config/pickman.conf or GITLAB_TOKEN env var') + tout.error('Set token in ~/.config/pickman.conf or ' + 'GITLAB_TOKEN env var') return None remote_url = get_remote_url(remote) @@ -625,7 +629,8 @@ def check_permissions(remote): # pylint: disable=too-many-return-statements except gitlab.exceptions.GitlabGetError: pass - access_name = ACCESS_LEVELS.get(access_level, f'Unknown ({access_level})') + access_name = ACCESS_LEVELS.get(access_level, + f'Unknown ({access_level})') return PermissionInfo( user=user.username, -- 2.43.0

From: Simon Glass <simon.glass@canonical.com> Make some simple tweaks to reduces the size of lines: - Rename format_history_summary() -> format_history() - Rename update_history_with_review() -> update_history() - Rename update_mr_description() -> update_mr_desc() - Rename SIGNAL_ALREADY_APPLIED -> SIGNAL_APPLIED - Import gitlab_api as 'gitlab' in ftest.py - Shorten test hash strings by 1 character - Remove unused _cmd variable assignment - Shorten exception message 'branch not found' -> 'not found' Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/agent.py | 2 +- tools/pickman/control.py | 16 +-- tools/pickman/ftest.py | 278 ++++++++++++++++++------------------ tools/pickman/gitlab_api.py | 2 +- 4 files changed, 150 insertions(+), 148 deletions(-) diff --git a/tools/pickman/agent.py b/tools/pickman/agent.py index 9b37428af3e..d2a9ba562f8 100644 --- a/tools/pickman/agent.py +++ b/tools/pickman/agent.py @@ -21,7 +21,7 @@ SIGNAL_FILE = '.pickman-signal' # Signal status codes SIGNAL_SUCCESS = 'success' -SIGNAL_ALREADY_APPLIED = 'already_applied' +SIGNAL_APPLIED = 'already_applied' SIGNAL_CONFLICT = 'conflict' # Check if claude_agent_sdk is available diff --git a/tools/pickman/control.py b/tools/pickman/control.py index 7a52b99a9ed..a6789b930ba 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -537,7 +537,7 @@ def handle_skip_comments(remote, mr_iid, title, unresolved, dbs): return True -def format_history_summary(source, commits, branch_name): +def format_history(source, commits, branch_name): """Format a summary of the cherry-pick operation Args: @@ -575,7 +575,7 @@ def get_history(fname, source, commits, branch_name, conv_log): tuple: (content, commit_msg) where content is the updated history and commit_msg is the git commit message """ - summary = format_history_summary(source, commits, branch_name) + summary = format_history(source, commits, branch_name) entry = f"""{summary} ### Conversation log @@ -735,7 +735,7 @@ def handle_already_applied(dbs, source, commits, branch_name, conv_log, args, # Use merge commit subject as title with [skip] prefix title = f'{SKIPPED_TAG} [pickman] {commits[-1].subject}' - summary = format_history_summary(source, commits, branch_name) + summary = format_history(source, commits, branch_name) description = (f'{summary}\n\n' f'**Status:** Commits already applied to {target} ' f'with different hashes.\n\n' @@ -778,7 +778,7 @@ def execute_apply(dbs, source, commits, branch_name, args): # pylint: disable=t # Check for signal file from agent signal_status, signal_commit = agent.read_signal_file() - if signal_status == agent.SIGNAL_ALREADY_APPLIED: + if signal_status == agent.SIGNAL_APPLIED: ret = handle_already_applied(dbs, source, commits, branch_name, conv_log, args, signal_commit) return ret, False, conv_log @@ -809,7 +809,7 @@ def execute_apply(dbs, source, commits, branch_name, args): # pylint: disable=t title = f'[pickman] {commits[-1].subject}' # Description matches .pickman-history entry # (summary + conversation) - summary = format_history_summary(source, commits, branch_name) + summary = format_history(source, commits, branch_name) description = f'{summary}\n\n### Conversation log\n{conv_log}' mr_url = gitlab_api.push_and_create_mr( @@ -1000,10 +1000,10 @@ def process_single_mr(remote, merge_req, dbs, target): new_desc = (f"{old_desc}\n\n### Review response\n\n" f"**Comments addressed:**\n{comment_summary}\n\n" f"**Response:**\n{conversation_log}") - gitlab_api.update_mr_description(remote, mr_iid, new_desc) + gitlab_api.update_mr_desc(remote, mr_iid, new_desc) # Update .pickman-history - update_history_with_review(merge_req.source_branch, + update_history(merge_req.source_branch, unresolved, conversation_log) tout.info(f'Updated MR !{mr_iid} description and history') @@ -1047,7 +1047,7 @@ def process_mr_reviews(remote, mrs, dbs, target='master'): return processed -def update_history_with_review(branch_name, comments, conversation_log): +def update_history(branch_name, comments, conversation_log): """Append review handling to .pickman-history Args: diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 5f126984c9e..664825ef105 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -28,7 +28,7 @@ from pickman import __main__ as pickman from pickman import agent from pickman import control from pickman import database -from pickman import gitlab_api +from pickman import gitlab_api as gitlab # Test URL constants TEST_OAUTH_URL = 'https://oauth2:test-token@gitlab.com/group/project.git' @@ -1370,55 +1370,55 @@ class TestParseUrl(unittest.TestCase): def test_parse_ssh_url(self): """Test parsing SSH URL.""" - host, path = gitlab_api.parse_url( + host, path = gitlab.parse_url( 'git@gitlab.com:group/project.git') self.assertEqual(host, 'gitlab.com') self.assertEqual(path, 'group/project') def test_parse_ssh_url_no_git_suffix(self): """Test parsing SSH URL without .git suffix.""" - host, path = gitlab_api.parse_url( + host, path = gitlab.parse_url( 'git@gitlab.com:group/project') self.assertEqual(host, 'gitlab.com') self.assertEqual(path, 'group/project') def test_parse_ssh_url_nested_group(self): """Test parsing SSH URL with nested group.""" - host, path = gitlab_api.parse_url( + host, path = gitlab.parse_url( 'git@gitlab.denx.de:u-boot/custodians/u-boot-dm.git') self.assertEqual(host, 'gitlab.denx.de') self.assertEqual(path, 'u-boot/custodians/u-boot-dm') def test_parse_https_url(self): """Test parsing HTTPS URL.""" - host, path = gitlab_api.parse_url( + host, path = gitlab.parse_url( 'https://gitlab.com/group/project.git') self.assertEqual(host, 'gitlab.com') self.assertEqual(path, 'group/project') def test_parse_https_url_no_git_suffix(self): """Test parsing HTTPS URL without .git suffix.""" - host, path = gitlab_api.parse_url( + host, path = gitlab.parse_url( 'https://gitlab.com/group/project') self.assertEqual(host, 'gitlab.com') self.assertEqual(path, 'group/project') def test_parse_http_url(self): """Test parsing HTTP URL.""" - host, path = gitlab_api.parse_url( + host, path = gitlab.parse_url( 'http://gitlab.example.com/group/project.git') self.assertEqual(host, 'gitlab.example.com') self.assertEqual(path, 'group/project') def test_parse_invalid_url(self): """Test parsing invalid URL.""" - host, path = gitlab_api.parse_url('not-a-valid-url') + host, path = gitlab.parse_url('not-a-valid-url') self.assertIsNone(host) self.assertIsNone(path) def test_parse_empty_url(self): """Test parsing empty URL.""" - host, path = gitlab_api.parse_url('') + host, path = gitlab.parse_url('') self.assertIsNone(host) self.assertIsNone(path) @@ -1428,16 +1428,16 @@ class TestCheckAvailable(unittest.TestCase): def test_check_available_false(self): """Test check_available returns False when gitlab not installed.""" - with mock.patch.object(gitlab_api, 'AVAILABLE', False): + with mock.patch.object(gitlab, 'AVAILABLE', False): with terminal.capture(): - result = gitlab_api.check_available() + result = gitlab.check_available() self.assertFalse(result) def test_check_available_true(self): """Test check_available returns True when gitlab is installed.""" - with mock.patch.object(gitlab_api, 'AVAILABLE', True): + with mock.patch.object(gitlab, 'AVAILABLE', True): with terminal.capture(): - result = gitlab_api.check_available() + result = gitlab.check_available() self.assertTrue(result) @@ -1446,36 +1446,36 @@ class TestGetPushUrl(unittest.TestCase): def test_get_push_url_success(self): """Test successful push URL generation.""" - with mock.patch.object(gitlab_api, 'get_token', + with mock.patch.object(gitlab, 'get_token', return_value='test-token'): with mock.patch.object( - gitlab_api, 'get_remote_url', + gitlab, 'get_remote_url', return_value=TEST_SSH_URL): - url = gitlab_api.get_push_url('origin') + url = gitlab.get_push_url('origin') self.assertEqual(url, TEST_OAUTH_URL) def test_get_push_url_no_token(self): """Test returns None when no token available.""" - with mock.patch.object(gitlab_api, 'get_token', return_value=None): - url = gitlab_api.get_push_url('origin') + with mock.patch.object(gitlab, 'get_token', return_value=None): + url = gitlab.get_push_url('origin') self.assertIsNone(url) def test_get_push_url_invalid_remote(self): """Test returns None for invalid remote URL.""" - with mock.patch.object(gitlab_api, 'get_token', + with mock.patch.object(gitlab, 'get_token', return_value='test-token'): - with mock.patch.object(gitlab_api, 'get_remote_url', + with mock.patch.object(gitlab, 'get_remote_url', return_value='not-a-valid-url'): - url = gitlab_api.get_push_url('origin') + url = gitlab.get_push_url('origin') self.assertIsNone(url) def test_get_push_url_https_remote(self): """Test with HTTPS remote URL.""" - with mock.patch.object(gitlab_api, 'get_token', + with mock.patch.object(gitlab, 'get_token', return_value='test-token'): - with mock.patch.object(gitlab_api, 'get_remote_url', + with mock.patch.object(gitlab, 'get_remote_url', return_value=TEST_HTTPS_URL): - url = gitlab_api.get_push_url('origin') + url = gitlab.get_push_url('origin') self.assertEqual(url, TEST_OAUTH_URL) @@ -1484,10 +1484,10 @@ class TestPushBranch(unittest.TestCase): def test_push_branch_force_with_remote_ref(self): """Test force push when remote branch exists uses --force-with-lease.""" - with mock.patch.object(gitlab_api, 'get_push_url', + with mock.patch.object(gitlab, 'get_push_url', return_value=TEST_SHORT_OAUTH_URL): with mock.patch.object(command, 'output') as mock_output: - result = gitlab_api.push_branch('ci', 'test-branch', force=True) + result = gitlab.push_branch('ci', 'test-branch', force=True) self.assertTrue(result) # Should fetch first, then push with --force-with-lease @@ -1496,11 +1496,12 @@ class TestPushBranch(unittest.TestCase): self.assertEqual(calls[0], mock.call('git', 'fetch', 'ci', 'test-branch')) push_args = calls[1][0] - self.assertIn('--force-with-lease=refs/remotes/ci/test-branch', push_args) + self.assertIn('--force-with-lease=refs/remotes/ci/test-branch', + push_args) def test_push_branch_force_no_remote_ref(self): """Test force push when remote branch doesn't exist uses --force.""" - with mock.patch.object(gitlab_api, 'get_push_url', + with mock.patch.object(gitlab, 'get_push_url', return_value=TEST_SHORT_OAUTH_URL): with mock.patch.object(command, 'output') as mock_output: # Fetch fails (branch doesn't exist on remote) @@ -1509,10 +1510,11 @@ class TestPushBranch(unittest.TestCase): command.CommandResult()), # fetch fails None, # push succeeds ] - result = gitlab_api.push_branch('ci', 'new-branch', force=True) + result = gitlab.push_branch('ci', 'new-branch', force=True) self.assertTrue(result) - # Should try fetch, fail, then push with --force (not --force-with-lease) + # Should try fetch, fail, then push with --force + # (not --force-with-lease) calls = mock_output.call_args_list self.assertEqual(len(calls), 2) push_args = calls[1][0] @@ -1521,10 +1523,10 @@ class TestPushBranch(unittest.TestCase): def test_push_branch_no_force(self): """Test regular push without force doesn't fetch or use force flags.""" - with mock.patch.object(gitlab_api, 'get_push_url', + with mock.patch.object(gitlab, 'get_push_url', return_value=TEST_SHORT_OAUTH_URL): with mock.patch.object(command, 'output') as mock_output: - result = gitlab_api.push_branch('ci', 'test-branch', force=False) + result = gitlab.push_branch('ci', 'test-branch', force=False) self.assertTrue(result) # Should only push, no fetch, no force flags @@ -1552,16 +1554,16 @@ class TestConfigFile(unittest.TestCase): with open(self.config_file, 'w', encoding='utf-8') as fhandle: fhandle.write('[gitlab]\ntoken = test-config-token\n') - with mock.patch.object(gitlab_api, 'CONFIG_FILE', self.config_file): - token = gitlab_api.get_token() + with mock.patch.object(gitlab, 'CONFIG_FILE', self.config_file): + token = gitlab.get_token() self.assertEqual(token, 'test-config-token') def test_get_token_fallback_to_env(self): """Test falling back to environment variable.""" # Config file doesn't exist - with mock.patch.object(gitlab_api, 'CONFIG_FILE', '/nonexistent/path'): + with mock.patch.object(gitlab, 'CONFIG_FILE', '/nonexistent/path'): with mock.patch.dict(os.environ, {'GITLAB_TOKEN': 'env-token'}): - token = gitlab_api.get_token() + token = gitlab.get_token() self.assertEqual(token, 'env-token') def test_get_token_config_missing_section(self): @@ -1569,9 +1571,9 @@ class TestConfigFile(unittest.TestCase): with open(self.config_file, 'w', encoding='utf-8') as fhandle: fhandle.write('[other]\nkey = value\n') - with mock.patch.object(gitlab_api, 'CONFIG_FILE', self.config_file): + with mock.patch.object(gitlab, 'CONFIG_FILE', self.config_file): with mock.patch.dict(os.environ, {'GITLAB_TOKEN': 'env-token'}): - token = gitlab_api.get_token() + token = gitlab.get_token() self.assertEqual(token, 'env-token') def test_get_config_value(self): @@ -1579,17 +1581,17 @@ class TestConfigFile(unittest.TestCase): with open(self.config_file, 'w', encoding='utf-8') as fhandle: fhandle.write('[section1]\nkey1 = value1\n') - with mock.patch.object(gitlab_api, 'CONFIG_FILE', self.config_file): - value = gitlab_api.get_config_value('section1', 'key1') + with mock.patch.object(gitlab, 'CONFIG_FILE', self.config_file): + value = gitlab.get_config_value('section1', 'key1') self.assertEqual(value, 'value1') class TestCheckPermissions(unittest.TestCase): """Tests for check_permissions function.""" - @mock.patch.object(gitlab_api, 'get_remote_url') - @mock.patch.object(gitlab_api, 'get_token') - @mock.patch.object(gitlab_api, 'AVAILABLE', True) + @mock.patch.object(gitlab, 'get_remote_url') + @mock.patch.object(gitlab, 'get_token') + @mock.patch.object(gitlab, 'AVAILABLE', True) def test_check_permissions_developer(self, mock_token, mock_url): """Test checking permissions for a developer.""" mock_token.return_value = 'test-token' @@ -1610,7 +1612,7 @@ class TestCheckPermissions(unittest.TestCase): mock_glab.projects.get.return_value = mock_project with mock.patch('gitlab.Gitlab', return_value=mock_glab): - perms = gitlab_api.check_permissions('origin') + perms = gitlab.check_permissions('origin') self.assertIsNotNone(perms) self.assertEqual(perms.user, 'testuser') @@ -1620,30 +1622,30 @@ class TestCheckPermissions(unittest.TestCase): self.assertTrue(perms.can_create_mr) self.assertFalse(perms.can_merge) - @mock.patch.object(gitlab_api, 'AVAILABLE', False) + @mock.patch.object(gitlab, 'AVAILABLE', False) def test_check_permissions_not_available(self): """Test check_permissions when gitlab not available.""" with terminal.capture(): - perms = gitlab_api.check_permissions('origin') + perms = gitlab.check_permissions('origin') self.assertIsNone(perms) - @mock.patch.object(gitlab_api, 'get_token') - @mock.patch.object(gitlab_api, 'AVAILABLE', True) + @mock.patch.object(gitlab, 'get_token') + @mock.patch.object(gitlab, 'AVAILABLE', True) def test_check_permissions_no_token(self, mock_token): """Test check_permissions when no token set.""" mock_token.return_value = None with terminal.capture(): - perms = gitlab_api.check_permissions('origin') + perms = gitlab.check_permissions('origin') self.assertIsNone(perms) class TestUpdateMrDescription(unittest.TestCase): - """Tests for update_mr_description function.""" + """Tests for update_mr_desc function.""" - @mock.patch.object(gitlab_api, 'get_remote_url') - @mock.patch.object(gitlab_api, 'get_token') - @mock.patch.object(gitlab_api, 'AVAILABLE', True) - def test_update_mr_description_success(self, mock_token, mock_url): + @mock.patch.object(gitlab, 'get_remote_url') + @mock.patch.object(gitlab, 'get_token') + @mock.patch.object(gitlab, 'AVAILABLE', True) + def test_update_mr_desc_success(self, mock_token, mock_url): """Test successful MR description update.""" mock_token.return_value = 'test-token' mock_url.return_value = 'git@gitlab.com:group/project.git' @@ -1655,36 +1657,36 @@ class TestUpdateMrDescription(unittest.TestCase): with mock.patch('gitlab.Gitlab') as mock_gitlab: mock_gitlab.return_value.projects.get.return_value = mock_project - result = gitlab_api.update_mr_description('origin', 123, + result = gitlab.update_mr_desc('origin', 123, 'New description') self.assertTrue(result) self.assertEqual(mock_mr.description, 'New description') mock_mr.save.assert_called_once() - @mock.patch.object(gitlab_api, 'AVAILABLE', False) - def test_update_mr_description_not_available(self): - """Test update_mr_description when gitlab not available.""" + @mock.patch.object(gitlab, 'AVAILABLE', False) + def test_update_mr_desc_not_available(self): + """Test update_mr_desc when gitlab not available.""" with terminal.capture(): - result = gitlab_api.update_mr_description('origin', 123, 'desc') + result = gitlab.update_mr_desc('origin', 123, 'desc') self.assertFalse(result) - @mock.patch.object(gitlab_api, 'get_token') - @mock.patch.object(gitlab_api, 'AVAILABLE', True) - def test_update_mr_description_no_token(self, mock_token): - """Test update_mr_description when no token set.""" + @mock.patch.object(gitlab, 'get_token') + @mock.patch.object(gitlab, 'AVAILABLE', True) + def test_update_mr_desc_no_token(self, mock_token): + """Test update_mr_desc when no token set.""" mock_token.return_value = None with terminal.capture(): - result = gitlab_api.update_mr_description('origin', 123, 'desc') + result = gitlab.update_mr_desc('origin', 123, 'desc') self.assertFalse(result) class TestGetPickmanMrs(unittest.TestCase): """Tests for get_pickman_mrs function.""" - @mock.patch.object(gitlab_api, 'get_remote_url') - @mock.patch.object(gitlab_api, 'get_token') - @mock.patch.object(gitlab_api, 'AVAILABLE', True) + @mock.patch.object(gitlab, 'get_remote_url') + @mock.patch.object(gitlab, 'get_token') + @mock.patch.object(gitlab, 'AVAILABLE', True) def test_get_pickman_mrs_sorted_oldest_first(self, mock_token, mock_url): """Test that MRs are requested sorted by created_at ascending.""" mock_token.return_value = 'test-token' @@ -1719,7 +1721,7 @@ class TestGetPickmanMrs(unittest.TestCase): with mock.patch('gitlab.Gitlab') as mock_gitlab: mock_gitlab.return_value.projects.get.return_value = mock_project - result = gitlab_api.get_pickman_mrs('origin', state='opened') + result = gitlab.get_pickman_mrs('origin', state='opened') # Verify the list call includes sorting parameters mock_project.mergerequests.list.assert_called_once_with( @@ -1734,8 +1736,8 @@ class TestGetPickmanMrs(unittest.TestCase): class TestCreateMr(unittest.TestCase): """Tests for create_mr function.""" - @mock.patch.object(gitlab_api, 'get_token') - @mock.patch.object(gitlab_api, 'AVAILABLE', True) + @mock.patch.object(gitlab, 'get_token') + @mock.patch.object(gitlab, 'AVAILABLE', True) def test_create_mr_409_returns_existing(self, mock_token): """Test that 409 error returns existing MR URL.""" tout.init(tout.INFO) @@ -1750,13 +1752,13 @@ class TestCreateMr(unittest.TestCase): # Simulate 409 Conflict error mock_project.mergerequests.create.side_effect = \ - gitlab_api.MrCreateError(response_code=409) + gitlab.MrCreateError(response_code=409) with mock.patch('gitlab.Gitlab') as mock_gitlab: mock_gitlab.return_value.projects.get.return_value = mock_project with terminal.capture(): - result = gitlab_api.create_mr( + result = gitlab.create_mr( 'gitlab.com', 'group/project', 'cherry-abc', 'master', 'Test MR') @@ -1883,7 +1885,7 @@ class TestStep(unittest.TestCase): def test_step_with_pending_mr(self): """Test step does nothing when MR is pending.""" - mock_mr = gitlab_api.PickmanMr( + mock_mr = gitlab.PickmanMr( iid=123, title='[pickman] Test MR', web_url='https://gitlab.com/mr/123', @@ -1891,9 +1893,9 @@ class TestStep(unittest.TestCase): description='Test', ) with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=[]): - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=[mock_mr]): args = argparse.Namespace(cmd='step', source='us/next', remote='ci', target='master', @@ -1905,7 +1907,7 @@ class TestStep(unittest.TestCase): def test_step_gitlab_error(self): """Test step when GitLab API returns error.""" - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=None): args = argparse.Namespace(cmd='step', source='us/next', remote='ci', target='master', @@ -1917,9 +1919,9 @@ class TestStep(unittest.TestCase): def test_step_open_mrs_error(self): """Test step when get_open_pickman_mrs returns error.""" - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=[]): - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=None): args = argparse.Namespace(cmd='step', source='us/next', remote='ci', target='master', @@ -1931,7 +1933,7 @@ class TestStep(unittest.TestCase): def test_step_allows_below_max(self): """Test step allows new MR when count is below max_mrs.""" - mock_mr = gitlab_api.PickmanMr( + mock_mr = gitlab.PickmanMr( iid=123, title='[pickman] Test MR', web_url='https://gitlab.com/mr/123', @@ -1939,9 +1941,9 @@ class TestStep(unittest.TestCase): description='Test', ) with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=[]): - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=[mock_mr]): with mock.patch.object(control, 'do_apply', return_value=0) as mock_apply: @@ -1958,7 +1960,7 @@ class TestStep(unittest.TestCase): def test_step_blocks_at_max(self): """Test step blocks new MR when at max_mrs limit.""" mock_mrs = [ - gitlab_api.PickmanMr( + gitlab.PickmanMr( iid=i, title=f'[pickman] Test MR {i}', web_url=f'https://gitlab.com/mr/{i}', @@ -1968,9 +1970,9 @@ class TestStep(unittest.TestCase): for i in range(3) ] with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=[]): - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=mock_mrs): with mock.patch.object(control, 'do_apply') as mock_apply: args = argparse.Namespace(cmd='step', source='us/next', @@ -2005,7 +2007,7 @@ class TestReview(unittest.TestCase): def test_review_no_mrs(self): """Test review when no open MRs found.""" - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=[]): args = argparse.Namespace(cmd='review', remote='ci') with terminal.capture(): @@ -2015,7 +2017,7 @@ class TestReview(unittest.TestCase): def test_review_gitlab_error(self): """Test review when GitLab API returns error.""" - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=None): args = argparse.Namespace(cmd='review', remote='ci') with terminal.capture(): @@ -2025,7 +2027,7 @@ class TestReview(unittest.TestCase): class TestUpdateHistoryWithReview(unittest.TestCase): - """Tests for update_history_with_review function.""" + """Tests for update_history function.""" def setUp(self): """Set up test fixtures.""" @@ -2045,20 +2047,20 @@ class TestUpdateHistoryWithReview(unittest.TestCase): os.chdir(self.orig_dir) shutil.rmtree(self.test_dir) - def test_update_history_with_review(self): + def test_update_history(self): """Test that review handling is appended to history.""" comments = [ - gitlab_api.MrComment(id=1, author='reviewer1', + gitlab.MrComment(id=1, author='reviewer1', body='Please fix the indentation here', created_at='2025-01-01', resolvable=True, resolved=False), - gitlab_api.MrComment(id=2, author='reviewer2', body='Add a docstring', + gitlab.MrComment(id=2, author='reviewer2', body='Add a docstring', created_at='2025-01-01', resolvable=True, resolved=False), ] conversation_log = 'Fixed indentation and added docstring.' - control.update_history_with_review('cherry-abc123', comments, + control.update_history('cherry-abc123', comments, conversation_log) # Check history file was created @@ -2083,10 +2085,10 @@ class TestUpdateHistoryWithReview(unittest.TestCase): subprocess.run(['git', 'commit', '-m', 'Initial'], check=True, capture_output=True) - comments = [gitlab_api.MrComment(id=1, author='reviewer', body='Fix this', - created_at='2025-01-01', resolvable=True, - resolved=False)] - control.update_history_with_review('cherry-xyz', comments, 'Fixed it') + comms = [gitlab.MrComment(id=1, author='reviewer', body='Fix this', + created_at='2025-01-01', resolvable=True, + resolved=False)] + control.update_history('cherry-xyz', comms, 'Fixed it') with open(control.HISTORY_FILE, 'r', encoding='utf-8') as fhandle: content = fhandle.read() @@ -2132,7 +2134,7 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): dbs.comment_mark_processed(100, 1) dbs.commit() - mrs = [gitlab_api.PickmanMr( + mrs = [gitlab.PickmanMr( iid=100, title='[pickman] Test MR', source_branch='cherry-test', @@ -2142,25 +2144,25 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): # Comment 1 is processed, comment 2 is new comments = [ - gitlab_api.MrComment(id=1, author='reviewer', body='Old comment', + gitlab.MrComment(id=1, author='reviewer', body='Old comment', created_at='2025-01-01', resolvable=True, resolved=False), - gitlab_api.MrComment(id=2, author='reviewer', body='New comment', + gitlab.MrComment(id=2, author='reviewer', body='New comment', created_at='2025-01-01', resolvable=True, resolved=False), ] with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_mr_comments', + with mock.patch.object(gitlab, 'get_mr_comments', return_value=comments): with mock.patch.object(agent, 'handle_mr_comments', - return_value=(True, 'Done')) as mock_agent: - with mock.patch.object(gitlab_api, 'update_mr_description'): - with mock.patch.object(control, 'update_history_with_review'): + return_value=(True, 'Done')) as moc: + with mock.patch.object(gitlab, 'update_mr_desc'): + with mock.patch.object(control, 'update_history'): control.process_mr_reviews('ci', mrs, dbs) # Agent should only receive the new comment - call_args = mock_agent.call_args + call_args = moc.call_args passed_comments = call_args[0][2] self.assertEqual(len(passed_comments), 1) self.assertEqual(passed_comments[0].id, 2) @@ -2174,7 +2176,7 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): dbs.start() # MR needs rebase but has no comments - mrs = [gitlab_api.PickmanMr( + mrs = [gitlab.PickmanMr( iid=100, title='[pickman] Test MR', source_branch='cherry-test', @@ -2185,17 +2187,17 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): )] with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_mr_comments', + with mock.patch.object(gitlab, 'get_mr_comments', return_value=[]): with mock.patch.object(agent, 'handle_mr_comments', - return_value=(True, 'Rebased')) as mock_agent: - with mock.patch.object(gitlab_api, 'update_mr_description'): - with mock.patch.object(control, 'update_history_with_review'): + return_value=(True, 'Rebased')) as m: + with mock.patch.object(gitlab, 'update_mr_desc'): + with mock.patch.object(control, 'update_history'): control.process_mr_reviews('ci', mrs, dbs) # Agent should be called with needs_rebase=True - mock_agent.assert_called_once() - call_kwargs = mock_agent.call_args[1] + m.assert_called_once() + call_kwargs = m.call_args[1] self.assertTrue(call_kwargs.get('needs_rebase')) self.assertFalse(call_kwargs.get('has_conflicts')) @@ -2208,7 +2210,7 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): dbs.start() # MR has no comments and doesn't need rebase - mrs = [gitlab_api.PickmanMr( + mrs = [gitlab.PickmanMr( iid=100, title='[pickman] Test MR', source_branch='cherry-test', @@ -2219,14 +2221,14 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): )] with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_mr_comments', + with mock.patch.object(gitlab, 'get_mr_comments', return_value=[]): with mock.patch.object(agent, 'handle_mr_comments', - return_value=(True, 'Done')) as mock_agent: + return_value=(True, 'Done')) as moc: control.process_mr_reviews('ci', mrs, dbs) # Agent should NOT be called - mock_agent.assert_not_called() + moc.assert_not_called() dbs.close() @@ -2360,20 +2362,20 @@ class TestParseInstruction(unittest.TestCase): class TestFormatHistorySummary(unittest.TestCase): - """Tests for format_history_summary function.""" + """Tests for format_history function.""" - def test_format_history_summary(self): + def test_format_history(self): """Test formatting history summary.""" commits = [ control.CommitInfo('aaa111', 'aaa111a', 'First commit', 'Author 1'), - control.CommitInfo('bbb222', 'bbb222b', 'Second commit', 'Author 2'), + control.CommitInfo('bbb222', 'bbb222b', 'Second one', 'Author 2'), ] - result = control.format_history_summary('us/next', commits, 'cherry-abc') + result = control.format_history('us/next', commits, 'cherry-abc') self.assertIn('us/next', result) self.assertIn('Branch: cherry-abc', result) self.assertIn('- aaa111a First commit', result) - self.assertIn('- bbb222b Second commit', result) + self.assertIn('- bbb222b Second one', result) class TestGetHistory(unittest.TestCase): @@ -2472,7 +2474,7 @@ Other content """Test get_history with multiple commits.""" commits = [ control.CommitInfo('aaa111', 'aaa111a', 'First commit', 'Author 1'), - control.CommitInfo('bbb222', 'bbb222b', 'Second commit', 'Author 2'), + control.CommitInfo('bbb222', 'bbb222b', 'Second one', 'Author 2'), control.CommitInfo('ccc333', 'ccc333c', 'Third commit', 'Author 3'), ] content, commit_msg = control.get_history( @@ -2480,13 +2482,13 @@ Other content # Verify all commits in content self.assertIn('- aaa111a First commit', content) - self.assertIn('- bbb222b Second commit', content) + self.assertIn('- bbb222b Second one', content) self.assertIn('- ccc333c Third commit', content) # Verify commit message self.assertIn('pickman: Record cherry-pick of 3 commits', commit_msg) self.assertIn('- aaa111a First commit', commit_msg) - self.assertIn('- bbb222b Second commit', commit_msg) + self.assertIn('- bbb222b Second one', commit_msg) self.assertIn('- ccc333c Third commit', commit_msg) @@ -2684,7 +2686,7 @@ class TestExecuteApply(unittest.TestCase): return_value=(True, 'log')): with mock.patch.object(control, 'run_git', return_value='abc123'): - with mock.patch.object(gitlab_api, 'push_and_create_mr', + with mock.patch.object(gitlab, 'push_and_create_mr', return_value='https://mr/url'): ret, success, _ = control.execute_apply( dbs, 'us/next', commits, 'cherry-branch', args) @@ -2710,7 +2712,7 @@ class TestExecuteApply(unittest.TestCase): return_value=(True, 'log')): with mock.patch.object(control, 'run_git', return_value='abc123'): - with mock.patch.object(gitlab_api, 'push_and_create_mr', + with mock.patch.object(gitlab, 'push_and_create_mr', return_value=None): ret, success, _ = control.execute_apply( dbs, 'us/next', commits, 'cherry-branch', args) @@ -2735,7 +2737,7 @@ class TestExecuteApply(unittest.TestCase): with mock.patch.object(control.agent, 'cherry_pick_commits', return_value=(True, 'aborted log')): with mock.patch.object(control, 'run_git', - side_effect=Exception('branch not found')): + side_effect=Exception('not found')): ret, success, _ = control.execute_apply( dbs, 'us/next', commits, 'cherry-branch', args) @@ -2766,7 +2768,7 @@ class TestExecuteApply(unittest.TestCase): with mock.patch.object(control.agent, 'cherry_pick_commits', return_value=(True, 'already applied log')): with mock.patch.object(control.agent, 'read_signal_file', - return_value=(agent.SIGNAL_ALREADY_APPLIED, + return_value=(agent.SIGNAL_APPLIED, 'hhh888')): ret, success, _ = control.execute_apply( dbs, 'us/next', commits, 'cherry-branch', args) @@ -2971,9 +2973,9 @@ class TestGetNextCommitsEmptyLine(unittest.TestCase): call_count = [0] + # pylint: disable=unused-argument def mock_git(pipe_list): call_count[0] += 1 - _cmd = pipe_list[0] if pipe_list else [] # pylint: disable=unused-variable # First call: get first-parent log if call_count[0] == 1: return command.CommandResult(stdout=fp_log) @@ -3047,7 +3049,7 @@ class TestDoPushBranch(unittest.TestCase): tout.init(tout.INFO) args = argparse.Namespace(cmd='push-branch', branch='test-branch', remote='ci', force=False, run_ci=False) - with mock.patch.object(gitlab_api, 'push_branch', + with mock.patch.object(gitlab, 'push_branch', return_value=True) as mock_push: with terminal.capture(): ret = control.do_push_branch(args, None) @@ -3060,7 +3062,7 @@ class TestDoPushBranch(unittest.TestCase): tout.init(tout.INFO) args = argparse.Namespace(cmd='push-branch', branch='test-branch', remote='origin', force=True, run_ci=False) - with mock.patch.object(gitlab_api, 'push_branch', + with mock.patch.object(gitlab, 'push_branch', return_value=True) as mock_push: with terminal.capture(): ret = control.do_push_branch(args, None) @@ -3073,7 +3075,7 @@ class TestDoPushBranch(unittest.TestCase): tout.init(tout.INFO) args = argparse.Namespace(cmd='push-branch', branch='test-branch', remote='ci', force=False) - with mock.patch.object(gitlab_api, 'push_branch', + with mock.patch.object(gitlab, 'push_branch', return_value=False): with terminal.capture(): ret = control.do_push_branch(args, None) @@ -3114,7 +3116,7 @@ class TestDoReviewWithMrs(unittest.TestCase): """Test review with open MRs but no comments.""" tout.init(tout.INFO) - mock_mr = gitlab_api.PickmanMr( + mock_mr = gitlab.PickmanMr( iid=123, title='[pickman] Test MR', web_url='https://gitlab.com/mr/123', @@ -3122,9 +3124,9 @@ class TestDoReviewWithMrs(unittest.TestCase): description='Test', ) with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=[mock_mr]): - with mock.patch.object(gitlab_api, 'get_mr_comments', + with mock.patch.object(gitlab, 'get_mr_comments', return_value=[]): args = argparse.Namespace(cmd='review', remote='ci', target='master') @@ -3157,10 +3159,10 @@ class TestProcessMergedMrs(unittest.TestCase): with terminal.capture(): dbs = database.Database(self.db_path) dbs.start() - dbs.source_set('us/next', 'aaa111aaa111aaa111aaa111aaa111aaa111aaa1') + dbs.source_set('us/next', 'aaa111aaa111aaa111aaa111aaa111aaa111aaa') dbs.commit() - merged_mrs = [gitlab_api.PickmanMr( + merged_mrs = [gitlab.PickmanMr( iid=100, title='[pickman] Test MR', description='## 2025-01-01: us/next\n\n- bbb222b Subject', @@ -3176,7 +3178,7 @@ class TestProcessMergedMrs(unittest.TestCase): return '' return '' - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=merged_mrs): with mock.patch.object(control, 'run_git', mock_git): processed = control.process_merged_mrs('ci', 'us/next', dbs) @@ -3196,7 +3198,7 @@ class TestProcessMergedMrs(unittest.TestCase): dbs.source_set('us/next', 'bbb222bbb222bbb222bbb222bbb222bbb222bbb2') dbs.commit() - merged_mrs = [gitlab_api.PickmanMr( + merged_mrs = [gitlab.PickmanMr( iid=100, title='[pickman] Test MR', description='## 2025-01-01: us/next\n\n- aaa111a Subject', @@ -3212,7 +3214,7 @@ class TestProcessMergedMrs(unittest.TestCase): raise RuntimeError('Not an ancestor') return '' - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=merged_mrs): with mock.patch.object(control, 'run_git', mock_git): processed = control.process_merged_mrs('ci', 'us/next', dbs) diff --git a/tools/pickman/gitlab_api.py b/tools/pickman/gitlab_api.py index 94af6880b7c..bfaf828edae 100644 --- a/tools/pickman/gitlab_api.py +++ b/tools/pickman/gitlab_api.py @@ -455,7 +455,7 @@ def reply_to_mr(remote, mr_iid, message): return False -def update_mr_description(remote, mr_iid, desc): +def update_mr_desc(remote, mr_iid, desc): """Update a merge request's description Args: -- 2.43.0

From: Simon Glass <simon.glass@canonical.com> Add per-commit delta-checking and build-validation to the cherry-pick and review agents: - Check commit deltas after each cherry-pick to catch problems early - Run buildman after each commit instead of only at the end - Add recovery strategies for commits with large deltas - Include delta checking in rebase operations - Improve error handling and reporting Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/agent.py | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/tools/pickman/agent.py b/tools/pickman/agent.py index d2a9ba562f8..89014df0120 100644 --- a/tools/pickman/agent.py +++ b/tools/pickman/agent.py @@ -91,19 +91,35 @@ Steps to follow: Cherry-pick one commit at a time to handle each appropriately. IMPORTANT: Always include merge commits even if they result in empty commits. The merge commit message is important for tracking history. -4. If there are conflicts: +4. AFTER EACH SUCCESSFUL CHERRY-PICK: + a) Check commit delta: 'git show --stat <cherry-picked-hash>' and 'git show --stat <original-hash>' + Compare the changed files and line counts. If they differ significantly (>20% lines changed + or different files modified), this indicates a problem with the cherry-pick. + b) Build test: Run 'buildman -L --board sandbox -w -o /tmp/pickman' to verify the build passes + c) If delta is too large: + - Reset the commit: 'git reset --hard HEAD~1' + - Try to manually apply just the changes from the original commit: + 'git show <original-hash> | git apply --3way' + - If that succeeds, create a new commit with the original message + - If fails, try to apply the patch manually. + - If manual apply fails, create an empty commit to preserve the commit sequence: + 'git commit --allow-empty -m "<original-subject> [FAILED]"' + - Continue to the next commit + d) If build fails and you cannot resolve it, report the issue and abort +5. If there are conflicts: - Show the conflicting files - Try to resolve simple conflicts automatically - For complex conflicts, describe what needs manual resolution and abort - When fix-ups are needed, amend the commit to add a one-line note at the end of the commit message describing the changes made -5. After ALL cherry-picks complete, verify with +6. After ALL cherry-picks complete, verify with 'git log --oneline -n {len(commits) + 2}' Ensure all {len(commits)} commits are present. -6. Run 'buildman -L --board sandbox -w -o /tmp/pickman' to verify the build -7. Report the final status including: +7. Run final build: 'buildman -L --board sandbox -w -o /tmp/pickman' to verify everything still works together +8. Report the final status including: - Build result (ok or list of warnings/errors) - Any fix-ups that were made + - Any commits with concerning deltas The cherry-pick branch will be left ready for pushing. Do NOT merge it back to any other branch. @@ -243,6 +259,10 @@ Rebase instructions: - The MR is behind the target branch and needs rebasing - Use: git rebase --keep-empty {remote}/{target} - This preserves empty merge commits which are important for tracking +- AFTER EACH REBASED COMMIT: Check delta and build + a) Compare commit delta before/after rebase using 'git show --stat' + b) Run 'buildman -L --board sandbox -w -o /tmp/pickman' to verify build passes + c) If delta is significantly different or build fails, report and abort - If there are conflicts, try to resolve them automatically - For complex conflicts that cannot be resolved, describe them and abort ''' @@ -297,7 +317,7 @@ Steps to follow: 2. {step2} 3. {step3} {comment_steps} -4. Run 'buildman -L --board sandbox -w -o /tmp/pickman' to verify the build +4. Run 'buildman -L --board sandbox -w -o /tmp/pickman' to verify the build passes 5. Create a local branch with suffix '-v2' (or increment: -v3, -v4, etc.) 6. Force push to the ORIGINAL remote branch to update the MR: ./tools/pickman/pickman push-branch {branch_name} -r {remote} -f -- 2.43.0

From: Simon Glass <simon.glass@canonical.com> Add a --diff/-d option to the check command that displays the difference between the original commit and the cherry-picked commit for commits which show a large discrepancy. This helps identify what changed during the cherry-pick process. Also add a --no-colour option to disable coloured output when needed, e.g. when outputting to a log file. Features: - Shows unified diff between original and cherry-picked patch content - Uses coloured output by default, can be disabled with --no-colour - Works in both verbose and summary modes - Includes comprehensive tests for both colour modes Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/README.rst | 2 + tools/pickman/__main__.py | 4 ++ tools/pickman/control.py | 118 ++++++++++++++++++++++++++++++-------- tools/pickman/ftest.py | 93 ++++++++++++++++++++++++++++++ 4 files changed, 192 insertions(+), 25 deletions(-) diff --git a/tools/pickman/README.rst b/tools/pickman/README.rst index 3b280e718d3..cc1ce5f81d5 100644 --- a/tools/pickman/README.rst +++ b/tools/pickman/README.rst @@ -227,6 +227,8 @@ Options: - ``-t, --threshold``: Delta threshold as fraction (default: 0.2 = 20%) - ``-m, --min-lines``: Minimum lines changed to check (default: 10) - ``-v, --verbose``: Show detailed analysis for all commits +- ``--diff``: Show patch differences for problem commits +- ``--no-colour``: Disable color output in patch differences Example output:: diff --git a/tools/pickman/__main__.py b/tools/pickman/__main__.py index 87077d1d10a..e450a9142fc 100755 --- a/tools/pickman/__main__.py +++ b/tools/pickman/__main__.py @@ -53,6 +53,8 @@ def add_main_commands(subparsers): '(default: 10)') check_cmd.add_argument('-v', '--verbose', action='store_true', help='Show detailed stats for all commits') + check_cmd.add_argument('-d', '--diff', action='store_true', + help='Show source code diff for problem commits') check_gl = subparsers.add_parser('check-gitlab', help='Check GitLab permissions') @@ -148,6 +150,8 @@ def parse_args(argv): Namespace: Parsed arguments """ parser = argparse.ArgumentParser(description='Check commit differences') + parser.add_argument('--no-colour', action='store_true', + help='Disable colour output') subparsers = parser.add_subparsers(dest='cmd', required=True) add_main_commands(subparsers) diff --git a/tools/pickman/control.py b/tools/pickman/control.py index 648fd66947a..ac9b57e10e3 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -11,6 +11,7 @@ from datetime import date import os import re import sys +import tempfile import time import unittest @@ -464,12 +465,87 @@ def get_branch_commits(): return current_branch, commits +def show_commit_diff(res, no_colour=False): + """Show the difference between original and cherry-picked commit patches + + Args: + res (CheckResult): Check result with commit hashes + no_colour (bool): Disable colour output + """ + tout.info(f'\n--- Patch diff between original {res.orig_hash[:8]} and ' + f'cherry-picked {res.chash[:8]} ---') + + # Get the patch content of each commit + orig_patch = run_git(['show', '--no-ext-diff', res.orig_hash]) + cherry_patch = run_git(['show', '--no-ext-diff', res.chash]) + + # Create temporary files and diff them + with tempfile.NamedTemporaryFile(mode='w', suffix='_orig.patch', + delete=False) as orig_file: + orig_file.write(orig_patch) + orig_path = orig_file.name + + with tempfile.NamedTemporaryFile(mode='w', suffix='_cherry.patch', + delete=False) as cherry_file: + cherry_file.write(cherry_patch) + cherry_path = cherry_file.name + + try: + # Diff the two patch files using system diff + diff_args = ['diff', '-u'] + if not no_colour: + diff_args.append('--color=always') + diff_args.extend([orig_path, cherry_path]) + + patch_diff = command.output(*diff_args, raise_on_error=False) + if patch_diff: + print(patch_diff) + else: + tout.info('(Patches are identical)') + finally: + # Clean up temporary files + os.unlink(orig_path) + os.unlink(cherry_path) + + tout.info('--- End patch diff ---\n') + + +def show_check_summary(bad, verbose, threshold, show_diff, no_colour): + """Show summary of check results + + Args: + bad (list): List of CheckResult objects with problems + verbose (bool): Whether to show verbose output + threshold (float): Delta threshold for problems + show_diff (bool): Whether to show diffs for problems + no_colour (bool): Whether to disable colour in diffs + + Returns: + int: 0 if no problems, 1 if problems found + """ + if bad: + if verbose: + tout.info(f'Found {len(bad)} commit(s) with large deltas:') + tout.info('') + print_check_header() + for res in bad: + tout.info(format_problem_commit(res, threshold)) + if show_diff: + show_commit_diff(res, no_colour) + else: + tout.info(f'{len(bad)} problem commit(s) found') + return 1 + if verbose: + tout.info('All cherry-picks have acceptable deltas ✓') + return 0 + + def do_check(args, dbs): # pylint: disable=unused-argument """Check current branch for cherry-picks with large deltas Args: args (Namespace): Parsed arguments with 'threshold', 'min_lines', - and 'verbose' attributes + 'verbose', and 'diff' attributes dbs (Database): Database instance Returns: @@ -478,6 +554,7 @@ def do_check(args, dbs): # pylint: disable=unused-argument threshold = args.threshold min_lines = args.min_lines verbose = args.verbose + show_diff = args.diff current_branch, commits = get_branch_commits() @@ -493,33 +570,24 @@ def do_check(args, dbs): # pylint: disable=unused-argument # Process commits using the generator for res in check_commits(commits, min_lines): + is_problem = not res.reason and res.delta_ratio > threshold + if verbose: check_verbose(res, threshold) - if not res.reason and res.delta_ratio > threshold: - bad.append(res) - else: + elif is_problem: # Non-verbose: only show problems on one line - if not res.reason and res.delta_ratio > threshold: - if not header_printed: - print_check_header() - header_printed = True - tout.info(format_problem_commit(res, threshold)) - bad.append(res) - - # Summary - if bad: - if verbose: - tout.info(f'Found {len(bad)} commit(s) with large deltas:') - tout.info('') - print_check_header() - for res in bad: - tout.info(format_problem_commit(res, threshold)) - else: - tout.info(f'{len(bad)} problem commit(s) found') - return 1 - if verbose: - tout.info('All cherry-picks have acceptable deltas ✓') - return 0 + if not header_printed: + print_check_header() + header_printed = True + tout.info(format_problem_commit(res, threshold)) + + if is_problem: + bad.append(res) + if show_diff: + show_commit_diff(res, args.no_colour) + + return show_check_summary(bad, verbose, threshold, show_diff, + args.no_colour) def do_check_gitlab(args, dbs): # pylint: disable=unused-argument diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 2b829e4c75a..5d88e65c7cb 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -3438,6 +3438,99 @@ This is a normal commit without cherry-pick info. result = results[0] self.assertEqual(result.reason, 'small_commit_3_lines') + @mock.patch('pickman.control.command') + @mock.patch('pickman.control.run_git') + @mock.patch('tempfile.NamedTemporaryFile') + @mock.patch('os.unlink') + def test_show_commit_diff_with_colour(self, unused_unlink, mock_temp, + mock_run_git, mock_command): + """Test show_commit_diff with colour enabled.""" + # Mock temporary files + mock_temp.side_effect = [ + mock.mock_open()(), # orig file + mock.mock_open()() # cherry file + ] + mock_temp.return_value.__enter__.return_value.name = '/tmp/test.patch' + + # Mock git show outputs + mock_run_git.side_effect = [ + 'orig patch content', + 'cherry patch content' + ] + + # Mock diff command output + mock_command.output.return_value = 'diff output' + + # Test data + res = control.CheckResult( + chash='abc123', + orig_hash='def456', + subject='Test', + delta_ratio=0.5, + orig_stats=None, + cherry_stats=None, + reason=None + ) + + with terminal.capture(): + control.show_commit_diff(res, no_colour=False) + + # Verify git show was called for both commits + expected_calls = [ + mock.call(['show', '--no-ext-diff', 'def456']), + mock.call(['show', '--no-ext-diff', 'abc123']) + ] + mock_run_git.assert_has_calls(expected_calls) + + # Verify diff was called with colour + mock_command.output.assert_called_once() + args, kwargs = mock_command.output.call_args + self.assertIn('--color=always', args) + self.assertEqual(kwargs.get('raise_on_error'), False) + + @mock.patch('pickman.control.command') + @mock.patch('pickman.control.run_git') + @mock.patch('tempfile.NamedTemporaryFile') + @mock.patch('os.unlink') + def test_show_commit_diff_no_colour(self, unused_unlink, mock_temp, + mock_run_git, mock_command): + """Test show_commit_diff with colour disabled.""" + # Mock temporary files + mock_temp.side_effect = [ + mock.mock_open()(), # orig file + mock.mock_open()() # cherry file + ] + mock_temp.return_value.__enter__.return_value.name = '/tmp/test.patch' + + # Mock git show outputs + mock_run_git.side_effect = [ + 'orig patch content', + 'cherry patch content' + ] + + # Mock diff command output + mock_command.output.return_value = 'diff output' + + # Test data + res = control.CheckResult( + chash='abc123', + orig_hash='def456', + subject='Test', + delta_ratio=0.5, + orig_stats=None, + cherry_stats=None, + reason=None + ) + + with terminal.capture(): + control.show_commit_diff(res, no_colour=True) + + # Verify diff was called without colour + mock_command.output.assert_called_once() + args, kwargs = mock_command.output.call_args + self.assertNotIn('--color=always', args) + self.assertEqual(kwargs.get('raise_on_error'), False) + if __name__ == '__main__': unittest.main() -- 2.43.0

From: Simon Glass <simon.glass@canonical.com> When a cherry pick has already been completed this can confuse pickman and make it hard for a reviewer to figure out what is going on. Attempt to detect already-applied commits (by commit subject) as a way to reduce duplicate cherry-picks and improve the reliability of the automation. The agent now compares actual patch content using git show and diff, only skipping commits that are similar with minor differences like line numbers or conflict resolutions. This should reduces false positives while maintaining robust duplicate detection. Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/README.rst | 29 ++++++++---- tools/pickman/agent.py | 62 +++++++++++++++++++------- tools/pickman/control.py | 73 +++++++++++++++++++++++++++--- tools/pickman/ftest.py | 96 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 230 insertions(+), 30 deletions(-) diff --git a/tools/pickman/README.rst b/tools/pickman/README.rst index cc1ce5f81d5..857edb09f85 100644 --- a/tools/pickman/README.rst +++ b/tools/pickman/README.rst @@ -129,18 +129,29 @@ Already-Applied Detection Sometimes commits have already been applied to the target branch through a different path (e.g., directly merged or cherry-picked with different hashes). -Pickman's Claude agent detects this situation automatically. +Pickman detects this situation automatically in two ways. -**How it works** +**Pre-Cherry-Pick Detection** -When the first cherry-pick fails with conflicts, the agent checks if the -commits are already present in the target branch by searching for matching -commit subjects:: +Before starting cherry-picks, pickman checks for potentially already-applied +commits by searching for matching commit subjects in the target branch:: git log --oneline ci/master --grep="<subject>" -1 -If all commits in the set are found (same subjects, different hashes), the -agent: +Commits that match are marked as "maybe already applied" and passed to the +Claude agent with the hash of the potentially matching commit. The agent then: + +1. Compares the actual patch content between the original and found commits +2. Uses ``git show`` and ``diff`` to analyze the changes +3. Skips commits that are similar with only minor differences (line numbers, + context, conflict resolutions) +4. Proceeds with commits that differ significantly in actual changes + +**Fallback Detection** + +If pre-detection missed something and the first cherry-pick fails with +conflicts, the agent performs the same subject search and patch comparison +process. If all commits in the set are verified as already applied, the agent: 1. Aborts the cherry-pick 2. Writes a signal file (``.pickman-signal``) with status ``already_applied`` @@ -148,7 +159,8 @@ agent: **What pickman does** -When pickman detects the ``already_applied`` signal: +When pickman detects the ``already_applied`` signal or when the agent reports +pre-detected applied commits: 1. Marks all commits as 'skipped' in the database 2. Updates the source position to advance past these commits @@ -161,6 +173,7 @@ This ensures: - The source position advances so the next ``poll`` iteration processes new commits - No manual intervention is required to continue +- False positives are minimized by comparing actual patch content CI Pipelines ------------ diff --git a/tools/pickman/agent.py b/tools/pickman/agent.py index 1d5a3df2442..b081c36b504 100644 --- a/tools/pickman/agent.py +++ b/tools/pickman/agent.py @@ -45,12 +45,12 @@ def check_available(): return True -async def run(commits, source, branch_name, repo_path=None): +async def run(commits, source, branch_name, repo_path=None): # pylint: disable=too-many-locals """Run the Claude agent to cherry-pick commits Args: commits (list): list of AgentCommit namedtuples with fields: - hash, chash, subject + hash, chash, subject, applied_as source (str): source branch name branch_name (str): name for the new branch to create repo_path (str): path to repository (defaults to current directory) @@ -69,18 +69,42 @@ async def run(commits, source, branch_name, repo_path=None): if os.path.exists(signal_path): os.remove(signal_path) - # Build commit list for the prompt - commit_list = '\n'.join( - f' - {commit.chash}: {commit.subject}' - for commit in commits - ) + # Build commit list for the prompt, marking potentially applied commits + commit_entries = [] + has_applied = False + for commit in commits: + entry = f' - {commit.chash}: {commit.subject}' + if commit.applied_as: + entry += f' (maybe already applied as {commit.applied_as})' + has_applied = True + commit_entries.append(entry) + + commit_list = '\n'.join(commit_entries) + + # Add note about checking for already applied commits + applied_note = '' + if has_applied: + applied_note = ''' + +IMPORTANT: Some commits may already be applied. Before cherry-picking commits +marked as "maybe already applied as <hash>", verify they are truly the same commit: +1. Compare the actual changes between the original and found commits: + - Use: git show --no-ext-diff <original-hash> > /tmp/orig.patch + - Use: git show --no-ext-diff <found-hash> > /tmp/found.patch + - Compare: diff /tmp/orig.patch /tmp/found.patch +2. If the patches are similar with only minor differences (like line numbers, + context, or conflict resolutions), SKIP the cherry-pick and report that + it was already applied. +3. If the patches differ significantly in the actual changes being made, + proceed with the cherry-pick as they are different commits. +''' # Get full hash of last commit for signal file last_commit_hash = commits[-1].hash prompt = f"""Cherry-pick the following commits from {source} branch: -{commit_list} +{commit_list}{applied_note} Steps to follow: 1. First run 'git status' to check the repository state is clean @@ -113,10 +137,11 @@ Steps to follow: - For complex conflicts, describe what needs manual resolution and abort - When fix-ups are needed, amend the commit to add a one-line note at the end of the commit message describing the changes made -6. After ALL cherry-picks complete, verify with +6. After ALL cherry-picks complete, verify with 'git log --oneline -n {len(commits) + 2}' Ensure all {len(commits)} commits are present. -7. Run final build: 'buildman -L --board sandbox -w -o /tmp/pickman' to verify everything still works together +7. Run final build: 'buildman -L --board sandbox -w -o /tmp/pickman' to verify + everything still works together 8. Report the final status including: - Build result (ok or list of warnings/errors) - Any fix-ups that were made @@ -132,11 +157,16 @@ Important: CRITICAL - Detecting Already-Applied Commits: If the FIRST cherry-pick fails with conflicts, BEFORE aborting, check if the commits -are already present in ci/master with different hashes. Do this by searching for -commit subjects in ci/master: - git log --oneline ci/master --grep="<subject>" -1 -If ALL commits are already in ci/master (same subjects, different hashes), this means -the series was already applied via a different path. In this case: +are already present in ci/master with different hashes. For each commit: +1. Search for the commit subject: git log --oneline ci/master --grep="<subject>" -1 +2. If found, verify it's actually the same commit by comparing patches: + - git show --no-ext-diff <original-hash> > /tmp/orig.patch + - git show --no-ext-diff <found-hash> > /tmp/found.patch + - diff /tmp/orig.patch /tmp/found.patch +3. Only consider it "already applied" if the patches are similar with minor + differences (like line numbers, context, or conflict resolutions) +If ALL commits are verified as already applied (same content, different hashes), +this means the series was already applied via a different path. In this case: 1. Abort the cherry-pick: git cherry-pick --abort 2. Delete the branch: git branch -D {branch_name} 3. Write a signal file to indicate this status: @@ -206,7 +236,7 @@ def cherry_pick_commits(commits, source, branch_name, repo_path=None): Args: commits (list): list of AgentCommit namedtuples with fields: - hash, chash, subject + hash, chash, subject, applied_as source (str): source branch name branch_name (str): name for the new branch to create repo_path (str): path to repository (defaults to current directory) diff --git a/tools/pickman/control.py b/tools/pickman/control.py index 4a993c72b88..3ae085d3507 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -73,7 +73,7 @@ CheckResult = namedtuple('CheckResult', [ # Named tuple for commit with author # hash: Full SHA-1 commit hash (40 characters) -# chash: Abbreviated commit hash (typically 7-8 characters) +# chash: Abbreviated commit hash (typically 7-8 characters) # subject: First line of commit message (commit subject) # author: Commit author name and email in format "Name <email>" CommitInfo = namedtuple('CommitInfo', @@ -83,7 +83,9 @@ CommitInfo = namedtuple('CommitInfo', # hash: Full SHA-1 commit hash (40 characters) # chash: Abbreviated commit hash (typically 7-8 characters) # subject: First line of commit message (commit subject) -AgentCommit = namedtuple('AgentCommit', ['hash', 'chash', 'subject']) +# applied_as: Short hash if potentially already applied, None otherwise +AgentCommit = namedtuple('AgentCommit', + ['hash', 'chash', 'subject', 'applied_as']) # Named tuple for prepare_apply result ApplyInfo = namedtuple('ApplyInfo', @@ -475,6 +477,43 @@ def get_branch_commits(): return current_branch, commits +def check_already_applied(commits, target_branch='ci/master'): + """Check which commits are already applied to the target branch + + Args: + commits (list): List of CommitInfo tuples to check + target_branch (str): Branch to check against (default: ci/master) + + Returns: + tuple: (new_commits, applied) where: + new_commits: list of CommitInfo for commits not yet applied + applied: list of CommitInfo for commits already applied + """ + new_commits = [] + applied = [] + + for commit in commits: + # Check if a commit with the same subject exists in target branch + try: + # Use git log with --grep to search for the subject + # Escape any special characters in the subject for grep + escaped_subject = commit.subject.replace('"', '\\"') + result = run_git(['log', '--oneline', target_branch, + f'--grep={escaped_subject}', '-1']) + if result.strip(): + # Found a commit with the same subject + applied.append(commit) + tout.info(f'Skipping {commit.chash} (already applied): ' + f'{commit.subject}') + else: + new_commits.append(commit) + except Exception: # pylint: disable=broad-except + # If grep fails, assume the commit is not applied + new_commits.append(commit) + + return new_commits, applied + + def show_commit_diff(res, no_colour=False): """Show the difference between original and cherry-picked commit patches @@ -522,7 +561,7 @@ def show_commit_diff(res, no_colour=False): def show_check_summary(bad, verbose, threshold, show_diff, no_colour): """Show summary of check results - + Args: bad (list): List of CheckResult objects with problems verbose (bool): Whether to show verbose output @@ -1216,7 +1255,23 @@ def execute_apply(dbs, source, commits, branch_name, args): # pylint: disable=t tuple: (ret, success, conv_log) where ret is 0 on success, 1 on failure """ - # Add commits to database with 'pending' status + # Check for already applied commits before proceeding + _, applied = check_already_applied(commits) + + # Build mapping of applied commits by hash + applied_map = {} + if applied: + for c in applied: + # Get the hash of the applied commit in target branch + escaped_subject = c.subject.replace('"', '\\"') + result = run_git(['log', '--oneline', 'ci/master', + f'--grep={escaped_subject}', '-1']) + if result.strip(): + applied_hash = result.split()[0] + applied_map[c.hash] = applied_hash + tout.info(f'Found {len(applied)} potentially already applied commit(s)') + + # Add all commits to database with 'pending' status (agent updates later) source_id = dbs.source_get_id(source) for commit in commits: dbs.commit_add(commit.hash, source_id, commit.subject, commit.author, @@ -1224,9 +1279,10 @@ def execute_apply(dbs, source, commits, branch_name, args): # pylint: disable=t dbs.commit() # Convert CommitInfo to AgentCommit format expected by agent - agent_commits = [AgentCommit(c.hash, c.chash, c.subject) for c in commits] + agent_commits = [AgentCommit(c.hash, c.chash, c.subject, + applied_map.get(c.hash)) for c in commits] success, conv_log = agent.cherry_pick_commits(agent_commits, source, - branch_name) + branch_name) # Check for signal file from agent signal_status, signal_commit = agent.read_signal_file() @@ -1273,6 +1329,11 @@ def execute_apply(dbs, source, commits, branch_name, args): # pylint: disable=t tout.info(f"Use 'pickman commit-source {source} " f"{commits[-1].chash}' to update the database") + # Update database with the last processed commit if successful + if success: + dbs.source_set(source, commits[-1].hash) + dbs.commit() + return ret, success, conv_log diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 5d88e65c7cb..d6f9e537a04 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -3532,5 +3532,101 @@ This is a normal commit without cherry-pick info. self.assertEqual(kwargs.get('raise_on_error'), False) +class TestCheckAlreadyApplied(unittest.TestCase): + """Tests for the check_already_applied function.""" + + def setUp(self): + """Set up test data.""" + self.commits = [ + control.CommitInfo('abc123def456', 'abc123d', 'Add new feature', + 'Author <email>'), + control.CommitInfo('def456abc123', 'def456a', 'Fix bug', + 'Author <email>') + ] + self.quoted_commit = [ + control.CommitInfo('abc123def456', 'abc123d', + 'Add "quoted" feature', 'Author <email>') + ] + self.single_commit = [ + control.CommitInfo('abc123def456', 'abc123d', 'Add new feature', + 'Author <email>') + ] + + @mock.patch('pickman.control.run_git') + @mock.patch('pickman.control.tout') + def test_check_already_applied_none_applied(self, mock_tout, mock_run_git): + """Test check_already_applied when no commits are already applied.""" + # Mock git log returning empty (no matches) + mock_run_git.return_value = '' + + new_commits, applied = control.check_already_applied(self.commits) + + self.assertEqual(len(new_commits), 2) + self.assertEqual(len(applied), 0) + self.assertEqual(new_commits, self.commits) + mock_tout.info.assert_not_called() + + @mock.patch('pickman.control.run_git') + @mock.patch('pickman.control.tout') + def test_check_already_applied_some_applied(self, mock_tout, mock_run_git): + """Test check_already_applied when some commits are already applied.""" + # First commit returns a match, second doesn't + mock_run_git.side_effect = ['xyz789 Add new feature', ''] + + new_commits, applied = control.check_already_applied(self.commits) + + self.assertEqual(len(new_commits), 1) + self.assertEqual(len(applied), 1) + self.assertEqual(new_commits[0].hash, 'def456abc123') + self.assertEqual(applied[0].hash, 'abc123def456') + mock_tout.info.assert_called_once() + + @mock.patch('pickman.control.run_git') + @mock.patch('pickman.control.tout') + def test_check_already_applied_all_applied(self, mock_tout, mock_run_git): + """Test check_already_applied when all commits are already applied.""" + # Both commits return matches + mock_run_git.side_effect = ['xyz789 Add new feature', 'uvw123 Fix bug'] + + new_commits, applied = control.check_already_applied(self.commits) + + self.assertEqual(len(new_commits), 0) + self.assertEqual(len(applied), 2) + self.assertEqual(applied, self.commits) + self.assertEqual(mock_tout.info.call_count, 2) + + @mock.patch('pickman.control.run_git') + @mock.patch('pickman.control.tout') + def test_check_already_applied_with_quotes_in_subject( + self, unused_mock_tout, mock_run_git): + """Test check_already_applied handles quotes in commit subjects.""" + mock_run_git.return_value = '' + + new_commits, applied = control.check_already_applied(self.quoted_commit) + + # Verify git was called with escaped quotes + mock_run_git.assert_called_once_with([ + 'log', '--oneline', 'ci/master', + '--grep=Add \\"quoted\\" feature', '-1' + ]) + self.assertEqual(len(new_commits), 1) + self.assertEqual(len(applied), 0) + + @mock.patch('pickman.control.run_git') + @mock.patch('pickman.control.tout') + def test_check_already_applied_git_error(self, unused_mock_tout, + mock_run_git): + """Test check_already_applied handles git errors gracefully.""" + # Mock git command raising an exception + mock_run_git.side_effect = Exception('Git error') + + new_commits, applied = control.check_already_applied(self.single_commit) + + # Should treat as not applied when git fails + self.assertEqual(len(new_commits), 1) + self.assertEqual(len(applied), 0) + self.assertEqual(new_commits, self.single_commit) + + if __name__ == '__main__': unittest.main() -- 2.43.0