Stephen Fincane <[email protected]> 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 <[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?
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=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 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? Regards, Daniel _______________________________________________ Patchwork mailing list [email protected] https://lists.ozlabs.org/listinfo/patchwork
