From patchwork Wed Dec 24 21:30:32 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Glass X-Patchwork-Id: 1072 Return-Path: X-Original-To: u-boot-concept@u-boot.org Delivered-To: u-boot-concept@u-boot.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611865; bh=Z3YxxdgHhZOIpWUNTNAbD7iJW/Sa0rKcuAsxNDAJI8M=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=FyQTH8aZprLa9kI9buz1k8++ILxvS9tRt4kO1RHkvaZsXP2zQ4RvW0vY2x+7k3f8H WyhUT+/1pkDpvJcK9AdAswnh/U4T1nePKkS6CqZ1oRc4HcbimOPO0QPZsdwACx7UvL qBXhbZKHKjxA9yylEU26PVXAKn4b8+TgbELiRCxAwPLlJLCKbfzL0IijrY9VQjd/A9 kyhhJYisiu03B3cBNOaDiS9VDmDVt+hrd6e8pPftURwWbt0nabEkjGS2gyuBVSQwXJ tjESHw0dBCnwLcFPvZz4LFHxXMwUGJWabdpwIuogEnY9H+yAdy6MoEcHShA6wTrKOJ nkk9QlNGClmjg== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id D54E364E31 for ; Wed, 24 Dec 2025 14:31:05 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10024) with ESMTP id 1iIpdU2OI13l for ; Wed, 24 Dec 2025 14:31:05 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611865; bh=Z3YxxdgHhZOIpWUNTNAbD7iJW/Sa0rKcuAsxNDAJI8M=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=FyQTH8aZprLa9kI9buz1k8++ILxvS9tRt4kO1RHkvaZsXP2zQ4RvW0vY2x+7k3f8H WyhUT+/1pkDpvJcK9AdAswnh/U4T1nePKkS6CqZ1oRc4HcbimOPO0QPZsdwACx7UvL qBXhbZKHKjxA9yylEU26PVXAKn4b8+TgbELiRCxAwPLlJLCKbfzL0IijrY9VQjd/A9 kyhhJYisiu03B3cBNOaDiS9VDmDVt+hrd6e8pPftURwWbt0nabEkjGS2gyuBVSQwXJ tjESHw0dBCnwLcFPvZz4LFHxXMwUGJWabdpwIuogEnY9H+yAdy6MoEcHShA6wTrKOJ nkk9QlNGClmjg== Received: from mail.u-boot.org (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id C520C64E2C for ; Wed, 24 Dec 2025 14:31:05 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611863; bh=cdlh7f5vrMCpmyVbbvwJ1GXuof2G2nBAyOmvMeVfDtg=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=uJRNL4ysbLk2ns1rv6tVgZhwcoadYucsNjqokKcjAixhqflvp/6B9wgGMRdeMuft6 ADbWduUq0O9BKVncbEfUZKEeDEdfMVRRCZ/XBfeVp2yqYItK/fnU4pIOBY1N+I2spJ pISaSSxFyVZLd6crPime4YhGVCJlKbF+L+yU2s5gyWDO7CmaCR3o1MHQPber4a6trt 12w/0kHjPN9+QTrJQBVPuorzrMv2acj4HjBW1czpa+c35X8IPROe7G4jNnL/nRZ+lv NnRcLhY5fBVJ+9PT4P12dLP3PAOK6b1NatKBaXLEgZ9LYqmkTdZUNkgm6kwurOStIC 4xPmC8PskDvjQ== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id A262E64E3C; Wed, 24 Dec 2025 14:31:03 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10026) with ESMTP id ZXIpl1gs2qw2; Wed, 24 Dec 2025 14:31:03 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611857; bh=R2/Ei4Dd483riJX1tXZtV1mNHmVPI779NSlNk168rmI=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=sBV8EfmYAA0DAIPf1Tdu1C/asm4P73nMtMKTd5jHtrNEqLm8XRzwj6X7uP1YbIqSO 0fctg4kv45szRZfSQUfa+aoHZip4dv8VaTOob8Y7sZGsEGqdpJGKEHtmlrQoWiLDvu eqLvt179NVcGLsZM8ggkpffefOGQQpncuhvgH1bmpQ6JFTAHjgx6PuRjW8flNIUp2r wA5pk3tN6ZZC9niqMnVa+3NCs2+sTVODxQSgQ0xDkYSiqoyhTivhKrvMovJ1ZL4wXm 4TdwfyMH4LUy1Www0eCJo6n7hQsl6aZn65dGSO9oQjc7hfqqExsTcIg8QYSa3MrPrW 4N85CN8sBIYbQ== Received: from u-boot.org (unknown [73.34.74.121]) by mail.u-boot.org (Postfix) with ESMTPSA id 5BDE964CCC; Wed, 24 Dec 2025 14:30:57 -0700 (MST) From: Simon Glass To: U-Boot Concept Date: Wed, 24 Dec 2025 14:30:32 -0700 Message-ID: <20251224213045.3010514-2-sjg@u-boot.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20251224213045.3010514-1-sjg@u-boot.org> References: <20251224213045.3010514-1-sjg@u-boot.org> MIME-Version: 1.0 Message-ID-Hash: KOG3JA3BWSFKB4KU6CGWUB6MFDU4A74P X-Message-ID-Hash: KOG3JA3BWSFKB4KU6CGWUB6MFDU4A74P X-MailFrom: sjg@u-boot.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Simon Glass , Claude X-Mailman-Version: 3.3.10 Precedence: list Subject: [Concept] [PATCH 1/9] pickman: Fix 80-column line length compliance List-Id: Discussion and patches related to U-Boot Concept Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: From: Simon Glass 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 Signed-off-by: Simon Glass --- 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 - For merge commits (identified by "Merge" in subject): git cherry-pick -x -m 1 --allow-empty @@ -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, From patchwork Wed Dec 24 21:30:33 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Glass X-Patchwork-Id: 1073 Return-Path: X-Original-To: u-boot-concept@u-boot.org Delivered-To: u-boot-concept@u-boot.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611870; bh=c/i4TlOveaz/7RIxZUO5eUAzxC2qlO/yUbKAV6ck5tY=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=HsWbX/U38NzC0444pOtbKUJQCsUtuIu5JgM/56e+VCWFlXH/E5QDwxx/7jDO3eokd gTZv1xJMGnKwWr1434MtZGHISitF34QZn/PP9F4+YlNrcizW7GdEBZWl44evdhTjco zxri0yF6IkaFIx2pBgTKOiWw3+Jb5zd4S/E333RBbRkxFPbdBL29k6Nf+r13xYqDwc 1EAmDQl3HM88B+b0CpqLKLAEz70an679BnYdzhjOUf8cDn1kYzP4cRBtFeeCypbbvO lDHFQwc1uDYs/3O0QyJ7sv1/0LEmHYMOTOBd75imky0/4bxiHXLe2zHHkiTdxjcMqV 1csBymxFa149A== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 61BDF64CA9 for ; Wed, 24 Dec 2025 14:31:10 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10024) with ESMTP id M5sAbIT7L4tl for ; Wed, 24 Dec 2025 14:31:10 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611870; bh=c/i4TlOveaz/7RIxZUO5eUAzxC2qlO/yUbKAV6ck5tY=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=HsWbX/U38NzC0444pOtbKUJQCsUtuIu5JgM/56e+VCWFlXH/E5QDwxx/7jDO3eokd gTZv1xJMGnKwWr1434MtZGHISitF34QZn/PP9F4+YlNrcizW7GdEBZWl44evdhTjco zxri0yF6IkaFIx2pBgTKOiWw3+Jb5zd4S/E333RBbRkxFPbdBL29k6Nf+r13xYqDwc 1EAmDQl3HM88B+b0CpqLKLAEz70an679BnYdzhjOUf8cDn1kYzP4cRBtFeeCypbbvO lDHFQwc1uDYs/3O0QyJ7sv1/0LEmHYMOTOBd75imky0/4bxiHXLe2zHHkiTdxjcMqV 1csBymxFa149A== Received: from mail.u-boot.org (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 4BCAD64DD8 for ; Wed, 24 Dec 2025 14:31:10 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611868; bh=6uJ1Z8TQcpekjXTSpdqOzq1Ettw49JofhPTUqcrukT8=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Sn+V7vGqdt3SoZ0YRamZzGTb49Ot8d/NK2LbeHpWklB8SG8VlZu3NR8MHUUGclT5O /YKvKmCdu6INiUZPo1dNVV7EUNiRs8I1oHVtfnmOel4HWy5ixxpHiicKQSNQgN4hMN GyquzZGlhA18VdFMlRzw37nhjxi+UCUHpuRxmMugLeYy6OMitGaAv0NAi+y18COJpL 3msv978ullKrnkCGBlp/7z/bf5ABGNziMTFlHI2AxhKO8ZzJSEyM9XzeVh/JONQuTN CGoCDe5+gYb1w4LxL654t+QMeZMFgnkGqv5waZDqDlopJ0EVCRN2ESs1zXL5FOTQGc 59GQlrc/wBmkg== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 4676464CA9; Wed, 24 Dec 2025 14:31:08 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10026) with ESMTP id tW-aLGdXJEK3; Wed, 24 Dec 2025 14:31:08 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611862; bh=9JvJqnVGBLDEqs6WRYDV2LdF48Oh0AvQfuxswOehmB4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Fby+UDn+XSHxzqeeLQGC1jPH60QO6IMe/XIaGMThCnP/ghRsAgAxIyXqi4/FHAgXj fZnQuEDiYt8UdIwnhlWoPgB0Pw1IH6lqVxOf0Zoxk3K9g+H6JaaLYMJdAICsV2juT9 C8jfqLAUxXmRQiydeUnvn86Y/3iYyu7/4dwKhkMVfoevw9Nix220VH4Ku4TgcvfD4t bExL3Z4mzwfTzqJaSTH5JQUuWExiRW7j+DAA2HCZjI4yUNnlNRZzr/oOW2qs/TjZvf feEv0ByCFOZr2TuNzq0zD0R4lmq5h9wUFYrWWM44bBKucB2l0FmRfqKDj9Z7iuFtD7 qapyUx75FtRIw== Received: from u-boot.org (unknown [73.34.74.121]) by mail.u-boot.org (Postfix) with ESMTPSA id 35E8E64DD8; Wed, 24 Dec 2025 14:31:02 -0700 (MST) From: Simon Glass To: U-Boot Concept Date: Wed, 24 Dec 2025 14:30:33 -0700 Message-ID: <20251224213045.3010514-3-sjg@u-boot.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20251224213045.3010514-1-sjg@u-boot.org> References: <20251224213045.3010514-1-sjg@u-boot.org> MIME-Version: 1.0 Message-ID-Hash: 5HAU2T6ROJL62EGAJRXOXNOHRFYP5YBI X-Message-ID-Hash: 5HAU2T6ROJL62EGAJRXOXNOHRFYP5YBI X-MailFrom: sjg@u-boot.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Simon Glass , Claude X-Mailman-Version: 3.3.10 Precedence: list Subject: [Concept] [PATCH 2/9] pickman: Add URL constants to improve test readability List-Id: Discussion and patches related to U-Boot Concept Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: From: Simon Glass 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 Signed-off-by: Simon Glass --- 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] From patchwork Wed Dec 24 21:30:34 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Glass X-Patchwork-Id: 1074 Return-Path: X-Original-To: u-boot-concept@u-boot.org Delivered-To: u-boot-concept@u-boot.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611874; bh=+RnC4dYH95l8U51NtQDTC44RiaVWarGBo0Z8IEqYeb8=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=ol62FWXjGxeSjSXnRsnt64E9bvjVKNi6W/GC3RvDHJ5u+7Sw6bgeUCpeeS0OvlK60 5S9PYqjaECgZcwJJ3RQXifFyLf/fu0vrm47FWm4p/nM/1Tpj/G042s2ZUudh8QltcV 5r04BNTLEeENOyUIdv2OcfCqhO8smFaHuUwhfoD4ge8+e4vGm5WJVQxzATs9lOkyPE lvN/mimtVNbJVAd2hZKtzLIxzlO53tKWTo8M2gljKWUettCOWSphAPPvDk/HMBEzfU 8BW+xxXxk//jw25Me32NRFwvlsm68OT+2tOdjV8rHI+JgBMApy1xTUNofLRXDsVPFb +8Eiuybq5wkKw== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 5CB7F64E50 for ; Wed, 24 Dec 2025 14:31:14 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10024) with ESMTP id RqBLTw4Lp9uo for ; Wed, 24 Dec 2025 14:31:14 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611874; bh=+RnC4dYH95l8U51NtQDTC44RiaVWarGBo0Z8IEqYeb8=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=ol62FWXjGxeSjSXnRsnt64E9bvjVKNi6W/GC3RvDHJ5u+7Sw6bgeUCpeeS0OvlK60 5S9PYqjaECgZcwJJ3RQXifFyLf/fu0vrm47FWm4p/nM/1Tpj/G042s2ZUudh8QltcV 5r04BNTLEeENOyUIdv2OcfCqhO8smFaHuUwhfoD4ge8+e4vGm5WJVQxzATs9lOkyPE lvN/mimtVNbJVAd2hZKtzLIxzlO53tKWTo8M2gljKWUettCOWSphAPPvDk/HMBEzfU 8BW+xxXxk//jw25Me32NRFwvlsm68OT+2tOdjV8rHI+JgBMApy1xTUNofLRXDsVPFb +8Eiuybq5wkKw== Received: from mail.u-boot.org (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 4A30664DD8 for ; Wed, 24 Dec 2025 14:31:14 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611872; bh=EvBBlVgBO2bzH4hg36BQgblEd+w1uZaPxzfz0PIHNms=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=KCPlxUO5uzuKrn9Ob5S97bTR7ALdjkXGv2tNIWUJ8tTjYxt8qYbCZN/Qi9zWxkDrH yY79P+pf6OneSOyXUJGgYWlXiOVgRaxDPrArIco/es6eihu8rJ2JJ/t+9DbYo/flhI iJ1woVbqowjk8mBaI4XheF9QxOAfHKJgXRTWe/uUT12GFk6EYCC5lkJAqb96regYt6 hBkan0YR/qgxAHVHaFIZWOiRIoFMpHXMh9f+DHlO9NKtNb74uZrVhQk7hX5EuYozcM 09yDIHWySkNlrUZZt/zkWMPpmfgsD08dC8zuJKT/pbWscAfJ1aDckMEhPSuC2RFCHu dnvvtAGPQ7bZg== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 4456864DD5; Wed, 24 Dec 2025 14:31:12 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10026) with ESMTP id ykx6CkRoEryW; Wed, 24 Dec 2025 14:31:12 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611867; bh=HLuG5IfapMPwHno1ozHVZK/1Umd1jvSR7GOiI5EVw2M=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=hHA7HOAtrfu8aTHSQlmwcj4jIcKMiCj3mqGD31nCksJjA2xYTRuGULgVzS2wtiFl/ 6+UBEWEVypvrD8AKJa7VOIIQ7E1h9SAVVrZu4v9xeegfDvAlWbxPtRYmZea0BezfOT 1c/xxoeTYjCTU5iWUsE/+MFXUl4G1VmYvA1c7EGyhO1YutO54iiW2SkO0FCrTAznID Xq6+yWT2ZlzFPWwazJaWyjeLZbrM/N4ycQ9kFo9pippd3RM2MUcrwm/qfx5Dv/GymU x1ENLmL4hbRdmo2HEeHNkA5G5ygjo9opBzlhxHTOSeBa5hivWORNHI/NXvZuCkWm9W eoo0y5rQfwpOw== Received: from u-boot.org (unknown [73.34.74.121]) by mail.u-boot.org (Postfix) with ESMTPSA id 0187B64CCC; Wed, 24 Dec 2025 14:31:06 -0700 (MST) From: Simon Glass To: U-Boot Concept Date: Wed, 24 Dec 2025 14:30:34 -0700 Message-ID: <20251224213045.3010514-4-sjg@u-boot.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20251224213045.3010514-1-sjg@u-boot.org> References: <20251224213045.3010514-1-sjg@u-boot.org> MIME-Version: 1.0 Message-ID-Hash: W7F7RFGWWDOGZIB4FHCNA6FRYIFV5GQW X-Message-ID-Hash: W7F7RFGWWDOGZIB4FHCNA6FRYIFV5GQW X-MailFrom: sjg@u-boot.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Simon Glass , Claude X-Mailman-Version: 3.3.10 Precedence: list Subject: [Concept] [PATCH 3/9] pickman: Improve function names and line-length compliance List-Id: Discussion and patches related to U-Boot Concept Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: From: Simon Glass 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 Signed-off-by: Simon Glass --- 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: From patchwork Wed Dec 24 21:30:36 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Glass X-Patchwork-Id: 1075 Return-Path: X-Original-To: u-boot-concept@u-boot.org Delivered-To: u-boot-concept@u-boot.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611884; bh=qRTS9JcvWNOB5fL44sIfVnVobxMDT7TTbHXU245M8rg=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=nA5P1MJ6RV1ThXILfw/+F9TmWDHHLWKh8yFaMd6/MJF82BymRfFNz/rVYyULmtPYx U00xHLBGQ8By+5wKzPDRxNLBlprS82ZdEspH26WYP2DIRG9j8725dfREXD2WElfpHO MQLQ6vU/xzydPfAQxxzq46/EIwHgu4XgibX6ci7tJ+0cnTXxOeqtpN/bSrZxzFIxEB JvKAvd7Nd9Tkl2XFx56tDgNo7VndiAssVOr4CvHvGT8/guuv48JTlsxjt90ynKG9Er FQEFOGHXdlI1JQ0ZdqTodBXWynEP6tPHDQXJJk+cbOglxhKjTIcQMWe2WcBdjezp3G z+zl/RlaLW0ag== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 07CE564E4A for ; Wed, 24 Dec 2025 14:31:24 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10024) with ESMTP id 3lXTzoNESBJB for ; Wed, 24 Dec 2025 14:31:23 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611883; bh=qRTS9JcvWNOB5fL44sIfVnVobxMDT7TTbHXU245M8rg=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=bU/ZFoq6MgLtxwjkJ9ITCKhZ97r6P8a/QQn0PJtXJ63rMf9HTmcmzy27hJn6knktQ 5THN0TxJXTCcH8s9vLDAZNhfuFAI5RmdRaVoN80sYhx1zNf5YHaHbNSILbBRR910yW tXO4nGPp6IvbKyOzAzhnRoC0QmoTHnSkmNbX2RwIuB+SvulMZfxtLQQYjpZBYWk2WN QL3NYbfJ/6zJmIvQA04QSZbHolOtQMtpApckVPOzgFgHQI/HM5dY9fkd82gaxj/IFT AdFRa+ti0dMUlPYwEtCeKlNmu+z5w5fFVIyZGpPxadTJLfATwsoVWhm6RKQaBWPo0V Ou5MqjDPKFlLQ== Received: from mail.u-boot.org (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id E1D2064DF6 for ; Wed, 24 Dec 2025 14:31:23 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611881; bh=/1Nkbtn0NbdBrlXvW/Xzc6QGSPfsfSM5oKzTmwnju8c=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=NQxWsjnQFTtFIMX3Mzww+0pRdgRTZB720QIhUavRZlK/UD6BOKPtSDXzVdvy0ibHN 2nPd1b9Po5SNjd2skBMPGTQkO6SPnOCJdN+tJ8uXzM2SfVpjHhbx4zHAsbNt8W9eVw wpRf/i9HkjUKVtkp17Q6mmotKU+JXFD5Awkml7h/cPu9SoMzpq2iT76LM/HXpYPaJt PBN4MAvH0kHGwCP0Hgmhy4/WpZChTEXs55FJne+DI0v7kOsvz5uThbmwFroZDclFBl zd6qkzONwlTFuWLIwr02QWCfVVZs20PrXApCIn/nknQTOcZryVELkyzGQU4VaklNrm /1gj//9nIRo0g== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 9DCB364CA9; Wed, 24 Dec 2025 14:31:21 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10026) with ESMTP id Qe58skGeAGGT; Wed, 24 Dec 2025 14:31:21 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611877; bh=sVf26+Us+ZREnOu3WWGjOqjGHWYMUyDfY3sfl8n/yhY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=wON8wUPeTsSUF5fQ8Pf21yPMCabrLTQb3txKHJ43vyr8A2/ciR3uqWpWsYdV9bqKO lgjfltZBG1xVvLlFn1qOnenlOhKi7XmSjNcDj3f+N6tLGYymxSmqbhpUwP7wgOEHn8 qubvMJd6mXjLC4fXcw1GyNpK1WPhPe4ZVDyvyv2jQMrVhX3XDO+DJouu1ck2kVMYxw vIRBHo4K4ZmBOpNHcStSl+8laQd61lp/6LeYlfDAo8J0bhKTjbAu+HGM+/JV8Rbjs9 NwUbWoWkHrk9g0BT/bkpqTaKtPT2/sfV1gJi3Ucyjtjc8QdKWz6t4hp0zgOwefAJ/N pAiWiBWII5Quw== Received: from u-boot.org (unknown [73.34.74.121]) by mail.u-boot.org (Postfix) with ESMTPSA id 2866664DD8; Wed, 24 Dec 2025 14:31:17 -0700 (MST) From: Simon Glass To: U-Boot Concept Date: Wed, 24 Dec 2025 14:30:36 -0700 Message-ID: <20251224213045.3010514-6-sjg@u-boot.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20251224213045.3010514-1-sjg@u-boot.org> References: <20251224213045.3010514-1-sjg@u-boot.org> MIME-Version: 1.0 Message-ID-Hash: CZ5VGJBANVBSA4LIAIACBOBKHIJUE7J7 X-Message-ID-Hash: CZ5VGJBANVBSA4LIAIACBOBKHIJUE7J7 X-MailFrom: sjg@u-boot.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Simon Glass , Claude X-Mailman-Version: 3.3.10 Precedence: list Subject: [Concept] [PATCH 5/9] pickman: Enhance agent prompts with per-commit validation List-Id: Discussion and patches related to U-Boot Concept Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: From: Simon Glass 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 Signed-off-by: Simon Glass --- 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 ' and 'git show --stat ' + 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 | 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 " [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 From patchwork Wed Dec 24 21:30:39 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Glass X-Patchwork-Id: 1076 Return-Path: X-Original-To: u-boot-concept@u-boot.org Delivered-To: u-boot-concept@u-boot.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611899; bh=OGeqY3b7+8VFvMXmzcpSVu71VeEp3ueabDY0e4wbux0=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=ReMzLZ4W6K9nMNUFf/118VRArE4tkPozQFB0HFls7pYKjxHyAujnB33BpXf9vT5EK 6t/khsGiPRd3Cn4cQAbOwXrdil1/GKNyhu8lsaV46YPBTHzOszXBtOev3q06JAVNr1 l10l1MZyKXx271DR3d+Q5LHR0CnFa8edJ195BIiYRMGTsJ2mshOIakTkqLdXrGf6iT z3IEQnWike9DhmyPB/p4181zP3AOwdswFU+ZjL9IzvmYwKgl0FmhOlaWOd14q4sz0o 5PUa18+XdOnlXRRhiUTWAYMQWcXKUcRSFlLz+WMX9u3D/uro+Q9zFdNBIO7wNEyW17 o7oc5uLuT1QKg== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 16F9364E50 for ; Wed, 24 Dec 2025 14:31:39 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10024) with ESMTP id V4p8z0b8pC0U for ; Wed, 24 Dec 2025 14:31:39 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611899; bh=OGeqY3b7+8VFvMXmzcpSVu71VeEp3ueabDY0e4wbux0=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=ReMzLZ4W6K9nMNUFf/118VRArE4tkPozQFB0HFls7pYKjxHyAujnB33BpXf9vT5EK 6t/khsGiPRd3Cn4cQAbOwXrdil1/GKNyhu8lsaV46YPBTHzOszXBtOev3q06JAVNr1 l10l1MZyKXx271DR3d+Q5LHR0CnFa8edJ195BIiYRMGTsJ2mshOIakTkqLdXrGf6iT z3IEQnWike9DhmyPB/p4181zP3AOwdswFU+ZjL9IzvmYwKgl0FmhOlaWOd14q4sz0o 5PUa18+XdOnlXRRhiUTWAYMQWcXKUcRSFlLz+WMX9u3D/uro+Q9zFdNBIO7wNEyW17 o7oc5uLuT1QKg== Received: from mail.u-boot.org (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 0255F64DF6 for ; Wed, 24 Dec 2025 14:31:39 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611896; bh=2zGUAde677HLL3GUBwdIVWLm9lDzgprWyaGGH9fTfhk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=RM6TYTUyWdkdrum3NliZklNR1PGBI/2Ept/4H/zfe+UoWUhCK6zHgVpvuVZPUvxxh FWn15WQIlpAV7GSloHz3a88B97onEZY++3s18LKw/mPH8Nn1/ADmiQfKyJsWf0vBQw Aq1elAlrh2OrRwqwDvp7fFwWJWl74J6T60/LT8ecuxnGgu/nlzu6gYp8llNpA4IkGV VdiuCth41yjWv4sjydo19prqw4PCVd/APHVC4hthlm/UXharNxVZARKK7cNpH4V3iw ZkKgbFbGI7az+MZnbC/lyY4ftS4WcxeyVNkf8xE7il0sNyNmcwwGxQP5rRMbwi1D00 idIUeDDiKkx2A== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id F139864DD8; Wed, 24 Dec 2025 14:31:36 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10026) with ESMTP id S-rlilqUin72; Wed, 24 Dec 2025 14:31:36 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611892; bh=x3kic8ZaC9NK+cBilQrfDSaFAWeo9vYDX0vABuFuvDM=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=AQL60+VfbsNSpVSiNuoQuOvZP0PZwv+C2PAi+PFmU0MmcwqfxmHZlHfA/GsUS/Lbp DaWFVeNQ9eJMnaLf4NSF0l/HC6l93qaPZJKOOwgiwaIpNGsLmZV/BKwOLBw1lGAzmm q1fuKvD0AEoOibwAR3WDZuna3EL9E3YL0vg7svJFa0l5MqKA2mUxflRZqrGVFUzYDO /8e+GPkkjuHfSGwsJT4bXEAUt73VPYZEZK3j45ARJr5kXJ/YK50eDF8jLgMvr9skRS Poc8Rvg+As+nWo4dTX/vbbPr1SCOlgZug50L+vgSy+Z65k0D0QsRu9boYTLZa+x3ui 6zshWJcIKsbXg== Received: from u-boot.org (unknown [73.34.74.121]) by mail.u-boot.org (Postfix) with ESMTPSA id C182664CA9; Wed, 24 Dec 2025 14:31:31 -0700 (MST) From: Simon Glass To: U-Boot Concept Date: Wed, 24 Dec 2025 14:30:39 -0700 Message-ID: <20251224213045.3010514-9-sjg@u-boot.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20251224213045.3010514-1-sjg@u-boot.org> References: <20251224213045.3010514-1-sjg@u-boot.org> MIME-Version: 1.0 Message-ID-Hash: SMVXU5QJW73MYXCNQ7MMF2DGBV7QPLWS X-Message-ID-Hash: SMVXU5QJW73MYXCNQ7MMF2DGBV7QPLWS X-MailFrom: sjg@u-boot.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Simon Glass , Claude X-Mailman-Version: 3.3.10 Precedence: list Subject: [Concept] [PATCH 8/9] pickman: Use named tuples for better code clarity List-Id: Discussion and patches related to U-Boot Concept Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: From: Simon Glass 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 Signed-off-by: Simon Glass --- 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 " 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 From patchwork Wed Dec 24 21:30:40 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Glass X-Patchwork-Id: 1077 Return-Path: X-Original-To: u-boot-concept@u-boot.org Delivered-To: u-boot-concept@u-boot.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611904; bh=kqWFYx2UyNip0tYd2wAOnxSKGQLne/SqcQz/oLx8uD4=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=B2A3FeCuKjtJtm1E4mMTGFWbWgvIg8+DqIyeWEVeTql+ck+CYrSgzlo7p52WD8OX7 fFROGIsa4u0WpTzD1MKtAubSMla5R94FtPooKA1nq0ZIPkHE1UffOww4Q+hZ9B/I2J tdyG7r3YSLmBF5ePXyo+00uwk9c4s7g9mBdod0mBDS3AmCVLlAnvVWeZ+4PFClodU2 vcIIU4Xxif6z2zmf2c6NPvefpcvek+N47oJQR7i3yzgEZV3VIhHYCiqWkTTgv9qSDl ZbYuUqKGNKPaH/OgdE/iLYru9GCgiA4PUnI3kxbzfYJfZYWr34AM6hwkrFL6cZZ/gs gpC+g1mnyecHQ== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 8F4C864E4F for ; Wed, 24 Dec 2025 14:31:44 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10024) with ESMTP id RRwEheyk8t7t for ; Wed, 24 Dec 2025 14:31:44 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611904; bh=kqWFYx2UyNip0tYd2wAOnxSKGQLne/SqcQz/oLx8uD4=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=B2A3FeCuKjtJtm1E4mMTGFWbWgvIg8+DqIyeWEVeTql+ck+CYrSgzlo7p52WD8OX7 fFROGIsa4u0WpTzD1MKtAubSMla5R94FtPooKA1nq0ZIPkHE1UffOww4Q+hZ9B/I2J tdyG7r3YSLmBF5ePXyo+00uwk9c4s7g9mBdod0mBDS3AmCVLlAnvVWeZ+4PFClodU2 vcIIU4Xxif6z2zmf2c6NPvefpcvek+N47oJQR7i3yzgEZV3VIhHYCiqWkTTgv9qSDl ZbYuUqKGNKPaH/OgdE/iLYru9GCgiA4PUnI3kxbzfYJfZYWr34AM6hwkrFL6cZZ/gs gpC+g1mnyecHQ== Received: from mail.u-boot.org (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 7DC5864D70 for ; Wed, 24 Dec 2025 14:31:44 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611901; bh=5ANgP3KIOdPepoQ4lORhQRMxYoNkvhyr+ylmG/O7G6E=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=QBYC50OKdvWyN45+lSTPTMUTFQh8mSKtdOhQ8JggB/iYvy6ozZ+sGKdZRDTix3gLP DpAqG8pC/P8HLeh2Wh4T6JgmN1r7Ds5EoaKCbVG8h/ZROnA9v3kyCwe0872qJObMOC lvUVS10pyl8p+M+2uuV+6zmuEhNalME71jDZ6kOpG+UsvUBxeROa7uaQSiAtJa59jD fiIt0/TbsPIwW5vrSvweGsjNZb0P8nmZuGDtUJHV9mdrxlI9PImdgicMtVtRkNwW7F 1JifGUmwNlJnUbKfzzSPGcilIk+FCnOjz37oxB3kx1jnLattuCqFUzwde07R9FsBe1 Z1n1vbxXJU/ZA== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 94E5C64D70; Wed, 24 Dec 2025 14:31:41 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10026) with ESMTP id 6wDgirILYmng; Wed, 24 Dec 2025 14:31:41 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1766611896; bh=/RRTfNoFi/ubBemIeOMTcJ63aCSrUCwjaEkM4oBrEz0=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=LkFYqAswBw5QKlqdhTptLTBmIcA0Aokp+zCVGakqdjjXMwqURnfIHvfF5KivCEP0R lNha9KlcnKO7jPgkDeSjJY2BzATIpAimus6jG/yShVPeeu54/rXApeDWcV/Y9BpQ+H Ff6JP355yYRzdO8Edw+jlTP8KjYzLZpYbXZc7eXtUjJYqjoijjb2buBMyUT3Drdi9X 1ST7nt1BZZug8Y1czzufj3/9XwrDX8YSJoywsa82nkn3u4UeQwfKb13M7fXXeA+XOt pj7uCl6F/tCsP/7zYn2GdtS73oSjGLScL3qamnA6STStIDGogPe0vaMeYnsieVxaTg GwNVtDcqxQwKw== Received: from u-boot.org (unknown [73.34.74.121]) by mail.u-boot.org (Postfix) with ESMTPSA id 7C15B64DD5; Wed, 24 Dec 2025 14:31:36 -0700 (MST) From: Simon Glass To: U-Boot Concept Date: Wed, 24 Dec 2025 14:30:40 -0700 Message-ID: <20251224213045.3010514-10-sjg@u-boot.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20251224213045.3010514-1-sjg@u-boot.org> References: <20251224213045.3010514-1-sjg@u-boot.org> MIME-Version: 1.0 Message-ID-Hash: SQ62IIUC32ZFW4L7Z2JUHMMKB3RDVIIE X-Message-ID-Hash: SQ62IIUC32ZFW4L7Z2JUHMMKB3RDVIIE X-MailFrom: sjg@u-boot.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Simon Glass , Claude X-Mailman-Version: 3.3.10 Precedence: list Subject: [Concept] [PATCH 9/9] pickman: Add already-applied commit detection / comparison List-Id: Discussion and patches related to U-Boot Concept Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: From: Simon Glass 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 Signed-off-by: Simon Glass --- 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="" -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 ", verify they are truly the same commit: +1. Compare the actual changes between the original and found commits: + - Use: git show --no-ext-diff > /tmp/orig.patch + - Use: git show --no-ext-diff > /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="" -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="" -1 +2. If found, verify it's actually the same commit by comparing patches: + - git show --no-ext-diff > /tmp/orig.patch + - git show --no-ext-diff > /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 " 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 '), + control.CommitInfo('def456abc123', 'def456a', 'Fix bug', + 'Author ') + ] + self.quoted_commit = [ + control.CommitInfo('abc123def456', 'abc123d', + 'Add "quoted" feature', 'Author ') + ] + self.single_commit = [ + control.CommitInfo('abc123def456', 'abc123d', 'Add new feature', + 'Author ') + ] + + @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()