[PATCH v1 3/9] parser: Parse "Depends-on" tags in emails

Adam Hassick ahassick at iol.unh.edu
Tue Jul 23 01:59:48 AEST 2024


There's an ongoing discussion, no v2 submitted yet. I will submit it
to this list when it's ready.
Thanks for checking in.

On Thu, Jul 18, 2024 at 12:24 PM Stephen Finucane <stephen at that.guru> wrote:
>
> On Fri, 2024-07-12 at 15:28 -0400, Adam Hassick wrote:
> > On Fri, Jul 12, 2024 at 12:02 PM Stephen Finucane <stephen at that.guru> wrote:
> > >
> > > On Mon, 2024-06-17 at 18:18 -0400, Adam Hassick wrote:
> > > > Add a new function to parse "Depends-on" tags to the parser. The value
> > > > may either be a "reference" to a patch or series object or the web URL
> > > > to the object. For example, a reference may look like "patch-1234" or
> > > > "series-5678". When this tag is found, the parser will add the series
> > > > (or the series the patch belongs to) as a dependency to the patch series
> > > > it is creating.
> > >
> > > I know the DPDK folks are using this integer ID-based format for dependencies
> > > (per [1]), but I'm not a huge fan of it. My concerns are two-fold: firstly,
> > > we've been trying to abstract/hide the integer IDs in favour of message IDs over
> > > recent releases and we no longer expose in most of the web UI (they're still
> > > exposed via the REST API but that's for historical reasons). This would be a
> > > step backwards on this path. Secondly, something like 'series-5678' gives the
> > > casual observer very little information about the location of that dependency.
> > > You'd need to know that Patchwork was being used as well as the specific
> > > location of the Patchwork instance in question. I wonder if, rather than
> > > supporting this syntax, we could support Message IDs (and URLs) instead? That's
> > > a unique identifier that is searchable both online and off (assuming you're
> > > saving mail locally). DPDK could of course choose to add patches on top to
> > > support their existing syntax, though they could also choose to migrate to the
> > > new pattern. wdyt?
> >
> > Sure. We can use message IDs instead of the reference format we came
> > up with. I'll reach out to the DPDK community and see if they are
> > receptive to changing the procedure. My guess is they would prefer
> > that over running Patchwork with custom patches applied.
>
> Any updates on this? I'm assuming we're waiting on that before I can expect a
> v2? Or is the v2 gone elsewhere and I simply haven't spotted it?
>
> Stephen
>
> >
> > > Some other comments on this patch as-is below.
> > >
> > > [1] https://github.com/getpatchwork/git-pw/issues/71
> > >
> > > >
> > > > Signed-off-by: Adam Hassick <ahassick at iol.unh.edu>
> > > > ---
> > > >  patchwork/parser.py | 109 +++++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 108 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/patchwork/parser.py b/patchwork/parser.py
> > > > index 09a53a0..90ec63b 100644
> > > > --- a/patchwork/parser.py
> > > > +++ b/patchwork/parser.py
> > > > @@ -14,11 +14,14 @@ from email.errors import HeaderParseError
> > > >  from fnmatch import fnmatch
> > > >  import logging
> > > >  import re
> > > > +from urllib.parse import urlparse
> > > >
> > > >  from django.contrib.auth.models import User
> > > >  from django.db.utils import IntegrityError
> > > >  from django.db import transaction
> > > >  from django.utils import timezone as tz_utils
> > > > +from django.urls import resolve, Resolver404
> > > > +from django.conf import settings
> > > >
> > > >  from patchwork.models import Cover
> > > >  from patchwork.models import CoverComment
> > > > @@ -32,7 +35,6 @@ from patchwork.models import Series
> > > >  from patchwork.models import SeriesReference
> > > >  from patchwork.models import State
> > > >
> > > > -
> > > >  _msgid_re = re.compile(r'<[^>]+>')
> > > >  _hunk_re = re.compile(r'^\@\@ -\d+(?:,(\d+))? \+\d+(?:,(\d+))? \@\@')
> > > >  _filename_re = re.compile(r'^(---|\+\+\+) (\S+)')
> > > > @@ -1054,6 +1056,102 @@ def parse_pull_request(content):
> > > >      return None
> > > >
> > > >
> > > > +def find_series_from_url(url):
> > > > +    """
> > > > +    Get series or patch from URL.
> > > > +    """
> > > > +
> > > > +    parse_result = urlparse(url)
> > > > +
> > > > +    # Resolve the URL path to see if this is a patch or series detail URL.
> > > > +    try:
> > > > +        result = resolve(parse_result.path)
> > > > +    except Resolver404:
> > > > +        return None
> > > > +
> > > > +    if result.view_name == 'patch-list' and parse_result.query:
> > > > +        # Parse the query string.
> > > > +        # This can be replaced with something much friendlier once the
> > > > +        # series detail view is implemented.
> > > > +        series_query_param = next(
> > > > +            filter(
> > > > +                lambda x: len(x) == 2 and x[0] == 'series',
> > > > +                map(lambda x: x.split('='), parse_result.query.split('&')),
> > > > +            )
> > > > +        )
> > > > +
> > > > +        if series_query_param:
> > >
> > > nit:
> > >
> > >     if not series_query_param:
> > >         return None
> > >
> > >     series_id = series_query_param[1]
> > >
> > >     ...
> > >
> > > > +            series_id = series_query_param[1]
> > > > +
> > > > +            try:
> > > > +                series_id_num = int(series_id)
> > > > +            except ValueError:
> > > > +                return None
> > > > +
> > > > +            return Series.objects.filter(id=series_id_num).first()
> > > > +    elif result.view_name == 'patch-detail':
> > > > +        msgid = Patch.decode_msgid(result.kwargs['msgid'])
> > > > +        patch = Patch.objects.filter(msgid=msgid).first()
> > > > +
> > > > +        if patch:
> > > > +            return patch.series
> > > > +
> > > > +
> > > > +def find_series_from_ref(match):
> > > > +    (obj_type, obj_id_str) = match
> > >
> > > nit: don't need those extra brackets
> > >
> > > > +
> > > > +    try:
> > > > +        object_id = int(obj_id_str)
> > > > +    except ValueError:
> > > > +        return None
> > > > +
> > > > +    if obj_type == 'series':
> > > > +        series = Series.objects.filter(id=object_id).first()
> > >
> > > nit:
> > >
> > >     return Series.objects.filter(...)
> > >
> > > > +    elif obj_type == 'patch':
> > > > +        patch = Patch.objects.filter(id=object_id).first()
> > > > +
> > > > +        if not patch:
> > > > +            return None
> > > > +
> > > > +        series = patch.series
> > >
> > > nit:
> > >
> > >     return patch.series
> > >
> > > > +
> > > > +    return series
> > > > +
> > > > +
> > > > +def parse_depends_on(content):
> > > > +    """Parses any dependency hints in the comments."""
> > > > +    depends_patterns = [
> > > > +        (
> > > > +            re.compile(
> > > > +                r'^Depends-on: ((?:patch)?(?:series)?)-([\d]+)(?: \("[^\n\r"]+"\))?\s*$',
> > > > +                re.MULTILINE | re.IGNORECASE,
> > > > +            ),
> > > > +            find_series_from_ref,
> > > > +        ),
> > > > +        (
> > > > +            re.compile(
> > > > +                r'^Depends-on: (http[s]?:\/\/[\w\d\-.\/=&@:%?_\+()]+)\s*$',
> > > > +                re.MULTILINE | re.IGNORECASE,
> > > > +            ),
> > > > +            find_series_from_url,
> > > > +        ),
> > > > +    ]
> > > > +
> > > > +    dependencies = list()
> > >
> > > nit: Can you use a literal?
> > >
> > >     dependencies = []
> > >
> > > > +
> > > > +    for pat, mapper in depends_patterns:
> > > > +        matches = pat.findall(content)
> > > > +
> > > > +        # Produce a list of tuples containing type and ID of each dependency.
> > > > +        # Eliminate elements where we could not parse the ID as an integer.
> > > > +        dependencies.extend(
> > > > +            filter(lambda series: series is not None, map(mapper, matches))
> > > > +        )
> > >
> > > This is a little too clever for my liking :)
> > >
> > >      for match in re.findall(
> > >          r'^Depends-on: ((?:patch)?(?:series)?)-([\d]+)(?:
> > > \("[^\n\r"]+"\))?\s*$',
> > >          re.MULTILINE | re.IGNORECASE,
> > >      ):
> > >          if series := find_series_from_ref(match):
> > >              dependencies.append(series)
> > >
> > >      ...
> >
> > I was looking to make this as easily extensible as possible, so that
> > if another contributor in the future wants to come along and add
> > another format to refer to a series/patch it can be added easily.
> > But we can unwind this into two loops, one for message ID and one for
> > URL parsing.
> >
> > > > +
> > > > +    # Return list of series objects to depend on.
> > > > +    return dependencies
> > > > +
> > > > +
> > > >  def find_state(mail):
> > > >      """Return the state with the given name or the default."""
> > > >      state_name = clean_header(mail.get('X-Patchwork-State', ''))
> > > > @@ -1171,6 +1269,12 @@ def parse_mail(mail, list_id=None):
> > > >
> > > >      pull_url = parse_pull_request(message)
> > > >
> > > > +    # Only look for "Depends-on" tags if the setting is enabled.
> > > > +    if settings.ENABLE_DEPENDS_ON_PARSING:
> > > > +        dependencies = parse_depends_on(message)
> > > > +    else:
> > > > +        dependencies = []
> > > > +
> > >
> > > The patch that adds this should come earlier in the series (before this one). We
> > > can discuss whether it's needed there. However, if we keep it can you move the
> > > check for it into 'parse_depends_on' and move the call to 'parse_depends_on'
> > > below so that we can avoid parsing dependencies for non-cover letter/patch
> > > emails?
> >
> > Makes sense to me.
> >
> > >
> > > >      # build objects
> > > >
> > > >      if not is_comment and (diff or pull_url):  # patches or pull requests
> > > > @@ -1308,6 +1412,8 @@ def parse_mail(mail, list_id=None):
> > > >              # always have a series
> > > >              series.add_patch(patch, x)
> > > >
> > > > +        series.add_dependencies(dependencies)
> > >
> > >     dependencies = parse_depends_on(message)
> > >     series.add_dependencies(dependencies)
> > >
> > > > +
> > > >          return patch
> > > >      elif x == 0:  # (potential) cover letters
> > > >          # if refs are empty, it's implicitly a cover letter. If not,
> > > > @@ -1374,6 +1480,7 @@ def parse_mail(mail, list_id=None):
> > > >              logger.debug('Cover letter saved')
> > > >
> > > >              series.add_cover_letter(cover_letter)
> > > > +            series.add_dependencies(dependencies)
> > >
> > >     dependencies = parse_depends_on(message)
> > >     series.add_dependencies(dependencies)
> > >
> > > >              return cover_letter
> > > >
> > >
>


More information about the Patchwork mailing list