[v2] pwclient: Add heuristics to find a whole series of patches

Add a new filter option '-r' that attempts to list all patches in a
series.  Since there's no built-in way in patman to do this, we use
some heuristics to try to find the series.

Signed-off-by: Doug Anderson <dianders@chromium.org>

---
Changes in v2:
- Handle more tag formats; use tags besides just version/num parts
  (like RFC, REPOST, etc) to identify a series

 apps/patchwork/bin/pwclient |  151 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 147 insertions(+), 4 deletions(-)

Em Thu, 20 Dec 2012 13:06:07 -0800
Doug Anderson <dianders@chromium.org> escreveu:

> Add a new filter option '-r' that attempts to list all patches in a
> series.  Since there's no built-in way in patman to do this, we use
> some heuristics to try to find the series.

Instead of adding it at the client level, IMHO, it would be a way better
to add it at the patchwork server, allowing to show the patch group
as such and letting tag the entire patch group with the same tag
(as a v2 of a patch series superseeds a v1 of the same series).

Regards,
Mauro

> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> 
> ---
> Changes in v2:
> - Handle more tag formats; use tags besides just version/num parts
>   (like RFC, REPOST, etc) to identify a series
> 
>  apps/patchwork/bin/pwclient |  151 +++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 147 insertions(+), 4 deletions(-)
> 
> diff --git a/apps/patchwork/bin/pwclient b/apps/patchwork/bin/pwclient
> index 9588615..77d78c7 100755
> --- a/apps/patchwork/bin/pwclient
> +++ b/apps/patchwork/bin/pwclient
> @@ -23,11 +23,14 @@ import os
>  import sys
>  import xmlrpclib
>  import getopt
> +import re
>  import string
> +import time
>  import tempfile
>  import subprocess
>  import base64
>  import ConfigParser
> +import collections
>  
>  # Default Patchwork remote XML-RPC server URL
>  # This script will check the PW_XMLRPC_URL environment variable
> @@ -79,6 +82,81 @@ class Filter:
>          """Return human-readable description of the filter."""
>          return str(self.d)
>  
> +class Patch(object):
> +    """Nicer representation of a patch from the server."""
> +
> +    def __init__(self, patch_dict):
> +        """Patch constructor.
> +
> +        @patch_dict: The dictionary version of the patch.
> +        """
> +        # Make it easy to compare times of patches by getting an int.
> +        self.time = time.mktime(time.strptime(patch_dict["date"],
> +                                              "%Y-%m-%d %H:%M:%S"))
> +
> +        self.version, self.part_num, self.num_parts, self.series_tags = \
> +            self._parse_patch_name(patch_dict["name"])
> +
> +        # Add a few things to make it easier...
> +        self.id = patch_dict["id"]
> +        self.project_id = patch_dict["project_id"]
> +        self.name = patch_dict["name"]
> +        self.submitter_id = patch_dict["submitter_id"]
> +
> +        # Keep the dict in case we need anything else...
> +        self.dict = patch_dict
> +
> +    @staticmethod
> +    def _parse_patch_name(name):
> +        """Parse tags out of a patch name.
> +
> +
> +        @name: The patch name.
> +        @return: version: integer version of the patch
> +        @return: part_num: integer part number of the patch
> +        @return: num_parts: integer number of parts in the patch
> +        @return: series_tags: A tuple of tags that should be shared by all
> +                 patches in this series.  Should be treated as opaque other
> +                 than comparing equality with other patches.
> +        """
> +        version = 1
> +        part_num = 1
> +        num_parts = 1
> +        series_tags = []
> +
> +        # Pull out tags between []; bail if tags aren't found.
> +        mo = re.match(r"\[([^\]]*)\]", name)
> +        if mo:
> +            tags = mo.group(1).split(',')
> +
> +            # Work on one tag at a time
> +            for tag in tags:
> +                mo = re.match(r"(\d*)/(\d*)", tag)
> +                if mo:
> +                    part_num = int(mo.group(1))
> +                    num_parts = int(mo.group(2))
> +                    continue
> +
> +                mo = re.match(r"[vV](\d*)", tag)
> +                if mo:
> +                    version = int(mo.group(1))
> +
> +                series_tags.append(tag)
> +
> +        # Add num_parts to the series tags
> +        series_tags.append("%d parts" % num_parts)
> +
> +        # Turn series_tags into a tuple so it's hashable
> +        series_tags = tuple(series_tags)
> +
> +        return (version, part_num, num_parts, series_tags)
> +
> +    def __str__(self):
> +        return str(self.dict)
> +
> +    def __repr__(self):
> +        return repr(self.dict)
> +
>  class BasicHTTPAuthTransport(xmlrpclib.SafeTransport):
>  
>      def __init__(self, username = None, password = None, use_https = False):
> @@ -128,7 +206,8 @@ def usage():
>          -w <who>      : Filter by submitter (name, e-mail substring search)
>          -d <who>      : Filter by delegate (name, e-mail substring search)
>          -n <max #>    : Restrict number of results
> -        -m <messageid>: Filter by Message-Id\n""")
> +        -m <messageid>: Filter by Message-Id
> +        -r <ID>       : Filter by patches in the same series as <ID>\n""")
>      sys.stderr.write("""\nActions that take an ID argument can also be \
>  invoked with:
>          -h <hash>     : Lookup by patch hash\n""")
> @@ -162,6 +241,56 @@ def person_ids_by_name(rpc, name):
>      people = rpc.person_list(name, 0)
>      return map(lambda x: x['id'], people)
>  
> +def patch_id_to_series(rpc, patch_id):
> +    """Take a patch ID and return a list of patches in the same series.
> +
> +    This function uses the following heuristics to find patches in a series:
> +    - It searches for all patches with the same submitter that the same version
> +      number and same number of parts.
> +    - It allows patches to span multiple projects (though they must all be on
> +      the same patchwork server), though it prefers patches that are part of
> +      the same project.  This handles cases where some parts in a series might
> +      have only been sent to a topic project (like "linux-mmc").
> +    - For each part number it finds the matching patch that has a date value
> +      closest to the original patch.
> +
> +    It would be nice to use "Message-ID" and "In-Reply-To", but that's not
> +    exported to the xmlrpc interface as far as I can tell.  :(
> +
> +    @patch_id: The patch ID that's part of the series.
> +    @return: A list of patches in the series.
> +    """
> +    # Find this patch
> +    patch = Patch(rpc.patch_get(patch_id))
> +
> +    # Get the all patches by the submitter, ignoring project.
> +    filter = Filter()
> +    filter.add("submitter_id", patch.submitter_id)
> +    all_patches = [Patch(p) for p in rpc.patch_list(filter.d)]
> +
> +    # Whittle down--only those with matching series_tags.
> +    all_patches = [p for p in all_patches if p.series_tags == patch.series_tags]
> +
> +    # Organize by part_num.
> +    by_part_num = collections.defaultdict(list)
> +    for p in all_patches:
> +        by_part_num[p.part_num].append(p)
> +
> +    # Find the part that's closest in time to ours for each part num.
> +    final_list = []
> +    for part_num, patch_list in sorted(by_part_num.iteritems()):
> +        # Create a list of tuples to make sorting easier.  We want to find
> +        # the patch that has the closet time.  If there's a tie then we want
> +        # the patch that has the same project ID...
> +        patch_list = [(abs(p.time - patch.time),
> +                       abs(p.project_id - patch.project_id),
> +                       p) for p in patch_list]
> +
> +        best = sorted(patch_list)[0][-1]
> +        final_list.append(best)
> +
> +    return final_list
> +
>  def list_patches(patches):
>      """Dump a list of patches to stdout."""
>      print("%-5s %-12s %s" % ("ID", "State", "Name"))
> @@ -169,9 +298,20 @@ def list_patches(patches):
>      for patch in patches:
>          print("%-5d %-12s %s" % (patch['id'], patch['state'], patch['name']))
>  
> -def action_list(rpc, filter, submitter_str, delegate_str):
> +def action_list(rpc, filter, submitter_str, delegate_str, series_str):
>      filter.resolve_ids(rpc)
>  
> +    if series_str != "":
> +        try:
> +            patch_id = int(series_str)
> +        except:
> +            sys.stderr.write("Invalid patch ID given\n")
> +            sys.exit(1)
> +
> +        patches = patch_id_to_series(rpc, patch_id)
> +        list_patches([patch.dict for patch in patches])
> +        return
> +
>      if submitter_str != "":
>          ids = person_ids_by_name(rpc, submitter_str)
>          if len(ids) == 0:
> @@ -320,7 +460,7 @@ auth_actions = ['update']
>  
>  def main():
>      try:
> -        opts, args = getopt.getopt(sys.argv[2:], 's:p:w:d:n:c:h:m:')
> +        opts, args = getopt.getopt(sys.argv[2:], 's:p:w:d:n:c:h:m:r:')
>      except getopt.GetoptError, err:
>          print str(err)
>          usage()
> @@ -337,6 +477,7 @@ def main():
>      project_str = ""
>      commit_str = ""
>      state_str = ""
> +    series_str = ""
>      hash_str = ""
>      msgid_str = ""
>      url = DEFAULT_URL
> @@ -354,6 +495,8 @@ def main():
>      for name, value in opts:
>          if name == '-s':
>              state_str = value
> +        elif name == '-r':
> +            series_str = value
>          elif name == '-p':
>              project_str = value
>          elif name == '-w':
> @@ -424,7 +567,7 @@ def main():
>      if action == 'list' or action == 'search':
>          if len(args) > 0:
>              filt.add("name__icontains", args[0])
> -        action_list(rpc, filt, submitter_str, delegate_str)
> +        action_list(rpc, filt, submitter_str, delegate_str, series_str)
>  
>      elif action.startswith('project'):
>          action_projects(rpc)

