[RFC PATCH 00/16] RFC: patman: Collect review tags and comments from Patchwork

Wed Jul 15 11:10:06 CEST 2020

Hi Simon,
On 06. 07. 20 5:41, Simon Glass wrote:
> Patman is a useful tool for creating, checking sending out patches. It
> automates the creation of patches, simplifies checking them and handles
> cover letters and change logs.
> 
> However once patches are sent and reviewers add Reviewed-by tags, etc.,
> these must be manually added into the commits using git before the next
> version of the series is sent. Also, the only way to look at patches is
> in the web interface.
> 
> It would be nice if patman could talk to Patchwork and collect the review
> tags itself. Even better if it could show the review comments in a
> command-line view patch-by-patch to speed up addressing comments.
> 
> This series adds these features to patman, talking directly to the web
> URLs. While pwclient seems like it could handle some of this, or at least
> provide a library to patman, the documentation[1] doesn't really explain
> what the commands do and it doesn't seem to work with the current
> Patchwork (e.g. pwclient git-am requires a patch number but Patchwork
> seems to use an ID now).
> 
> With these changes, which currently lack tests, it is possible to type
> 'patman status' and see a list of new review tags. It is also possible
> to create a new branch with those tags and list out the review comments
> as small snippets on the command line.
> 
> I have been using these features for a short while and they seem useful,
> so herewith a series.
> 
> This series depends on u-boot-dm/next so you might find it easier to get
> it from u-boot-dm/patman-working[2]
> 
> [1] https://patchwork.readthedocs.io/projects/pwclient/en/latest/usage/
> [2] https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/patman-working
> 
> 
> Simon Glass (16):
>   patman: Use test_util to show test results
>   patman: Move main code out to a control module
>   patman: Add a test that uses gitpython
>   patman: Allow creating patches for another branch
>   patman: Allow skipping patches at the end
>   patman: Convert to ArgumentParser
>   patman: Allow different commands
>   patman: Add a 'test' subcommand
>   patman: Allow disabling 'bright' mode with Print output
>   patman: Support collecting response tags in Patchstream
>   patman: Allow linking a series with patchwork
>   patman: Add a -D option to enable debugging
>   patchstream: Support parsing of review snippets
>   patman: Support checking for review tags in patchwork
>   patman: Support updating a branch with review tags
>   patman: Support listing comments from patchwork
> 
>  .azure-pipelines.yml        |   2 +-
>  .gitlab-ci.yml              |   2 +-
>  .travis.yml                 |   2 +-
>  test/run                    |   2 +-
>  tools/patman/README         |  93 ++++++-
>  tools/patman/checkpatch.py  |   6 +
>  tools/patman/commit.py      |  14 ++
>  tools/patman/control.py     | 214 ++++++++++++++++
>  tools/patman/func_test.py   | 170 ++++++++++++-
>  tools/patman/gitutil.py     |  23 +-
>  tools/patman/main.py        | 219 ++++++++---------
>  tools/patman/patchstream.py |  82 ++++++-
>  tools/patman/series.py      |   4 +-
>  tools/patman/settings.py    |  10 +-
>  tools/patman/status.py      | 476 ++++++++++++++++++++++++++++++++++++
>  tools/patman/terminal.py    |   4 +-
>  tools/patman/test_util.py   |   8 +-
>  tools/patman/tools.py       |   4 +-
>  18 files changed, 1178 insertions(+), 157 deletions(-)
>  create mode 100644 tools/patman/control.py
>  create mode 100644 tools/patman/status.py
> 

I have tried your series because I use patman for quite a long time.
I have done these steps:

...
git pull https://gitlab.denx.de/u-boot/custodians/u-boot-dm.git
patman-working
git-cherry-pick <sha1> (my sifive v2 series)
-- Add Series-link based on patchwork which points to v2

HEAD commit looks like this:

commit b99568c40f4a2017a5dccbf96c4e5bb5ab0c7ea3 (HEAD -> simon)
Author:     Michal Simek <michal.simek at xilinx.com>
AuthorDate: Thu Jul 9 10:58:47 2020 +0200
Commit:     Michal Simek <michal.simek at xilinx.com>
CommitDate: Wed Jul 15 10:05:51 2020 +0200

    serial: Fix SIFIVE debug serial dependency

    The commit 4cc24aeaf420 ("serial: Add missing Kconfig dependencies for
    debug consoles") has added incorrect dependency for SIFIVE debug
uart which
    should depend on SIFIVE driver instead of PL01x.

    Fixes: 4cc24aeaf420 ("serial: Add missing Kconfig dependencies for
debug consoles")
    Signed-off-by: Michal Simek <michal.simek at xilinx.com>

    Series-to: uboot
    Series-version: 2
    Series-changes: 2
    - Add fixes tag - asked by Simon

    Series-link:
https://patchwork.ozlabs.org/project/uboot/list/?series=188865

And the running status on last one patch (because your series has also
other patches.
[u-boot](simon)$ rm -f 0* && /mnt/disk/u-boot/tools/patman/patman status
-c 1 -D
patman: ValueError: Cannot parse sequence spec 'v2'

Traceback (most recent call last):
  File "/mnt/disk/u-boot/tools/patman/patman", line 167, in <module>
    args.show_comments)
  File
"/home/monstr/data/disk/u-boot/tools/patman/../patman/control.py", line
214, in patchwork_status
    show_comments)
  File "/home/monstr/data/disk/u-boot/tools/patman/../patman/status.py",
line 433, in check_patchwork_status
    patches = collect_patches(series, url)
  File "/home/monstr/data/disk/u-boot/tools/patman/../patman/status.py",
line 271, in collect_patches
    patch.add(name, col.get_text())
  File "/home/monstr/data/disk/u-boot/tools/patman/../patman/status.py",
line 135, in add
    raise ValueError("Cannot parse sequence spec '%s'" % seq_info)
ValueError: Cannot parse sequence spec 'v2'

What do you think can be wrong? I expect your regex strings.

Anyway here are my comments.
1) -e options is not recorded in readme

2) this is the first time I read about patman test and I see that README
should be also updated and remove at least --test which you switched to
subcommand test
Would be also good to see what that command is really doing. It means
having verbose mode or so would be helpful.
Anyway based on strace I see that it touches patman/test/ folder that's
why it indicates that it is just patman internal tests.
If yes - this patchwork feature should be also added there.

3) You are collecting responses as Fixes, Tested-by, etc.
I am missing there also Reported-by:

4)
Last but not least is that Series-link. It seems to me that it is quite
time consuming to search for it and this would require more work to do.
When patman send message to mailing list it also generate Message-id. I
think you could be able to append/amend it when message is sent.
Then you should use this message-id to get status back based on it.
It could be enough for communication with patchwork and collect tags and
create another series.

Another thing what I see is that after sending you have all message ID
and you can use the same style as Linux is using for setting up Link: to
lore.kernel.org/r/
With the same style you can add Link:
https://patchwork.ozlabs.org/project/uboot/patch/<Message-id>
when you send patches.

Then you can use this Link which will be recorded in every patch to show
status/feedback/collect tags, etc.

Thanks,
Michal