]> git.dujemihanovic.xyz Git - u-boot.git/commitdiff
patman: Support listing comments from patchwork
authorSimon Glass <sjg@chromium.org>
Fri, 30 Oct 2020 03:46:38 +0000 (21:46 -0600)
committerSimon Glass <sjg@chromium.org>
Thu, 5 Nov 2020 16:11:31 +0000 (09:11 -0700)
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
tools/patman/control.py
tools/patman/func_test.py
tools/patman/main.py
tools/patman/patchstream.py
tools/patman/status.py

index 15da6dc33cbf2b2d13cc70a4a409cc83d842ceaf..49b73590cf00c73b50c576be0bf5bfb449e40047 100644 (file)
@@ -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
-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>
 
@@ -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
index 6ac258d41d70624a4202227bfbf52d4c876188b1..f4a6ca145d4fb9b247a7f799d43741c3df62ee55 100644 (file)
@@ -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)
index bbee4b77d66b725c07d65a7035291e87c51025a3..e2adf32c73911d4c316b56d1ae69c7668901da45 100644 (file)
@@ -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,
-                                  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}})
@@ -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,
-                                  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:
@@ -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()
@@ -914,16 +915,16 @@ 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,
+            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),
@@ -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))
index 8f139a6e3ba6a1512e39c200c2d52db918a02207..c7f425522b2a5c970d3439f834a28dc958ec685a 100755 (executable)
@@ -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)
index c165bc18251d04ab18bf352aa851d577384f6078..772e4b56617e658c3e5ac0d916dc88ce266a9c20 100644 (file)
@@ -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
index f3a654160ecff9c627ad5c93b921559107a52227..a369d655c5e48b7695ad549e74672840e444d3de 100644 (file)
@@ -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
@@ -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 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)
@@ -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),
-            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()
@@ -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 '',