While reviewing feedback it is helpful to see the review comments on the command line to check that each has been addressed. Add an option to support that.
Update the workflow documentation to describe the new features.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/patman/README | 36 +++++- tools/patman/control.py | 11 +- tools/patman/main.py | 5 +- tools/patman/status.py | 278 +++++++++++++++++++++++++++------------- 4 files changed, 227 insertions(+), 103 deletions(-)
diff --git a/tools/patman/README b/tools/patman/README index 595a0d616b..0ce299d2c0 100644 --- a/tools/patman/README +++ b/tools/patman/README @@ -14,6 +14,7 @@ This tool is a Python script which: It also has some Patchwork features: - shows review tags from Patchwork so you can update your local patches - pulls these down into a new branch on request +- lists comments received on a series
It is intended to automate patch creation and make it a less error-prone process. It is useful for U-Boot and Linux work so far, @@ -391,6 +392,8 @@ but has the new review tags in it. You can check that this worked with:
which should show that there are no new responses compared to this new branch.
+There is also a -C option to list the comments received for each patch. +
Example Work Flow ================= @@ -475,17 +478,33 @@ people on the list don't see your secret info. Of course patches often attract comments and you need to make some updates. Let's say one person sent comments and you get an Acked-by: on one patch. Also, the patch on the list that you were waiting for has been merged, -so you can drop your wip commit. So you resync with upstream: +so you can drop your wip commit. + +Take a look on patchwork and find out the URL of the series. This will be +something like http://patchwork.ozlabs.org/project/uboot/list/?series=187331 +Add this to a tag in your top commit: + + Series-link: http://patchwork.ozlabs.org/project/uboot/list/?series=187331 + +You can use then patman to collect the Acked-by tag to the correct commit, +creating a new 'version 2' branch for us-cmd: + + patman status -d us-cmd2 + git checkout us-cmd2 + +You can look at the comments in Patchwork or with: + + patman status -C + +Then you can resync with upstream:
git fetch origin (or whatever upstream is called) git rebase origin/master
-and use git rebase -i to edit the commits, dropping the wip one. You add -the ack tag to one commit: - - Acked-by: Heiko Schocher hs@denx.de +and use git rebase -i to edit the commits, dropping the wip one.
-update the Series-cc: in the top commit: +Then update the Series-cc: in the top commit to add the person who reviewed +the v1 series:
Series-cc: bfin, marex, Heiko Schocher hs@denx.de
@@ -524,7 +543,9 @@ so to send them:
and it will create and send the version 2 series.
-General points: + +General points +==============
1. When you change back to the us-cmd branch days or weeks later all your information is still there, safely stored in the commits. You don't need @@ -606,3 +627,4 @@ a bad thing. Simon Glass sjg@chromium.org v1, v2, 19-Oct-11 revised v3 24-Nov-11 +revised v4 Independence Day 2020, with Patchwork integration diff --git a/tools/patman/control.py b/tools/patman/control.py index b6ba0a56c0..b7d23b58c1 100644 --- a/tools/patman/control.py +++ b/tools/patman/control.py @@ -174,11 +174,13 @@ def send(args): args.limit, args.dry_run, args.in_reply_to, args.thread, args.smtp_server)
-def patchwork_status(branch, count, start, end, dest_branch, force): +def patchwork_status(branch, count, start, end, dest_branch, force, + show_comments): """Check the status of patches in patchwork
This finds the series in patchwork using the Series-link tag, checks for new - review tags, displays then and creates a new branch with the review tags. + comments and review tags, displays then and creates a new branch with the + review tags.
Args: branch (str): Branch to create patches from (None = current) @@ -190,6 +192,8 @@ def patchwork_status(branch, count, start, end, dest_branch, force): dest_branch (str): Name of new branch to create with the updated tags (None to not create a branch) force (bool): With dest_branch, force overwriting an existing branch + show_comments (bool): True to display snippets from the comments + provided by reviewers
Raises: ValueError: if the branch has no Series-link value @@ -206,4 +210,5 @@ def patchwork_status(branch, count, start, end, dest_branch, force): # Import this here to avoid failing on other commands if the dependencies # are not present from patman import status - status.check_patchwork_status(series, link, branch, dest_branch, force) + status.check_patchwork_status(series, link, branch, dest_branch, force, +show_comments) diff --git a/tools/patman/main.py b/tools/patman/main.py index c8fc035c10..6e4e23763d 100755 --- a/tools/patman/main.py +++ b/tools/patman/main.py @@ -90,6 +90,8 @@ AddCommonArgs(test_parser)
status = subparsers.add_parser('status', help='Check status of patches in patchwork') +status.add_argument('-C', '--show-comments', action='store_true', + help='Show comments from each patch') status.add_argument('-d', '--dest-branch', type=str, help='Name of branch to create with collected responses') status.add_argument('-f', '--force', action='store_true', @@ -161,7 +163,8 @@ elif args.cmd == 'status': ret_code = 0 try: control.patchwork_status(args.branch, args.count, args.start, args.end, - args.dest_branch, args.force) + args.dest_branch, args.force, + args.show_comments) except Exception as e: print('patman: %s: %s' % (type(e).__name__, e)) if args.debug: diff --git a/tools/patman/status.py b/tools/patman/status.py index 71124d6e5e..9e3cb97e72 100644 --- a/tools/patman/status.py +++ b/tools/patman/status.py @@ -2,44 +2,45 @@ # # Copyright 2020 Google LLC # -# This talks to the patchwork service to figure out what patches have been -# reviewed and commented on.
-from bs4 import BeautifulSoup +"""Talks to the patchwork service to figure out what patches have been +reviewed and commented on. Provides a way to display review tags and comments. +Allows creation of a new branch based on the old but with the review tags +collected from patchwork. +""" + import collections import concurrent.futures import io -import pygit2 from itertools import repeat import re -import requests -import time from urllib.parse import urljoin +import pygit2 +import requests +from bs4 import BeautifulSoup
from patman import commit -from patman import gitutil from patman import patchstream from patman import terminal -from patman.terminal import Color
# Patches which are part of a multi-patch series are shown with a prefix like # [prefix, version, sequence], for example '[RFC, v2, 3/5]'. All but the last # part is optional. This decodes the string into groups. For single patches # the [] part is not present: # Groups: (ignore, ignore, ignore, prefix, version, sequence, subject) -re_patch = re.compile('([(((.*),)?(.*),)?(.*)]\s)?(.*)$') +RE_PATCH = re.compile(r'([(((.*),)?(.*),)?(.*)]\s)?(.*)$')
# This decodes the sequence string into a patch number and patch count -re_seq = re.compile('(\d+)/(\d+)') +RE_SEQ = re.compile(r'(\d+)/(\d+)')
def to_int(vals): - """Convert a lsit of strings into integers, using 0 if not an integer + """Convert a list of strings into integers, using 0 if not an integer
Args: - vals: List of strings + vals (list): List of strings
Returns: - List of integers, one for each input string + list: List of integers, one for each input string """ out = [int(val) if val.isdigit() else 0 for val in vals] return out @@ -59,28 +60,33 @@ class Patch(dict): The other properties come from other columns in the display.
Properties: - seq: Sequence number within series (1=first) parsed from sequence string - count: Number of patches in series, parsed from sequence string - prefix: Prefix string or None - subject: Patch subject with [..] part removed (same as commit subject) - series: Series name - acked: Number of Acked-by rtags - fixes: Number of Fixes rtags - reviewed: Number of Reviewed-by rtags - tested: Number of Tested-by rtags - success: Success value - warning: Warning value - fail: Failure value - date: Date as a string - Submitter: Submitter name - Delegate: Deleted ID - state: Current state (e.g. 'New') - url: URL pointing to the patch on patchwork + seq (int): Sequence number within series (1=first) parsed from sequence + string + count (int): Number of patches in series, parsed from sequence string + prefix (str): Prefix string or None (e.g. 'RFC') + version (str): Version string or None (e.g. 'v2') + subject (str): Patch subject with [..] part removed (same as commit + subject) + series (str): Series name + acked (int): Number of Acked-by rtags, or None if not known + fixes (int): Number of Fixes rtags, or None if not known + reviewed (int): Number of Reviewed-by rtags, or None if not known + tested (int): Number of Tested-by rtags, or None if not known + success (int): Success value, or None if not known + warning (int): Warning value, or None if not known + fail (int): Failure value, or None if not known + date (str): Date as a string + Submitter (str): Submitter name + Delegate (str): Delegate ID + state (str): Current state (e.g. 'New') + url (str): URL pointing to the patch on patchwork """ def __init__(self): + super().__init__() self.seq = None self.count = None self.prefix = None + self.version = None self.subject = None self.series = None self.acked = None @@ -103,24 +109,28 @@ class Patch(dict): def __getattr__(self, name): return self[name]
- def Add(self, name, value): + def add(self, name, value): """Add information obtained from the web page
This sets the object properties based on @name.
Args: - name: Column name in lower-case, e.g. 'Series' or 'State' - value: Text obtained for this patch from that column, e.g. 'New' + name (str): Column name in lower-case, e.g. 'Series' or 'State' + value (str): Text obtained for this patch from that column, + e.g. 'New' + + Raises: + ValueError: if the name cannot be parsed or is not recognised """ if name in self: self[name] = value elif name == 'patch': - mat = re_patch.search(value.strip()) + mat = RE_PATCH.search(value.strip()) if not mat: raise ValueError("Cannot parse subject '%s'" % value) self.prefix = mat.group(1) self.prefix, self.version, seq_info, self.subject = mat.groups()[3:] - mat_seq = re_seq.match(seq_info) + mat_seq = RE_SEQ.match(seq_info) if not mat_seq: raise ValueError("Cannot parse sequence spec '%s'" % seq_info) self.seq = int(mat_seq.group(1)) @@ -133,16 +143,35 @@ class Patch(dict): else: print('Unknown field: %s\n' % name)
- def SetUrl(self, url): + def set_url(self, url): """Sets the URL for a patch
Args: - url: URL to set + url (str): URL to set """ self.url = url
-def FindResponses(url): +class Review: + """Represents a single review email collected in Patchwork + + Patches can attract multiple reviews. Each consists of an author/date and + a variable number of 'snippets', which are groups of quoted and unquoted + text. + """ + def __init__(self, meta, snippets): + """Create new Review object + + Args: + meta (str): Text containing review author and date + snippets (list): List of snippets in th review, each a list of text + lines + """ + self.meta = ' : '.join([line for line in meta.splitlines() if line]) + self.snippets = snippets + + +def find_responses(url): """Collect the response tags (rtags) to a patch found in patchwork
Reads the URL and checks each comment for an rtags (e.g. Reviewed-by). @@ -151,13 +180,18 @@ def FindResponses(url): new rtags.
Args: - url: URL of the patch + url (str): URL of the patch
Returns: - Dict of rtags: - key: rtag type, e.g. 'Acked-by' - value: Set of people providing that rtag. Each member of the set - is a name/email, e.g. 'Bin Meng bmeng.cn@gmail.com' + Tuple: + Dict of rtags: + key: rtag type, e.g. 'Acked-by' + value: Set of people providing that rtag. Each member of the set + is a name/email, e.g. 'Bin Meng bmeng.cn@gmail.com' + List of Review objects for the patch + + Raises: + ValueError: if the URL could not be read """ web_data = requests.get(url) if web_data.status_code != 200: @@ -171,31 +205,37 @@ def FindResponses(url):
if comments: for comment in comments: - ps = patchstream.PatchStream(None) - ps.commit = commit.Commit(None) + pstrm = patchstream.PatchStream(None) + pstrm.commit = commit.Commit(None) meta = comment.find(class_='meta') content = comment.find(class_='content') infd = io.StringIO(content.get_text()) outfd = io.StringIO() - ps.ProcessStream(infd, outfd) - for response, people in ps.commit.rtags.items(): + pstrm.ProcessStream(infd, outfd) + for response, people in pstrm.commit.rtags.items(): rtags[response].update(people) - return rtags + if pstrm.snippets: + reviews.append(Review(meta.get_text(), pstrm.snippets)) + return rtags, reviews
-def CollectPatches(series, url): +def collect_patches(series, url): """Collect patch information about a series from patchwork
Reads the web page and collects information provided by patchwork about the status of each patch.
Args: - series: Series object corresponding to the local branch containing the - series - url: URL pointing to patchwork's view of this series, possibly an + series (Series): Series object corresponding to the local branch + containing the series + url (str): URL pointing to patchwork's view of this series, possibly an earlier versions of it
Returns: - List of patches sorted by sequence number, each a Patch object + list: List of patches sorted by sequence number, each a Patch object + + Raises: + ValueError: if the URL could not be read or the web page does not follow + the expected structure """ response = requests.get(url) if response.status_code != 200: @@ -228,11 +268,12 @@ def CollectPatches(series, url): patch = Patch() for seq, col in enumerate(row.findAll('td')): name = cols[seq].lower() - patch.Add(name, col.get_text()) + patch.add(name, col.get_text()) if name == 'patch': - patch.SetUrl(urljoin(url, col.a['href'])) + patch.set_url(urljoin(url, col.a['href'])) if patch.count != count: - raise ValueError("Patch %d '%s' has count of %d, expected %d" % + raise ValueError( + "Patch %d '%s' has count of %d, expected %d" % (patch.seq, patch.subject, patch.count, num_commits)) patches.append(patch)
@@ -240,25 +281,31 @@ def CollectPatches(series, url): patches = sorted(patches, key=lambda x: x.seq) return patches
-def FindNewResponses(new_rtag_list, seq, commit, patch): +def find_new_responses(new_rtag_list, review_list, seq, cmt, patch, + force_load): """Find new rtags collected by patchwork that we don't know about
This is designed to be run in parallel, once for each commit/patch
Args: - new_rtag_list: New rtags are written to new_rtag_list[seq] + new_rtag_list (list): New rtags are written to new_rtag_list[seq] list, each a dict: key: Response tag (e.g. 'Reviewed-by') value: Set of people who gave that response, each a name/email string - seq: Position in new_rtag_list to update - commit: Commit object for this commit - patch: Corresponding Patch object for this patch + review_list (list): New reviews are written to review_list[seq] + list, each a + List of reviews for the patch, each a Review + seq (int): Position in new_rtag_list to update + cmt (Commit): Commit object for this commit + patch (Patch): Corresponding Patch object for this patch + force_load (bool): True to always load the patch from patchwork, False + to only load it if patchwork has additional response tags """ - acked = commit.rtags['Acked-by'] - reviewed = commit.rtags['Reviewed-by'] - tested = commit.rtags['Tested-by'] - fixes = commit.rtags['Fixes'] + acked = cmt.rtags['Acked-by'] + reviewed = cmt.rtags['Reviewed-by'] + tested = cmt.rtags['Tested-by'] + fixes = cmt.rtags['Fixes']
# Figure out the number of new rtags by subtracting what we have in # the commit from what patchwork's summary reports. @@ -271,9 +318,10 @@ def FindNewResponses(new_rtag_list, seq, commit, patch): # If patchwork agrees with the commit, we don't need to read the web # page, so save some time. If not, we need to add the new ones to the commit new_rtags = collections.defaultdict(set) - if extra: - pw_rtags = FindResponses(patch.url) - base_rtags = commit.rtags + reviews = None + if extra or force_load: + pw_rtags, reviews = find_responses(patch.url) + base_rtags = cmt.rtags new_count = 0 for tag, people in pw_rtags.items(): for who in people: @@ -283,16 +331,20 @@ def FindNewResponses(new_rtag_list, seq, commit, patch): new_rtags[tag].add(who) new_count += 1 new_rtag_list[seq] = new_rtags + review_list[seq] = reviews
-def ShowResponses(rtags, indent, is_new): +def show_responses(rtags, indent, is_new): """Show rtags collected
Args: - rtags: dict: + rtags (dict): review tags to show key: Response tag (e.g. 'Reviewed-by') value: Set of people who gave that response, each a name/email string - indent: Indentation string to write before each line - is_new: True if this output should be highlighted + indent (str): Indentation string to write before each line + is_new (bool): True if this output should be highlighted + + Returns: + int: Number of review tags displayed """ col = terminal.Color() count = 0 @@ -304,13 +356,32 @@ def ShowResponses(rtags, indent, is_new): count += 1 return count
-def CreateBranch(series, new_rtag_list, branch, dest_branch, overwrite): +def create_branch(series, new_rtag_list, branch, dest_branch, overwrite): + """Create a new branch with review tags added + + Args: + series (Series): Series object for the existing branch + new_rtag_list (list): List of review tags to add, one for each commit, + each a dict: + key: Response tag (e.g. 'Reviewed-by') + value: Set of people who gave that response, each a name/email + string + branch (str): Existing branch to update + dest_branch (str): Name of new branch to create + overwrite (bool): True to force overwriting dest_branch if it exists + + Returns: + int: Total number of review tags added across all commits + + Raises: + ValueError: if the destination branch name is the same as the original + branch, or it already exists and @overwrite is False + """ if branch == dest_branch: raise ValueError( 'Destination branch must not be the same as the original branch') repo = pygit2.Repository('.') count = len(series.commits) - old_br = repo.branches[branch] new_br = repo.branches.get(dest_branch) if new_br: if not overwrite: @@ -322,17 +393,15 @@ def CreateBranch(series, new_rtag_list, branch, dest_branch, overwrite):
num_added = 0 for seq in range(count): - basket = repo.branches.get(dest_branch) + parent = repo.branches.get(dest_branch) cherry = repo.revparse_single('%s~%d' % (branch, count - seq - 1))
- base = repo.merge_base(cherry.oid, basket.target) + repo.merge_base(cherry.oid, parent.target) base_tree = cherry.parents[0].tree
- index = repo.merge_trees(base_tree, basket, cherry) + index = repo.merge_trees(base_tree, parent, cherry) tree_id = index.write_tree(repo)
- author = cherry.author - committer = cherry.committer lines = [] for tag, people in new_rtag_list[seq].items(): for who in people: @@ -340,21 +409,37 @@ def CreateBranch(series, new_rtag_list, branch, dest_branch, overwrite): num_added += 1 message = cherry.message + '\n'.join(lines)
- basket = repo.create_commit( - basket.name, cherry.author, cherry.committer, message, tree_id, - [basket.target]) + repo.create_commit( + parent.name, cherry.author, cherry.committer, message, tree_id, + [parent.target]) return num_added
-def check_patchwork_status(series, url, branch, dest_branch, force): - patches = CollectPatches(series, url) +def check_patchwork_status(series, url, branch, dest_branch, force, + show_comments): + """Check the status of a series on Patchwork + + This finds review tags and comments for a series in Patchwork, displaying + them and optionally creating a new branch like the old but with the new + review tags. + + Args: + series (Series): Series object for the existing branch + url (str): URL pointing to the patch on patchwork + branch (str): Existing branch to update + dest_branch (str): Name of new branch to create + force (bool): True to force overwriting dest_branch if it exists + show_comments (bool): True to show the comments on each patch + """ + patches = collect_patches(series, url) col = terminal.Color() count = len(patches) new_rtag_list = [None] * count + review_list = [None] * count
with concurrent.futures.ThreadPoolExecutor(max_workers=16) as executor: futures = executor.map( - FindNewResponses, repeat(new_rtag_list), range(count), - series.commits, patches) + find_new_responses, repeat(new_rtag_list), repeat(review_list), + range(count), series.commits, patches, repeat(show_comments)) for fresponse in futures: if fresponse: raise fresponse.exception() @@ -363,20 +448,29 @@ def check_patchwork_status(series, url, branch, dest_branch, force): for seq, patch in enumerate(patches): terminal.Print('%3d %s' % (patch.seq, patch.subject[:50]), colour=col.BLUE) - commit = series.commits[seq] - base_rtags = commit.rtags + cmt = series.commits[seq] + base_rtags = cmt.rtags new_rtags = new_rtag_list[seq]
indent = ' ' * 2 - ShowResponses(base_rtags, indent, False) - num_to_add += ShowResponses(new_rtags, indent, True) + show_responses(base_rtags, indent, False) + num_to_add += show_responses(new_rtags, indent, True) + if show_comments: + for review in review_list[seq]: + terminal.Print('Review: %s' % review.meta, colour=col.RED) + for snippet in review.snippets: + for line in snippet: + quoted = line.startswith('>') + terminal.Print(' %s' % line, + colour=col.MAGENTA if quoted else None) + print()
print("%d new response%s available in patchwork%s" % (num_to_add, 's' if num_to_add != 1 else '', '' if dest_branch else ' (use -d to write them to a new branch)'))
if dest_branch: - num_added = CreateBranch(series, new_rtag_list, branch, - dest_branch, force) + num_added = create_branch(series, new_rtag_list, branch, + dest_branch, force) print("%d response%s added from patchwork into new branch '%s'" % (num_added, 's' if num_added != 1 else '', dest_branch))