[PATCH 0/9] pickman: Provide better ways to check cherry-picks
From: Simon Glass <simon.glass@canonical.com> After a few weeks of using this tool a couple of things have come to light, mostly in a recent attempt to cherry-pick ~260 commits. Firstly, it is hard to manually check a cherry-pick against the original. The GitLab GUI allows you to click on both, but it does not give a sense of the detail. Secondly, commits have sometimes already been applied to the tree, but in this case effort is still paid to cherry pick it, sometimes with unfortunate results. Thirdly, building once at the end does not catch every problems. In the case in question, about five commits were mangled, three of which would have been caught by doing a build. This series introduces a new 'check' command which provides a simple check of the diff delta between an original commit and its cherry pick., thus providing a list of suspect cherry-picks, e.g. (omitting some long lines): Cherry-pick Delta% Original Subject ----------- ------ ---------- ------- aaea489b2a 100 9bab7d2a7c net: wget: let wget_with_dns work with e557daec17 100 f0315babfb hash: Plumb crc8 into the hash functions 08a86f1769 40 6acada5daa configs: j7*: Enable TI_COMMON_CMD_OPTIONS de37f6abb6 100 fc37a73e66 fdt: Swap the signature for d1437b065a 100 e2cc9b4fc1 tools: binman: add 'fit, encrypt' property 09b800b0df 100 12d7be498a Docker/CI: Only test Xtensa on amd64 hosts 0256d8140c 100 ece1631f5e test/cmd/wget: replace bogus response with e005799d8f 33 15e0c5e390 lmb: Remove lmb_alloc_addr_flags() eaba5aae8f 79 99afa58e6d sandbox: Correct guard around readq/writeq c347fb4b1a 41 99145eec2d x86: select CONFIG_64BIT for X86_64 14192a60e0 89 60a684e0a9 trace: add support for 'trace wipe' 8f58cfe76d 66 905204ddcf test: test_trace.py: test 'trace wipe' 12 problem commit(s) found The -d option shows a diff of the patch diffs, which can aid a fast review. This series also includes some updates to the agent prompt to build after each commit, some refactoring and tidy-ups to remove pylint warnings and a way to tell the agent about possible already-applied commits. Further refinements will likely be needed as time goes on. Simon Glass (9): pickman: Fix 80-column line length compliance pickman: Add URL constants to improve test readability pickman: Improve function names and line-length compliance pickman: Disable pylint too-many-positional-arguments warnings pickman: Enhance agent prompts with per-commit validation pickman: Add a check command for cherry-pick delta analysis pickman: Add an option to show patch diffs pickman: Use named tuples for better code clarity pickman: Add already-applied commit detection / comparison tools/pickman/README.rst | 61 ++- tools/pickman/__main__.py | 73 +++- tools/pickman/agent.py | 105 +++-- tools/pickman/control.py | 596 +++++++++++++++++++++++++++-- tools/pickman/database.py | 11 +- tools/pickman/ftest.py | 744 ++++++++++++++++++++++++++++-------- tools/pickman/gitlab_api.py | 17 +- 7 files changed, 1341 insertions(+), 266 deletions(-) -- 2.43.0 base-commit: 34b50a4e155de4b6cc079cfcbf9980ec50a0888b branch: picka
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> Replace long, hardcoded URLs in tests with named constants defined at the top of the file. This improves code maintainability by providing a single point of change for test URLs and helps with line-length violations. Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/ftest.py | 48 ++++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 9d075586c76..5f126984c9e 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -30,6 +30,15 @@ from pickman import control from pickman import database from pickman import gitlab_api +# Test URL constants +TEST_OAUTH_URL = 'https://oauth2:test-token@gitlab.com/group/project.git' +TEST_HTTPS_URL = 'https://gitlab.com/group/project.git' +TEST_SSH_URL = 'git@gitlab.com:group/project.git' +TEST_MR_URL = 'https://gitlab.com/group/project/-/merge_requests/42' +TEST_MR_42_URL = 'https://gitlab.com/mr/42' +TEST_MR_1_URL = 'https://gitlab.com/mr/1' +TEST_SHORT_OAUTH_URL = 'https://oauth2:token@gitlab.com/g/p.git' + class TestCommit(unittest.TestCase): """Tests for the Commit namedtuple.""" @@ -230,7 +239,8 @@ class TestMain(unittest.TestCase): self.assertEqual(ret, 0) # Filter out database migration messages output_lines = [l for l in stdout.getvalue().splitlines() - if not l.startswith(('Update database', 'Creating'))] + if not l.startswith(('Update database', + 'Creating'))] lines = iter(output_lines) self.assertEqual('Commits in us/next not in ci/master: 10', next(lines)) @@ -562,7 +572,7 @@ class TestDatabaseMergereq(unittest.TestCase): # Add a merge request dbs.mergereq_add(source_id, 'cherry-abc123', 42, 'open', - 'https://gitlab.com/mr/42', '2025-01-15') + TEST_MR_42_URL, '2025-01-15') dbs.commit() # Get the merge request @@ -572,7 +582,7 @@ class TestDatabaseMergereq(unittest.TestCase): self.assertEqual(result[2], 'cherry-abc123') # branch_name self.assertEqual(result[3], 42) # mr_id self.assertEqual(result[4], 'open') # status - self.assertEqual(result[5], 'https://gitlab.com/mr/42') # url + self.assertEqual(result[5], TEST_MR_42_URL) # url self.assertEqual(result[6], '2025-01-15') # created_at dbs.close() @@ -598,7 +608,7 @@ class TestDatabaseMergereq(unittest.TestCase): # Add merge requests dbs.mergereq_add(source_id, 'branch-1', 1, 'open', - 'https://gitlab.com/mr/1', '2025-01-01') + TEST_MR_1_URL, '2025-01-01') dbs.mergereq_add(source_id, 'branch-2', 2, 'merged', 'https://gitlab.com/mr/2', '2025-01-02') dbs.mergereq_add(source_id, 'branch-3', 3, 'open', @@ -630,7 +640,7 @@ class TestDatabaseMergereq(unittest.TestCase): source_id = dbs.source_get_id('us/next') dbs.mergereq_add(source_id, 'branch-1', 42, 'open', - 'https://gitlab.com/mr/42', '2025-01-15') + TEST_MR_42_URL, '2025-01-15') dbs.commit() # Update status @@ -671,7 +681,7 @@ class TestDatabaseCommitMergereq(unittest.TestCase): # Add merge request dbs.mergereq_add(source_id, 'branch-1', 42, 'open', - 'https://gitlab.com/mr/42', '2025-01-15') + TEST_MR_42_URL, '2025-01-15') dbs.commit() mr = dbs.mergereq_get(42) mr_id = mr[0] # id field @@ -701,7 +711,7 @@ class TestDatabaseCommitMergereq(unittest.TestCase): # Add merge request dbs.mergereq_add(source_id, 'branch-1', 42, 'open', - 'https://gitlab.com/mr/42', '2025-01-15') + TEST_MR_42_URL, '2025-01-15') dbs.commit() mr = dbs.mergereq_get(42) mr_id = mr[0] @@ -1438,10 +1448,11 @@ class TestGetPushUrl(unittest.TestCase): """Test successful push URL generation.""" with mock.patch.object(gitlab_api, 'get_token', return_value='test-token'): - with mock.patch.object(gitlab_api, 'get_remote_url', - return_value='git@gitlab.com:group/project.git'): + with mock.patch.object( + gitlab_api, 'get_remote_url', + return_value=TEST_SSH_URL): url = gitlab_api.get_push_url('origin') - self.assertEqual(url, 'https://oauth2:test-token@gitlab.com/group/project.git') + self.assertEqual(url, TEST_OAUTH_URL) def test_get_push_url_no_token(self): """Test returns None when no token available.""" @@ -1463,9 +1474,9 @@ class TestGetPushUrl(unittest.TestCase): with mock.patch.object(gitlab_api, 'get_token', return_value='test-token'): with mock.patch.object(gitlab_api, 'get_remote_url', - return_value='https://gitlab.com/group/project.git'): + return_value=TEST_HTTPS_URL): url = gitlab_api.get_push_url('origin') - self.assertEqual(url, 'https://oauth2:test-token@gitlab.com/group/project.git') + self.assertEqual(url, TEST_OAUTH_URL) class TestPushBranch(unittest.TestCase): @@ -1474,7 +1485,7 @@ 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', - return_value='https://oauth2:token@gitlab.com/g/p.git'): + 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) @@ -1482,14 +1493,15 @@ class TestPushBranch(unittest.TestCase): # Should fetch first, then push with --force-with-lease calls = mock_output.call_args_list self.assertEqual(len(calls), 2) - self.assertEqual(calls[0], mock.call('git', 'fetch', 'ci', 'test-branch')) + 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) 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', - return_value='https://oauth2:token@gitlab.com/g/p.git'): + return_value=TEST_SHORT_OAUTH_URL): with mock.patch.object(command, 'output') as mock_output: # Fetch fails (branch doesn't exist on remote) mock_output.side_effect = [ @@ -1510,7 +1522,7 @@ 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', - return_value='https://oauth2:token@gitlab.com/g/p.git'): + 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) @@ -1682,7 +1694,7 @@ class TestGetPickmanMrs(unittest.TestCase): mock_mr1 = mock.MagicMock() mock_mr1.iid = 1 mock_mr1.title = '[pickman] Older MR' - mock_mr1.web_url = 'https://gitlab.com/mr/1' + mock_mr1.web_url = TEST_MR_1_URL mock_mr1.source_branch = 'cherry-1' mock_mr1.description = 'desc1' mock_mr1.has_conflicts = False @@ -1731,7 +1743,7 @@ class TestCreateMr(unittest.TestCase): # Create mock existing MR mock_existing_mr = mock.MagicMock() - mock_existing_mr.web_url = 'https://gitlab.com/group/project/-/merge_requests/42' + mock_existing_mr.web_url = TEST_MR_URL mock_project = mock.MagicMock() mock_project.mergerequests.list.return_value = [mock_existing_mr] -- 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 too-many-positional-arguments to existing pylint-disable comments for functions with many parameters: - agent.py: 4 functions updated - control.py: 1 function updated - database.py: 2 functions updated - gitlab_api.py: 1 function updated These functions require many parameters for their interface contracts and the warnings don't add value. Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> -- 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> Some cherry-picks end up with large deltas compared to their original commits, indicating potential problems. Add a check command to identify these commits for review. The check command: - Analyses all commits on current branch vs ci/master - Compares original vs cherry-picked commit statistics - Shows commits with deltas above threshold (default 20%) - Supports verbose mode with detailed analysis - Color codes results: red ≥50%, yellow ≥threshold - Skips merge commits and small commits (default <10 lines) Also enhance the cherry-pick agent to: - Check delta after each commit and build validation - Attempt recovery for large deltas before continuing - Build each commit individually for early problem detection Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/README.rst | 30 +++ tools/pickman/__main__.py | 47 ++++- tools/pickman/control.py | 421 +++++++++++++++++++++++++++++++++++--- tools/pickman/ftest.py | 231 ++++++++++++++++++++- 4 files changed, 687 insertions(+), 42 deletions(-) diff --git a/tools/pickman/README.rst b/tools/pickman/README.rst index 37c3aac038f..3b280e718d3 100644 --- a/tools/pickman/README.rst +++ b/tools/pickman/README.rst @@ -210,6 +210,36 @@ This shows: master branch (ci/master) - The last common commit between the two branches +To check current branch for problematic cherry-picks:: + + ./tools/pickman/pickman check + +This analyzes commits on the current branch and identifies cherry-picks with +large deltas compared to their original commits. By default, it: + +- Shows only problematic commits (above 20% delta threshold) +- Ignores small commits (less than 10 lines changed) +- Skips merge commits (which have different delta characteristics) +- Uses color coding: red for ≥80% delta, yellow for ≥50% delta + +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 + +Example output:: + + Cherry-pick Delta% Original Subject + ----------- ------ ---------- ------- + aaea489b2a 100 9bab7d2a7c net: wget: let wget_with_dns work with dns disabled + e557daec17 89 f0315babfb hash: Plumb crc8 into the hash functions + + 2 problem commit(s) found + +This helps identify cherry-picks that may have been applied incorrectly or +need manual review due to significant differences from the original commits. + To check GitLab permissions for the configured token:: ./tools/pickman/pickman check-gitlab diff --git a/tools/pickman/__main__.py b/tools/pickman/__main__.py index d11734f1f25..87077d1d10a 100755 --- a/tools/pickman/__main__.py +++ b/tools/pickman/__main__.py @@ -21,18 +21,12 @@ from pickman import ftest from u_boot_pylib import test_util -def parse_args(argv): - """Parse command line arguments. +def add_main_commands(subparsers): + """Add main pickman commands to the argument parser Args: - argv (list): Command line arguments - - Returns: - Namespace: Parsed arguments + subparsers (ArgumentParser): ArgumentParser subparsers object """ - parser = argparse.ArgumentParser(description='Check commit differences') - subparsers = parser.add_subparsers(dest='cmd', required=True) - add_source = subparsers.add_parser('add-source', help='Add a source branch to track') add_source.add_argument('source', help='Source branch name') @@ -48,6 +42,18 @@ def parse_args(argv): apply_cmd.add_argument('-t', '--target', default='master', help='Target branch for MR (default: master)') + check_cmd = subparsers.add_parser( + 'check', + help='Check current branch for cherry-picks with large deltas') + check_cmd.add_argument('-t', '--threshold', type=float, default=0.2, + help='Delta threshold as fraction (default: 0.2 = ' + '20%%)') + check_cmd.add_argument('-m', '--min-lines', type=int, default=10, + help='Minimum lines changed to check delta ' + '(default: 10)') + check_cmd.add_argument('-v', '--verbose', action='store_true', + help='Show detailed stats for all commits') + check_gl = subparsers.add_parser('check-gitlab', help='Check GitLab permissions') check_gl.add_argument('-r', '--remote', default='ci', @@ -114,6 +120,13 @@ def parse_args(argv): push_cmd.add_argument('--run-ci', action='store_true', help='Run CI pipeline (default: skip for new MRs)') + +def add_test_commands(subparsers): + """Add test-related commands to the argument parser + + Args: + subparsers (ArgumentParser): ArgumentParser subparsers object + """ test_cmd = subparsers.add_parser('test', help='Run tests') test_cmd.add_argument('-P', '--processes', type=int, help='Number of processes to run tests ' @@ -124,6 +137,22 @@ def parse_args(argv): help='Verbosity level (0-4, default: 1)') test_cmd.add_argument('tests', nargs='*', help='Specific tests to run') + +def parse_args(argv): + """Parse command line arguments. + + Args: + argv (list): Command line arguments + + Returns: + Namespace: Parsed arguments + """ + parser = argparse.ArgumentParser(description='Check commit differences') + subparsers = parser.add_subparsers(dest='cmd', required=True) + + add_main_commands(subparsers) + add_test_commands(subparsers) + return parser.parse_args(argv) diff --git a/tools/pickman/control.py b/tools/pickman/control.py index a6789b930ba..648fd66947a 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -24,6 +24,7 @@ from pickman import database from pickman import ftest from pickman import gitlab_api from u_boot_pylib import command +from u_boot_pylib import terminal from u_boot_pylib import tout # Default database filename @@ -33,12 +34,45 @@ DB_FNAME = '.pickman.db' BRANCH_MASTER = 'ci/master' BRANCH_SOURCE = 'us/next' +# Git stat output parsing patterns +RE_GIT_STAT_SUMMARY = re.compile( + r'(\d+)\s+files?\s+changed' + r'(?:,\s*(\d+)\s+insertions?\([+]\))?' + r'(?:,\s*(\d+)\s+deletions?\([-]\))?' +) +RE_GIT_STAT_FILE = re.compile(r'^([^|]+)\s*\|') + +# Extract hash from line like "(cherry picked from commit abc123def)" +RE_CHERRY_PICK = re.compile(r'cherry picked from commit ([a-f0-9]+)') + # Named tuple for commit info -Commit = namedtuple('Commit', ['hash', 'short_hash', 'subject', 'date']) +Commit = namedtuple('Commit', ['hash', 'chash', 'subject', 'date']) + +# Named tuple for git stat output +# files: Number of files changed +# inserted: Number of lines inserted +# deleted: Number of lines deleted +# file_set: Set of modified file paths +GitStat = namedtuple('GitStat', ['files', 'inserted', 'deleted', 'file_set']) + +# Named tuple for check results +# chash: Cherry-pick commit hash (full) +# orig_hash: Original commit hash that was cherry-picked +# subject: Commit subject line +# delta_ratio: Ratio of differences between original and cherry-pick +# (0.0=identical, 1.0=completely different) +# orig_stats: Stats from original commit (files, insertions, deletions, +# file_set) +# cherry_stats: Stats from cherry-pick commit +# reason: Reason for skipping analysis (None if analyzed) +CheckResult = namedtuple('CheckResult', [ + 'chash', 'orig_hash', 'subject', 'delta_ratio', + 'orig_stats', 'cherry_stats', 'reason' +]) # Named tuple for commit with author CommitInfo = namedtuple('CommitInfo', - ['hash', 'short_hash', 'subject', 'author']) + ['hash', 'chash', 'subject', 'author']) # Named tuple for prepare_apply result ApplyInfo = namedtuple('ApplyInfo', @@ -70,9 +104,9 @@ def compare_branches(master, source): # Get details about the merge-base commit info = run_git(['log', '-1', '--format=%H%n%h%n%s%n%ci', base]) - full_hash, short_hash, subject, commit_date = info.split('\n') + full_hash, chash, subject, commit_date = info.split('\n') - return count, Commit(full_hash, short_hash, subject, commit_date) + return count, Commit(full_hash, chash, subject, commit_date) def do_add_source(args, dbs): @@ -94,14 +128,14 @@ def do_add_source(args, dbs): # Get commit details for display info = run_git(['log', '-1', '--format=%h%n%s', base_hash]) - short_hash, subject = info.split('\n') + chash, subject = info.split('\n') # Store in database dbs.source_set(source, base_hash) dbs.commit() tout.info(f"Added source '{source}' with base commit:") - tout.info(f' Hash: {short_hash}') + tout.info(f' Hash: {chash}') tout.info(f' Subject: {subject}') return 0 @@ -141,13 +175,353 @@ def do_compare(args, dbs): # pylint: disable=unused-argument tout.info(f'Commits in {BRANCH_SOURCE} not in {BRANCH_MASTER}: {count}') tout.info('') tout.info('Last common commit:') - tout.info(f' Hash: {base.short_hash}') + tout.info(f' Hash: {base.chash}') tout.info(f' Subject: {base.subject}') tout.info(f' Date: {base.date}') return 0 +def parse_git_stat_output(stat_output): + """Parse git show --stat output to extract file change statistics + + Args: + stat_output (str): Output from 'git show --stat <hash>' + + Returns: + GitStat: Named tuple with files, insertions, deletions, file_set + """ + lines = stat_output.strip().split('\n') + files_changed = 0 + insertions = 0 + deletions = 0 + changed_files = set() + + # Parse summary line: "5 files changed, 42 insertions(+), 13 deletions(-)" + for line in lines: + match = RE_GIT_STAT_SUMMARY.search(line) + if match: + files_changed = int(match.group(1)) + insertions = int(match.group(2)) if match.group(2) else 0 + deletions = int(match.group(3)) if match.group(3) else 0 + break + + # Parse individual file lines: "path/to/file.ext | 42 ++++----" + for line in lines: + match = RE_GIT_STAT_FILE.match(line) + if match: + filename = match.group(1).strip() + if filename: + changed_files.add(filename) + + return GitStat(files_changed, insertions, deletions, changed_files) + + +def calc_ratio(orig, cherry): + """Get the ratio of differences between original and cherry-picked commits + + Args: + orig (GitStat): Stats for original commit + cherry (GitStat): Stats for cherry-pick commit + + Returns: + float: Delta ratio (0.0 = identical, 1.0 = completely + different) + """ + # If both commits have no changes, they're identical + if not (orig.inserted + orig.deleted) and not (cherry.inserted + + cherry.deleted): + return 0.0 + + # Calculate file set difference + if orig.file_set or cherry.file_set: + union = orig.file_set | cherry.file_set + intersection = orig.file_set & cherry.file_set + similarity = (len(intersection) / len(union) if union else 1.0) + else: + similarity = 1.0 + + # Calculate line change difference + orig_lines = orig.inserted + orig.deleted + cherry_lines = cherry.inserted + cherry.deleted + + if not orig_lines and not cherry_lines: + line_similarity = 1.0 + elif not orig_lines or not cherry_lines: + line_similarity = 0.0 + else: + line_ratio = (min(orig_lines, cherry_lines) / + max(orig_lines, cherry_lines)) + line_similarity = line_ratio + + # Overall similarity is the minimum of file and line similarity + overall_similarity = min(similarity, line_similarity) + + # Delta ratio is 1 - similarity + return 1.0 - overall_similarity + + +def get_orig_commit(cherry_commit_hash): + """Find the original commit hash from a cherry-pick commit + + Args: + cherry_commit_hash (str): Hash of the cherry-picked commit + + Returns: + str: Original commit hash, or None if not found + """ + try: + # Get the commit message + commit_msg = run_git(['log', '-1', '--format=%B', cherry_commit_hash]) + + # Look for "(cherry picked from commit <hash>)" line + for line in commit_msg.split('\n'): + if 'cherry picked from commit' in line: + match = RE_CHERRY_PICK.search(line) + if match: + return match.group(1) + + return None + except Exception: # pylint: disable=broad-except + return None + + +def check_commits(commits, min_lines): + """Yield CheckResult entries for commits with delta analysis + + Args: + commits (list): List of (commit_hash, chash, subject) tuples + min_lines (int): Minimum lines changed to analyze + + Yields: + CheckResult: Analysis result for each commit + """ + for chash, _, subject in commits: + # Skip merge commits + is_merge = False + try: + parents = run_git(['log', '-1', '--format=%P', chash]).split() + if len(parents) > 1: + is_merge = True + except Exception: # pylint: disable=broad-except + pass + + # Also check subject for merge indicators + if not is_merge and (subject.startswith('Merge ') or + 'Merge branch' in subject or + 'Merge tag' in subject): + is_merge = True + + if is_merge: + yield CheckResult( + chash, None, subject, 0.0, + None, None, 'merge_commit' + ) + continue + + # Find original commit + orig_hash = get_orig_commit(chash) + if not orig_hash: + yield CheckResult( + chash, None, subject, 0.0, + None, None, 'not_cherry_pick' + ) + continue + + # Get stats for both commits + orig_stat = run_git(['show', '--stat', orig_hash]) + cherry_stat = run_git(['show', '--stat', chash]) + + # Parse statistics + orig_stats = parse_git_stat_output(orig_stat) + cherry_stats = parse_git_stat_output(cherry_stat) + + # Skip small commits + orig_total_lines = orig_stats.inserted + orig_stats.deleted + cherry_total_lines = cherry_stats.inserted + cherry_stats.deleted + max_lines = max(orig_total_lines, cherry_total_lines) + + if max_lines < min_lines: + yield CheckResult( + chash, orig_hash, subject, 0.0, + orig_stats, cherry_stats, f'small_commit_{max_lines}_lines' + ) + continue + + # Calculate delta ratio + delta_ratio = calc_ratio(orig_stats, cherry_stats) + + yield CheckResult( + chash, orig_hash, subject, delta_ratio, + orig_stats, cherry_stats, None + ) + + +def check_verbose(result, threshold): + """Print verbose output for a single check result + + Args: + result (CheckResult): The check result to print + threshold (float): Delta threshold for highlighting problems + """ + chash_short = result.chash[:10] + + if result.reason: + if result.reason == 'merge_commit': + tout.info(f'{chash_short}: {result.subject}') + tout.info(' → Skipped (merge commit)') + tout.info('') + elif result.reason == 'not_cherry_pick': + tout.info(f'{chash_short}: {result.subject}') + tout.info(' → Not a cherry-pick (no original commit found)') + tout.info('') + elif result.reason.startswith('small_commit'): + lines = result.reason.split('_')[2] + tout.info(f'{chash_short}: {result.subject}') + tout.info(f' → Skipped (only {lines} lines changed)') + tout.info('') + elif result.reason.startswith('error'): + error = result.reason[6:] # Remove 'error_' prefix + tout.info(f'{chash_short}: {result.subject}') + tout.info(f' → Error checking delta: {error}') + tout.info('') + else: + # Valid result with analysis + tout.info(f'{chash_short}: {result.subject}') + tout.info(f' → Original: {result.orig_hash[:12]} ' + f'({result.orig_stats.files} files, ' + f'{result.orig_stats.inserted}+/' + f'{result.orig_stats.deleted}- lines)') + tout.info(f' → Cherry-pick: {result.cherry_stats.files} files, ' + f'{result.cherry_stats.inserted}+/' + f'{result.cherry_stats.deleted}- lines') + if result.delta_ratio > threshold: + tout.info(f' → Delta ratio: {result.delta_ratio:.1%} ' + f'⚠️ LARGE DELTA!') + else: + tout.info(f' → Delta ratio: {result.delta_ratio:.1%} ✓') + tout.info('') + + +def print_check_header(): + """Print the standard header for check output table""" + header = (f'{"Cherry-pick":<11} {"Delta%":>6} ' + f'{"Original":<10} Subject') + dashes = f'{"-" * 11} {"-" * 6} {"-" * 10} -------' + tout.info(header) + tout.info(dashes) + + +def format_problem_commit(result, threshold): + """Format a problematic commit in the standard table format + + Args: + result (CheckResult): The check result to format + threshold (float): Delta threshold for coloring + + Returns: + str: Formatted commit line + """ + delta_pct_val = result.delta_ratio * 100 + delta_pct = f'{delta_pct_val:.0f}' + pct_field = f'{delta_pct:>6}' + + # Apply color + col = terminal.Color() + threshold_pct = threshold * 100 + if delta_pct_val >= 50: + pct_field = col.build(terminal.Color.RED, pct_field) + elif delta_pct_val >= threshold_pct: + pct_field = col.build(terminal.Color.YELLOW, pct_field) + + return (f'{result.chash[:10]} {pct_field} ' + f'{result.orig_hash[:10]} {result.subject}') + + +def get_branch_commits(): + """Get commits on current branch that differ from ci/master + + Returns: + tuple: (current_branch, commits) where commits is a list of + (full_hash, short_hash, subject) tuples + """ + current_branch = run_git(['rev-parse', '--abbrev-ref', 'HEAD']) + + # Get all commits on current branch that aren't in ci/master + commit_list = run_git(['log', '--reverse', '--format=%H|%h|%s', + f'{BRANCH_MASTER}..HEAD']) + + if not commit_list: + return current_branch, [] + + # Parse commit_list format: "full_hash|short_hash|subject" per line + commits = [] + for line in commit_list.split('\n'): + if line: + parts = line.split('|', 2) + commits.append((parts[0], parts[1], parts[2])) + + return current_branch, commits + + +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 + dbs (Database): Database instance + + Returns: + int: 0 on success, 1 on failure + """ + threshold = args.threshold + min_lines = args.min_lines + verbose = args.verbose + + current_branch, commits = get_branch_commits() + + if verbose: + tout.info(f'Checking branch: {current_branch}') + tout.info(f'Delta threshold: {threshold:.1%}') + tout.info(f'Minimum lines to check: {min_lines}') + tout.info(f'Found {len(commits)} commits to check') + tout.info('') + + bad = [] + header_printed = False + + # Process commits using the generator + for res in check_commits(commits, min_lines): + if verbose: + check_verbose(res, threshold) + if not res.reason and res.delta_ratio > threshold: + bad.append(res) + else: + # 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 + + def do_check_gitlab(args, dbs): # pylint: disable=unused-argument """Check GitLab permissions for the configured token @@ -250,7 +624,7 @@ def get_next_commits(dbs, source): continue parts = line.split('|') commit_hash = parts[0] - short_hash = parts[1] + chash = parts[1] author = parts[2] subject = '|'.join(parts[3:-1]) # Subject may contain separator @@ -258,7 +632,7 @@ def get_next_commits(dbs, source): if dbs.commit_get(commit_hash): continue - commits.append(CommitInfo(commit_hash, short_hash, subject, author)) + commits.append(CommitInfo(commit_hash, chash, subject, author)) if commits: return commits, True, None @@ -281,14 +655,14 @@ def get_next_commits(dbs, source): continue parts = line.split('|') commit_hash = parts[0] - short_hash = parts[1] + chash = parts[1] author = parts[2] subject = '|'.join(parts[3:-1]) if dbs.commit_get(commit_hash): continue - commits.append(CommitInfo(commit_hash, short_hash, subject, author)) + commits.append(CommitInfo(commit_hash, chash, subject, author)) return commits, False, None @@ -321,7 +695,7 @@ def do_next_set(args, dbs): 'no merge found):') for commit in commits: - tout.info(f' {commit.short_hash} {commit.subject}') + tout.info(f' {commit.chash} {commit.subject}') return 0 @@ -363,15 +737,15 @@ def do_next_merges(args, dbs): continue parts = line.split('|', 2) commit_hash = parts[0] - short_hash = parts[1] + chash = parts[1] subject = parts[2] if len(parts) > 2 else '' - merges.append((commit_hash, short_hash, subject)) + merges.append((commit_hash, chash, subject)) if len(merges) >= count: break tout.info(f'Next {len(merges)} merges from {source}:') - for i, (_, short_hash, subject) in enumerate(merges, 1): - tout.info(f' {i}. {short_hash} {subject}') + for i, (_, chash, subject) in enumerate(merges, 1): + tout.info(f' {i}. {chash} {subject}') return 0 @@ -549,7 +923,7 @@ def format_history(source, commits, branch_name): str: Formatted summary text """ commit_list = '\n'.join( - f'- {c.short_hash} {c.subject}' + f'- {c.chash} {c.subject}' for c in commits ) @@ -603,7 +977,7 @@ def get_history(fname, source, commits, branch_name, conv_log): # Generate commit message 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) + commit_msg += '\n'.join(f'- {c.chash} {c.subject}' for c in commits) return content, commit_msg @@ -660,7 +1034,7 @@ def prepare_apply(dbs, source, branch): branch_name = branch if not branch_name: # Use first commit's short hash as part of branch name - branch_name = f'cherry-{commits[0].short_hash}' + branch_name = f'cherry-{commits[0].chash}' # Delete branch if it already exists try: @@ -678,7 +1052,7 @@ def prepare_apply(dbs, source, branch): tout.info(f' Branch: {branch_name}') for commit in commits: - tout.info(f' {commit.short_hash} {commit.subject}') + tout.info(f' {commit.chash} {commit.subject}') tout.info('') return ApplyInfo(commits, branch_name, original_branch, merge_found), 0 @@ -772,7 +1146,7 @@ def execute_apply(dbs, source, commits, branch_name, args): # pylint: disable=t dbs.commit() # Convert CommitInfo to tuple format expected by agent - commit_tuples = [(c.hash, c.short_hash, c.subject) for c in commits] + commit_tuples = [(c.hash, c.chash, c.subject) for c in commits] success, conv_log = agent.cherry_pick_commits(commit_tuples, source, branch_name) @@ -819,7 +1193,7 @@ def execute_apply(dbs, source, commits, branch_name, args): # pylint: disable=t ret = 1 else: tout.info(f"Use 'pickman commit-source {source} " - f"{commits[-1].short_hash}' to update the database") + f"{commits[-1].chash}' to update the database") return ret, success, conv_log @@ -874,7 +1248,7 @@ def do_push_branch(args, dbs): # pylint: disable=unused-argument Returns: int: 0 on success, 1 on failure """ - skip_ci = not getattr(args, 'run_ci', False) + skip_ci = not args.run_ci success = gitlab_api.push_branch(args.remote, args.branch, args.force, skip_ci=skip_ci) return 0 if success else 1 @@ -1327,6 +1701,7 @@ def do_test(args, dbs): # pylint: disable=unused-argument COMMANDS = { 'add-source': do_add_source, 'apply': do_apply, + 'check': do_check, 'check-gitlab': do_check_gitlab, 'commit-source': do_commit_source, 'compare': do_compare, diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 664825ef105..2b829e4c75a 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -52,7 +52,7 @@ class TestCommit(unittest.TestCase): '2024-01-15 10:30:00 -0600' ) self.assertEqual(commit.hash, 'abc123def456') - self.assertEqual(commit.short_hash, 'abc123d') + self.assertEqual(commit.chash, 'abc123d') self.assertEqual(commit.subject, 'Test commit subject') self.assertEqual(commit.date, '2024-01-15 10:30:00 -0600') @@ -93,7 +93,7 @@ class TestCompareBranches(unittest.TestCase): self.assertEqual(count, 42) self.assertEqual(commit.hash, 'abc123def456789') - self.assertEqual(commit.short_hash, 'abc123d') + self.assertEqual(commit.chash, 'abc123d') self.assertEqual(commit.subject, 'Test subject') self.assertEqual(commit.date, '2024-01-15 10:30:00 -0600') finally: @@ -116,7 +116,7 @@ class TestCompareBranches(unittest.TestCase): count, commit = control.compare_branches('branch1', 'branch2') self.assertEqual(count, 0) - self.assertEqual(commit.short_hash, 'def456a') + self.assertEqual(commit.chash, 'def456a') finally: command.TEST_RESULT = None @@ -1248,8 +1248,8 @@ class TestGetNextCommits(unittest.TestCase): self.assertIsNone(error) self.assertTrue(merge_found) self.assertEqual(len(commits), 2) - self.assertEqual(commits[0].short_hash, 'aaa111a') - self.assertEqual(commits[1].short_hash, 'bbb222b') + self.assertEqual(commits[0].chash, 'aaa111a') + self.assertEqual(commits[1].chash, 'bbb222b') dbs.close() @@ -1861,7 +1861,7 @@ Branch: cherry-abc""" self.assertIsNone(source) self.assertIsNone(last_hash) - def test_parse_mr_description_ignores_short_hashes(self): + def test_parse_mr_description_ignores_chashes(self): """Test that short numbers in conversation log are not matched.""" description = """## 2025-01-15: us/next @@ -2903,7 +2903,7 @@ class TestGetNextCommitsEmptyLine(unittest.TestCase): self.assertFalse(merge_found) # Only second commit should be returned (first is in DB) self.assertEqual(len(commits), 1) - self.assertEqual(commits[0].short_hash, 'bbb222b') + self.assertEqual(commits[0].chash, 'bbb222b') dbs.close() def test_get_next_commits_all_in_db(self): @@ -2993,8 +2993,8 @@ class TestGetNextCommitsEmptyLine(unittest.TestCase): self.assertTrue(merge_found) # Should return commits from second merge (first was skipped) self.assertEqual(len(commits), 2) - self.assertEqual(commits[0].short_hash, 'ccc333c') - self.assertEqual(commits[1].short_hash, 'merge2m') + self.assertEqual(commits[0].chash, 'ccc333c') + self.assertEqual(commits[1].chash, 'merge2m') dbs.close() @@ -3074,7 +3074,7 @@ class TestDoPushBranch(unittest.TestCase): """Test push failure.""" tout.init(tout.INFO) args = argparse.Namespace(cmd='push-branch', branch='test-branch', - remote='ci', force=False) + remote='ci', force=False, run_ci=False) with mock.patch.object(gitlab, 'push_branch', return_value=False): with terminal.capture(): @@ -3228,5 +3228,216 @@ class TestProcessMergedMrs(unittest.TestCase): dbs.close() +class TestCheck(unittest.TestCase): + """Tests for check command.""" + + def setUp(self): + """Set up test fixtures.""" + self.old_branch = 'old-branch' + + def test_parse_git_stat_output(self): + """Test parsing git show --stat output.""" + stat_output = """commit abc123def456 +Author: Test Author <test@example.com> +Date: Mon Jan 15 10:30:00 2024 -0600 + + Test commit message + + file1.c | 15 +++++++++++++++ + file2.h | 3 +-- + 2 files changed, 16 insertions(+), 2 deletions(-)""" + + result = control.parse_git_stat_output(stat_output) + files, insertions, deletions, file_set = result + self.assertEqual(files, 2) + self.assertEqual(insertions, 16) + self.assertEqual(deletions, 2) + self.assertEqual(file_set, {'file1.c', 'file2.h'}) + + def test_parse_git_stat_output_empty(self): + """Test parsing empty git show --stat output.""" + stat_output = """commit abc123def456 +Author: Test Author <test@example.com> +Date: Mon Jan 15 10:30:00 2024 -0600 + + Empty commit message + + 0 files changed""" + + result = control.parse_git_stat_output(stat_output) + files, insertions, deletions, file_set = result + self.assertEqual(files, 0) + self.assertEqual(insertions, 0) + self.assertEqual(deletions, 0) + self.assertEqual(file_set, set()) + + def test_calc_ratio_identical(self): + """Test delta ratio calculation for identical commits.""" + orig_stats = control.GitStat(2, 15, 3, {'file1.c', 'file2.h'}) + cherry_stats = control.GitStat(2, 15, 3, {'file1.c', 'file2.h'}) + + ratio = control.calc_ratio(orig_stats, cherry_stats) + self.assertEqual(ratio, 0.0) + + def test_calc_ratio_different_files(self): + """Test delta ratio calculation for different files.""" + orig_stats = control.GitStat(2, 15, 3, {'file1.c', 'file2.h'}) + cherry_stats = control.GitStat(3, 15, 3, {'file1.c', 'file2.h', 'file3.c'}) + + ratio = control.calc_ratio(orig_stats, cherry_stats) + self.assertGreater(ratio, 0.0) + self.assertLessEqual(ratio, 1.0) + + def test_calc_ratio_different_lines(self): + """Test delta ratio calculation for different line counts.""" + orig_stats = control.GitStat(2, 15, 3, {'file1.c', 'file2.h'}) + cherry_stats = control.GitStat(2, 30, 6, {'file1.c', 'file2.h'}) + + ratio = control.calc_ratio(orig_stats, cherry_stats) + self.assertGreater(ratio, 0.0) + self.assertLessEqual(ratio, 1.0) + + def test_get_orig_commit(self): + """Test finding original commit from cherry-pick message.""" + with mock.patch('pickman.control.run_git') as mock_run_git: + commit_msg = """Test commit subject + +This is the commit body. + +(cherry picked from commit abc123def456789) +""" + mock_run_git.return_value = commit_msg + + orig = control.get_orig_commit('def456abc123') + self.assertEqual(orig, 'abc123def456789') + + def test_get_orig_commit_not_cherry_pick(self): + """Test finding original commit for non-cherry-pick.""" + with mock.patch('pickman.control.run_git') as mock_run_git: + commit_msg = """Test commit subject + +This is a normal commit without cherry-pick info. +""" + mock_run_git.return_value = commit_msg + + orig = control.get_orig_commit('def456abc123') + self.assertIsNone(orig) + + def test_check_commits_no_commits(self): + """Test check_commits with empty commit list.""" + commits = [] + results = list(control.check_commits(commits, 10)) + self.assertEqual(len(results), 0) + + def test_check_commits_large_delta(self): + """Test check_commits finding commits with large deltas.""" + commits = [('abc123', 'abc123d', 'Test commit subject')] + + with mock.patch('pickman.control.run_git') as mock_run_git: + with mock.patch('pickman.control.get_orig_commit') as \ + mock_find_orig: + with mock.patch('pickman.control.parse_git_stat_output') as \ + mock_parse: + with mock.patch('pickman.control.calc_ratio') as mock_calc: + # Mock responses + mock_run_git.side_effect = [ + ['def456'], # parents (single parent) + 'orig_stat_output', # original commit stats + 'cherry_stat_output' # cherry-pick commit stats + ] + mock_find_orig.return_value = 'def456original' + mock_parse.side_effect = [ + control.GitStat(2, 15, 3, {'file1.c', 'file2.h'}), + control.GitStat(3, 30, 6, + {'file1.c', 'file2.h', 'file3.c'}) + ] + mock_calc.return_value = 0.5 # 50% delta + + results = list(control.check_commits(commits, 10)) + self.assertEqual(len(results), 1) + + result = results[0] + self.assertEqual(result.chash, 'abc123') + self.assertEqual(result.orig_hash, 'def456original') + self.assertEqual(result.delta_ratio, 0.5) + self.assertIsNone(result.reason) + + def test_check_commits_normal_commit(self): + """Test check_commits processing a normal commit.""" + commits = [('abc123', 'abc123d', 'Test commit subject')] + + with mock.patch('pickman.control.run_git') as mock_run_git: + with mock.patch('pickman.control.get_orig_commit') as \ + mock_find_orig: + with mock.patch('pickman.control.parse_git_stat_output') as \ + mock_parse: + with mock.patch('pickman.control.calc_ratio') as mock_calc: + # Mock responses + mock_run_git.side_effect = [ + ['def456'], # parents (single parent) + 'orig_stat_output', # original commit stats + 'cherry_stat_output' # cherry-pick commit stats + ] + mock_find_orig.return_value = 'def456original' + mock_parse.side_effect = [ + control.GitStat(2, 15, 3, {'file1.c', 'file2.h'}), + control.GitStat(3, 30, 6, + {'file1.c', 'file2.h', 'file3.c'}) + ] + mock_calc.return_value = 0.1 # 10% delta (low) + + results = list(control.check_commits(commits, 10)) + self.assertEqual(len(results), 1) + + result = results[0] + self.assertEqual(result.chash, 'abc123') + self.assertEqual(result.orig_hash, 'def456original') + self.assertEqual(result.subject, 'Test commit subject') + self.assertEqual(result.delta_ratio, 0.1) + self.assertIsNone(result.reason) + + def test_check_commits_merge_skip(self): + """Test check_commits skips merge commits.""" + commits = [('abc123', 'abc123d', 'Merge branch feature')] + + with mock.patch('pickman.control.run_git') as mock_run_git: + # Mock multiple parents (merge commit) + mock_run_git.return_value = ['parent1', 'parent2'] + + results = list(control.check_commits(commits, 10)) + self.assertEqual(len(results), 1) + + result = results[0] + self.assertEqual(result.reason, 'merge_commit') + + def test_check_commits_small_commit_skip(self): + """Test check_commits skips small commits.""" + commits = [('abc123', 'abc123d', 'Fix typo')] + + with mock.patch('pickman.control.run_git') as mock_run_git: + with mock.patch('pickman.control.get_orig_commit') as \ + mock_find_orig: + with mock.patch('pickman.control.parse_git_stat_output') as \ + mock_parse: + # Mock responses for small commit + mock_run_git.side_effect = [ + ['def456'], # single parent + 'orig_stat_output', + 'cherry_stat_output' + ] + mock_find_orig.return_value = 'def456original' + mock_parse.side_effect = [ + # 3 total lines (< 10) + control.GitStat(1, 2, 1, {'file1.c'}), + control.GitStat(1, 2, 1, {'file1.c'}) + ] + + results = list(control.check_commits(commits, 10)) + self.assertEqual(len(results), 1) + + result = results[0] + self.assertEqual(result.reason, 'small_commit_3_lines') + + if __name__ == '__main__': unittest.main() -- 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> Create an AgentCommit namedtuple for passing data to the agent to save confusing about ordering. Document CommitInfo while we are here. Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/agent.py | 12 +++++++----- tools/pickman/control.py | 16 +++++++++++++--- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/tools/pickman/agent.py b/tools/pickman/agent.py index 89014df0120..1d5a3df2442 100644 --- a/tools/pickman/agent.py +++ b/tools/pickman/agent.py @@ -49,7 +49,8 @@ async def run(commits, source, branch_name, repo_path=None): """Run the Claude agent to cherry-pick commits Args: - commits (list): list of (hash, short_hash, subject) tuples + commits (list): list of AgentCommit namedtuples with fields: + hash, chash, subject 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) @@ -70,12 +71,12 @@ async def run(commits, source, branch_name, repo_path=None): # Build commit list for the prompt commit_list = '\n'.join( - f' - {short_hash}: {subject}' - for _, short_hash, subject in commits + f' - {commit.chash}: {commit.subject}' + for commit in commits ) # Get full hash of last commit for signal file - last_commit_hash = commits[-1][0] + last_commit_hash = commits[-1].hash prompt = f"""Cherry-pick the following commits from {source} branch: @@ -204,7 +205,8 @@ def cherry_pick_commits(commits, source, branch_name, repo_path=None): """Synchronous wrapper for running the cherry-pick agent Args: - commits (list): list of (hash, short_hash, subject) tuples + commits (list): list of AgentCommit namedtuples with fields: + hash, chash, subject 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 ac9b57e10e3..4a993c72b88 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -72,9 +72,19 @@ 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) +# subject: First line of commit message (commit subject) +# author: Commit author name and email in format "Name <email>" CommitInfo = namedtuple('CommitInfo', ['hash', 'chash', 'subject', 'author']) +# Named tuple for simplified commit data passed to agent +# 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']) + # Named tuple for prepare_apply result ApplyInfo = namedtuple('ApplyInfo', ['commits', 'branch_name', 'original_branch', @@ -1213,9 +1223,9 @@ def execute_apply(dbs, source, commits, branch_name, args): # pylint: disable=t status='pending') dbs.commit() - # Convert CommitInfo to tuple format expected by agent - commit_tuples = [(c.hash, c.chash, c.subject) for c in commits] - success, conv_log = agent.cherry_pick_commits(commit_tuples, source, + # Convert CommitInfo to AgentCommit format expected by agent + agent_commits = [AgentCommit(c.hash, c.chash, c.subject) for c in commits] + success, conv_log = agent.cherry_pick_commits(agent_commits, source, branch_name) # Check for signal file from agent -- 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
participants (1)
-
Simon Glass