[PATCH v2] Fix parsing of interesing series reply structures

[Stephen Fincane]

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?

> +                   )):
>              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=series,
> 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