From patchwork Wed May 6 15:29:54 2026 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Simon Glass X-Patchwork-Id: 2277 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=1778081446; bh=LokLv1WQFFFBObzJxzxvPSVEmqqzWQF1g+wS0MhLAvc=; 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=sFAEYdcjRXzXha8FXVrYeIb+FFH4GPrjEbfnyclRNAGHqao0XwxxfacIBirNz1CbO RTvboEkwz5HYeWLqdYM5hhEGnA+X1pKTXTtermZHFttiFa/x9DXeyljRiU/H78ce4U SnYE5smlQc2VIhf8b9HXCSYuCXL1QJkvI6gz+WHw= Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id A8F6B6A87B for ; Wed, 6 May 2026 09:30:46 -0600 (MDT) 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 LyX25Bt2KTtn for ; Wed, 6 May 2026 09:30:46 -0600 (MDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1778081445; bh=LokLv1WQFFFBObzJxzxvPSVEmqqzWQF1g+wS0MhLAvc=; 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=jW29fI7qdVoK9n+pQfab6lfHQKJow9Tz53uzOSXdtScaDJfqOFZh/OMoB67kWv+KO wNz3fDyRyc1H/uc4MVyo5CD6gZIvGopTecgAJPtIgZx8kGMaXlcu5otQIfuxh/+1WX MyW1oqMHVMh6k29uw1Lp93qeX4sC9L2B3Gu0wmJ8= Received: from mail.u-boot.org (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id A821B6A949 for ; Wed, 6 May 2026 09:30:45 -0600 (MDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1778081444; bh=ztGla4SxUm/eW3u6P03kIg8XsDNQWeYxv2pGKp0s75s=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=qpj7TZDTi1MMnpVLBElzGCdrAT7+bi0cDxWOukq5SoF6Gs0A20JngsGCRJKHGUKJf NvhZ4DwmyYPLaWWRJxOx+UfY5CtpcQSKuTtRCx8H0YURlXtVNtks4iGdoVrlnEaLbf Ym2sPCYjtziDezhDPtREHRl8RHzv0NkoOGPpxqHU= Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 215CF6A92E; Wed, 6 May 2026 09:30:44 -0600 (MDT) 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 MPUWRy5AnDJ0; Wed, 6 May 2026 09:30:44 -0600 (MDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1778081443; bh=1cc86kGTZRmO6FQ6cLJrffifQ38V9V9u469YKjFDmRc=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=s0XaStlxEl/IrPnUYul5D6Ydop96vWLrPJxX3wXW2ypzNO7QWQ/a6WA25Dn/UFYc5 goruNXBbO4I8irkEuTaR3cmNvjtSd6HMUTyNHre3gpgDER+35fPMd7J49iukeu3PIA 6GlCr5qbr9I71+MbD5GO3/+R8uclPkseBMmcUebI= Received: from u-boot.org (unknown [174.51.25.52]) by mail.u-boot.org (Postfix) with ESMTPSA id 421326A87B; Wed, 6 May 2026 09:30:43 -0600 (MDT) From: Simon Glass To: U-Boot Concept Date: Wed, 6 May 2026 09:29:54 -0600 Message-ID: <20260506153006.529909-6-sjg@u-boot.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20260506153006.529909-1-sjg@u-boot.org> References: <20260506153006.529909-1-sjg@u-boot.org> MIME-Version: 1.0 Message-ID-Hash: 6QYEW2YEFQHXC7DYMEJPXLOT7DU6QW2C X-Message-ID-Hash: 6QYEW2YEFQHXC7DYMEJPXLOT7DU6QW2C 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 X-Mailman-Version: 3.3.10 Precedence: list Subject: [Concept] [PATCH 5/6] patman: Run reviews in a per-series git worktree 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 The review code currently applies the patch series onto the user's main checkout: it captures the original branch, stashes any local work (including untracked files), creates the review branch, applies, runs the agent, then tries to restore the original state on exit. This has a long tail of failure modes: Ctrl-C in the wrong place leaves the tree dirty, untracked build artefacts can leak in or out, two reviews cannot run at once, and the user cannot do other work in the same checkout while a review is in progress. Run each review in its own worktree under
/.git/patman/worktrees/. The worktree shares the .git directory of the main repo (so refs and objects are free) but has its own working tree and HEAD, completely isolated from whatever the user is doing. Use the new gitutil.ensure_worktree() helper to create or reset the worktree on demand, with the patman-specific path policy living in cser_helper.review_worktree_path(). Thread the two paths through ReviewContext: ctx.repo_path is the worktree (used as cwd for apply/build/agent) and ctx.main_repo is the user's main checkout (used when a real .git directory is needed for ref-based operations like 'git log upstream..branch'). Drop the 'git checkout -b' step from the apply agent's prompt since the worktree is already on the right branch when the agent starts. The user's main checkout is never touched, so the original-branch and stash dance is no longer needed. Remove the now-unused _git_restore() helper, the 'from u_boot_pylib import command' import that only fed it, and the matching mocks from the cseries test suite. While here, initialise patch_selection in ReviewContext.__init__ so the assignment in do_review() does not trip pylint's W0201 (attribute-defined-outside-init). Signed-off-by: Simon Glass --- tools/patman/cser_helper.py | 13 +++ tools/patman/review.py | 149 +++++++++++++++-------------------- tools/patman/test_cseries.py | 14 ++-- 3 files changed, 83 insertions(+), 93 deletions(-) -- 2.43.0 diff --git a/tools/patman/cser_helper.py b/tools/patman/cser_helper.py index 0274e28f42b..00dd80906e7 100644 --- a/tools/patman/cser_helper.py +++ b/tools/patman/cser_helper.py @@ -49,6 +49,19 @@ SHORTEN_STATE = { AUTOLINK = namedtuple('autolink', 'name,version,link,desc,result') +def review_worktree_path(repo, branch_name): + """Get the on-disk path for a per-review worktree + + Args: + repo (str): Top-level dir of the main checkout + branch_name (str): Review branch name + + Return: + str: Path where the review worktree lives (may not yet exist) + """ + return os.path.join(repo, '.git', 'patman', 'worktrees', branch_name) + + def oid(oid_val): """Convert a hash string into a shortened hash diff --git a/tools/patman/review.py b/tools/patman/review.py index 524a06df3a0..2842bb4ab00 100644 --- a/tools/patman/review.py +++ b/tools/patman/review.py @@ -23,12 +23,12 @@ import tempfile import aiohttp from u_boot_pylib import claude as claude_mod -from u_boot_pylib import command from u_boot_pylib import gitutil from u_boot_pylib import terminal from u_boot_pylib import tools from u_boot_pylib import tout +from patman import cser_helper from patman import database from patman import gmail from patman import patchstream @@ -46,7 +46,10 @@ class ReviewContext: # pylint: disable=R0902 reviewer_name (str): Reviewer's name reviewer_email (str): Reviewer's email series_data (dict): Series data from patchwork - repo_path (str): Path to the git repository + main_repo (str): Top-level of the user's main checkout (where + the .git dir lives and where shared refs are resolved) + repo_path (str): Path to the per-review worktree (used as cwd + for apply/build/agent) signoff (str or None): Sign-off text for reviews with comments spelling (str): Spelling convention comments_path (str or None): Path to existing patchwork comments @@ -58,6 +61,8 @@ class ReviewContext: # pylint: disable=R0902 branch_name (str or None): Branch with applied patches upstream_branch (str or None): Upstream branch ref patch_count (int or None): Number of patches + patch_selection (set of int or None): Patch indices to review + (None means all) """ def __init__(self, pwork, cser, series_data): @@ -66,6 +71,7 @@ class ReviewContext: # pylint: disable=R0902 self.series_data = series_data self.reviewer_name = None self.reviewer_email = None + self.main_repo = None self.repo_path = None self.signoff = None self.spelling = 'British' @@ -76,6 +82,7 @@ class ReviewContext: # pylint: disable=R0902 self.branch_name = None self.upstream_branch = None self.patch_count = None + self.patch_selection = None self.cover_content = None self.previous_reviews = {} self.diffstat = None @@ -130,28 +137,29 @@ async def fetch_mbox(pwork, link): def _build_apply_prompt(mbox_path, branch_name, upstream_branch): """Build the Claude agent prompt for applying patches - The agent will create a branch and apply the mbox using git am, - handling any conflicts that arise. + The cwd is already a per-review worktree on branch '{branch_name}' + reset to '{upstream_branch}', so the agent just needs to run git am + and handle any conflicts. Args: mbox_path (str): Path to the downloaded mbox file - branch_name (str): Name for the new branch - upstream_branch (str): Branch to start from (e.g. 'origin/master') + branch_name (str): Name of the (already-checked-out) review branch + upstream_branch (str): Upstream ref the branch is based on Returns: str: The prompt text for the agent """ return f'''Apply a patch series from a patchwork mbox \ -file to a new git branch. +file to the current branch. -TASK: -1. Create a new branch called '{branch_name}' from '{upstream_branch}': - git checkout -b {branch_name} {upstream_branch} +You are in a git worktree on branch '{branch_name}', already reset to +'{upstream_branch}'. Do NOT create a new branch. -2. Apply the patches from the mbox file: +TASK: +1. Apply the patches from the mbox file: git am {mbox_path} -3. If git am fails, try this recovery sequence: +2. If git am fails, try this recovery sequence: a. First, abort the failed git am: git am --abort @@ -183,10 +191,10 @@ TASK: If a patch is completely irrelevant (e.g. already applied), skip it and note which patch was skipped. -4. After all patches are applied (or skipped), run: +3. After all patches are applied (or skipped), run: git log --oneline {upstream_branch}..HEAD -5. Report the result: +4. Report the result: - How many patches were applied successfully - Which patches (if any) were skipped and why - The branch name: {branch_name} @@ -1079,7 +1087,9 @@ async def review_patches(ctx): return {} commit_range = f'{ctx.upstream_branch}..{ctx.branch_name}' - git_dir = os.path.join(ctx.repo_path, '.git') + # The branch is a shared ref, so use the main repo's .git directory + # — the worktree's .git is a pointer file, not a real git_dir + git_dir = os.path.join(ctx.main_repo, '.git') series = patchstream.get_metadata_for_list(commit_range, git_dir=git_dir) all_commits = [(i + 1, cmt.hash, cmt.subject) for i, cmt in enumerate(series.commits)] @@ -1283,24 +1293,6 @@ def search_series(pwork, title): return str(best['id']) -def _git_restore(orig_branch, had_stash, repo_path): - """Restore git branch and stash after review - - Args: - orig_branch (str or None): Branch to restore - had_stash (bool): Whether changes were stashed - repo_path (str or None): Repository path - """ - - try: - if orig_branch and repo_path: - gitutil.checkout_branch(orig_branch, repo_path) - if had_stash: - gitutil.stash_pop(repo_path) - except command.CommandExc: - pass - - def create_drafts(ctx, args, review_bodies, review_ids): """Create Gmail drafts for review emails @@ -1776,29 +1768,30 @@ def _get_upstream_branch(args, cser): def _apply_and_check(ctx, link): """Download, apply patches and verify they applied + Runs in the per-review worktree at ctx.repo_path; the branch + ctx.branch_name has already been created and reset to upstream by + worktree.ensure_worktree(), so the agent only needs to 'git am'. + Args: ctx (ReviewContext): Review context (uses pwork, cser, series_id, - version, upstream_branch) + version, upstream_branch, branch_name, repo_path) link (str): Patchwork series link/ID Returns: - str or None: Branch name, or None on failure + bool: True if the patches applied cleanly """ - ups = ctx.pwork.upstream if ctx.pwork else None - branch_name = _make_review_name(link, ups) - repo_path = gitutil.get_top_level() - success, branch_name = apply_series_sync(ctx.pwork, link, branch_name, - ctx.upstream_branch, repo_path) + success, _ = apply_series_sync(ctx.pwork, link, ctx.branch_name, + ctx.upstream_branch, ctx.repo_path) if success: applied = gitutil.count_revs( - repo_path, f'{ctx.upstream_branch}..{branch_name}') + ctx.repo_path, f'{ctx.upstream_branch}..{ctx.branch_name}') if not applied: # Zero commits, or branch missing because apply was interrupted success = False elif applied != ctx.patch_count: tout.error(f'Only {applied} of {ctx.patch_count} patches ' - f'applied to {branch_name}; aborting. Fix the ' + f'applied to {ctx.branch_name}; aborting. Fix the ' 'conflicts manually and retry.') success = False @@ -1806,9 +1799,9 @@ def _apply_and_check(ctx, link): tout.error('Failed to apply patches to branch') ctx.cser.db.ser_ver_remove(ctx.series_id, ctx.version) ctx.cser.commit() - return None - tout.notice(f'Patches applied to branch: {branch_name}') - return branch_name + return False + tout.notice(f'Patches applied to branch: {ctx.branch_name}') + return True def _fetch_cover_content(pwork, series_data): @@ -1979,47 +1972,33 @@ def do_review(args, pwork, cser): return 0 ctx.series_id, ctx.svid = result - orig_branch = None - had_stash = False + ctx.upstream_branch = _get_upstream_branch(args, cser) + ctx.main_repo = gitutil.get_top_level() + ups = pwork.upstream if pwork else None + ctx.branch_name = _make_review_name(link, ups) + wt_path = cser_helper.review_worktree_path(ctx.main_repo, ctx.branch_name) + tout.notice(f'Using review worktree {wt_path}') + ctx.repo_path = gitutil.ensure_worktree( + ctx.main_repo, wt_path, ctx.branch_name, ctx.upstream_branch) - try: - ctx.upstream_branch = _get_upstream_branch(args, cser) - ctx.repo_path = gitutil.get_top_level() - orig_branch = gitutil.current_branch(ctx.repo_path) - try: - # Stash untracked files too, so build artefacts from the - # current branch don't leak into the review branch - stash_out = gitutil.stash_save(ctx.repo_path, - include_untracked=True) - if 'No local changes' not in stash_out: - had_stash = True - except command.CommandExc: - pass - - ctx.branch_name = _apply_and_check(ctx, link) - if not ctx.branch_name: - return 1 - - if args.apply_only: - tout.notice('Apply-only mode; skipping review') - return 0 - - ctx.patch_selection = parse_patch_selection(args.patches) - ctx.reviewer_name, ctx.reviewer_email = _parse_reviewer(args) - ctx.signoff = args.signoff or None - if ctx.signoff: - ctx.signoff = ctx.signoff.replace('\\n', '\n') - ctx.spelling = args.spelling - ctx.comments_path = _write_comments_file(series_data, pwork) - - _run_and_store_reviews(ctx, args) - workflow.reviewed(cser, ctx.series_id, ctx.svid) - - _git_restore(orig_branch, had_stash, ctx.repo_path) - orig_branch = None - - finally: - _git_restore(orig_branch, had_stash, ctx.repo_path) + if not _apply_and_check(ctx, link): + return 1 + + if args.apply_only: + tout.notice('Apply-only mode; skipping review') + return 0 + + ctx.patch_selection = parse_patch_selection(args.patches) + ctx.reviewer_name, ctx.reviewer_email = _parse_reviewer(args) + ctx.signoff = args.signoff or None + if ctx.signoff: + ctx.signoff = ctx.signoff.replace('\\n', '\n') + ctx.spelling = args.spelling + ctx.comments_path = _write_comments_file(series_data, pwork) + + _run_and_store_reviews(ctx, args) + workflow.reviewed(cser, ctx.series_id, ctx.svid) + gitutil.remove_worktree(ctx.main_repo, wt_path) return 0 diff --git a/tools/patman/test_cseries.py b/tools/patman/test_cseries.py index 7d6f99b3536..c21a0357d1d 100644 --- a/tools/patman/test_cseries.py +++ b/tools/patman/test_cseries.py @@ -4642,16 +4642,15 @@ Date: .* """Context manager to mock apply, upstream, git and AI review""" fake_review = {1: f'Reviewed-by: {self.REVIEWER}'} return (mock.patch('patman.review._apply_and_check', - return_value='review1'), + return_value=True), mock.patch('patman.review._get_upstream_branch', return_value='origin/master'), mock.patch('patman.review.review_patches_sync', return_value=fake_review), mock.patch('patman.review.gitutil.get_top_level', return_value=self.tmpdir), - mock.patch('patman.review.command.output', - return_value='pati'), - mock.patch('patman.review._git_restore')) + mock.patch('patman.review.gitutil.ensure_worktree', + return_value=self.tmpdir)) def test_review_new_series(self): """Test reviewing a new series creates database records""" @@ -4772,14 +4771,13 @@ Date: .* pwork.project_set(self.PROJ_ID, self.PROJ_LINK_NAME) with mock.patch('patman.review._apply_and_check', - return_value=None), \ + return_value=False), \ mock.patch('patman.review._get_upstream_branch', return_value='origin/master'), \ mock.patch('patman.review.gitutil.get_top_level', return_value=self.tmpdir), \ - mock.patch('patman.review.command.output', - return_value='test'), \ - mock.patch('patman.review._git_restore'): + mock.patch('patman.review.gitutil.ensure_worktree', + return_value=self.tmpdir): with terminal.capture() as _: self.run_review('-s', str(self.REVIEW_LINK), pwork=pwork, expect_ret=1)