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 <[email protected]>
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 <[email protected]> > --- > > 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 _______________________________________________ Patchwork mailing list [email protected] https://lists.ozlabs.org/listinfo/patchwork