Hi Doug,

> Add a new filter option '-r' that attempts to list all patches in a
> series.  Since there's no built-in way in patman to do this, we use
> some heuristics to try to find the series.

Interesting idea, something I'm sure would be useful for many patchwork 
users.

Do you think it would be possible to do this in the actual server code 
instead, so that the "series relations" are captured in the database? 
This way, the series would be available through the web interface too, 
and the pwclient method of accessing these series would be much more 
straightforward.

The difficult part of this is how to present the series relations in the 
web UI, but I can work something out there.

Cheers,


Jeremy

Jeremy,

On Thu, Dec 20, 2012 at 5:23 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Doug,
>
>
>> Add a new filter option '-r' that attempts to list all patches in a
>> series.  Since there's no built-in way in patman to do this, we use
>> some heuristics to try to find the series.
>
>
> Interesting idea, something I'm sure would be useful for many patchwork
> users.
>
> Do you think it would be possible to do this in the actual server code
> instead, so that the "series relations" are captured in the database? This
> way, the series would be available through the web interface too, and the
> pwclient method of accessing these series would be much more
> straightforward.

I will git it a shot.  I'll need to get a local patchwork server setup
for testing (so far I'm just using patchwork.kernel.org for my
development on the client), so there might be a bit of a lag before I
post a patch for that.


