[PATCH v2 30/30] patman: Support listing comments from patchwork
Simon Glass
sjg at chromium.org
Mon Oct 26 12:04:42 AEDT 2020
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 at chromium.org>
---
Changes in v2:
- Many changes to the whole series, including new patches
- Use of REST API instead of the web site
tools/patman/README | 36 +++++--
tools/patman/control.py | 11 ++-
tools/patman/func_test.py | 186 +++++++++++++++++++++++++++++++++---
tools/patman/main.py | 5 +-
tools/patman/patchstream.py | 7 +-
tools/patman/status.py | 59 ++++++++++--
6 files changed, 268 insertions(+), 36 deletions(-)
diff --git a/tools/patman/README b/tools/patman/README
index d99bfb3127b..79a9830208a 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,
@@ -399,6 +400,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
=================
@@ -483,17 +486,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 at 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 at denx.de>
@@ -532,7 +551,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
@@ -612,3 +633,4 @@ a bad thing.
Simon Glass <sjg at 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 5ccf46570bd..b11580c9df6 100644
--- a/tools/patman/control.py
+++ b/tools/patman/control.py
@@ -176,11 +176,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)
@@ -192,6 +194,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
@@ -223,4 +227,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, found[0], branch, dest_branch, force)
+ status.check_patchwork_status(series, found[0], branch, dest_branch, force,
+ show_comments)
diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py
index 0ddd908de1f..04bbb88e8cd 100644
--- a/tools/patman/func_test.py
+++ b/tools/patman/func_test.py
@@ -870,15 +870,16 @@ Series-changes: 2
self.patches = [patch1, patch2]
count = 2
new_rtag_list = [None] * count
+ review_list = [None, None]
# Check that the tags are picked up on the first patch
- status.find_new_responses(new_rtag_list, 0, commit1, patch1,
- self._fake_patchwork2)
+ status.find_new_responses(new_rtag_list, review_list, 0, commit1,
+ patch1, self._fake_patchwork2)
self.assertEqual(new_rtag_list[0], {'Reviewed-by': {self.joe}})
# Now the second patch
- status.find_new_responses(new_rtag_list, 1, commit2, patch2,
- self._fake_patchwork2)
+ status.find_new_responses(new_rtag_list, review_list, 1, commit2,
+ patch2, self._fake_patchwork2)
self.assertEqual(new_rtag_list[1], {
'Reviewed-by': {self.mary, self.fred},
'Tested-by': {self.leb}})
@@ -887,16 +888,16 @@ Series-changes: 2
# 'new' tags when scanning comments
new_rtag_list = [None] * count
commit1.rtags = {'Reviewed-by': {self.joe}}
- status.find_new_responses(new_rtag_list, 0, commit1, patch1,
- self._fake_patchwork2)
+ status.find_new_responses(new_rtag_list, review_list, 0, commit1,
+ patch1, self._fake_patchwork2)
self.assertEqual(new_rtag_list[0], {})
# For the second commit, add Ed and Fred, so only Mary should be left
commit2.rtags = {
'Tested-by': {self.leb},
'Reviewed-by': {self.fred}}
- status.find_new_responses(new_rtag_list, 1, commit2, patch2,
- self._fake_patchwork2)
+ status.find_new_responses(new_rtag_list, review_list, 1, commit2,
+ patch2, self._fake_patchwork2)
self.assertEqual(new_rtag_list[1], {'Reviewed-by': {self.mary}})
# Check that the output patches expectations:
@@ -911,7 +912,7 @@ Series-changes: 2
series = Series()
series.commits = [commit1, commit2]
terminal.SetPrintTestMode()
- status.check_patchwork_status(series, '1234', None, None, False,
+ status.check_patchwork_status(series, '1234', None, None, False, False,
self._fake_patchwork2)
lines = iter(terminal.GetPrintTestLines())
col = terminal.Color()
@@ -927,16 +928,16 @@ Series-changes: 2
self.assertEqual(terminal.PrintLine(' 2 Subject 2', col.BLUE),
next(lines))
self.assertEqual(
- terminal.PrintLine(' Tested-by: ', col.GREEN, newline=False,
+ terminal.PrintLine(' Reviewed-by: ', col.GREEN, newline=False,
bright=False),
next(lines))
- self.assertEqual(terminal.PrintLine(self.leb, col.WHITE, bright=False),
+ self.assertEqual(terminal.PrintLine(self.fred, col.WHITE, bright=False),
next(lines))
self.assertEqual(
- terminal.PrintLine(' Reviewed-by: ', col.GREEN, newline=False,
+ terminal.PrintLine(' Tested-by: ', col.GREEN, newline=False,
bright=False),
next(lines))
- self.assertEqual(terminal.PrintLine(self.fred, col.WHITE, bright=False),
+ self.assertEqual(terminal.PrintLine(self.leb, col.WHITE, bright=False),
next(lines))
self.assertEqual(
terminal.PrintLine(' + Reviewed-by: ', col.GREEN, newline=False),
@@ -1023,7 +1024,7 @@ Series-changes: 2
terminal.SetPrintTestMode()
status.check_patchwork_status(series, '1234', branch, dest_branch,
- False, self._fake_patchwork3, repo)
+ False, False, self._fake_patchwork3, repo)
lines = terminal.GetPrintTestLines()
self.assertEqual(12, len(lines))
self.assertEqual(
@@ -1041,6 +1042,7 @@ Series-changes: 2
'Reviewed-by': {self.fred, self.mary}},
new_series.commits[1].rtags)
+ @unittest.skipIf(not HAVE_PYGIT2, 'Missing python3-pygit2')
def testParseSnippets(self):
"""Test parsing of review snippets"""
text = '''Hi Fred,
@@ -1123,3 +1125,159 @@ line8
'now a very long comment in a different file',
'line2', 'line3', 'line4', 'line5', 'line6', 'line7', 'line8']],
pstrm.snippets)
+
+ @unittest.skipIf(not HAVE_PYGIT2, 'Missing python3-pygit2')
+ def testReviewSnippets(self):
+ """Test showing of review snippets"""
+ def _to_submitter(who):
+ m_who = re.match('(.*) <(.*)>', who)
+ return {
+ 'name': m_who.group(1),
+ 'email': m_who.group(2)
+ }
+
+ commit1 = Commit('abcd')
+ commit1.subject = 'Subject 1'
+ commit2 = Commit('ef12')
+ commit2.subject = 'Subject 2'
+
+ patch1 = status.Patch('1')
+ patch1.parse_subject('[1/2] Subject 1')
+ patch1.name = patch1.raw_subject
+ patch1.content = 'This is my patch content'
+ comment1a = {'submitter': _to_submitter(self.joe),
+ 'content': '''Hi Fred,
+
+On some date Fred wrote:
+
+> diff --git a/file.c b/file.c
+> Some code
+> and more code
+
+Here is my comment above the above...
+
+
+Reviewed-by: %s
+''' % self.joe}
+
+ patch1.comments = [comment1a]
+
+ patch2 = status.Patch('2')
+ patch2.parse_subject('[2/2] Subject 2')
+ patch2.name = patch2.raw_subject
+ patch2.content = 'Some other patch content'
+ comment2a = {
+ 'content': 'Reviewed-by: %s\nTested-by: %s\n' %
+ (self.mary, self.leb)}
+ comment2b = {'submitter': _to_submitter(self.fred),
+ 'content': '''Hi Fred,
+
+On some date Fred wrote:
+
+> diff --git a/tools/patman/commit.py b/tools/patman/commit.py
+> @@ -41,6 +41,9 @@ class Commit:
+> self.rtags = collections.defaultdict(set)
+> self.warn = []
+>
+> + def __str__(self):
+> + return self.subject
+> +
+> def AddChange(self, version, info):
+> """Add a new change line to the change list for a version.
+>
+A comment
+
+Reviewed-by: %s
+''' % self.fred}
+ patch2.comments = [comment2a, comment2b]
+
+ # This test works by setting up commits and patch for use by the fake
+ # Rest API function _fake_patchwork2(). It calls various functions in
+ # the status module after setting up tags in the commits, checking that
+ # things behaves as expected
+ self.commits = [commit1, commit2]
+ self.patches = [patch1, patch2]
+
+ # Check that the output patches expectations:
+ # 1 Subject 1
+ # Reviewed-by: Joe Bloggs <joe at napierwallies.co.nz>
+ # 2 Subject 2
+ # Tested-by: Lord Edmund Blackaddër <weasel at blackadder.org>
+ # Reviewed-by: Fred Bloggs <f.bloggs at napier.net>
+ # + Reviewed-by: Mary Bloggs <mary at napierwallies.co.nz>
+ # 1 new response available in patchwork
+
+ series = Series()
+ series.commits = [commit1, commit2]
+ terminal.SetPrintTestMode()
+ status.check_patchwork_status(series, '1234', None, None, False, True,
+ self._fake_patchwork2)
+ lines = iter(terminal.GetPrintTestLines())
+ col = terminal.Color()
+ self.assertEqual(terminal.PrintLine(' 1 Subject 1', col.BLUE),
+ next(lines))
+ self.assertEqual(
+ terminal.PrintLine(' + Reviewed-by: ', col.GREEN, newline=False),
+ next(lines))
+ self.assertEqual(terminal.PrintLine(self.joe, col.WHITE), next(lines))
+
+ self.assertEqual(terminal.PrintLine('Review: %s' % self.joe, col.RED),
+ next(lines))
+ self.assertEqual(terminal.PrintLine(' Hi Fred,', None), next(lines))
+ self.assertEqual(terminal.PrintLine('', None), next(lines))
+ self.assertEqual(terminal.PrintLine(' > File: file.c', col.MAGENTA),
+ next(lines))
+ self.assertEqual(terminal.PrintLine(' > Some code', col.MAGENTA),
+ next(lines))
+ self.assertEqual(terminal.PrintLine(' > and more code', col.MAGENTA),
+ next(lines))
+ self.assertEqual(terminal.PrintLine(
+ ' Here is my comment above the above...', None), next(lines))
+ self.assertEqual(terminal.PrintLine('', None), next(lines))
+
+ self.assertEqual(terminal.PrintLine(' 2 Subject 2', col.BLUE),
+ next(lines))
+ self.assertEqual(
+ terminal.PrintLine(' + Reviewed-by: ', col.GREEN, newline=False),
+ next(lines))
+ self.assertEqual(terminal.PrintLine(self.fred, col.WHITE),
+ next(lines))
+ self.assertEqual(
+ terminal.PrintLine(' + Reviewed-by: ', col.GREEN, newline=False),
+ next(lines))
+ self.assertEqual(terminal.PrintLine(self.mary, col.WHITE),
+ next(lines))
+ self.assertEqual(
+ terminal.PrintLine(' + Tested-by: ', col.GREEN, newline=False),
+ next(lines))
+ self.assertEqual(terminal.PrintLine(self.leb, col.WHITE),
+ next(lines))
+
+ self.assertEqual(terminal.PrintLine('Review: %s' % self.fred, col.RED),
+ next(lines))
+ self.assertEqual(terminal.PrintLine(' Hi Fred,', None), next(lines))
+ self.assertEqual(terminal.PrintLine('', None), next(lines))
+ self.assertEqual(terminal.PrintLine(
+ ' > File: tools/patman/commit.py', col.MAGENTA), next(lines))
+ self.assertEqual(terminal.PrintLine(
+ ' > Line: 41 / 41: class Commit:', col.MAGENTA), next(lines))
+ self.assertEqual(terminal.PrintLine(
+ ' > + return self.subject', col.MAGENTA), next(lines))
+ self.assertEqual(terminal.PrintLine(
+ ' > +', col.MAGENTA), next(lines))
+ self.assertEqual(
+ terminal.PrintLine(' > def AddChange(self, version, info):',
+ col.MAGENTA),
+ next(lines))
+ self.assertEqual(terminal.PrintLine(
+ ' > """Add a new change line to the change list for a version.',
+ col.MAGENTA), next(lines))
+ self.assertEqual(terminal.PrintLine(
+ ' >', col.MAGENTA), next(lines))
+ self.assertEqual(terminal.PrintLine(
+ ' A comment', None), next(lines))
+ self.assertEqual(terminal.PrintLine('', None), next(lines))
+
+ self.assertEqual(terminal.PrintLine(
+ '4 new responses available in patchwork (use -d to write them to a new branch)',
+ None), next(lines))
diff --git a/tools/patman/main.py b/tools/patman/main.py
index 8f139a6e3ba..c7f425522b2 100755
--- a/tools/patman/main.py
+++ b/tools/patman/main.py
@@ -92,6 +92,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',
@@ -171,7 +173,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:
terminal.Print('patman: %s: %s' % (type(e).__name__, e),
colour=terminal.Color.RED)
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py
index 0b344614c70..798c96d9163 100644
--- a/tools/patman/patchstream.py
+++ b/tools/patman/patchstream.py
@@ -89,11 +89,12 @@ class PatchStream:
self.blank_count = 0 # Number of blank lines stored up
self.state = STATE_MSG_HEADER # What state are we in?
self.commit = None # Current commit
- self.snippets = [] # List of unquoted test blocks
+ # List of unquoted test blocks, each a list of str lines
+ self.snippets = []
self.cur_diff = None # Last 'diff' line seen (str)
self.cur_line = None # Last context (@@) line seen (str)
- self.recent_diff= None # 'diff' line for current snippet (str)
- self.recent_line= None # '@@' line for current snippet (str)
+ self.recent_diff = None # 'diff' line for current snippet (str)
+ self.recent_line = None # '@@' line for current snippet (str)
self.recent_quoted = collections.deque([], 5)
self.recent_unquoted = queue.Queue()
self.was_quoted = None
diff --git a/tools/patman/status.py b/tools/patman/status.py
index 52f401e7a40..fe48daed508 100644
--- a/tools/patman/status.py
+++ b/tools/patman/status.py
@@ -3,8 +3,9 @@
# Copyright 2020 Google LLC
#
"""Talks to the patchwork service to figure out what patches have been reviewed
-and commented on. Allows creation of a new branch based on the old but with the
-review tags collected from patchwork.
+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
@@ -123,6 +124,25 @@ class Patch(dict):
self.seq = 1
self.count = 1
+
+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 compare_with_series(series, patches):
"""Compare a list of patches with a series it came from
@@ -240,7 +260,8 @@ def collect_patches(series, series_id, rest_api=call_rest_api):
patches = sorted(patches, key=lambda x: x.seq)
return patches
-def find_new_responses(new_rtag_list, seq, cmt, patch, rest_api=call_rest_api):
+def find_new_responses(new_rtag_list, review_list, seq, cmt, patch,
+ rest_api=call_rest_api):
"""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
@@ -251,6 +272,9 @@ def find_new_responses(new_rtag_list, seq, cmt, patch, rest_api=call_rest_api):
key: Response tag (e.g. 'Reviewed-by')
value: Set of people who gave that response, each a name/email
string
+ 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
@@ -270,8 +294,13 @@ def find_new_responses(new_rtag_list, seq, cmt, patch, rest_api=call_rest_api):
data = rest_api('patches/%s/comments/' % patch.id)
+ reviews = []
for comment in data:
pstrm = PatchStream.process_text(comment['content'], True)
+ if pstrm.snippets:
+ submitter = comment['submitter']
+ person = '%s <%s>' % (submitter['name'], submitter['email'])
+ reviews.append(Review(person, pstrm.snippets))
for response, people in pstrm.commit.rtags.items():
rtags[response].update(people)
@@ -285,6 +314,7 @@ def find_new_responses(new_rtag_list, seq, cmt, patch, rest_api=call_rest_api):
if is_new:
new_rtags[tag].add(who)
new_rtag_list[seq] = new_rtags
+ review_list[seq] = reviews
def show_responses(rtags, indent, is_new):
"""Show rtags collected
@@ -301,8 +331,9 @@ def show_responses(rtags, indent, is_new):
"""
col = terminal.Color()
count = 0
- for tag, people in rtags.items():
- for who in people:
+ for tag in sorted(rtags.keys()):
+ people = rtags[tag]
+ for who in sorted(people):
terminal.Print(indent + '%s %s: ' % ('+' if is_new else ' ', tag),
newline=False, colour=col.GREEN, bright=is_new)
terminal.Print(who, colour=col.WHITE, bright=is_new)
@@ -374,7 +405,8 @@ def create_branch(series, new_rtag_list, branch, dest_branch, overwrite,
return num_added
def check_patchwork_status(series, series_id, branch, dest_branch, force,
- rest_api=call_rest_api, test_repo=None):
+ show_comments, rest_api=call_rest_api,
+ test_repo=None):
"""Check the status of a series on Patchwork
This finds review tags and comments for a series in Patchwork, displaying
@@ -386,6 +418,7 @@ def check_patchwork_status(series, series_id, branch, dest_branch, force,
branch (str): Existing branch to update, or None
dest_branch (str): Name of new branch to create, or None
force (bool): True to force overwriting dest_branch if it exists
+ show_comments (bool): True to show the comments on each patch
rest_api (function): API function to call to access Patchwork, for
testing
test_repo (pygit2.Repository): Repo to use (use None unless testing)
@@ -394,6 +427,7 @@ def check_patchwork_status(series, series_id, branch, dest_branch, force,
col = terminal.Color()
count = len(series.commits)
new_rtag_list = [None] * count
+ review_list = [None] * count
patch_for_commit, _, warnings = compare_with_series(series, patches)
for warn in warnings:
@@ -403,8 +437,8 @@ def check_patchwork_status(series, series_id, branch, dest_branch, force,
with concurrent.futures.ThreadPoolExecutor(max_workers=16) as executor:
futures = executor.map(
- find_new_responses, repeat(new_rtag_list), range(count),
- series.commits, patch_list, repeat(rest_api))
+ find_new_responses, repeat(new_rtag_list), repeat(review_list),
+ range(count), series.commits, patch_list, repeat(rest_api))
for fresponse in futures:
if fresponse:
raise fresponse.exception()
@@ -423,6 +457,15 @@ def check_patchwork_status(series, series_id, branch, dest_branch, force,
indent = ' ' * 2
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)
+ terminal.Print()
terminal.Print("%d new response%s available in patchwork%s" %
(num_to_add, 's' if num_to_add != 1 else '',
--
2.29.0.rc2.309.g374f81d7ae-goog
More information about the Patchwork
mailing list