[PATCH v2] Fix parsing of interesing series reply structures

[Stephen Fincane]

On Sat, 2016-11-19 at 00:13 +1100, Daniel Axtens wrote:
> Stephen Fincane <stephen at that.guru> writes:
> 
> > 
> > On Thu, 2016-11-17 at 16:11 +1100, Daniel Axtens wrote:
> > > 
> > > There are some things you probably shouldn't do on public
> > > mailing lists, but which people do anyway.
> > > 
> > > The first, and most understandable, is this:
> > > 
> > >           - [PATCH 1/2] test: Add some lorem ipsum
> > >             - [PATCH 2/2] test: Convert to Markdown
> > >               - [PATCH v2 1/2] test: Add some lorem ipsum
> > >                 - [PATCH v2 2/2] test: Convert to Markdown
> > > 
> > > We should correctly parse these by:
> > >  - creating a new series if the version number changes
> > >  - when deciding whether to create a SeriesReference, search by
> > >    message-id alone, not the message-id/series pair. (Otherwise,
> > >    we try to create a series ref for v1 2/2 in the series for v2,
> > >    which breaks a uniqueness constraint.
> > > 
> > > The second, and less excusable, is this:
> > > 
> > >           - [PATCH 1/2] test: Add some lorem ipsum
> > >             - [PATCH 2/2] test: Convert to Markdown
> > >               - [PATCH 1/2] test: Add some lorem ipsum
> > >                 - [PATCH 2/2] test: Convert to Markdown
> > > 
> > > With this patch:
> > >  - if we get a x/n for a series that already has an x/n, create a
> > >    new series for it.
> > > 
> > > Signed-off-by: Daniel Axtens <dja at axtens.net>
> > 
> > I've got two suggestions for improvement that I'd like to make when
> > merging, but they're non-blocking. Lovely, tidy fixes.
> > 
> > Reviewed-by: Stephen Finucane <stephen at that.guru>
> > 
> > > 
> > > ---
> > > 
> > > v2: - drop a hunk that I created during debugging but which was
> > > no
> > >       longer required.
> > >     - better support for the second scenario - I had mistakenly
> > > thought
> > >       it wouldn't work because of an earier typo.
> > > ---
> > >  patchwork/parser.py                                |  22 +-
> > >  patchwork/tests/series/bugs-nocover-noversion.mbox | 222
> > > +++++++++++++++++++++
> > >  patchwork/tests/series/bugs-nocover.mbox           | 222
> > > +++++++++++++++++++++
> > >  patchwork/tests/test_series.py                     |  43 ++++
> > >  4 files changed, 507 insertions(+), 2 deletions(-)
> > >  create mode 100644 patchwork/tests/series/bugs-nocover-
> > > noversion.mbox
> > >  create mode 100644 patchwork/tests/series/bugs-nocover.mbox
> > > 
> > > diff --git a/patchwork/parser.py b/patchwork/parser.py
> > > index b1544c951a3f..c7890ea18c2d 100644
> > > --- a/patchwork/parser.py
> > > +++ b/patchwork/parser.py
> > > @@ -41,6 +41,7 @@ from patchwork.models import Person
> > >  from patchwork.models import Project
> > >  from patchwork.models import Series
> > >  from patchwork.models import SeriesReference
> > > +from patchwork.models import SeriesPatch
> > >  from patchwork.models import State
> > >  from patchwork.models import Submission
> > >  
> > > @@ -789,7 +790,16 @@ def parse_mail(mail, list_id=None):
> > >              delegate = auto_delegate(project, filenames)
> > >  
> > >          series = find_series(mail)
> > > -        if not series and n:  # the series markers indicates a
> > > series
> > > +        # We will create a new series if:
> > > +        # - we have a patch number (x of n), and
> > > +        # - either:
> > > +        #    * there is no series, or
> > > +        #    * the version doesn't match
> > > +        #    * we have a patch with this number already
> > > +        if n and ((not series) or
> > > +                  (series.version != version) or
> > > +                  (SeriesPatch.objects.filter(series=series,
> > > number=x).count()
> > 
> > Instead of importing SeriesPatch, could we do:
> > 
> >     series.patches.filter(number=x).exists()
> > 
> > Seems a little cleaner?
> 
> Sounds good!
> 
> > 
> > 
> > > 
> > > +                   )):
> > >              series = Series(date=date,
> > >                              submitter=author,
> > >                              version=version,
> > > @@ -803,7 +813,15 @@ def parse_mail(mail, list_id=None):
> > >              # as the earlier patch does not reference the later
> > > one.
> > >              for ref in refs + [msgid]:
> > >                  # we don't want duplicates
> > > -                SeriesReference.objects.get_or_create(series=ser
> > > ies,
> > > msgid=ref)
> > > +                try:
> > > +                    # we could have a ref to a previous series.
> > > (For
> > > +                    # example, a series sent in reply to another
> > > +                    # series.) That should not create a series
> > > ref
> > > +                    # for this series, so check for the msg-id
> > > only,
> > > +                    # not the msg-id/series pair.
> > > +                    SeriesReference.objects.get(msgid=ref)
> > > +                except SeriesReference.DoesNotExist:
> > > +                    SeriesReference.objects.create(series=series
> > > ,
> > > msgid=ref)
> > 
> > I imagine that as soon as we meet a conflicting SeriesReference we
> > could (should?) exit. Python has a fancy try-except-else syntax for
> > exactly this use case:
> > 
> >     try:
> >         something
> >     except Exception:
> >         do something
> >     else:
> >         or something else
> 
> 
> We probably could exit, if we were confident about the order in which
> the refs are going to be traversed. We probably don't want to bail
> early
> if we're parsing msgid last - we wouldn't create the ref for the
> message
> we'd just received.
> 
> I'm not 100% sure how the try/except/else block would work - would
> you
> mind posting what you had in mind?

Ah, you're right. I'm unsure if msgid should actually be checked first
or not but that's an investigation for another day. Applied with above
change. Thanks, Daniel

Stephen