patman: Support listing comments from patchwork

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

@@ -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,
@@ -400,6 +401,8 @@ end. 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
 =================
@@ -484,17 +487,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
+and use git rebase -i to edit the commits, dropping the wip one.
-the ack tag to one commit:
 
-    Acked-by: Heiko Schocher <hs@denx.de>
+Then update the Series-cc: in the top commit to add the person who reviewed
-
+the v1 series:
-update the Series-cc: in the top commit:
 
     Series-cc: bfin, marex, Heiko Schocher <hs@denx.de>
 
@@ -533,7 +552,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
@@ -613,3 +634,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

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)

tools/patman/func_test.py

@@ -857,15 +857,16 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
         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,
+        status.find_new_responses(new_rtag_list, review_list, 0, commit1,
-                                  self._fake_patchwork2)
+                                  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,
+        status.find_new_responses(new_rtag_list, review_list, 1, commit2,
-                                  self._fake_patchwork2)
+                                  patch2, self._fake_patchwork2)
         self.assertEqual(new_rtag_list[1], {
             'Reviewed-by': {self.mary, self.fred},
             'Tested-by': {self.leb}})
@@ -874,16 +875,16 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
         # '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,
+        status.find_new_responses(new_rtag_list, review_list, 0, commit1,
-                                  self._fake_patchwork2)
+                                  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,
+        status.find_new_responses(new_rtag_list, review_list, 1, commit2,
-                                  self._fake_patchwork2)
+                                  patch2, self._fake_patchwork2)
         self.assertEqual(new_rtag_list[1], {'Reviewed-by': {self.mary}})
 
         # Check that the output patches expectations:
@@ -898,7 +899,7 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
         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()
@@ -913,18 +914,18 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
 
         self.assertEqual(terminal.PrintLine('  2 Subject 2', col.BLUE),
                          next(lines))
-        self.assertEqual(
-            terminal.PrintLine('    Tested-by: ', col.GREEN, newline=False,
-                               bright=False),
-            next(lines))
-        self.assertEqual(terminal.PrintLine(self.leb, col.WHITE, bright=False),
-                         next(lines))
         self.assertEqual(
             terminal.PrintLine('    Reviewed-by: ', col.GREEN, newline=False,
                                bright=False),
             next(lines))
         self.assertEqual(terminal.PrintLine(self.fred, col.WHITE, bright=False),
                          next(lines))
+        self.assertEqual(
+            terminal.PrintLine('    Tested-by: ', col.GREEN, newline=False,
+                               bright=False),
+            next(lines))
+        self.assertEqual(terminal.PrintLine(self.leb, col.WHITE, bright=False),
+                         next(lines))
         self.assertEqual(
             terminal.PrintLine('  + Reviewed-by: ', col.GREEN, newline=False),
             next(lines))
@@ -1010,7 +1011,7 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
 
         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(
@@ -1044,6 +1045,7 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
         self.assertEqual('Reviewed-by: %s' % self.mary, next(lines))
         self.assertEqual('Tested-by: %s' % self.leb, next(lines))
 
+    @unittest.skipIf(not HAVE_PYGIT2, 'Missing python3-pygit2')
     def testParseSnippets(self):
         """Test parsing of review snippets"""
         text = '''Hi Fred,
@@ -1126,3 +1128,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@napierwallies.co.nz>
+        #   2 Subject 2
+        #     Tested-by: Lord Edmund Blackaddër <weasel@blackadder.org>
+        #     Reviewed-by: Fred Bloggs <f.bloggs@napier.net>
+        #   + Reviewed-by: Mary Bloggs <mary@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))

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)

tools/patman/patchstream.py

@@ -89,7 +89,8 @@ 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)

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
+and commented on. Provides a way to display review tags and comments.
-review tags collected from patchwork.
+Allows creation of a new branch based on the old but with the review tags
+collected from patchwork.
 """
 
 import collections
@@ -124,6 +125,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
 
@@ -241,7 +261,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
@@ -252,6 +273,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
@@ -271,8 +295,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)
 
@@ -286,6 +315,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
@@ -302,8 +332,9 @@ def show_responses(rtags, indent, is_new):
     """
     col = terminal.Color()
     count = 0
-    for tag, people in rtags.items():
+    for tag in sorted(rtags.keys()):
-        for who in people:
+        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)
@@ -376,7 +407,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
@@ -388,6 +420,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)
@@ -396,6 +429,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:
@@ -405,8 +439,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),
+            find_new_responses, repeat(new_rtag_list), repeat(review_list),
-            series.commits, patch_list, repeat(rest_api))
+            range(count), series.commits, patch_list, repeat(rest_api))
     for fresponse in futures:
         if fresponse:
             raise fresponse.exception()
@@ -425,6 +459,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 '',