> The difficult part of this is how to present the series relations in the web
> UI, but I can work something out there.

Sure.  I'll try to add the backend and xmlrpc code for this (and test
it via pwclient) and leave any UI work to you.


-Doug

Hi Doug,

> I will git it a shot.  I'll need to get a local patchwork server setup
> for testing (so far I'm just using patchwork.kernel.org for my
> development on the client), so there might be a bit of a lag before I
> post a patch for that.

Should just be a matter of:

createdb patchwork
git clone git://git.ozlabs.org/home/jk/git/patchwork
cd patchwork/apps
touch local_settings.py

./manage.py syncdb
./manage.py runserver

xdg-open http://localhost:8000/

Feel free to ping me on IRC (jk- on freenode) if you need a hand.

Cheers,


Jeremy

On Thu, Dec 20, 2012 at 5:38 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
>
> Should just be a matter of:
>
> createdb patchwork
> git clone git://git.ozlabs.org/home/jk/git/patchwork
> cd patchwork/apps
> touch local_settings.py
>
> ./manage.py syncdb
> ./manage.py runserver
>
> xdg-open http://localhost:8000/

You make the assumption that I've already go some of the prereqs
installed and am familiar with them.  ;)  I've run apache servers
before, setup databases before, and got django installed before.
...but not on my current system.

I will get to it, but it will take some time...


> Feel free to ping me on IRC (jk- on freenode) if you need a hand.

Thanks for the offer.  If I get stuck I'll send you a ping.  :)


-Doug

Em Fri, 21 Dec 2012 09:23:30 +0800
Jeremy Kerr <jk@ozlabs.org> escreveu:

> Hi Doug,
> 
> > Add a new filter option '-r' that attempts to list all patches in a
> > series.  Since there's no built-in way in patman to do this, we use
> > some heuristics to try to find the series.
> 
> Interesting idea, something I'm sure would be useful for many patchwork 
> users.
> 
> Do you think it would be possible to do this in the actual server code 
> instead, so that the "series relations" are captured in the database? 
> This way, the series would be available through the web interface too, 
> and the pwclient method of accessing these series would be much more 
> straightforward.
> 
> The difficult part of this is how to present the series relations in the 
> web UI, but I can work something out there.

Btw, there are two cases that should likely be handled as well:

1) on a patch series with a patch 0/x, patches 1/x to x/x refers to
patch 0/x;

2) sometimes, someone sends an ack/nack/... tag to the a hole patch series,
replying to patch 0/x.

It would be really nice if patchwork server/database could handle both.

Regards,
Mauro

FWIW, here's the steps I used to get some basic functionality on my Ubuntu box:


# Install postgres and psycopg2
sudo aptitude install postgresql python2.7-psycopg2

# Add a postgres user for the current user; give ability
# to create databases
sudo -u postgres createuser -dSR "${USER}"

# Add users for www-data and nobody
sudo -u postgres createuser -DSR nobody
sudo -u postgres createuser -DSR www-data

# Create the patchwork database
createdb patchwork

# Mostly from docs/INSTALL
mkdir -p lib/packages lib/python

cd lib/packages

# Fix from docs: add django as dest folder
svn checkout http://code.djangoproject.com/svn/django/tags/releases/1.2 django
cd ../python
ln -s ../packages/django/django ./django

cd ../..

# Fix from docs: tablednd no longer has version number upstream
cd lib/packages
mkdir jquery
cd jquery
wget http://jqueryjs.googlecode.com/files/jquery-1.3.min.js
wget http://www.isocra.com/articles/jquery.tablednd.js.zip
unzip jquery.tablednd.js.zip jquery.tablednd.js
cd ../../../htdocs/js/
ln -s ../../lib/packages/jquery/jquery-1.3.min.js ./
rm jquery.tablednd_0_5.js
ln -s ../../lib/packages/jquery/jquery.tablednd.js jquery.tablednd_0_5.js

cd ../..

cd apps
echo 'ENABLE_XMLRPC = True' > local_settings.py

PYTHONPATH=../lib/python ./manage.py syncdb
PYTHONPATH=../lib/python ./manage.py runserver

xdg-open http://localhost:8000/admin

# Add project LKML with list ID linux-kernel.vger.kernel.org

# Run attached (rough) "imapfeeder.py" to populate database


I'll try to incorporate some of the more obvious fixes above and send
a patch for docs/INSTALL, but probably won't have time to add all of
the above...

-Doug

This series of patches replaces the old patch "pwclient: Add
heuristics to find a whole series of patches" and moves the concept of
a patch series over to the server side.

This patch series only adds enough support to get a series list from
pwclient.  Future patches could be submitted to:
* Add UI to the patchwork web site to expose this.
* Add better support for dealing with patch serieses that have a cover
  letter (like this one) that get acks or nacks to the cover letter.


Doug Anderson (3):
  models: Add to_series() method to models
  xmlrpc: Export the "to_series" method in patch objects
  pwclient: Add command to find a whole series of patches

 apps/patchwork/bin/pwclient    |   23 ++++-
 apps/patchwork/models.py       |  184 ++++++++++++++++++++++++++++++++++++++++
 apps/patchwork/views/xmlrpc.py |   22 +++++
 3 files changed, 225 insertions(+), 4 deletions(-)

Hi Doug,

> This series of patches replaces the old patch "pwclient: Add
> heuristics to find a whole series of patches" and moves the concept of
> a patch series over to the server side.

I'd like to propose a different way of achieving this: rather than try 
to piece-together the patch series at query time, I think it'd be better 
to construct the series when the patches are first parsed. Here we can 
use both the message ids (ie, In-Reply-To and References headers) and 
the subjects to link patches into their correct series.

I'm working on some changes to do this, which will need some updates to 
the patchwork model structure. Either:

1) Keeping the Patch objects as they are, and introducing a Series model 
which keeps track of patches within a series; or

2) Adding a superclass model ("Item"), which has three subclasses: 
Patch, PullRequest and Series. All of the current "patch lists" now 
become lists of these Item instances, which may actually be one of the 
three subclasses.

The second approach is more work, but might make for cleaner views. So, 
I'm doing some experimentation now, and will post my code in a separate 
branch for some initial testing once it's workable. Comments and 
suggestions most welcome :)

One really helpful thing would be some contributions for testcases; 
especially when the parser receives patches out-of-order.

Cheers,


Jeremy

Jeremy,

On Mon, Jan 7, 2013 at 2:55 AM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Doug,
>
>
>> This series of patches replaces the old patch "pwclient: Add
>> heuristics to find a whole series of patches" and moves the concept of
>> a patch series over to the server side.
>
>
> I'd like to propose a different way of achieving this: rather than try to
> piece-together the patch series at query time, I think it'd be better to
> construct the series when the patches are first parsed. Here we can use both
> the message ids (ie, In-Reply-To and References headers) and the subjects to
> link patches into their correct series.

Constructing the series as things are parsed does seem cleaner.
However, a few thoughts:

* It looks like "In-Reply-To" isn't super easy as a method for
collecting groups of patches since git send-email can run in a number
of different modes (chained replies vs not and also the "in-reply-to"
option).  It could be used (and would be more robust than my
heuristics), but we need to be careful to test all of the different
modes.

* In practice, the heuristics that I've used seem to work really well
to identify groups of patches.  I have yet to see them fail as long as
all of the patches are actually visible.


> I'm working on some changes to do this, which will need some updates to the
> patchwork model structure. Either:
>
> 1) Keeping the Patch objects as they are, and introducing a Series model
> which keeps track of patches within a series; or
>
> 2) Adding a superclass model ("Item"), which has three subclasses: Patch,
> PullRequest and Series. All of the current "patch lists" now become lists of
> these Item instances, which may actually be one of the three subclasses.
>
> The second approach is more work, but might make for cleaner views. So, I'm
> doing some experimentation now, and will post my code in a separate branch
> for some initial testing once it's workable. Comments and suggestions most
> welcome :)

OK, sounds good.  I don't have a strong opinion since I don't know
that part of the patchwork code too well.  I'm happy to look over your
changes, though I probably won't be able to keep doing heavy
contribution of patches, since I've got to get back to my normal job.
;)


> One really helpful thing would be some contributions for testcases;
> especially when the parser receives patches out-of-order.

What format are you looking for for test cases?  I'm happy to dig
through email folders and find some interesting ones.


-Doug

Hi Doug,

> * It looks like "In-Reply-To" isn't super easy as a method for
> collecting groups of patches since git send-email can run in a number
> of different modes (chained replies vs not and also the "in-reply-to"
> option).  It could be used (and would be more robust than my
> heuristics), but we need to be careful to test all of the different
> modes.

Yes, and I think we can handle all of these, especially as we keep the 
message-id of every patch and comment; we could scan the contents of the 
In-Reply-To and References header looking for linkages to patches in the 
same series.

> * In practice, the heuristics that I've used seem to work really well
> to identify groups of patches.  I have yet to see them fail as long as
> all of the patches are actually visible.

I was planning to use some of your logic in the patch parser too :)

>> One really helpful thing would be some contributions for testcases;
>> especially when the parser receives patches out-of-order.
>
> What format are you looking for for test cases?  I'm happy to dig
> through email folders and find some interesting ones.

Ideally, these would be additions to the existing tests in 
apps/patchwork/tests. Check out patchparser.py in particular. Of course, 
this will depend on which implementation we end up using, so maybe just 
collect a few examples of different threading styles for now.

Cheers,


Jeremy